Re: dpnet: Use registry for lookup in IDirectPlay8Peer (try 2)
On Thu, Feb 13, 2014 at 1:23 AM, Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> 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
Hi Bruno, On 14/02/2014 10:18 AM, Bruno Jesus wrote:
On Thu, Feb 13, 2014 at 1:23 AM, Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> 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
On Fri, Feb 14, 2014 at 5:28 AM, Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> 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 <leslie_alistair(a)hotmail.com> 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
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
participants (2)
-
Alistair Leslie-Hughes -
Bruno Jesus