Re: dpnet: Use registry for lookup in IDirectPlay8Peer (try 2)

[Bruno Jesus]

On Thu, Feb 13, 2014 at 1:23 AM, Alistair Leslie-Hughes
 wrote:
> Hi,
> Corrected pointer arithmetic.
> Corrected assign memory for RegGetValueW calls.
>
> Changelog:
>       dpnet: Use registry for lookup in IDirectPlay8Peer
>
> Best Regards
>   Alistair Leslie-Hughes

Hi, here are my thoughts about this patch, as I told you before I'm
really looking forward for your dpnet work =)
I'm not used to review patches so if I do something wrong I ask for
sorry in advance.

+    if(!pcReturned || !pcbEnumData)
+        return E_POINTER;

This checks together with the error tests could be in a different
patch as they are not related to the main subject of the patch.

+        while(nextKeyNameResult == ERROR_SUCCESS)
+        {
+            res = RegGetValueW(key, provider, friendly,
RRF_RT_REG_SZ, NULL, NULL, &size);
+            if(res == ERROR_SUCCESS)
+            {
+                WCHAR *buf = HeapAlloc(GetProcessHeap(), 0, size);
+                if(buf)
+                {
+                    res = RegGetValueW( key, provider, friendly,
RRF_RT_REG_SZ, NULL, buf, &size);
+                    if(res == ERROR_SUCCESS)
+                    {
+                        TRACE("%s=%s\n", debugstr_w(provider),
debugstr_w(buf));
+
+                        req_size += sizeof(DPN_SERVICE_PROVIDER_INFO)
+ (size * sizeof(WCHAR));
+
+                        (*pcReturned)++;
+                    }

You are only reading the key value in the first loop to trace it,
maybe you could simplify this loop with a single RegGetValueW call and
no memory allocation just to get the size required and in the next
loop where you really copy the data you could trace it.

+        char *infoend = ((char *)pSPInfoBuffer +
(sizeof(DPN_SERVICE_PROVIDER_INFO) * (*pcReturned)));

Since you are copying WCHAR data infoend should be a WCHAR pointer and
it could be done like this AFAICS:
WCHAR *infoend = (WCHAR *)(pSPInfoBuffer + *pcReturned);

+        while(nextKeyNameResult == ERROR_SUCCESS)
+        {
+            res = RegGetValueW(key, provider, friendly,
RRF_RT_REG_SZ, NULL, NULL, &size);
+
+            if(res == ERROR_SUCCESS)
+            {
+                WCHAR *buf = HeapAlloc(GetProcessHeap(), 0, size);
+                if(buf)
+                {
+                    res = RegGetValueW(key, provider, friendly,
RRF_RT_REG_SZ, NULL, buf, &size);
+                    if(res == ERROR_SUCCESS)
+                    {
+                        pSPInfoBuffer[count].guid = CLSID_DP8SP_TCPIP;
+                        pSPInfoBuffer[count].pwszName =
(LPWSTR)(infoend + offset);
+                        lstrcpyW(pSPInfoBuffer[count].pwszName, buf);
+

What if you read data directly over infoend buffer? This would
eliminate the need for buf memory allocation and the strcpy. You would
only have to set the pwszName.

+                        offset += size-1;

If you do a -1 here the next string will overwrite the NULL terminator
from the previous string, won't it?

If you decide to copy directly over the infoend buffer you could also
get rid from the offset variable since you could do infoend += size
instead.

Best regards,
Bruno