Re: Should I give up?

[Vitaliy Margolen]

Damjan Jovanovic wrote:
> Hi
>
> My work on the still image system for wine has
> highlighted a bug in setupapi's
> SetupDiOpenClassRegKeyExW(). In short, the registry
> keys for device classes have the form
>
> HKEY_LOCAL_MACHINE\System\CurrentControlSet\
>   Class{xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx}
>
> while wine incorrectly tries to open
> HKEY_LOCAL_MACHINE\System\CurrentControlSet\
>   Class\xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
> @@ -1453,21 +1454,30 @@
>      if (UuidToStringW((UUID*)ClassGuid, &lpGuidString) != RPC_S_OK)
>      {
>    RegCloseKey(hClassesKey);
> - return FALSE;
> - return INVALID_HANDLE_VALUE;
> }

Please send this as a separate patch.
> - lpBracedGuidString = (LPWSTR) HeapAlloc(GetProcessHeap(), 0,
> -
>    (1 + strlenW(lpGuidString) + 1 + 1) * sizeof(WCHAR));
> - lpBracedGuidString[0] = (WCHAR) '{';
> - memcpy(&lpBracedGuidString[1], lpGuidString,
> -
>    strlenW(lpGuidString) * sizeof(WCHAR));
> - lpBracedGuidString[1 + strlenW(lpGuidString)] = (WCHAR) '}';
> - lpBracedGuidString[1 + strlenW(lpGuidString) + 1] = (WCHAR) 0;
> - RpcStringFreeW(&lpGuidString);
> -

All that looks messy and way too many calls to strlenW for no reason. Length of GUID
string is known and hard-coded to 36.

lpBracedGuidString = HeapAlloc(GetProcessHeap(), 0, (36 +1+2) * sizeof(WCHAR));
    memcpy(lpBracedGuidString + 1, lpGuidString, 36 * sizeof(WCHAR));
    lpBracedGuidString[ 0] = '{';
    lpBracedGuidString[37] = '}';
    lpBracedGuidString[38] = 0;
    RpcStringFreeW(&lpGuidString);
> @@ -43,6 +45,7 @@
>      {
>          pSetupDiCreateDeviceInfoListExW = (void *)GetProcAddress(hSetupAPI, "SetupDiCreateDeviceInfoListExW");
>          pSetupDiDestroyDeviceInfoList = (void *)GetProcAddress(hSetupAPI, "SetupDiDestroyDeviceInfoList");
> -
>    pSetupDiOpenClassRegKeyExW = (void *)GetProcAddress(hSetupAPI, "SetupDiOpenClassRegKeyExW");
> }

Please try to test Ascii variants instead of Unicode. This way we will be testing both.
> - rpcStatus = UuidCreate(&guid);
> - if (rpcStatus != RPC_S_OK)
> - {
> -
>    ok(FALSE, "failed to generate test guid, error %ld", rpcStatus);
> -
>    return;
> - }

This is not correct. You shouldn't use ok() this way. Also it will already print "Test failed:"
so no need to repeat it again. Instead write something like:
    rpcStatus = UuidCreate(&guid);
    ok(rpcStatus == RPC_S_OK, "error %ld", rpcStatus);
    if (rpcStatus != RPC_S_OK) return;
> - /* Check return value for non-existant key */
> - hkey = pSetupDiOpenClassRegKeyExW(&guid, KEY_ALL_ACCESS,
> -
>    DIOCR_INSTALLER, NULL, NULL);
> - ok(hkey == INVALID_HANDLE_VALUE,
> -
>    "invalid return value %p from SetupDiOpenClassRegKeyExW "
> -
>    "for non-existant key, expected %p\n", hkey, INVALID_HANDLE_VALUE);

There is no need for such a long error message. Simple "got %p\n" would be enough.
Also don't forget to test LastError as well.
> -
>            ok(FALSE, "failed creating device key: error %ld\n", ret);

Please use trace() instead.

Vitaliy.