On 12.10.2016 12:12, Aric Stewart wrote:
Signed-off-by: Aric Stewart aric@codeweavers.com
dlls/winebus.sys/bus.h | 1 + dlls/winebus.sys/bus_udev.c | 70 +++++++++++++++++++++++++++++++++++++++++++++ dlls/winebus.sys/main.c | 11 +++++++ 3 files changed, 82 insertions(+)
0001-winebus.sys-Implement-IOCTL_HID_GET_STRING-for-hidraw.txt
diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h index 9ab242c..9094e17 100644 --- a/dlls/winebus.sys/bus.h +++ b/dlls/winebus.sys/bus.h @@ -24,6 +24,7 @@ typedef struct { int (*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev); NTSTATUS (*get_reportdescriptor)(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length);
- NTSTATUS (*get_string)(DEVICE_OBJECT *device, int index, WCHAR *buffer, DWORD length);
} platform_vtbl;
void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c index 22ea07d..4ded568 100644 --- a/dlls/winebus.sys/bus_udev.c +++ b/dlls/winebus.sys/bus_udev.c @@ -51,7 +51,9 @@ #include "winnls.h" #include "winternl.h" #include "ddk/wdm.h" +#include "ddk/hidtypes.h" #include "wine/debug.h" +#include "wine/unicode.h"
#include "bus.h"
@@ -144,10 +146,78 @@ static NTSTATUS hidraw_get_reportdescriptor(DEVICE_OBJECT *device, BYTE *buffer, #endif }
+static NTSTATUS hidraw_get_string(DEVICE_OBJECT *device, int index, WCHAR *buffer, DWORD length) +{
- struct udev_device *usbdev;
- struct platform_private *private = impl_from_DEVICE_OBJECT(device);
- usbdev = udev_device_get_parent_with_subsystem_devtype(private->udev_device, "usb", "usb_device");
- if (usbdev)
- {
WCHAR *str = NULL;switch (index){case HID_STRING_ID_IPRODUCT:str = get_sysattr_string(usbdev, "product");break;case HID_STRING_ID_IMANUFACTURER:str = get_sysattr_string(usbdev, "manufacturer");break;case HID_STRING_ID_ISERIALNUMBER:str = get_sysattr_string(usbdev, "serial");break;default:ERR("Unhandled string index %i\n", index);return STATUS_NOT_IMPLEMENTED;}if (str){if (strlenW(str) >= length){HeapFree(GetProcessHeap(), 0, str);return STATUS_BUFFER_TOO_SMALL;}strcpyW(buffer, str);HeapFree(GetProcessHeap(), 0, str);return STATUS_SUCCESS;}else if (length)buffer[0] = 0;return STATUS_SUCCESS;
Shouldn't it still return STATUS_BUFFER_TOO_SMALL when length == 0?
- }
- else
- {
+#ifdef HAVE_LINUX_HIDRAW_H
static char str[MAX_PATH] = {0};
Isn't there a risk of race-conditions that multiple HID devices attempt to use this static buffer at the same time?
switch (index){case HID_STRING_ID_IPRODUCT:ioctl(private->device_fd, HIDIOCGRAWNAME(MAX_PATH), &str);break;case HID_STRING_ID_IMANUFACTURER:break;case HID_STRING_ID_ISERIALNUMBER:break;default:ERR("Unhandled string index %i\n", index);return STATUS_NOT_IMPLEMENTED;}+#endif
if (str[0])
If HAVE_LINUX_HIDRAW_H is not available this will break compilation (str undeclared).
MultiByteToWideChar(CP_ACP, 0, str, -1, buffer, length);else if (length)buffer[0] = 0;
It would be better to do length checking also in this case.
return STATUS_SUCCESS;- }
+}
static const platform_vtbl hidraw_vtbl = { compare_platform_device, hidraw_get_reportdescriptor,
- hidraw_get_string,
};
static void try_add_device(struct udev_device *dev) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index ad09ec1..a7b76df 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -421,6 +421,17 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) irp->IoStatus.Information = length; break; }
case IOCTL_HID_GET_STRING:{DWORD index = (DWORD)irpsp->Parameters.DeviceIoControl.Type3InputBuffer;
This will trigger a compilation warning on 64-bit. You have to cast to a ptr-size integer type first, for example:
DWORD index = (ULONG_PTR)irpsp->Parameters.DeviceIoControl.Type3InputBuffer;
Also, accoding to MSDN, the content of the variable is a composite value:
"""Parameters.DeviceIoControl.Type3InputBuffer in the I/O stack location of the IRP contains a composite value. The two most significant bytes contain the language ID of the string to be retrieved. The two least significant bytes contain one of the following three constant values..."""
TRACE("IOCTL_HID_GET_STRING[%i]\n", index);irp->IoStatus.u.Status = status = ext->vtbl->get_string(device,index, (WCHAR*)(irp->UserBuffer),irpsp->Parameters.DeviceIoControl.OutputBufferLength/sizeof(WCHAR));if (status == STATUS_SUCCESS)irp->IoStatus.Information = (strlenW((WCHAR*)(irp->UserBuffer)) + 1) * sizeof(WCHAR);break;} default: { ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode;
On 10/12/16 4:02 PM, Sebastian Lackner wrote:
On 12.10.2016 12:12, Aric Stewart wrote:
Signed-off-by: Aric Stewart aric@codeweavers.com
dlls/winebus.sys/bus.h | 1 + dlls/winebus.sys/bus_udev.c | 70 +++++++++++++++++++++++++++++++++++++++++++++ dlls/winebus.sys/main.c | 11 +++++++ 3 files changed, 82 insertions(+)
0001-winebus.sys-Implement-IOCTL_HID_GET_STRING-for-hidraw.txt
diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h index 9ab242c..9094e17 100644 --- a/dlls/winebus.sys/bus.h +++ b/dlls/winebus.sys/bus.h @@ -24,6 +24,7 @@ typedef struct { int (*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev); NTSTATUS (*get_reportdescriptor)(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length);
- NTSTATUS (*get_string)(DEVICE_OBJECT *device, int index, WCHAR *buffer, DWORD length);
} platform_vtbl;
void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c index 22ea07d..4ded568 100644 --- a/dlls/winebus.sys/bus_udev.c +++ b/dlls/winebus.sys/bus_udev.c @@ -51,7 +51,9 @@ #include "winnls.h" #include "winternl.h" #include "ddk/wdm.h" +#include "ddk/hidtypes.h" #include "wine/debug.h" +#include "wine/unicode.h"
#include "bus.h"
@@ -144,10 +146,78 @@ static NTSTATUS hidraw_get_reportdescriptor(DEVICE_OBJECT *device, BYTE *buffer, #endif }
+static NTSTATUS hidraw_get_string(DEVICE_OBJECT *device, int index, WCHAR *buffer, DWORD length) +{
- struct udev_device *usbdev;
- struct platform_private *private = impl_from_DEVICE_OBJECT(device);
- usbdev = udev_device_get_parent_with_subsystem_devtype(private->udev_device, "usb", "usb_device");
- if (usbdev)
- {
WCHAR *str = NULL;switch (index){case HID_STRING_ID_IPRODUCT:str = get_sysattr_string(usbdev, "product");break;case HID_STRING_ID_IMANUFACTURER:str = get_sysattr_string(usbdev, "manufacturer");break;case HID_STRING_ID_ISERIALNUMBER:str = get_sysattr_string(usbdev, "serial");break;default:ERR("Unhandled string index %i\n", index);return STATUS_NOT_IMPLEMENTED;}if (str){if (strlenW(str) >= length){HeapFree(GetProcessHeap(), 0, str);return STATUS_BUFFER_TOO_SMALL;}strcpyW(buffer, str);HeapFree(GetProcessHeap(), 0, str);return STATUS_SUCCESS;}else if (length)buffer[0] = 0;return STATUS_SUCCESS;Shouldn't it still return STATUS_BUFFER_TOO_SMALL when length == 0?
- }
- else
- {
+#ifdef HAVE_LINUX_HIDRAW_H
static char str[MAX_PATH] = {0};Isn't there a risk of race-conditions that multiple HID devices attempt to use this static buffer at the same time?
switch (index){case HID_STRING_ID_IPRODUCT:ioctl(private->device_fd, HIDIOCGRAWNAME(MAX_PATH), &str);break;case HID_STRING_ID_IMANUFACTURER:break;case HID_STRING_ID_ISERIALNUMBER:break;default:ERR("Unhandled string index %i\n", index);return STATUS_NOT_IMPLEMENTED;}+#endif
if (str[0])If HAVE_LINUX_HIDRAW_H is not available this will break compilation (str undeclared).
MultiByteToWideChar(CP_ACP, 0, str, -1, buffer, length);else if (length)buffer[0] = 0;It would be better to do length checking also in this case.
return STATUS_SUCCESS;- }
+}
static const platform_vtbl hidraw_vtbl = { compare_platform_device, hidraw_get_reportdescriptor,
- hidraw_get_string,
};
static void try_add_device(struct udev_device *dev) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index ad09ec1..a7b76df 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -421,6 +421,17 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) irp->IoStatus.Information = length; break; }
case IOCTL_HID_GET_STRING:{DWORD index = (DWORD)irpsp->Parameters.DeviceIoControl.Type3InputBuffer;This will trigger a compilation warning on 64-bit. You have to cast to a ptr-size integer type first, for example:
DWORD index = (ULONG_PTR)irpsp->Parameters.DeviceIoControl.Type3InputBuffer;
Also, accoding to MSDN, the content of the variable is a composite value:
"""Parameters.DeviceIoControl.Type3InputBuffer in the I/O stack location of the IRP contains a composite value. The two most significant bytes contain the language ID of the string to be retrieved. The two least significant bytes contain one of the following three constant values..."""
True, However a language ID of 00 means vendor default. Which is what I am pretty sure we get out of the underlying calls. Anything of another language will be unimplemented and should return that code.
-aric
TRACE("IOCTL_HID_GET_STRING[%i]\n", index);irp->IoStatus.u.Status = status = ext->vtbl->get_string(device,index, (WCHAR*)(irp->UserBuffer),irpsp->Parameters.DeviceIoControl.OutputBufferLength/sizeof(WCHAR));if (status == STATUS_SUCCESS)irp->IoStatus.Information = (strlenW((WCHAR*)(irp->UserBuffer)) + 1) * sizeof(WCHAR);break;} default: { ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode;
On 12.10.2016 17:27, Aric Stewart wrote:
True, However a language ID of 00 means vendor default. Which is what I am pretty sure we get out of the underlying calls. Anything of another language will be unimplemented and should return that code.
If you want to handle it in the vtbl functions thats also fine, but the type should probably be unsigned then (otherwise it might be negative depending on what is stored in the upper word).
-
Aric Stewart -
Sebastian Lackner