Should I give up?

[Damjan Jovanovic]

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
ie. no curly braced around the guid.
And in addition it incorrectly returns FALSE for a
return type of HKEY (instead of INVALID_HANDLE_VALUE).

I've submitted a test and patch to wine-patches 7
times now, and received very little feedback, which
I've always followed.

If this is how difficult it is to submit a patch, no
wonder I hear people complaining about how wine
doesn't have enough developers. How are you ever going
to accept my still image code, which is 4000+ lines
long and growing, when you can't accept a simple 182
line long patch?

Normally I'm very patient - but this is ridiculous.

I'm attaching my latest patch and would appreciate any
comments.

Thank you
Damjan

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com

--- a/dlls/setupapi/tests/Makefile.in	2006-07-10 18:01:08.000000000 +0200
+++ b/dlls/setupapi/tests/Makefile.in	2006-08-31 21:29:12.000000000 +0200
@@ -3,7 +3,7 @@
 SRCDIR    = @srcdir@
 VPATH     = @srcdir@
 TESTDLL   = setupapi.dll
-IMPORTS   = setupapi kernel32
+IMPORTS   = setupapi kernel32 advapi32 rpcrt4

CTESTS = \
    devinst.c \
--- a/dlls/setupapi/devinst.c	2006-08-28 20:35:44.000000000 +0200
+++ b/dlls/setupapi/devinst.c	2006-08-31 22:27:19.000000000 +0200
@@ -1413,6 +1413,7 @@
         PVOID Reserved)
 {
     LPWSTR lpGuidString;
+    LPWSTR lpBracedGuidString;
     HKEY hClassesKey;
     HKEY hClassKey;
     LPCWSTR lpKeyName;
@@ -1453,21 +1454,30 @@
     if (UuidToStringW((UUID*)ClassGuid, &lpGuidString) != RPC_S_OK)
     {
    RegCloseKey(hClassesKey);
-	return FALSE;
+	return INVALID_HANDLE_VALUE;
     }

+    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);
+
     if (RegOpenKeyExW(hClassesKey,
-		      lpGuidString,
+		      lpBracedGuidString,
    	      0,
    	      KEY_ALL_ACCESS,
    	      &hClassKey))
     {
-	RpcStringFreeW(&lpGuidString);
+        RpcStringFreeW(&lpBracedGuidString);
    RegCloseKey(hClassesKey);
-	return FALSE;
+	return INVALID_HANDLE_VALUE;
     }

-    RpcStringFreeW(&lpGuidString);
+    RpcStringFreeW(&lpBracedGuidString);
     RegCloseKey(hClassesKey);

return hClassKey;
--- a/dlls/setupapi/tests/devinst.c	2006-08-28 20:35:53.000000000 +0200
+++ b/dlls/setupapi/tests/devinst.c	2006-08-31 22:14:36.000000000 +0200
@@ -26,6 +26,7 @@
 #include "wingdi.h"
 #include "winuser.h"
 #include "winreg.h"
+#include "rpc.h"
 #include "setupapi.h"

#include "wine/test.h"
@@ -34,6 +35,7 @@
 static HMODULE hSetupAPI;
 static HDEVINFO (WINAPI *pSetupDiCreateDeviceInfoListExW)(GUID*,HWND,PCWSTR,PVOID);
 static BOOL     (WINAPI *pSetupDiDestroyDeviceInfoList)(HDEVINFO);
+static HKEY     (WINAPI *pSetupDiOpenClassRegKeyExW)(GUID*,REGSAM,DWORD,PCWSTR,PVOID);

static void init_function_pointers(void)
 {
@@ -43,6 +45,7 @@
     {
         pSetupDiCreateDeviceInfoListExW = (void *)GetProcAddress(hSetupAPI, "SetupDiCreateDeviceInfoListExW");
         pSetupDiDestroyDeviceInfoList = (void *)GetProcAddress(hSetupAPI, "SetupDiDestroyDeviceInfoList");
+        pSetupDiOpenClassRegKeyExW = (void *)GetProcAddress(hSetupAPI, "SetupDiOpenClassRegKeyExW");
     }
 }

@@ -79,6 +82,86 @@
     ok(ret, "SetupDiDestroyDeviceInfoList failed : %ld\n", error);
 }

+static void test_SetupDiOpenClassRegKeyExW()
+{
+    GUID guid;
+    RPC_STATUS rpcStatus;
+    HKEY hkey;
+    WCHAR *unbracedGuid;
+    WCHAR *bracedGuid;
+
+    rpcStatus = UuidCreate(&guid);
+    if (rpcStatus != RPC_S_OK)
+    {
+        ok(FALSE, "failed to generate test guid, error %ld", rpcStatus);
+        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);
+
+    rpcStatus = UuidToStringW(&guid, &unbracedGuid);
+    if (rpcStatus != RPC_S_OK)
+    {
+        ok(FALSE, "failed to get string form of guid, error %ld", rpcStatus);
+        return;
+    }
+
+    bracedGuid = HeapAlloc(GetProcessHeap(), 0,
+        (lstrlenW(unbracedGuid) + 3) * sizeof(WCHAR));
+    if (bracedGuid != NULL)
+    {
+        HKEY classesKey;
+
+        bracedGuid[0] = (WCHAR) '{';
+        memcpy(&bracedGuid[1], unbracedGuid,
+            lstrlenW(unbracedGuid) * sizeof(WCHAR));
+        bracedGuid[1 + lstrlenW(unbracedGuid)] = (WCHAR) '}';
+        bracedGuid[1 + lstrlenW(unbracedGuid) + 1] = (WCHAR) 0;
+
+        SetLastError(0xdeadbeef);
+        classesKey = pSetupDiOpenClassRegKeyExW(NULL, KEY_ALL_ACCESS,
+            DIOCR_INSTALLER, NULL, NULL);
+        ok(classesKey != INVALID_HANDLE_VALUE,
+            "failed to open the device classes registry key: error %ld\n",
+            GetLastError());
+        if (classesKey != INVALID_HANDLE_VALUE)
+        {
+            DWORD ret;
+            HKEY classKey;
+            ret = RegCreateKeyW(classesKey, bracedGuid, &classKey);
+            if (ret == ERROR_SUCCESS)
+            {
+                RegCloseKey(classKey);
+                SetLastError(0xdeadbeef);
+                classKey = pSetupDiOpenClassRegKeyExW(&guid, KEY_ALL_ACCESS,
+                     DIOCR_INSTALLER, NULL, NULL);
+                ok(classKey != INVALID_HANDLE_VALUE,
+                    "failed opening class key, error %ld\n", GetLastError());
+                ret = RegCloseKey(classKey);
+                ok(ret == ERROR_SUCCESS,
+                    "failed closing class key, error %ld\n", ret);
+
+                RegDeleteKeyW(classesKey, bracedGuid);
+            }
+            else
+                ok(FALSE, "failed creating device key: error %ld\n", ret);
+
+            RegCloseKey(classesKey);
+        }
+
+        HeapFree(GetProcessHeap(), 0, bracedGuid);
+    }
+    else
+        ok(FALSE, "failed to allocate memory for braced guid\n");
+
+    RpcStringFreeW(&unbracedGuid);
+}
+
 START_TEST(devinst)
 {
     init_function_pointers();
@@ -88,5 +171,10 @@
     if (pSetupDiCreateDeviceInfoListExW && pSetupDiDestroyDeviceInfoList)
         test_SetupDiCreateDeviceInfoListEx();
     else
-        trace("Needed calls not all available, skipping tests.\n");
+        trace("Needed calls for SetupDiCreateDeviceInfoListEx not all available, skipping test.\n");
+
+    if (pSetupDiOpenClassRegKeyExW)
+        test_SetupDiOpenClassRegKeyExW();
+    else
+        trace("Needed call for SetupDiOpenClassRegKeyExW not available, skipping test.\n");
 }

[Kai Blin]

On Wednesday 06 September 2006 13:25, Damjan Jovanovic wrote:
Hi,
> I've submitted a test and patch to wine-patches 7
> times now, and received very little feedback, which
> I've always followed.
>
> If this is how difficult it is to submit a patch, no
> wonder I hear people complaining about how wine
> doesn't have enough developers. How are you ever going
> to accept my still image code, which is 4000+ lines
> long and growing, when you can't accept a simple 182
> line long patch?

I'm not entirely sure why it wasn't accepted. I would recommend catching 
Alexandre in IRC to ask about that. He's on vacation right now, though.
> Normally I'm very patient - but this is ridiculous.
>
> I'm attaching my latest patch and would appreciate any
> comments.

Looks ok for me, but I've never had anything to do with setupapi stuff. Maybe 
someone who worked on this before could have a look.

Cheers,
Kai

[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.

[Mike Hearn]

On Wed, 06 Sep 2006 04:25:02 -0700, Damjan Jovanovic wrote:
> I've submitted a test and patch to wine-patches 7
> times now, and received very little feedback, which
> I've always followed.

It may be just that Alexandre is a bit overloaded ... have you tried
chasing him in IRC about it? Vitaliys feedback seems sound but if
Alexandre really objected to those issues he'd probably have let you know.
I suspect it is somehow slipping through the cracks (maybe a spam filter).

Please don't give up! We've all been in this situation, it is not much fun
I know ...

thanks -mike

[Dan Kegel]

On 9/6/06, Mike Hearn  wrote:
> On Wed, 06 Sep 2006 04:25:02 -0700, Damjan Jovanovic wrote:
> > I've submitted a test and patch to wine-patches 7
> > times now, and received very little feedback, which
> > I've always followed.
>
> It may be just that Alexandre is a bit overloaded ...

He was also on vacation.

Don't give up; keep improving and reposting!  (I had to do it five times
recently, took ten days and many improvements to get in.)
- Dan

Should I give up? => Please do not give up

[Detlef Riekenberg]

Hi Damjan:
> My work on the still image system for wine has
> while wine incorrectly tries to open
> HKEY_LOCAL_MACHINE\System\CurrentControlSet\
>   Class\xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
> ie. no curly braced around the guid.
> And in addition it incorrectly returns FALSE for a
> return type of HKEY (instead of INVALID_HANDLE_VALUE).

It's always a good Idea, to fix only one Bug at once.
> I've submitted a test and patch to wine-patches 7
> times now, and received very little feedback, which
> I've always followed.

Please do not give up.

When a Patch goes in, depends not only on the actual
Patch-Quality / Size and what Patches came from the
Creator before, but also the actual amount of Patches
in the Pipeline and the Area, the Patch is for.
(I have similar Problems for my Patches about printing,
but at the moment, there was/is: Google SoC, 
Crossover-Office Mac beta, AJ on Holiday, wineconf)
Also comments from other Developer about Patch-review
can affect the time, a Patch needs to go in the tree.
When there are some People working for the same Area
(d3d as example), patches are commented / reviewed by
others from the group (most times on #winehackers).

Reviewing Patches for an unusual Area is boring
and Applications do not use Device-installation
very often, but I took a look.

You mixed TAB and SPACE. (Common used for idention is 4 SPACE)
As already mentioned, testing the ANSI-version is prefered
and simplifies the test.
UNICODE - Tests do not test the String - convertations.
> @@ -1453,21 +1454,30 @@
>      if (UuidToStringW((UUID*)ClassGuid, &lpGuidString) != RPC_S_OK)
> - 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);

After looking in rpcrt4_main.c, RpcStringFreeW looks ok.
> -
>   RpcStringFreeW(&lpGuidString);
> -
>    RpcStringFreeW(&lpBracedGuidString);

You should use HeapFree here, because you used HeapAlloc above.
This makes you independant from changes inside rpcrt4.
>     RegCloseKey(hClassesKey);
> -
>   return FALSE;
> -
>   return INVALID_HANDLE_VALUE;
> }

With one fix as a time, the patch get's smaller and can be
reviewed more easy.
> - RpcStringFreeW(&lpGuidString);
> - RpcStringFreeW(&lpBracedGuidString);

Same as above: HeaAlloc -> HeapFree
> +static void test_SetupDiOpenClassRegKeyExW()

Parameter not used requires:  (VOID)
> - if (rpcStatus != RPC_S_OK)
> - {
> -
>    ok(FALSE, "failed to generate test guid, error %ld",
>
> rpcStatus);

Already mentioned in other Mails.
> - /* 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);

The Message is very long. I suggest not more as:
ok(hkey == INVALID_HANDLE_VALUE, 
    "returned %p (expected INVALID_HANDLE_VALUE)\n", hkey)
"Test failed:" is prefixed by ok()
> - bracedGuid = HeapAlloc(GetProcessHeap(), 0,
> -
>    (lstrlenW(unbracedGuid) + 3) * sizeof(WCHAR));
> - if (bracedGuid != NULL)
> - {

When the allocation failed, return from the test here.
This saves you a lot of idention.

But since you are testing for a non-existend value,
do you really need an unique Value here?
Can this be tested with "all '0'" or "all 'F'" ?
In this case, all value can be const string.
The test will be a lot short and even rpcrt4.dll
is not needed.