[PATCH 1/5] winebus.sys: Return STATUS_SUCCESS from IRP_MN_SURPRISE_REMOVAL.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 23195dfa75a..713db9c7f27 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -704,6 +704,7 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) remove_pending_irps(device); ext->removed = TRUE; LeaveCriticalSection(&ext->cs); + status = STATUS_SUCCESS; break; case IRP_MN_REMOVE_DEVICE: -- 2.32.0
Breaking out of the switch will leave it already. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 713db9c7f27..3d040b1af5b 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -927,7 +927,6 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) if (status != STATUS_SUCCESS) { irp->IoStatus.u.Status = status; - LeaveCriticalSection(&ext->cs); break; } @@ -946,7 +945,6 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) if (status != STATUS_SUCCESS) { irp->IoStatus.u.Status = status; - LeaveCriticalSection(&ext->cs); break; } if (!ext->last_report_read) -- 2.32.0
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 3d040b1af5b..0ae1f01c822 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -822,6 +822,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) NTSTATUS status = irp->IoStatus.u.Status; IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp); struct device_extension *ext = (struct device_extension *)device->DeviceExtension; + ULONG code, buffer_len = irpsp->Parameters.DeviceIoControl.OutputBufferLength; TRACE("(%p, %p)\n", device, irp); @@ -841,14 +842,14 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) return STATUS_DELETE_PENDING; } - switch (irpsp->Parameters.DeviceIoControl.IoControlCode) + switch ((code = irpsp->Parameters.DeviceIoControl.IoControlCode)) { case IOCTL_HID_GET_DEVICE_ATTRIBUTES: { HID_DEVICE_ATTRIBUTES *attr = (HID_DEVICE_ATTRIBUTES *)irp->UserBuffer; TRACE("IOCTL_HID_GET_DEVICE_ATTRIBUTES\n"); - if (irpsp->Parameters.DeviceIoControl.OutputBufferLength < sizeof(*attr)) + if (buffer_len < sizeof(*attr)) { irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL; break; @@ -870,7 +871,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DWORD length; TRACE("IOCTL_HID_GET_DEVICE_DESCRIPTOR\n"); - if (irpsp->Parameters.DeviceIoControl.OutputBufferLength < sizeof(*descriptor)) + if (buffer_len < sizeof(*descriptor)) { irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL; break; @@ -898,23 +899,18 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) break; } case IOCTL_HID_GET_REPORT_DESCRIPTOR: - { - DWORD length = irpsp->Parameters.DeviceIoControl.OutputBufferLength; TRACE("IOCTL_HID_GET_REPORT_DESCRIPTOR\n"); - - irp->IoStatus.u.Status = status = ext->vtbl->get_reportdescriptor(device, irp->UserBuffer, length, &length); - irp->IoStatus.Information = length; + irp->IoStatus.u.Status = status = ext->vtbl->get_reportdescriptor(device, irp->UserBuffer, buffer_len, &buffer_len); + irp->IoStatus.Information = buffer_len; break; - } case IOCTL_HID_GET_STRING: { - DWORD length = irpsp->Parameters.DeviceIoControl.OutputBufferLength / sizeof(WCHAR); DWORD index = (ULONG_PTR)irpsp->Parameters.DeviceIoControl.Type3InputBuffer; TRACE("IOCTL_HID_GET_STRING[%08x]\n", index); - irp->IoStatus.u.Status = status = hid_get_native_string(device, index, (WCHAR *)irp->UserBuffer, length); + irp->IoStatus.u.Status = status = hid_get_native_string(device, index, (WCHAR *)irp->UserBuffer, buffer_len / sizeof(WCHAR)); if (status != STATUS_SUCCESS) - irp->IoStatus.u.Status = status = ext->vtbl->get_string(device, index, (WCHAR *)irp->UserBuffer, length); + irp->IoStatus.u.Status = status = ext->vtbl->get_string(device, index, (WCHAR *)irp->UserBuffer, buffer_len / sizeof(WCHAR)); if (status == STATUS_SUCCESS) irp->IoStatus.Information = (strlenW((WCHAR *)irp->UserBuffer) + 1) * sizeof(WCHAR); break; @@ -950,8 +946,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) if (!ext->last_report_read) { irp->IoStatus.u.Status = status = deliver_last_report(ext, - irpsp->Parameters.DeviceIoControl.OutputBufferLength, - irp->UserBuffer, &irp->IoStatus.Information); + buffer_len, irp->UserBuffer, &irp->IoStatus.Information); ext->last_report_read = TRUE; } else @@ -991,12 +986,9 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) break; } default: - { - ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode; FIXME("Unsupported ioctl %x (device=%x access=%x func=%x method=%x)\n", code, code >> 16, (code >> 14) & 3, (code >> 2) & 0xfff, code & 3); break; - } } LeaveCriticalSection(&ext->cs); -- 2.32.0
Looks fine to me, but maybe "output_len" would be better? (Also strikes me as weird that "code" isn't initialized in the same place. Also, maybe the patch subject could reflect that we're using local variables for multiple ioctl parameters?)
On 8/12/21 7:19 PM, Zebediah Figura (she/her) wrote:
Looks fine to me, but maybe "output_len" would be better?
As the thing it's writing to is named "UserBuffer" I probably found that it was making sense. I don't really mind though.
(Also strikes me as weird that "code" isn't initialized in the same place. Also, maybe the patch subject could reflect that we're using local variables for multiple ioctl parameters?)
Yeah, it was really long with the declaration on the same line, and the inline switch initialization looked nice. I'll put them on two lines. -- Rémi Bernon <rbernon(a)codeweavers.com>
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/bus_iohid.c | 2 -- dlls/winebus.sys/bus_sdl.c | 2 -- dlls/winebus.sys/bus_udev.c | 2 -- dlls/winebus.sys/main.c | 67 +++++++++++++++++------------------- 4 files changed, 32 insertions(+), 41 deletions(-) diff --git a/dlls/winebus.sys/bus_iohid.c b/dlls/winebus.sys/bus_iohid.c index 0123e73f7ba..f660a3c789a 100644 --- a/dlls/winebus.sys/bus_iohid.c +++ b/dlls/winebus.sys/bus_iohid.c @@ -79,8 +79,6 @@ #undef PAGE_SHIFT #endif /* HAVE_IOKIT_HID_IOHIDLIB_H */ -#define NONAMELESSUNION - #include "ntstatus.h" #define WIN32_NO_STATUS #include "windef.h" diff --git a/dlls/winebus.sys/bus_sdl.c b/dlls/winebus.sys/bus_sdl.c index f3fcc20893e..1580ba5ef92 100644 --- a/dlls/winebus.sys/bus_sdl.c +++ b/dlls/winebus.sys/bus_sdl.c @@ -31,8 +31,6 @@ # include <SDL.h> #endif -#define NONAMELESSUNION - #include "ntstatus.h" #define WIN32_NO_STATUS #include "windef.h" diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c index 725c50e08fc..090df3b0529 100644 --- a/dlls/winebus.sys/bus_udev.c +++ b/dlls/winebus.sys/bus_udev.c @@ -54,8 +54,6 @@ # endif #endif -#define NONAMELESSUNION - #include "ntstatus.h" #define WIN32_NO_STATUS #include "windef.h" diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 0ae1f01c822..335a1528941 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -20,9 +20,6 @@ #include "config.h" #include <stdarg.h> -#define NONAMELESSUNION -#define NONAMELESSSTRUCT - #include "ntstatus.h" #define WIN32_NO_STATUS #include "winternl.h" @@ -242,8 +239,8 @@ static void remove_pending_irps(DEVICE_OBJECT *device) while ((entry = RemoveHeadList(&ext->irp_queue)) != &ext->irp_queue) { - IRP *queued_irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.s.ListEntry); - queued_irp->IoStatus.u.Status = STATUS_DELETE_PENDING; + IRP *queued_irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); + queued_irp->IoStatus.Status = STATUS_DELETE_PENDING; queued_irp->IoStatus.Information = 0; IoCompleteRequest(queued_irp, IO_NO_INCREMENT); } @@ -420,7 +417,7 @@ static NTSTATUS build_device_relations(DEVICE_RELATIONS **devices) static NTSTATUS handle_IRP_MN_QUERY_DEVICE_RELATIONS(IRP *irp) { - NTSTATUS status = irp->IoStatus.u.Status; + NTSTATUS status = irp->IoStatus.Status; IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation( irp ); TRACE("IRP_MN_QUERY_DEVICE_RELATIONS\n"); @@ -445,7 +442,7 @@ static NTSTATUS handle_IRP_MN_QUERY_DEVICE_RELATIONS(IRP *irp) static NTSTATUS handle_IRP_MN_QUERY_ID(DEVICE_OBJECT *device, IRP *irp) { - NTSTATUS status = irp->IoStatus.u.Status; + NTSTATUS status = irp->IoStatus.Status; IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation( irp ); BUS_QUERY_ID_TYPE type = irpsp->Parameters.QueryId.IdType; @@ -637,7 +634,7 @@ static NTSTATUS fdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) switch (irpsp->MinorFunction) { case IRP_MN_QUERY_DEVICE_RELATIONS: - irp->IoStatus.u.Status = handle_IRP_MN_QUERY_DEVICE_RELATIONS(irp); + irp->IoStatus.Status = handle_IRP_MN_QUERY_DEVICE_RELATIONS(irp); break; case IRP_MN_START_DEVICE: mouse_device_create(); @@ -647,23 +644,23 @@ static NTSTATUS fdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) { if (sdl_driver_init() == STATUS_SUCCESS) { - irp->IoStatus.u.Status = STATUS_SUCCESS; + irp->IoStatus.Status = STATUS_SUCCESS; break; } } udev_driver_init(); iohid_driver_init(); - irp->IoStatus.u.Status = STATUS_SUCCESS; + irp->IoStatus.Status = STATUS_SUCCESS; break; case IRP_MN_SURPRISE_REMOVAL: - irp->IoStatus.u.Status = STATUS_SUCCESS; + irp->IoStatus.Status = STATUS_SUCCESS; break; case IRP_MN_REMOVE_DEVICE: udev_driver_unload(); iohid_driver_unload(); sdl_driver_unload(); - irp->IoStatus.u.Status = STATUS_SUCCESS; + irp->IoStatus.Status = STATUS_SUCCESS; IoSkipCurrentIrpStackLocation(irp); ret = IoCallDriver(bus_pdo, irp); IoDetachDevice(bus_pdo); @@ -680,7 +677,7 @@ static NTSTATUS fdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) { struct device_extension *ext = device->DeviceExtension; - NTSTATUS status = irp->IoStatus.u.Status; + NTSTATUS status = irp->IoStatus.Status; IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp); TRACE("device %p, irp %p, minor function %#x.\n", device, irp, irpsp->MinorFunction); @@ -721,7 +718,7 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) HeapFree(GetProcessHeap(), 0, ext->serial); HeapFree(GetProcessHeap(), 0, ext->last_report); - irp->IoStatus.u.Status = STATUS_SUCCESS; + irp->IoStatus.Status = STATUS_SUCCESS; IoCompleteRequest(irp, IO_NO_INCREMENT); IoDeleteDevice(device); @@ -740,7 +737,7 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) break; } - irp->IoStatus.u.Status = status; + irp->IoStatus.Status = status; IoCompleteRequest(irp, IO_NO_INCREMENT); return status; } @@ -819,7 +816,7 @@ static NTSTATUS hid_get_native_string(DEVICE_OBJECT *device, DWORD index, WCHAR static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) { - NTSTATUS status = irp->IoStatus.u.Status; + NTSTATUS status = irp->IoStatus.Status; IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp); struct device_extension *ext = (struct device_extension *)device->DeviceExtension; ULONG code, buffer_len = irpsp->Parameters.DeviceIoControl.OutputBufferLength; @@ -837,7 +834,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) if (ext->removed) { LeaveCriticalSection(&ext->cs); - irp->IoStatus.u.Status = STATUS_DELETE_PENDING; + irp->IoStatus.Status = STATUS_DELETE_PENDING; IoCompleteRequest(irp, IO_NO_INCREMENT); return STATUS_DELETE_PENDING; } @@ -851,7 +848,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) if (buffer_len < sizeof(*attr)) { - irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL; + irp->IoStatus.Status = status = STATUS_BUFFER_TOO_SMALL; break; } @@ -861,7 +858,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) attr->ProductID = ext->pid; attr->VersionNumber = ext->version; - irp->IoStatus.u.Status = status = STATUS_SUCCESS; + irp->IoStatus.Status = status = STATUS_SUCCESS; irp->IoStatus.Information = sizeof(*attr); break; } @@ -873,7 +870,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) if (buffer_len < sizeof(*descriptor)) { - irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL; + irp->IoStatus.Status = status = STATUS_BUFFER_TOO_SMALL; break; } @@ -881,7 +878,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) if (status != STATUS_SUCCESS && status != STATUS_BUFFER_TOO_SMALL) { WARN("Failed to get platform report descriptor length\n"); - irp->IoStatus.u.Status = status; + irp->IoStatus.Status = status; break; } @@ -894,13 +891,13 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) descriptor->DescriptorList[0].bReportType = HID_REPORT_DESCRIPTOR_TYPE; descriptor->DescriptorList[0].wReportLength = length; - irp->IoStatus.u.Status = status = STATUS_SUCCESS; + irp->IoStatus.Status = status = STATUS_SUCCESS; irp->IoStatus.Information = sizeof(*descriptor); break; } case IOCTL_HID_GET_REPORT_DESCRIPTOR: TRACE("IOCTL_HID_GET_REPORT_DESCRIPTOR\n"); - irp->IoStatus.u.Status = status = ext->vtbl->get_reportdescriptor(device, irp->UserBuffer, buffer_len, &buffer_len); + irp->IoStatus.Status = status = ext->vtbl->get_reportdescriptor(device, irp->UserBuffer, buffer_len, &buffer_len); irp->IoStatus.Information = buffer_len; break; case IOCTL_HID_GET_STRING: @@ -908,9 +905,9 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DWORD index = (ULONG_PTR)irpsp->Parameters.DeviceIoControl.Type3InputBuffer; TRACE("IOCTL_HID_GET_STRING[%08x]\n", index); - irp->IoStatus.u.Status = status = hid_get_native_string(device, index, (WCHAR *)irp->UserBuffer, buffer_len / sizeof(WCHAR)); + irp->IoStatus.Status = status = hid_get_native_string(device, index, (WCHAR *)irp->UserBuffer, buffer_len / sizeof(WCHAR)); if (status != STATUS_SUCCESS) - irp->IoStatus.u.Status = status = ext->vtbl->get_string(device, index, (WCHAR *)irp->UserBuffer, buffer_len / sizeof(WCHAR)); + irp->IoStatus.Status = status = ext->vtbl->get_string(device, index, (WCHAR *)irp->UserBuffer, buffer_len / sizeof(WCHAR)); if (status == STATUS_SUCCESS) irp->IoStatus.Information = (strlenW((WCHAR *)irp->UserBuffer) + 1) * sizeof(WCHAR); break; @@ -922,11 +919,11 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) status = ext->vtbl->begin_report_processing(device); if (status != STATUS_SUCCESS) { - irp->IoStatus.u.Status = status; + irp->IoStatus.Status = status; break; } - irp->IoStatus.u.Status = status = deliver_last_report(ext, + irp->IoStatus.Status = status = deliver_last_report(ext, packet->reportBufferLen, packet->reportBuffer, &irp->IoStatus.Information); @@ -940,18 +937,18 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) status = ext->vtbl->begin_report_processing(device); if (status != STATUS_SUCCESS) { - irp->IoStatus.u.Status = status; + irp->IoStatus.Status = status; break; } if (!ext->last_report_read) { - irp->IoStatus.u.Status = status = deliver_last_report(ext, + irp->IoStatus.Status = status = deliver_last_report(ext, buffer_len, irp->UserBuffer, &irp->IoStatus.Information); ext->last_report_read = TRUE; } else { - InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.s.ListEntry); + InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry); status = STATUS_PENDING; } break; @@ -961,7 +958,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) { HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); TRACE_(hid_report)("IOCTL_HID_WRITE_REPORT / IOCTL_HID_SET_OUTPUT_REPORT\n"); - irp->IoStatus.u.Status = status = ext->vtbl->set_output_report( + irp->IoStatus.Status = status = ext->vtbl->set_output_report( device, packet->reportId, packet->reportBuffer, packet->reportBufferLen, &irp->IoStatus.Information); break; @@ -970,7 +967,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) { HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); TRACE_(hid_report)("IOCTL_HID_GET_FEATURE\n"); - irp->IoStatus.u.Status = status = ext->vtbl->get_feature_report( + irp->IoStatus.Status = status = ext->vtbl->get_feature_report( device, packet->reportId, packet->reportBuffer, packet->reportBufferLen, &irp->IoStatus.Information); packet->reportBufferLen = irp->IoStatus.Information; @@ -980,7 +977,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) { HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); TRACE_(hid_report)("IOCTL_HID_SET_FEATURE\n"); - irp->IoStatus.u.Status = status = ext->vtbl->set_feature_report( + irp->IoStatus.Status = status = ext->vtbl->set_feature_report( device, packet->reportId, packet->reportBuffer, packet->reportBufferLen, &irp->IoStatus.Information); break; @@ -1034,9 +1031,9 @@ void process_hid_report(DEVICE_OBJECT *device, BYTE *report, DWORD length) { IO_STACK_LOCATION *irpsp; TRACE_(hid_report)("Processing Request\n"); - irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.s.ListEntry); + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); irpsp = IoGetCurrentIrpStackLocation(irp); - irp->IoStatus.u.Status = deliver_last_report(ext, + irp->IoStatus.Status = deliver_last_report(ext, irpsp->Parameters.DeviceIoControl.OutputBufferLength, irp->UserBuffer, &irp->IoStatus.Information); ext->last_report_read = TRUE; -- 2.32.0
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 61 ++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 35 deletions(-) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 335a1528941..6ceccd21442 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -816,10 +816,10 @@ static NTSTATUS hid_get_native_string(DEVICE_OBJECT *device, DWORD index, WCHAR static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) { - NTSTATUS status = irp->IoStatus.Status; IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp); struct device_extension *ext = (struct device_extension *)device->DeviceExtension; ULONG code, buffer_len = irpsp->Parameters.DeviceIoControl.OutputBufferLength; + NTSTATUS status; TRACE("(%p, %p)\n", device, irp); @@ -848,7 +848,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) if (buffer_len < sizeof(*attr)) { - irp->IoStatus.Status = status = STATUS_BUFFER_TOO_SMALL; + irp->IoStatus.Status = STATUS_BUFFER_TOO_SMALL; break; } @@ -858,7 +858,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) attr->ProductID = ext->pid; attr->VersionNumber = ext->version; - irp->IoStatus.Status = status = STATUS_SUCCESS; + irp->IoStatus.Status = STATUS_SUCCESS; irp->IoStatus.Information = sizeof(*attr); break; } @@ -870,15 +870,15 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) if (buffer_len < sizeof(*descriptor)) { - irp->IoStatus.Status = status = STATUS_BUFFER_TOO_SMALL; + irp->IoStatus.Status = STATUS_BUFFER_TOO_SMALL; break; } - status = ext->vtbl->get_reportdescriptor(device, NULL, 0, &length); - if (status != STATUS_SUCCESS && status != STATUS_BUFFER_TOO_SMALL) + irp->IoStatus.Status = ext->vtbl->get_reportdescriptor(device, NULL, 0, &length); + if (irp->IoStatus.Status != STATUS_SUCCESS && + irp->IoStatus.Status != STATUS_BUFFER_TOO_SMALL) { WARN("Failed to get platform report descriptor length\n"); - irp->IoStatus.Status = status; break; } @@ -891,13 +891,13 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) descriptor->DescriptorList[0].bReportType = HID_REPORT_DESCRIPTOR_TYPE; descriptor->DescriptorList[0].wReportLength = length; - irp->IoStatus.Status = status = STATUS_SUCCESS; + irp->IoStatus.Status = STATUS_SUCCESS; irp->IoStatus.Information = sizeof(*descriptor); break; } case IOCTL_HID_GET_REPORT_DESCRIPTOR: TRACE("IOCTL_HID_GET_REPORT_DESCRIPTOR\n"); - irp->IoStatus.Status = status = ext->vtbl->get_reportdescriptor(device, irp->UserBuffer, buffer_len, &buffer_len); + irp->IoStatus.Status = ext->vtbl->get_reportdescriptor(device, irp->UserBuffer, buffer_len, &buffer_len); irp->IoStatus.Information = buffer_len; break; case IOCTL_HID_GET_STRING: @@ -905,10 +905,10 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DWORD index = (ULONG_PTR)irpsp->Parameters.DeviceIoControl.Type3InputBuffer; TRACE("IOCTL_HID_GET_STRING[%08x]\n", index); - irp->IoStatus.Status = status = hid_get_native_string(device, index, (WCHAR *)irp->UserBuffer, buffer_len / sizeof(WCHAR)); - if (status != STATUS_SUCCESS) - irp->IoStatus.Status = status = ext->vtbl->get_string(device, index, (WCHAR *)irp->UserBuffer, buffer_len / sizeof(WCHAR)); - if (status == STATUS_SUCCESS) + irp->IoStatus.Status = hid_get_native_string(device, index, (WCHAR *)irp->UserBuffer, buffer_len / sizeof(WCHAR)); + if (irp->IoStatus.Status != STATUS_SUCCESS) + irp->IoStatus.Status = ext->vtbl->get_string(device, index, (WCHAR *)irp->UserBuffer, buffer_len / sizeof(WCHAR)); + if (irp->IoStatus.Status == STATUS_SUCCESS) irp->IoStatus.Information = (strlenW((WCHAR *)irp->UserBuffer) + 1) * sizeof(WCHAR); break; } @@ -916,40 +916,32 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) { HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); TRACE_(hid_report)("IOCTL_HID_GET_INPUT_REPORT\n"); - status = ext->vtbl->begin_report_processing(device); - if (status != STATUS_SUCCESS) - { - irp->IoStatus.Status = status; - break; - } + irp->IoStatus.Status = ext->vtbl->begin_report_processing(device); + if (irp->IoStatus.Status != STATUS_SUCCESS) break; - irp->IoStatus.Status = status = deliver_last_report(ext, + irp->IoStatus.Status = deliver_last_report(ext, packet->reportBufferLen, packet->reportBuffer, &irp->IoStatus.Information); - if (status == STATUS_SUCCESS) + if (irp->IoStatus.Status == STATUS_SUCCESS) packet->reportBufferLen = irp->IoStatus.Information; break; } case IOCTL_HID_READ_REPORT: { TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n"); - status = ext->vtbl->begin_report_processing(device); - if (status != STATUS_SUCCESS) - { - irp->IoStatus.Status = status; - break; - } + irp->IoStatus.Status = ext->vtbl->begin_report_processing(device); + if (irp->IoStatus.Status != STATUS_SUCCESS) break; if (!ext->last_report_read) { - irp->IoStatus.Status = status = deliver_last_report(ext, + irp->IoStatus.Status = deliver_last_report(ext, buffer_len, irp->UserBuffer, &irp->IoStatus.Information); ext->last_report_read = TRUE; } else { InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry); - status = STATUS_PENDING; + irp->IoStatus.Status = STATUS_PENDING; } break; } @@ -958,7 +950,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) { HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); TRACE_(hid_report)("IOCTL_HID_WRITE_REPORT / IOCTL_HID_SET_OUTPUT_REPORT\n"); - irp->IoStatus.Status = status = ext->vtbl->set_output_report( + irp->IoStatus.Status = ext->vtbl->set_output_report( device, packet->reportId, packet->reportBuffer, packet->reportBufferLen, &irp->IoStatus.Information); break; @@ -967,7 +959,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) { HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); TRACE_(hid_report)("IOCTL_HID_GET_FEATURE\n"); - irp->IoStatus.Status = status = ext->vtbl->get_feature_report( + irp->IoStatus.Status = ext->vtbl->get_feature_report( device, packet->reportId, packet->reportBuffer, packet->reportBufferLen, &irp->IoStatus.Information); packet->reportBufferLen = irp->IoStatus.Information; @@ -977,7 +969,7 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) { HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); TRACE_(hid_report)("IOCTL_HID_SET_FEATURE\n"); - irp->IoStatus.Status = status = ext->vtbl->set_feature_report( + irp->IoStatus.Status = ext->vtbl->set_feature_report( device, packet->reportId, packet->reportBuffer, packet->reportBufferLen, &irp->IoStatus.Information); break; @@ -990,9 +982,8 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) LeaveCriticalSection(&ext->cs); - if (status != STATUS_PENDING) - IoCompleteRequest(irp, IO_NO_INCREMENT); - + status = irp->IoStatus.Status; + if (status != STATUS_PENDING) IoCompleteRequest(irp, IO_NO_INCREMENT); return status; } -- 2.32.0
On 8/12/21 2:47 AM, Rémi Bernon wrote:
case IOCTL_HID_READ_REPORT: { TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n"); - status = ext->vtbl->begin_report_processing(device); - if (status != STATUS_SUCCESS) - { - irp->IoStatus.Status = status; - break; - } + irp->IoStatus.Status = ext->vtbl->begin_report_processing(device); + if (irp->IoStatus.Status != STATUS_SUCCESS) break; if (!ext->last_report_read) { - irp->IoStatus.Status = status = deliver_last_report(ext, + irp->IoStatus.Status = deliver_last_report(ext, buffer_len, irp->UserBuffer, &irp->IoStatus.Information); ext->last_report_read = TRUE; } else { InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry); - status = STATUS_PENDING; + irp->IoStatus.Status = STATUS_PENDING; } break; }
If you return STATUS_PENDING and queue the IRP here...
@@ -990,9 +982,8 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp)
LeaveCriticalSection(&ext->cs);
...and release the CS here, then the IRP can be completed before you access it...
- if (status != STATUS_PENDING) - IoCompleteRequest(irp, IO_NO_INCREMENT); - + status = irp->IoStatus.Status;
...here. This is kind of the reason that I've gone for the pattern switch (ioctl) { case X: status = STATUS_SUCCESS; break; case Y: status = STATUS_PENDING; IoMarkIrpPending(irp); queue_irp(irp); break; } if (status != STATUS_PENDING) { irp->IoStatus.Status = status; IoCompleteRequest(irp); } return status; in other ntoskrnl code; it's almost the same amount of code and less easy to get wrong.
+ if (status != STATUS_PENDING) IoCompleteRequest(irp, IO_NO_INCREMENT); return status; }
On 8/12/21 7:10 PM, Zebediah Figura (she/her) wrote:
On 8/12/21 2:47 AM, Rémi Bernon wrote:
case IOCTL_HID_READ_REPORT: { TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n"); - status = ext->vtbl->begin_report_processing(device); - if (status != STATUS_SUCCESS) - { - irp->IoStatus.Status = status; - break; - } + irp->IoStatus.Status = ext->vtbl->begin_report_processing(device); + if (irp->IoStatus.Status != STATUS_SUCCESS) break; if (!ext->last_report_read) { - irp->IoStatus.Status = status = deliver_last_report(ext, + irp->IoStatus.Status = deliver_last_report(ext, buffer_len, irp->UserBuffer, &irp->IoStatus.Information); ext->last_report_read = TRUE; } else { InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry); - status = STATUS_PENDING; + irp->IoStatus.Status = STATUS_PENDING; } break; }
If you return STATUS_PENDING and queue the IRP here...
@@ -990,9 +982,8 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) LeaveCriticalSection(&ext->cs);
...and release the CS here, then the IRP can be completed before you access it...
- if (status != STATUS_PENDING) - IoCompleteRequest(irp, IO_NO_INCREMENT); - + status = irp->IoStatus.Status;
...here.
This is kind of the reason that I've gone for the pattern
switch (ioctl) { case X: status = STATUS_SUCCESS; break;
case Y: status = STATUS_PENDING; IoMarkIrpPending(irp); queue_irp(irp); break; }
if (status != STATUS_PENDING) { irp->IoStatus.Status = status; IoCompleteRequest(irp); } return status;
in other ntoskrnl code; it's almost the same amount of code and less easy to get wrong.
+ if (status != STATUS_PENDING) IoCompleteRequest(irp, IO_NO_INCREMENT); return status; }
Indeed. I like the idea of passing the IO_STATUS_BLOCK to the callback directly, though, instead of having the length passed as parameters and then moved again into the irp after the call and the very long lines which set the status on every call. Same thing for the HID_XFER_PACKET, most of the time we get one already from the caller and we instead expand all its members as parameters. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 8/12/21 3:00 PM, Rémi Bernon wrote:
On 8/12/21 7:10 PM, Zebediah Figura (she/her) wrote:
On 8/12/21 2:47 AM, Rémi Bernon wrote:
case IOCTL_HID_READ_REPORT: { TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n"); - status = ext->vtbl->begin_report_processing(device); - if (status != STATUS_SUCCESS) - { - irp->IoStatus.Status = status; - break; - } + irp->IoStatus.Status = ext->vtbl->begin_report_processing(device); + if (irp->IoStatus.Status != STATUS_SUCCESS) break; if (!ext->last_report_read) { - irp->IoStatus.Status = status = deliver_last_report(ext, + irp->IoStatus.Status = deliver_last_report(ext, buffer_len, irp->UserBuffer, &irp->IoStatus.Information); ext->last_report_read = TRUE; } else { InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry); - status = STATUS_PENDING; + irp->IoStatus.Status = STATUS_PENDING; } break; }
If you return STATUS_PENDING and queue the IRP here...
@@ -990,9 +982,8 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) LeaveCriticalSection(&ext->cs);
...and release the CS here, then the IRP can be completed before you access it...
- if (status != STATUS_PENDING) - IoCompleteRequest(irp, IO_NO_INCREMENT); - + status = irp->IoStatus.Status;
...here.
This is kind of the reason that I've gone for the pattern
switch (ioctl) { case X: status = STATUS_SUCCESS; break;
case Y: status = STATUS_PENDING; IoMarkIrpPending(irp); queue_irp(irp); break; }
if (status != STATUS_PENDING) { irp->IoStatus.Status = status; IoCompleteRequest(irp); } return status;
in other ntoskrnl code; it's almost the same amount of code and less easy to get wrong.
+ if (status != STATUS_PENDING) IoCompleteRequest(irp, IO_NO_INCREMENT); return status; }
Indeed.
I like the idea of passing the IO_STATUS_BLOCK to the callback directly, though, instead of having the length passed as parameters and then moved again into the irp after the call and the very long lines which set the status on every call.
Same thing for the HID_XFER_PACKET, most of the time we get one already from the caller and we instead expand all its members as parameters.
True, it's less obviously better if you have to fill the Information as well. On the other hand, it's only one field. My preferred pattern, when I need to fill Information, is to fill it in the same place as I fill the output buffer, but to leave Status to be filled by the request handler. All this is just more evidence that we need a helper framework—whether WDF or homegrown—to deal with the ugliness of WDM...
participants (2)
-
Rémi Bernon -
Zebediah Figura (she/her)