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.