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"); }
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 -- Kai Blin, <kai Dot blin At gmail Dot com> WorldForge developer http://www.worldforge.org/ Wine developer http://wiki.winehq.org/KaiBlin/ -- Will code for cotton.
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.
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
On 9/6/06, Mike Hearn <mike(a)plan99.net> 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
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. -- By by ... Detlef
participants (6)
-
Damjan Jovanovic -
Dan Kegel -
Detlef Riekenberg -
Kai Blin -
Mike Hearn -
Vitaliy Margolen