Re: [PATCH (resend) 3/8] winebus.sys: Implement removing IOHID devices

[Ken Thomases]

On Nov 3, 2016, at 7:15 AM, Aric Stewart <aric(a)codeweavers.com> wrote:
> Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> ---
> dlls/winebus.sys/bus_iohid.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/winebus.sys/bus_iohid.c b/dlls/winebus.sys/bus_iohid.c
> index f3e282c..c25a3b8 100644
> --- a/dlls/winebus.sys/bus_iohid.c
> +++ b/dlls/winebus.sys/bus_iohid.c
> @@ -120,7 +120,9 @@ static DWORD CFNumberToDWORD(CFNumberRef num)
>
> static int compare_platform_device(DEVICE_OBJECT *device, void *platform_dev)
>  {
> -    return 0;
> +    IOHIDDeviceRef dev1 = *(IOHIDDeviceRef*)get_platform_private(device);
> +    IOHIDDeviceRef dev2 = (IOHIDDeviceRef)platform_dev;
> +    return dev1 == dev2;

The return value here has the reverse sense from how the caller interprets it.  The caller, bus_find_hid_device(), interprets it like the result from strcmp() or memcmp().  A value of 0 means equal.  Here, you're returning true (1) when the devices are equal.
> }

[PATCH (resend) 3/8] winebus.sys: Implement removing IOHID devices

[Ken Thomases]

On Nov 4, 2016, at 12:26 AM, Ken Thomases <ken(a)codeweavers.com> wrote:
> On Nov 3, 2016, at 7:15 AM, Aric Stewart <aric(a)codeweavers.com> wrote:
> > static int compare_platform_device(DEVICE_OBJECT *device, void *platform_dev)
> > {
> > -    return 0;
> > +    IOHIDDeviceRef dev1 = *(IOHIDDeviceRef*)get_platform_private(device);
> > +    IOHIDDeviceRef dev2 = (IOHIDDeviceRef)platform_dev;
> > +    return dev1 == dev2;
>
> The return value here has the reverse sense from how the caller interprets it.  The caller, bus_find_hid_device(), interprets it like the result from strcmp() or memcmp().  A value of 0 means equal.  Here, you're returning true (1) when the devices are equal.

By the way, I would recommend that you change this vtable entry to be equal_platform_device (or same_platform_device or matches_platform_device) and make its return type a boolean which is true when the devices are equal and false otherwise.

You can't really do the strcmp/memcmp scheme where < 0 means less than and > 0 means greater than.  Put another way, in the current scheme there's no clear value to return for "not equal" when the bus can't support an ordering of devices.

-Ken