Re: [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
>
> 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)
>  {