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

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

[Alistair Leslie-Hughes]

Hi Bruno,

On 14/02/2014 10:18 AM, Bruno Jesus wrote:
> On Thu, Feb 13, 2014 at 1:23 AM, Alistair Leslie-Hughes
>  wrote:
> 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.

Thanks for you feedback.  I've re-factored the patch and it's alot 
cleaner than the first few iterations.

Its still needs to be a char cast since RegGetValueW returns the size in
bytes which would make the offset incorrect.

Best Regards
  Alistair Leslie-Hughes

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

[Bruno Jesus]

On Fri, Feb 14, 2014 at 5:28 AM, Alistair Leslie-Hughes
 wrote:
> Hi Bruno,
>
> On 14/02/2014 10:18 AM, Bruno Jesus wrote:
> > On Thu, Feb 13, 2014 at 1:23 AM, Alistair Leslie-Hughes
> >  wrote:
> > 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.
>
> Thanks for you feedback.  I've re-factored the patch and it's alot cleaner
> than the first few iterations.
>
> Its still needs to be a char cast since RegGetValueW returns the size in
> bytes which would make the offset incorrect.

Not really, because when you add a size to a pointer it increments in
the pointer size and not bytes. So WCHAR *ptr=0; ptr+=10;
printf("%p\n", p); would print 0x14 (20 in decimal although we just
added 10 to it).
> Best Regards
>  Alistair Leslie-Hughes

Bruno

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

[Alistair Leslie-Hughes]

Hi Bruno,

On 14/02/2014 8:30 PM, Bruno Jesus wrote:
> On Fri, Feb 14, 2014 at 5:28 AM, Alistair Leslie-Hughes
> Not really, because when you add a size to a pointer it increments in
> the pointer size and not bytes. So WCHAR *ptr=0; ptr+=10;
> printf("%p\n", p); would print 0x14 (20 in decimal although we just
> added 10 to it).

The theory is correct.  However RegGetValueW returns the size of the 
buffer we would need to allocate including the null terminator. In your 
example it would return 20 (9+1*sizeof(WCHAR)) and not 10.

Best Regards
  Alistair Leslie-Hughes