Skip to content
Snippets Groups Projects

setupapi: use INF parser to read class GUID and class name

4 unresolved threads

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

Edited by Tuomas Räsänen

Merge request reports

Merge request pipeline #25705 skipped

Merge request pipeline skipped for 987695a4

Approved by

Merged by Alexandre JulliardAlexandre Julliard 11 months ago (Apr 25, 2024 9:17pm UTC)

Merge details

  • Changes merged into master with 987695a4.
  • Deleted the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Tuomas Räsänen changed the description

    changed the description

    • Ouch, I can't believe we were using profile APIs for this.

      Any chance we can get a test for this as well?

    • Sure! There was an existing test case for reading INF class, so I just added couple of tests to the end. Interestingly, the test code used profile API to write to the INF file, so I just decided to go with the flow and used WritePrivateProfileStringA() too.

      So, now there are tests, which use profile API to write to an INF file, to ensure the code it is testing does not use profile API to read from an INF file. How ironic! But I think that's fine, given that all we need from testing point of view is the ability to write variable definitions to sections.

    • Yeah, it's not ideal that the tests are using profile APIs either, but I don't think we really care about that.

    • Please register or sign in to reply
  • Ghost User requested review from @zfigura

    requested review from @zfigura

  • added 4 commits

    • 18262972...166895ae - 2 commits from branch wine:master
    • a9a72b33 - setupapi/tests: add tests for reading INF class with %strkey% tokens
    • 72ac1a4b - setupapi: use INF parser to read class GUID and class name

    Compare with previous version

  • added 2 commits

    • 6324207e - setupapi/tests: add tests for reading INF class with %strkey% tokens
    • d29446a0 - setupapi: use INF parser to read class GUID and class name

    Compare with previous version

    • Resolved by Elizabeth Figura

      @zfigura My end goal is to make Vernier's LabQuest Mini work with Wine. I don't know how much work it requires, or if it's even possible, in a practical sense. I'm not (yet) experienced with Wine codebase (nor with Win API), but by looking at setupapi, pnp, etc., I get a feeling that it should be possible.

      My current understanding, based on log messages, is that the main reason why it's not already working is that load_function_driver() cannot find the driver (the driver does not get installed correctly) on the device startup. And that there are few obstacles before that, the bug this MR tries to solve being the first one.

      So, what do you think? Is there hope and if yes, what would be the most obvious things I should be looking at next?

    • 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_todos 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.

    • But I guess it's still a good idea to introduce tests in a separate commit? At least the removal of wine_todos 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.

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 6324207e
2014 2016 todo_wine
2015 2017 ok(count == 4, "expected count==4, got %lu(%s)\n", count, cn);
2016 2018
2019 /* Test Strings substitution */
2020 WritePrivateProfileStringA("Version", "Class", "%ClassName%", filename);
2021 WritePrivateProfileStringA("Version", "ClassGUID", "%ClassGuid%", filename);
2022
2023 /* Without Strings section the ClassGUID is invalid */
2024 retval = SetupDiGetINFClassA(filename, &guid, cn, MAX_PATH, NULL);
2025 ok(!retval, "expected SetupDiGetINFClassA to fail because ClassGUID is invalid (has non-substituted strkey token)!\n");
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit d29446a0
  • 4351 have_name = SetupFindFirstLineW(hinf, Version, Class, &inf_ctx);
    4327 4352
    4328 if (dret >= MAX_PATH -1) FIXME("buffer might be too small\n");
    4353 class_name_len = 0;
    4354 if (have_name)
    4355 {
    4356 if (!SetupGetStringFieldW(&inf_ctx, 1, buffer, ARRAY_SIZE(buffer), NULL))
    4357 {
    4358 ERR("failed to get [Version].Class as a string from '%s'\n", debugstr_w(inf));
    4359 goto out;
    4360 }
    4361
    4362 class_name_len = lstrlenW(buffer);
    4363 }
    4364
    4365 if (class_name_len >= MAX_PATH - 1) FIXME("buffer might be too small\n");
    • That's impossible; "buffer" is of length MAX_PATH. If we're going to overrun the buffer then SetupGetStringFieldW() would have failed anyway.

    • This conditional FIXME was there already, so no change here (other than the local variable rename).

      But now that I just wrote that, I think I got what you mean and what this conditional FIXME was used for previously.

      So this FIXME was never about detecting overruns, because the overrun was impossible also previously. But it's point is to issue a warning that MAX_PATH might not be enough to capture the whole value of the key, i.e. if there was a risk to get a truncated value; if the buffer gets completely filled, it's possible that part of the value got left out.

      But now, SetupGetStringFieldW parses the whole value and checks if the buffer is actually big enough to store it, so indeed, this FIXME is completely redundant.

    • Tuomas Räsänen changed this line in version 4 of the diff

      changed this line in version 4 of the diff

    • Removed the redundant conditional FIXME.

    • Please register or sign in to reply
  • added 2 commits

    • b347745f - setupapi/tests: add tests for reading INF class with %strkey% tokens
    • 26238664 - setupapi: use INF parser to read class GUID and class name

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Alexandre Julliard approved this merge request

    approved this merge request

  • added 66 commits

    • 26238664...7f190e67 - 64 commits from branch wine:master
    • d8bbf6d0 - setupapi/tests: Add tests for reading INF class with %strkey% tokens.
    • 987695a4 - setupapi: Use INF parser to read class GUID and class name.

    Compare with previous version

  • Please register or sign in to reply
    Loading