[PATCH v4 0/2] MR5519: setupapi: use INF parser to read class GUID and class name
Previously, `SetupDiGetINFClassW()` read INF files with `GetPrivateProfileString()`, which does not substitute %strkey% tokens. This caused device installation to fail for devices which had driver INF files using %strkey% tokens in Version section. An example of such device is Vernier LabQuest Mini (08f7:0008) for which Vernier's LoggerPro application includes a driver. The INF file in question adds a new device setup class and has following entries in Version section: ``` Class = %ClassName% ClassGuid = %DeviceClassGUID% ``` Strings section includes following entries: ``` DeviceClassGUID = "{6B8429BF-10AD-4b66-9FBA-2FE72B891721}" ClassName = "VST_WinUSB" ``` Previously, when LoggerPro was installed and LabQuest Mini was hotplugged, device installation failed with the following error: ``` fixme:setupapi:SetupDiGetINFClassW failed to convert "L"%DeviceClassGUID"" into a guid ``` This caused GUID_NULL to be used and Class was not set to the registry for the device. With this commit, correct class GUID and names are set to the device registry entry. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56579 -- v4: setupapi: use INF parser to read class GUID and class name setupapi/tests: add tests for reading INF class with %strkey% tokens https://gitlab.winehq.org/wine/wine/-/merge_requests/5519
From: Tuomas Räsänen <tuomas.rasanen(a)opinsys.fi> --- dlls/setupapi/tests/devinst.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/dlls/setupapi/tests/devinst.c b/dlls/setupapi/tests/devinst.c index 2dc891d61f1..bc830aebc19 100644 --- a/dlls/setupapi/tests/devinst.c +++ b/dlls/setupapi/tests/devinst.c @@ -1876,6 +1876,8 @@ static void test_get_inf_class(void) static const char inffile[] = "winetest.inf"; static const char content[] = "[Version]\r\n\r\n"; static const char* signatures[] = {"\"$CHICAGO$\"", "\"$Windows NT$\""}; + static const GUID deadbeef_class_guid = {0xdeadbeef,0xdead,0xbeef,{0xde,0xad,0xbe,0xef,0xde,0xad,0xbe,0xef}}; + static const char deadbeef_class_name[] = "DeadBeef"; char cn[MAX_PATH]; char filename[MAX_PATH]; @@ -2014,6 +2016,27 @@ static void test_get_inf_class(void) todo_wine ok(count == 4, "expected count==4, got %lu(%s)\n", count, cn); + /* Test Strings substitution */ + WritePrivateProfileStringA("Version", "Class", "%ClassName%", filename); + WritePrivateProfileStringA("Version", "ClassGUID", "%ClassGuid%", filename); + + /* Without Strings section the ClassGUID is invalid (has non-substituted strkey token) */ + retval = SetupDiGetINFClassA(filename, &guid, cn, MAX_PATH, NULL); + ok(!retval, "expected SetupDiGetINFClassA to fail\n"); + ok(GetLastError() == ERROR_INVALID_PARAMETER, + "expected error ERROR_INVALID_PARAMETER, got %lu\n", GetLastError()); + + /* With Strings section the ClassGUID and Class should be substituted */ + WritePrivateProfileStringA("Strings", "ClassName", deadbeef_class_name, filename); + WritePrivateProfileStringA("Strings", "ClassGuid", "{deadbeef-dead-beef-dead-beefdeadbeef}", filename); + count = 0xdeadbeef; + retval = SetupDiGetINFClassA(filename, &guid, cn, MAX_PATH, &count); +todo_wine { + ok(retval, "expected SetupDiGetINFClassA to succeed! error %lu\n", GetLastError()); + ok(count == lstrlenA(deadbeef_class_name) + 1, "expected count=%d, got %lu\n", lstrlenA(deadbeef_class_name) + 1, count); + ok(!lstrcmpA(deadbeef_class_name, cn), "expected class_name='%s', got '%s'\n", deadbeef_class_name, cn); + ok(IsEqualGUID(&deadbeef_class_guid, &guid), "expected ClassGUID to be deadbeef-dead-beef-dead-beefdeadbeef\n"); +} DeleteFileA(filename); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5519
From: Tuomas Räsänen <tuomas.rasanen(a)opinsys.fi> Previously, SetupDiGetINFClassW read INF files with GetPrivateProfileString, which does not substitute %strkey% tokens. This caused device installation to fail for devices which had driver INF files using %strkey% tokens in Version section. An example of such device is Vernier LabQuest Mini (08f7:0008) for which Vernier's LoggerPro application includes a driver. The INF file in question adds a new device setup class and has following entries in Version section: Class = %ClassName% ClassGuid = %DeviceClassGUID% Strings section includes following entries: DeviceClassGUID = "{6B8429BF-10AD-4b66-9FBA-2FE72B891721}" ClassName = "VST_WinUSB" Previously, when LoggerPro was installed and LabQuest Mini was hotplugged, device installation failed with the following error: fixme:setupapi:SetupDiGetINFClassW failed to convert "L"%DeviceClassGUID"" into a guid This caused GUID_NULL to be used and Class was not set to the registry for the device. With this commit, correct class GUID and names are set to the device registry entry. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56579 --- dlls/setupapi/devinst.c | 67 +++++++++++++++++++++++++++-------- dlls/setupapi/tests/devinst.c | 3 +- 2 files changed, 54 insertions(+), 16 deletions(-) diff --git a/dlls/setupapi/devinst.c b/dlls/setupapi/devinst.c index 7e2d683daa6..019505b01ec 100644 --- a/dlls/setupapi/devinst.c +++ b/dlls/setupapi/devinst.c @@ -4280,8 +4280,11 @@ BOOL WINAPI SetupDiGetINFClassW(PCWSTR inf, LPGUID class_guid, PWSTR class_name, DWORD size, PDWORD required_size) { BOOL have_guid, have_name; - DWORD dret; + DWORD class_name_len; WCHAR buffer[MAX_PATH]; + INFCONTEXT inf_ctx; + HINF hinf; + BOOL retval = FALSE; if (!inf) { @@ -4302,30 +4305,63 @@ BOOL WINAPI SetupDiGetINFClassW(PCWSTR inf, LPGUID class_guid, PWSTR class_name, return FALSE; } - if (!GetPrivateProfileStringW(Version, Signature, NULL, buffer, MAX_PATH, inf)) + if ((hinf = SetupOpenInfFileW(inf, NULL, INF_STYLE_WIN4, NULL)) == INVALID_HANDLE_VALUE) + { + ERR("failed to open INF file %s\n", debugstr_w(inf)); return FALSE; + } + + if (!SetupFindFirstLineW(hinf, Version, Signature, &inf_ctx)) + { + ERR("INF file %s does not have mandatory [Version].Signature\n", debugstr_w(inf)); + goto out; + } + + if (!SetupGetStringFieldW(&inf_ctx, 1, buffer, ARRAY_SIZE(buffer), NULL)) + { + ERR("failed to get [Version].Signature string from %s\n", debugstr_w(inf)); + goto out; + } if (lstrcmpiW(buffer, Chicago) && lstrcmpiW(buffer, WindowsNT)) - return FALSE; + { + ERR("INF file %s has invalid [Version].Signature: %s\n", debugstr_w(inf), debugstr_w(buffer)); + goto out; + } + + have_guid = SetupFindFirstLineW(hinf, Version, ClassGUID, &inf_ctx); - buffer[0] = '\0'; - have_guid = 0 < GetPrivateProfileStringW(Version, ClassGUID, NULL, buffer, MAX_PATH, inf); if (have_guid) { + if (!SetupGetStringFieldW(&inf_ctx, 1, buffer, ARRAY_SIZE(buffer), NULL)) + { + ERR("failed to get [Version].ClassGUID as a string from '%s'\n", debugstr_w(inf)); + goto out; + } + buffer[lstrlenW(buffer)-1] = 0; if (RPC_S_OK != UuidFromStringW(buffer + 1, class_guid)) { - FIXME("failed to convert \"%s\" into a guid\n", debugstr_w(buffer)); + ERR("INF file %s has invalid [Version].ClassGUID: %s\n", debugstr_w(inf), debugstr_w(buffer)); SetLastError(ERROR_INVALID_PARAMETER); - return FALSE; + goto out; } } - buffer[0] = '\0'; - dret = GetPrivateProfileStringW(Version, Class, NULL, buffer, MAX_PATH, inf); - have_name = 0 < dret; + have_name = SetupFindFirstLineW(hinf, Version, Class, &inf_ctx); + + class_name_len = 0; + if (have_name) + { + if (!SetupGetStringFieldW(&inf_ctx, 1, buffer, ARRAY_SIZE(buffer), NULL)) + { + ERR("failed to get [Version].Class as a string from '%s'\n", debugstr_w(inf)); + goto out; + } + + class_name_len = lstrlenW(buffer); + } - if (dret >= MAX_PATH -1) FIXME("buffer might be too small\n"); if (have_guid && !have_name) { class_name[0] = '\0'; @@ -4334,7 +4370,7 @@ BOOL WINAPI SetupDiGetINFClassW(PCWSTR inf, LPGUID class_guid, PWSTR class_name, if (have_name) { - if (dret < size) lstrcpyW(class_name, buffer); + if (class_name_len < size) lstrcpyW(class_name, buffer); else { SetLastError(ERROR_INSUFFICIENT_BUFFER); @@ -4342,9 +4378,12 @@ BOOL WINAPI SetupDiGetINFClassW(PCWSTR inf, LPGUID class_guid, PWSTR class_name, } } - if (required_size) *required_size = dret + ((dret) ? 1 : 0); + if (required_size) *required_size = class_name_len + ((class_name_len) ? 1 : 0); - return (have_guid || have_name); + retval = (have_guid || have_name); +out: + SetupCloseInfFile(hinf); + return retval; } static LSTATUS get_device_property(struct device *device, const DEVPROPKEY *prop_key, DEVPROPTYPE *prop_type, diff --git a/dlls/setupapi/tests/devinst.c b/dlls/setupapi/tests/devinst.c index bc830aebc19..e54605d6987 100644 --- a/dlls/setupapi/tests/devinst.c +++ b/dlls/setupapi/tests/devinst.c @@ -2031,12 +2031,11 @@ static void test_get_inf_class(void) WritePrivateProfileStringA("Strings", "ClassGuid", "{deadbeef-dead-beef-dead-beefdeadbeef}", filename); count = 0xdeadbeef; retval = SetupDiGetINFClassA(filename, &guid, cn, MAX_PATH, &count); -todo_wine { ok(retval, "expected SetupDiGetINFClassA to succeed! error %lu\n", GetLastError()); ok(count == lstrlenA(deadbeef_class_name) + 1, "expected count=%d, got %lu\n", lstrlenA(deadbeef_class_name) + 1, count); ok(!lstrcmpA(deadbeef_class_name, cn), "expected class_name='%s', got '%s'\n", deadbeef_class_name, cn); ok(IsEqualGUID(&deadbeef_class_guid, &guid), "expected ClassGUID to be deadbeef-dead-beef-dead-beefdeadbeef\n"); -} + DeleteFileA(filename); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5519
On Wed Apr 24 19:05:34 2024 +0000, Tuomas Räsänen wrote:
changed this line in [version 4 of the diff](/wine/wine/-/merge_requests/5519/diffs?diff_id=111043&start_sha=d29446a06dbc49c50985833cd0b5b44a2b5517f8#5c8e393d12376e8f2a4f2aec734e275204aecb82_2025_2025) True, fixed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68769
On Wed Apr 24 19:05:34 2024 +0000, Tuomas Räsänen wrote:
changed this line in [version 4 of the diff](/wine/wine/-/merge_requests/5519/diffs?diff_id=111043&start_sha=d29446a06dbc49c50985833cd0b5b44a2b5517f8#ec76fe5c6a4ced08d2a20468fd1a904a90d50c2c_4365_4365) Removed the redundant conditional FIXME.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68770
On Wed Apr 24 19:07:57 2024 +0000, Elizabeth Figura wrote:
The tests are missing any todo_wine annotations, which is suspect. We need the tests to pass after every commit, which entails adding and removing todo_wine accordingly. Oh, I see, now I know.
But I guess it's still a good idea to introduce tests in a separate commit? At least the removal of `wine_todo`s gives good indication what the commit is aiming for.. Now the first commit brings in tests with some parts annotated with `todo_wine` and the second commit removes annotations. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68771
On Wed Apr 24 19:10:40 2024 +0000, Elizabeth Figura wrote:
Hard to say without knowing all the details, but a piece of domain-specific hardware which communicates via USB is generally plausible. Well, I'll continue my quest then.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68772
On Wed Apr 24 19:10:40 2024 +0000, Tuomas Räsänen wrote:
Well, I'll continue my quest then. Let me know (email or IRC is good) if you have any questions; this is certainly an area I have a particular interest in and I'd love to help.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68773
But I guess it's still a good idea to introduce tests in a separate commit? At least the removal of `wine_todo`s gives good indication what the commit is aiming for..
Yes, precisely. When it's feasible that helps a reviewer tell what the commit is trying to do. It also validates that it really is solving the problem it's supposed to, assuming the tests pass as they're supposed to. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519#note_68774
This merge request was approved by Elizabeth Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5519
participants (2)
-
Elizabeth Figura (＠zfigura) -
Tuomas Räsänen