[PATCH 5/6] xinput.sys: Create an internal PDO, on the XINPUT bus.

[Rémi Bernon]

On 8/30/21 6:22 PM, Zebediah Figura (she/her) wrote:
> On 8/27/21 3:55 PM, Rémi Bernon wrote:
>> On 8/27/21 10:31 PM, Zebediah Figura (she/her) wrote:
>>>> wcsrchr(device_id, '\\') + 1);
>>>>        wcscpy(ext->instance_id, instance_id);
>>>> +    ext->interface_guid = !wcsncmp(device_id, L"XINPUT\\", 7) ?
>>>> &GUID_DEVINTERFACE_XINPUT
>>>> +                                                              :
>>>> &GUID_DEVINTERFACE_HID;
>>>
>>> It's a matter of taste, I guess, but personally if I have to wrap a
>>> ternary operator, I take it as a sign I should be using if/else 
>>> instead ;-)
>>>
>>
>> Yeah, I spent a lot of time hesitating on the best looking way to write
>> that.
>>
>>> It also occurs to me that since this is very internal it might be a good
>>> idea to use WINEXINPUT or something rather than just XINPUT. Unlikely to
>>> make a difference, of course, but it also sort of communicates that this
>>> is internal, and more clearly tells anyone looking for the code where to
>>> look for the driver that reports WINEXINPUT devices (viz. elsewhere in
>>> the code tree.)
>>>
>>
>> Works for me, I'm still not convinced that winexinput.sys is much
>> clearer though, but I can probably live with it.
> 
> I'm fine with xinput.sys, but I'd rather see the device ID named 
> WINEXINPUT.
> 

That's what I did in the new version, although the driver is also named 
winexinput.sys now. I think it'd be less confusing that way.

>>
>>>> @@ -52,14 +54,24 @@ struct device_state
>>>>    {
>>>>        DEVICE_OBJECT *bus_pdo;
>>>>        DEVICE_OBJECT *gamepad_pdo;
>>>> +    DEVICE_OBJECT *xinput_pdo;
>>>>        WCHAR bus_id[MAX_DEVICE_ID_LEN];
>>>>        WCHAR instance_id[MAX_DEVICE_ID_LEN];
>>>> +
>>>> +    /* everything below requires holding the cs */
>>>> +    CRITICAL_SECTION cs;
>>>> +    IRP *pending_read;
>>>> +    BOOL pending_is_gamepad;
>>>
>>> Honestly after thinking about it I don't dislike "internal" as much I
>>> dislike "xinput" now. Because you'd think that "xinput" refers to the
>>> device we access from xinput, but actually it's the other way around.
>>
>> It's still the case there, but eventually (ie: in an eventual PATCH 7/6
>> where xinput is switched to the internal device interface class), this
>> is how it will indeed be. So to me it's actually intuitive.
> 
> Well, right, I'm talking about the eventual situation.
> 
> And, I hate to say it, but "to me it's actually intuitive" isn't great :-(
> 
>>
>> In any case, I don't think the driver is supposed to become more
>> complicated than these patches (it just misses the report translation
>> and that's it), it should be quite straightforward to get the whole
>> picture. With a few comments maybe?
> 
> I think a comment near the top is definitely going to be necessary 
> regardless of what terminology we eventually use.
> 
> My logic with "real"/"fake" is that the "real" device reports the "real" 
> data that we get from winebus; the "fake" device fixes it up and hides 
> some of it. But yeah, it's clearly not unambiguous either. The best 
> thing it has going for it is that they're clearly attributes of the 
> child devices rather than namespace prefixes or referring to internal 
> IOCTLs or anything else.
> 

In the new version I kept the names "gamepad_device" -for the bogus HID 
gamepad- with the "IG_" (as in I-something G-amepad) suffix, and 
"xinput_device" which will have an "XI_" (as in X-I-nput) suffix to 
distinguish them. Both will have the WINEXINPUT\ bus namespace.

I think it's the best I can come up with that doesn't sound like 
something too generic (fake/real is completely generic IMHO and could 
mean anything depending on the reader current understanding).

Cheers,
-- 
Rémi Bernon <rbernon at codeweavers.com>