dplayx: Cache the results of DirectPlayEnumerateAW()
Bruno Jesus
00cpxxx at gmail.com
Mon Jan 5 04:37:36 CST 2015
On Mon, Jan 5, 2015 at 7:19 AM, Stefan Dösinger
<stefandoesinger at gmail.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Bruno,
Hi, Stefan. Thanks for the review.
> Some tests for this behavior would be useful. Also consider adding tests that check if DirectPlayEnumerate always enumerates the same pointers.
I'll try.
> Am 2015-01-04 um 23:05 schrieb Bruno Jesus:
>> + static struct _data
>> + {
>> + GUID guid;
>> + WCHAR descriptionW[256];
>> + char descriptionA[256];
>> + } *providers_cache;
> You don't have to give this structure a name. (i.e., remove "_data").
OK.
>> + /* Some applications require that the data returned through the callback persist after the callbacks
>> + * are called, so we buffer the information first and do the callbacks later. We will always rewrite
>> + * the buffer as some applications may change it (on purpose or due to bugs).
>> + */
> Please name the application. 2 years from now nobody will remember it, and a search through wine-patches might not be successful.
Makes sense.
>> + HeapFree(GetProcessHeap(), 0, providers_cache);
>> + providers_cache = HeapAlloc(GetProcessHeap(), 0, sizeof(*providers_cache) * dwIndex);
>> + if (!providers_cache)
>> + {
>> + ERR(": failed to alloc required memory.\n");
>> + return DPERR_EXCEPTION;
>> + }
> Does the registry data change during runtime? I think it is created during prefix creation and then never touched again.
I don't know, I'm just worried that the application may destroy the
data, that's why I Always re-read it.
>> + if (sizeOfDescription > max_sizeOfDescriptionW)
>> + {
>> + HeapFree(GetProcessHeap(), 0, descriptionW);
>> + max_sizeOfDescriptionW = sizeOfDescription;
>> + }
>> + descriptionW = HeapAlloc(GetProcessHeap(), 0, sizeOfDescription);
> This can leak descriptionW. You're also hard-coding a size limit of 256 in providers_cache. One of the two isn't right, most likely the hard-coded limit.
This is the current code, I didn't change it. The patch may seem large
but it's an indentation difference.
>> + RegQueryValueExW(hkServiceProvider, descW,
>> + NULL, NULL, (LPBYTE) descriptionW, &sizeOfDescription);
>> +
>> + lstrcpynW(providers_cache[dwIndex].descriptionW, descriptionW, sizeof(providers_cache[0].descriptionW));
>> + WideCharToMultiByte(CP_ACP, 0, providers_cache[dwIndex].descriptionW, -1, providers_cache[dwIndex].descriptionA,
>> + sizeof(providers_cache[0].descriptionA), NULL, NULL);
> The old code queries two different values, "DescriptionA" and "DescriptionW". Your code queries "DescriptionW" and converts the content to ASCII. Do DescriptionA and DescriptionW always contain the same content?
I assumed it could be the same because in English and Portuguese
systems it is, but thinking more properly it may make a difference for
non-roman writing languages, I will cache both.
Best Regards,
Bruno
More information about the wine-devel
mailing list