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

[Ken Thomases]

On Nov 3, 2016, at 7:14 AM, Aric Stewart <aric(a)codeweavers.com> wrote:
> Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> ---
> dlls/winebus.sys/bus_iohid.c | 102 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 101 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/winebus.sys/bus_iohid.c b/dlls/winebus.sys/bus_iohid.c
> index 882b483..f3e282c 100644
> --- a/dlls/winebus.sys/bus_iohid.c
> +++ b/dlls/winebus.sys/bus_iohid.c
> @@ -98,14 +98,114 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
>  static DRIVER_OBJECT *iohid_driver_obj = NULL;
>  static IOHIDManagerRef hid_manager;
>
> +static const WCHAR busidW[] = {'I','O','H','I','D',0};
> +
> +#include "initguid.h"
> +DEFINE_GUID(GUID_DEVCLASS_IOHID, 0x989D309D,0x0470,0x4E1A,0x89,0x38,0x50,0x1F,0x42,0xBD,0x9A,0xCD);
> +
> +static void CFStringToWSTR(CFStringRef cstr, LPWSTR wstr, int length)
> +{
> +    int len = min(CFStringGetLength(cstr), length-1);
> +    CFStringGetCharacters(cstr, CFRangeMake(0, len), wstr);
> +    wstr[len] = 0;
> +}
> +
> +static DWORD CFNumberToDWORD(CFNumberRef num)
> +{
> +    DWORD dwNum = 0;
> +    if (num)
> +        CFNumberGetValue(num, kCFNumberIntType, &dwNum);

The local should be of type "int" to match kCFNumberIntType.
> +    return dwNum;
> +}
> +
> …
> +
> +static void handle_DeviceMatchingCallback(void *inContext, IOReturn inResult, void *inSender, IOHIDDeviceRef  inIOHIDDeviceRef)
> +{
> +    DEVICE_OBJECT *device;
> +    DWORD vid, pid, version;
> +    CFStringRef str = NULL;
> +    WCHAR serial_string[256];
> +    BOOL is_gamepad;
> +
> +    TRACE("OS/X IOHID Device Added %p\n", inIOHIDDeviceRef);
> +
> +    vid = CFNumberToDWORD(IOHIDDeviceGetProperty(inIOHIDDeviceRef, CFSTR(kIOHIDVendorIDKey)));
> +    pid = CFNumberToDWORD(IOHIDDeviceGetProperty(inIOHIDDeviceRef, CFSTR(kIOHIDProductIDKey)));
> +    version = CFNumberToDWORD(IOHIDDeviceGetProperty(inIOHIDDeviceRef, CFSTR(kIOHIDVersionNumberKey)));
> +    str = IOHIDDeviceGetProperty(inIOHIDDeviceRef, CFSTR(kIOHIDSerialNumberKey));
> +    if (str) CFStringToWSTR(str, serial_string, 256);

You should use sizeof(serial_string) / sizeof(WCHAR) instead of 256 here.
> +
> +    is_gamepad = (IOHIDDeviceConformsTo(inIOHIDDeviceRef, kHIDPage_GenericDesktop, kHIDUsage_GD_GamePad) ||
> +       IOHIDDeviceConformsTo(inIOHIDDeviceRef, kHIDPage_GenericDesktop, kHIDUsage_GD_Joystick));
> +
> +    device = bus_create_hid_device(iohid_driver_obj, busidW, vid, pid, version, 0, str?serial_string:NULL, is_gamepad, &GUID_DEVCLASS_IOHID, &iohid_vtbl, sizeof(inIOHIDDeviceRef));
> +    if (!device)
> +        ERR("Failed to create device\n");
> +    else
> +    {
> +        IOHIDDeviceRef* ext = get_platform_private(device);
> +        *ext = inIOHIDDeviceRef;
> +        IoInvalidateDeviceRelations(device, BusRelations);
> +    }
> +}
> +
>  /* This puts the relevent run loop for event handleing into a WINE thread */
>  static DWORD CALLBACK runloop_thread(VOID *args)
>  {
>      CFRunLoopRef run_loop;
>
> +    DRIVER_OBJECT *driver = (DRIVER_OBJECT*)args;
>      run_loop = CFRunLoopGetCurrent();

Minor style nit: the variable declarations would normally be grouped and separated from statements.  Of course, the declaration and setting of run_loop could be merged into one line (in the previous patch) to eliminate the distinction.
> IOHIDManagerSetDeviceMatching(hid_manager, NULL);
> +    IOHIDManagerRegisterDeviceMatchingCallback(hid_manager, handle_DeviceMatchingCallback, driver);
>      IOHIDManagerScheduleWithRunLoop(hid_manager, run_loop, kCFRunLoopDefaultMode);
>      if (IOHIDManagerOpen( hid_manager, 0 ) != kIOReturnSuccess)
>      {
> @@ -130,7 +230,7 @@ NTSTATUS WINAPI iohid_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registr
>      driver->MajorFunction[IRP_MJ_PNP] = common_pnp_dispatch;
>      driver->MajorFunction[IRP_MJ_INTERNAL_DEVICE_CONTROL] = hid_internal_dispatch;
>      hid_manager = IOHIDManagerCreate(kCFAllocatorDefault, 0L);
> -    if (!(run_loop_handle = CreateThread(NULL, 0, runloop_thread, NULL, 0, NULL)))
> +    if (!(run_loop_handle = CreateThread(NULL, 0, runloop_thread, driver, 0, NULL)))
>      {
>          ERR("Failed to initialize IOHID Manager thread\n");
>          iohid_driver_obj = NULL;