From: "Zebediah Figura (she/her)" Subject: Re: [PATCH 1/3] RawInput: Uppercase the device names up to the GUID. Message-Id: Date: Thu, 25 Feb 2021 09:54:15 -0600 In-Reply-To: <20210225094640.qc5dqrob6idialzw@failcraft.local> References: <20210222115038.359258-1-ahiler@codeweavers.com> <20210224085006.pvv7s5nuuhwpt42l@failcraft.local> <3f31b6ae-277a-d808-b029-8cb40bfd62de@codeweavers.com> <20210224131300.mg4a2ono2stngtt6@failcraft.local> <20210225094640.qc5dqrob6idialzw@failcraft.local> On 2/25/21 3:46 AM, Arkadiusz Hiler wrote: > On Wed, Feb 24, 2021 at 02:09:41PM -0600, Zebediah Figura (she/her) wrote: >> On 2/24/21 7:13 AM, Arkadiusz Hiler wrote: >>> On Wed, Feb 24, 2021 at 12:54:42PM +0100, Rémi Bernon wrote: >>>> On 2/24/21 9:50 AM, Arkadiusz Hiler wrote: >>>>> On Wed, Feb 24, 2021 at 08:55:07AM +0100, Rémi Bernon wrote: >>>>>> On 2/22/21 12:50 PM, Arkadiusz Hiler wrote: >>>>>>> This is a preparation for a patch that changes setupapi to return lowercased >>>>>>> device paths, which is the source of RawInput device names. On Windows >>>>>>> RawInput returns mostly uppercase names (except the GUID part) and we want to >>>>>>> keep Wine's old behavior in this regards as SDL 2.0.14+ looks for uppercase IG_ >>>>>>> to match xinput devices to their RawInput counterparts. >>>>>>> >>>>>>> Signed-off-by: Arkadiusz Hiler >>>>>>> --- >>>>>>> dlls/user32/rawinput.c | 5 ++++- >>>>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c >>>>>>> index ba11a121bc5..13aac0278d5 100644 >>>>>>> --- a/dlls/user32/rawinput.c >>>>>>> +++ b/dlls/user32/rawinput.c >>>>>>> @@ -94,7 +94,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) >>>>>>> SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail; >>>>>>> struct device *device; >>>>>>> HANDLE file; >>>>>>> - WCHAR *path; >>>>>>> + WCHAR *path, *pos; >>>>>>> DWORD size; >>>>>>> SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL); >>>>>>> @@ -121,6 +121,9 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) >>>>>>> } >>>>>>> heap_free(detail); >>>>>>> + /* upper case everything but the GUID */ >>>>>>> + for (pos = path; *pos && *pos != '{'; pos++) *pos = towupper(*pos); >>>>>>> + >>>>>>> file = CreateFileW(path, GENERIC_READ | GENERIC_WRITE, >>>>>>> FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0); >>>>>>> if (file == INVALID_HANDLE_VALUE) >>>>>>> >>>>>> >>>>>> Hi Arek! >>>>>> >>>>>> I'd think a test to validate this could be nice. I know we don't have any >>>>>> gamepad device available on the test bot, but maybe it's still possible to >>>>>> validate at least with with mouse / keyboard devices? It would also let us >>>>>> check the test manually on a real Windows with a gamepad plugged in. >>>>> >>>>> Hey, >>>>> >>>>> Thanks for the feedback. >>>>> >>>>> Looking at this a bit closer there seems to be more going on with the >>>>> paths on Windows: >>>>> >>>>> rawinput: \\?\HID#VID_054C&PID_0CE6&MI_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} >>>>> setupapi: \\?\hid#vid_054c&pid_0ce6&mi_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} >>>>> instance_id: HID\VID_054C&PID_0CE6&MI_03\7&207751A1&0&0000 >>>>> >>>>> or >>>>> >>>>> rawinput: \\?\HID#VID_046D&PID_C52B&MI_02&Col01#7&25fe7032&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} >>>>> setupapi: \\?\hid#vid_046d&pid_c52b&mi_01&col01#7&392e8c70&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} >>>>> instance_id: HID\VID_046D&PID_C52B&MI_01&COL01\7&392E8C70&0&0000 >>>>> >>>>> It's not even uppercasing up to second '#'. It looks like there's >>>>> another source of path/instance_id that is case sensitive (Col01 vs >>>>> COL01). So having a test that checks if everything is uppercase would >>>>> fail on Windows. >>>>> >>>>> I would treat the patch in current form more as "keeping the old >>>>> behavior that we know some software depends on[0]" instead of "fixing >>>>> this once and for all". >>>>> >>>>> [0]: https://github.com/libsdl-org/SDL/blob/fadfa5/src/joystick/windows/SDL_rawinputjoystick.c#L696 >>>>> >>>> >>>> Alright, then maybe not checking the whole string but at least a test >>>> reflecting the SDL assumption, with a comment pointing to it? >>> >>> CCed zf as she may have something to say. >>> >>> I may take a stab at a proper fix first though. >>> >>> 1. Both CM_Get_DeviceID() and SetupDiGetDeviceInstanceId() return strupr() version. >>> Looks like we handle this correctly. >> >> I assume you mean strlwr()? > > SetupAPI's InstanceId is strupr() and DevicePath is strlwr(). Eh, I wasn't paying attention to which function you were talking about. > >>> 2. RawInput seems to be using the values verbatim from the registry as they are in >>> HLM/System/CurrentControlSet/Enum and HLM/System/CurrentControlSet/Control/DeviceClasses >>> >>> Looks like we need to implement opening relevant keys and using >>> pathing from there in rawinput.c (NtQueryKey()?) >>> >>> We also need to add tests to SetupAPI to make sure that the casing is >>> preserved in the registy entries. >>> >> >> Yeah, this is what I noticed when I tested. If you manually change the >> casing with regedit (and log out and back in), user32 will even reflect >> that. >> >> That said, my take is it's probably not worth reimplementing (admittedly not >> that much of) setupapi just for this, at least, not if we can just uppercase >> the whole string and have done. > > I was thinking about doing something along the lines of: > > SetupDiGetDeviceInstanceId(instance_id); > instance_key = RegOpenKey(enum_key, instance_id); > NtQueryKey(instance_key, KeyNameInformation, cased_instance_id); > > /* \Registry\Machine\System\CurrentControlSet\Enum\HID\VID_0000&PID_0000\0&WINEMOUSE&0&0 */ > > Then we can take everything starting with HID and overwrite a part of the > device path, replacing '\' with '#'. > > If you think that uppercasing (either part of or the whole) device path > in user32/rawinput is good enough for now I am more than fine with > that. > I think it'd be fine to uppercase the whole path in user32, personally. -----BEGIN PGP SIGNATURE----- wsB5BAABCAAjFiEENTo75Twe9rbPART3DZ01igeheEAFAmA3yCcFAwAAAAAACgkQDZ01igeheEDA JAf+OhvJyqN8Ok7RNQDAJPVbYVipxnMvCZION565ybkJGlT9a/LPSbRQ/8uvPgegZBWDDARctS+/ NHypJQSvLBQHatZBAhCEThsbBjb9tRn6hot0gDCMl1IAoJkhzBSBpIwrC5nBkkg7MrE3y/CjQFgG YZy8u3BinO1fSY9bX91Y+O28GOqz7i9xv7k3qFsrJO5XB+V1Fkkp2I3DwJpK8Kj+EOyD6lPGMtGG qFYmpNAiWk9vAwHf8RKyeFLq1JBvo5azVAFQTlNQb0cg4GikF+4Fow62s15RUZzbTI7igf+3RBrp lloD0aoQ/Ezv+/Ezzy6qYgGMD4Bj9bDHKD/XZvWGiw== =Vjz4 -----END PGP SIGNATURE-----