Re: dplayx: Cache the results of DirectPlayEnumerateAW()

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Bruno,

Some tests for this behavior would be useful. Also consider adding tests that check if DirectPlayEnumerate always enumerates the same pointers.

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").
> - /* 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.
> - 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.
> - 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.
> - 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?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJUqlckAAoJEN0/YqbEcdMwPYcP/1zJL5U+YlDNVLsjHaiZQClj
x0gyVMUcAB/0IH5QA1l37o0OChMgPg+xjkKuHfV9yx1ESFiulbm6bb1Qc1MfHmSO
KB0hswxd1G9LVw8VUcbfCMmS3LK4JQl/Ha8Zj2RcFpcU+xIRc+p0LY+bg0QJyML2
i0XYSomKubZoKDeW1ZW8oAGXHc67kT2kBVXoZ9jCThjob8Lzy0bsZaKqEGiEjm5v
UqGCjfv7wLsd3egEAA6WBa/sxmV5pGUqPbYe0OLNxExtOOHmZE7u0Ti9bAdGLZLE
FnEB46IW6ECgMbdizIh9yZU/1pNB1AjVXKC1nhJevPPA4Bbln+4OkOrjKuYSQyqq
lITarHscB9wnDdhXc3iwvi1vOB1B0v0X7CvTSAc+xBkx7gjK8/pyRSShcm6ho7yj
7UeKJuTqZ/+WRoMX2ariim7vzSzedWw+P1V0vcO/J5bA6icWpAxoivihSM7OzXFT
lFYAr98BwlwVljr7QYKMtC1CkmBHIXIaaIoV7c6miTounpwXGvI4BdXvOt679jTV
bKfMo7p5rx9n2K2cro7/J/g0bPcIX6PAg0ay9g+jD7XzhWkxQTjzHQF79uko3XG7
U3Y3ApC1dRwzJ7PzDIiS3F0t1Bk1+PcaMK269cOISni9odiODz1I7uzQFsVJE0S8
H9a8YJIdWhzvjszLKn44
=76/t
-----END PGP SIGNATURE-----

dplayx: Cache the results of DirectPlayEnumerateAW()

[Bruno Jesus]

On Mon, Jan 5, 2015 at 7:19 AM, Stefan Dösinger
[email protected] 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