[PATCH resend 5/7] winebus.sys: reports for iohid

[Sebastian Lackner]

On 04.11.2016 13:46, Aric Stewart wrote:
> Signed-off-by: Aric Stewart <aric at codeweavers.com>
> ---
>  dlls/winebus.sys/bus_iohid.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> 
> 
> v2-0005-winebus.sys-reports-for-iohid.txt
> 
> 
> diff --git a/dlls/winebus.sys/bus_iohid.c b/dlls/winebus.sys/bus_iohid.c
> index e1a97ef..7208117 100644
> --- a/dlls/winebus.sys/bus_iohid.c
> +++ b/dlls/winebus.sys/bus_iohid.c
> @@ -119,6 +119,13 @@ static DWORD CFNumberToDWORD(CFNumberRef num)
>      return dwNum;
>  }
>  
> +static void handle_IOHIDDeviceIOHIDReportCallback(void * inContext,
> +        IOReturn inResult, void * inSender, IOHIDReportType inType,
> +        uint32_t inReportID, uint8_t * inReport, CFIndex inReportLength)

I would suggest to use parameter names without "in" prefix. Also, there usually
shouldn't be any space between "*" and the following variable name.

> +{

It might be better to store the DEVICE_OBJECT in a local variable before passing
it to the next function, to ensure the types are checked correctly. Otherwise
it remains unnoticed when the signature of process_hid_report changes in the future.

> +    process_hid_report(inContext, inReport, inReportLength);
> +}
> +
>  static int compare_platform_device(DEVICE_OBJECT *device, void *platform_dev)
>  {
>      ULONG_PTR dev1 = (ULONG_PTR)*(IOHIDDeviceRef*)get_platform_private(device);
> @@ -179,7 +186,17 @@ static NTSTATUS get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DW
>  
>  static NTSTATUS begin_report_processing(DEVICE_OBJECT *device)
>  {
> -    return STATUS_NOT_IMPLEMENTED;
> +    DWORD length;
> +    uint8_t *buffer;
> +    IOHIDDeviceRef dev = *(IOHIDDeviceRef*)get_platform_private(device);
> +    CFNumberRef num;

To be compatible with the rest of the code, this should only start processing
once. You might have to store the state of report processing in the platform
struct.

> +
> +    num = IOHIDDeviceGetProperty(dev, CFSTR(kIOHIDMaxInputReportSizeKey));
> +    length = CFNumberToDWORD(num);
> +    buffer = HeapAlloc(GetProcessHeap(), 0, length);

You might want to do some error checking here.

> +
> +    IOHIDDeviceRegisterInputReportCallback(dev, buffer, length, handle_IOHIDDeviceIOHIDReportCallback, device);
> +    return STATUS_SUCCESS;
>  }
>  
>  static NTSTATUS set_output_report(DEVICE_OBJECT *device, UCHAR id, BYTE *report, DWORD length, ULONG_PTR *written)
> @@ -244,6 +261,9 @@ static void handle_RemovalCallback(void *inContext, IOReturn inResult, void *inS
>  {
>      DEVICE_OBJECT *device;
>      TRACE("OS/X IOHID Device Removed %p\n", inIOHIDDeviceRef);
> +    IOHIDDeviceRegisterInputReportCallback(inIOHIDDeviceRef, NULL, 0, NULL, NULL);
> +    /* Note: Yes, we leak the buffer. But according to research there is no
> +             safe way to deallocate that buffer. */

Is it a problem when IOHIDDeviceRegisterInputReportCallback is called, although
report processing was never started? And why isn't it safe to deallocate the buffer
here? I don't think you will get additional reports when the device has been
removed.

>      device = bus_find_hid_device(&iohid_vtbl, inIOHIDDeviceRef);
>      if (device)
>      {
> 
> 
>