[PATCH v7] winebus.sys: Build initial hidraw device set

Aric Stewart aric at codeweavers.com
Mon Sep 12 08:16:10 CDT 2016



On 9/12/16 8:08 AM, Sebastian Lackner wrote:
> On 12.09.2016 14:41, Aric Stewart wrote:
>>>>
>>>> Does this work properly when elements are deleted? You might return the same index
>>>> for different devices.
>> As far as I understand, this index does not need to be unique to a device, just unique for in given device set of active devices, so this should be ok.
> 
> I don't think its guaranteed to be unique at all. Lets assume we have three
> devices with the same vidpid (index 1, 2, 3). The second one gets removed
> (index 1, 3) and a fourth one gets added. It will then attempt to use index
> 3 which is still in use. Is that not a problem?
> 

Yes, my logic is clearly flawed. I will re-works it, thanks for noticing!

>>>> At least for udev the device object has to be released. It looks like the logic
>>>> is currently missing (also in your draft patches). In order to fix that it will
>>>> probably be necessary to store additional information in each device object.
>>>>
>> That is coming in a later patch where I handle device removals. You are right that part of that will need to be moved here for the error case.
> 
> If it was part of your draft series I must have missed it. How exactly are you
> planning to implement it? Does it make sense to store a release function as part
> of each device, or maybe even a Vtable?
> 

Yeah, I was avoiding vtables as much as possible. You can see it in patch 126373. But clearly my error handling here needs work which I am giving it.

>>>> How does this correspond to similar code in hidclass.sys/device.c. Do we really need both? Why does
>>>> hidclass contain a call to SetupDiCreateDeviceInterfaceW but not this version?
>>>>
>> These part of registry manipulations are done on windows via device installation. So setupapi is used to make these keys and then they are just expected to be there for installed devices. Since we are trying to handle native devices that have not been installed in this method we have to add the proper registry information more dynamically. That both areas have to do similar things that is why the code is similar.
>>
>> These bus devices do not have any Interfaces, that is why we have no call to SetupDiCreateDeviceInterfaceW.
> 
> Okay, thanks for the explanation. I still find it a bit odd when we have all
> this code with a similar purpose at different places though, especially when
> it looks very different. The hidclass version for example will leak the devinfo
> handle on various error paths and returns ERROR_* codes instead of NTSTATUS
> values. This version here might have other issues (at least missing error
> handling for some of the functions). If the code duplication cannot be avoided,
> it might at least be useful to keep the code a bit more synchronized to these
> other versions (after fixing them, of course).
> 

I will work on the error handling in this chunk of code. Patches to fix up the hidclass stuff are welcome! :) I will try to circle back to that also since you have identified issues. 

I have not found a good way to avoid this duplication. There are functions like DiInstallDriver or UpdateDriverForPlugAndPlayDevices, but they expect an INF file with the driver information specified. I have not found a clean way to just do these step programmatically in the windows API. 

-aric

>>
>>
>> I will work on integrating all the other changes. thanks!
>>
>> -aric
>>
> 
> Regards,
> Sebastian
> 



More information about the wine-devel mailing list