[PATCH v4 0/9] MR432: Add support for SPDRP_BASE_CONTAINERID by querying drivers with BusQueryContainerID
This is the hidclass/ntoskrnl/setupapi counterpart to !359. As with that merge request, implementing `BusQueryContainerID` in the bus driver is left out and will be addressed in a different MR. I believe this is useful to expose the Container ID no matter how those end up being assigned by the bus driver. -- v4: ntoskrnl/tests: Add test for getting SPDRP_BASE_CONTAINERID from PnP driver https://gitlab.winehq.org/wine/wine/-/merge_requests/432
From: Claire Girka <claire(a)sitedethib.com> --- dlls/setupapi/devinst.c | 2 ++ include/setupapi.h | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/dlls/setupapi/devinst.c b/dlls/setupapi/devinst.c index 73e721753aa..1282285da4c 100644 --- a/dlls/setupapi/devinst.c +++ b/dlls/setupapi/devinst.c @@ -96,6 +96,7 @@ static const WCHAR Capabilities[] = {'C','a','p','a','b','i','l','i','t','i','e' static const WCHAR UINumber[] = {'U','I','N','u','m','b','e','r',0}; static const WCHAR UpperFilters[] = {'U','p','p','e','r','F','i','l','t','e','r','s',0}; static const WCHAR LowerFilters[] = {'L','o','w','e','r','F','i','l','t','e','r','s',0}; +static const WCHAR BaseContainerId[] = {'B','a','s','e','C','o','n','t','a','i','n','e','r','I','d',0}; static const WCHAR Phantom[] = {'P','h','a','n','t','o','m',0}; static const WCHAR SymbolicLink[] = {'S','y','m','b','o','l','i','c','L','i','n','k',0}; static const WCHAR Control[] = {'C','o','n','t','r','o','l',0}; @@ -654,6 +655,7 @@ static const struct PropertyMapEntry PropertyMap[] = { { REG_DWORD, "UINumber", UINumber }, { REG_MULTI_SZ, "UpperFilters", UpperFilters }, { REG_MULTI_SZ, "LowerFilters", LowerFilters }, + [0x24] = { REG_SZ, "BaseContainerId", BaseContainerId }, }; static BOOL SETUPDI_SetDeviceRegistryPropertyW(struct device *device, diff --git a/include/setupapi.h b/include/setupapi.h index 14e3774fc42..806df7baaf5 100644 --- a/include/setupapi.h +++ b/include/setupapi.h @@ -1315,7 +1315,8 @@ DECL_WINELIB_SETUPAPI_TYPE_AW(PSP_INF_SIGNER_INFO) #define SPDRP_REMOVAL_POLICY_HW_DEFAULT 0x00000020 #define SPDRP_REMOVAL_POLICY_OVERRIDE 0x00000021 #define SPDRP_INSTALL_STATE 0x00000022 -#define SPDRP_MAXIMUM_PROPERTY 0x00000023 +#define SPDRP_BASE_CONTAINERID 0x00000024 +#define SPDRP_MAXIMUM_PROPERTY 0x00000025 #define DPROMPT_SUCCESS 0 #define DPROMPT_CANCEL 1 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/432
On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka <claire(a)sitedethib.com>
--- dlls/setupapi/devinst.c | 2 ++ include/setupapi.h | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/dlls/setupapi/devinst.c b/dlls/setupapi/devinst.c index 73e721753aa..1282285da4c 100644 --- a/dlls/setupapi/devinst.c +++ b/dlls/setupapi/devinst.c @@ -96,6 +96,7 @@ static const WCHAR Capabilities[] = {'C','a','p','a','b','i','l','i','t','i','e' static const WCHAR UINumber[] = {'U','I','N','u','m','b','e','r',0}; static const WCHAR UpperFilters[] = {'U','p','p','e','r','F','i','l','t','e','r','s',0}; static const WCHAR LowerFilters[] = {'L','o','w','e','r','F','i','l','t','e','r','s',0}; +static const WCHAR BaseContainerId[] = {'B','a','s','e','C','o','n','t','a','i','n','e','r','I','d',0}; static const WCHAR Phantom[] = {'P','h','a','n','t','o','m',0}; static const WCHAR SymbolicLink[] = {'S','y','m','b','o','l','i','c','L','i','n','k',0}; static const WCHAR Control[] = {'C','o','n','t','r','o','l',0};
Can you please use wide-character constants for any new strings introduced?
@@ -654,6 +655,7 @@ static const struct PropertyMapEntry PropertyMap[] = { { REG_DWORD, "UINumber", UINumber }, { REG_MULTI_SZ, "UpperFilters", UpperFilters }, { REG_MULTI_SZ, "LowerFilters", LowerFilters }, + [0x24] = { REG_SZ, "BaseContainerId", BaseContainerId }, };
Please use SPDRP_BASE_CONTAINERID instead of hardcoding the value. Also, are you sure that "BaseContainerId" is correct? On a Windows machine I see only "ContainerID"; is this supposed to be different somehow?
From: Claire Girka <claire(a)sitedethib.com> Do not assume the underlying driver will return meaningful data, as it may not support BusQueryContainerID which will be queried in a next commit. --- dlls/hidclass.sys/pnp.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c index 873c55c4a67..afc7b6e933d 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -71,7 +71,7 @@ static minidriver *find_minidriver(DRIVER_OBJECT *driver) static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR *id) { IO_STACK_LOCATION *irpsp; - IO_STATUS_BLOCK irp_status; + IO_STATUS_BLOCK irp_status = { .Status = STATUS_NOT_IMPLEMENTED, .Information = 0 }; KEVENT event; IRP *irp; @@ -87,8 +87,14 @@ static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCH if (IoCallDriver(device, irp) == STATUS_PENDING) KeWaitForSingleObject(&event, Executive, KernelMode, FALSE, NULL); - wcscpy(id, (WCHAR *)irp_status.Information); - ExFreePool((WCHAR *)irp_status.Information); + if (irp_status.Information && !irp_status.Status) + wcscpy(id, (WCHAR *)irp_status.Information); + else + id[0] = 0; + + if (irp_status.Information) + ExFreePool((WCHAR *)irp_status.Information); + return irp_status.Status; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/432
On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka <claire(a)sitedethib.com>
Do not assume the underlying driver will return meaningful data, as it may not support BusQueryContainerID which will be queried in a next commit. --- dlls/hidclass.sys/pnp.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c index 873c55c4a67..afc7b6e933d 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -71,7 +71,7 @@ static minidriver *find_minidriver(DRIVER_OBJECT *driver) static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR *id) { IO_STACK_LOCATION *irpsp; - IO_STATUS_BLOCK irp_status; + IO_STATUS_BLOCK irp_status = { .Status = STATUS_NOT_IMPLEMENTED, .Information = 0 }; KEVENT event; IRP *irp;
@@ -87,8 +87,14 @@ static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCH if (IoCallDriver(device, irp) == STATUS_PENDING) KeWaitForSingleObject(&event, Executive, KernelMode, FALSE, NULL);
- wcscpy(id, (WCHAR *)irp_status.Information); - ExFreePool((WCHAR *)irp_status.Information); + if (irp_status.Information && !irp_status.Status)
I think "!irp_status.Status" implies "irp_status.Information".
+ wcscpy(id, (WCHAR *)irp_status.Information); + else + id[0] = 0; + + if (irp_status.Information) + ExFreePool((WCHAR *)irp_status.Information); + return irp_status.Status; }
I don't think it makes sense to return a failure status but still report output buffers. I would instead fill the default "empty" ID in the caller, and just don't modify the output buffer if the call fails.
From: Claire Girka <claire(a)sitedethib.com> --- dlls/hidclass.sys/hid.h | 1 + dlls/hidclass.sys/pnp.c | 1 + 2 files changed, 2 insertions(+) diff --git a/dlls/hidclass.sys/hid.h b/dlls/hidclass.sys/hid.h index e20f12809eb..b8cec55fb7c 100644 --- a/dlls/hidclass.sys/hid.h +++ b/dlls/hidclass.sys/hid.h @@ -81,6 +81,7 @@ typedef struct _BASE_DEVICE_EXTENSION * for convenience. */ WCHAR device_id[MAX_DEVICE_ID_LEN]; WCHAR instance_id[MAX_DEVICE_ID_LEN]; + WCHAR container_id[MAX_GUID_STRING_LEN]; const GUID *class_guid; BOOL is_fdo; diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c index afc7b6e933d..a187058a2d4 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -173,6 +173,7 @@ static NTSTATUS WINAPI driver_add_device(DRIVER_OBJECT *driver, DEVICE_OBJECT *b ext->u.fdo.hid_ext.NextDeviceObject = bus_pdo; swprintf(ext->device_id, ARRAY_SIZE(ext->device_id), L"HID\\%s", wcsrchr(device_id, '\\') + 1); wcscpy(ext->instance_id, instance_id); + get_device_id(bus_pdo, BusQueryContainerID, ext->container_id); is_xinput_class = !wcsncmp(device_id, L"WINEXINPUT\\", 7) && wcsstr(device_id, L"&XI_") != NULL; if (is_xinput_class) ext->class_guid = &GUID_DEVINTERFACE_WINEXINPUT; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/432
From: Claire Girka <claire(a)sitedethib.com> --- dlls/hidclass.sys/pnp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c index a187058a2d4..1c0ba839b9e 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -231,6 +231,7 @@ static void create_child(minidriver *minidriver, DEVICE_OBJECT *fdo) KeInitializeSpinLock( &pdo_ext->u.pdo.queues_lock ); wcscpy(pdo_ext->device_id, fdo_ext->device_id); wcscpy(pdo_ext->instance_id, fdo_ext->instance_id); + wcscpy(pdo_ext->container_id, fdo_ext->container_id); pdo_ext->class_guid = fdo_ext->class_guid; pdo_ext->u.pdo.information.VendorID = attr.VendorID; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/432
From: Claire Girka <claire(a)sitedethib.com> --- dlls/hidclass.sys/pnp.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c index 1c0ba839b9e..903d5e9d3dd 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -419,8 +419,16 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp) irp->IoStatus.Information = (ULONG_PTR)id; status = STATUS_SUCCESS; break; - case BusQueryContainerID: + if (ext->container_id[0]) { + lstrcpyW(id, ext->container_id); + irp->IoStatus.Information = (ULONG_PTR)id; + status = STATUS_SUCCESS; + } else { + ExFreePool(id); + } + break; + case BusQueryDeviceSerialNumber: FIXME("unimplemented id type %#x\n", irpsp->Parameters.QueryId.IdType); ExFreePool(id); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/432
From: Claire Girka <claire(a)sitedethib.com> Do not assume the underlying driver will return meaningful data, as it may not support BusQueryContainerID which will be queried in a next commit. --- dlls/ntoskrnl.exe/pnp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index 71c03586897..69f15d82875 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -75,7 +75,7 @@ static struct wine_rb_tree device_interfaces = { interface_rb_compare }; static NTSTATUS get_device_id( DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR **id ) { IO_STACK_LOCATION *irpsp; - IO_STATUS_BLOCK irp_status; + IO_STATUS_BLOCK irp_status = { .Information = 0 }; KEVENT event; IRP *irp; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/432
On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka <claire(a)sitedethib.com>
Do not assume the underlying driver will return meaningful data, as it may not support BusQueryContainerID which will be queried in a next commit. --- dlls/ntoskrnl.exe/pnp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index 71c03586897..69f15d82875 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -75,7 +75,7 @@ static struct wine_rb_tree device_interfaces = { interface_rb_compare }; static NTSTATUS get_device_id( DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR **id ) { IO_STACK_LOCATION *irpsp; - IO_STATUS_BLOCK irp_status; + IO_STATUS_BLOCK irp_status = { .Information = 0 }; KEVENT event; IRP *irp;
We never interpret Information if the IRP fails, so what's the point?
From: Claire Girka <claire(a)sitedethib.com> --- dlls/ntoskrnl.exe/pnp.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index 69f15d82875..e12e46f94f1 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -281,6 +281,13 @@ static BOOL install_device_driver( DEVICE_OBJECT *device, HDEVINFO set, SP_DEVIN sizeof_multiszW( ids ) * sizeof(WCHAR) ); ExFreePool( ids ); + if (!get_device_id(device, BusQueryContainerID, &ids) && ids) + { + SetupDiSetDeviceRegistryPropertyW( set, sp_device, SPDRP_BASE_CONTAINERID, (BYTE *)ids, + (lstrlenW( ids ) + 1) * sizeof(WCHAR) ); + ExFreePool( ids ); + } + if (!SetupDiBuildDriverInfoList( set, sp_device, SPDIT_COMPATDRIVER )) { ERR("Failed to build compatible driver list, error %#lx.\n", GetLastError()); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/432
On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka <claire(a)sitedethib.com>
--- dlls/ntoskrnl.exe/pnp.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index 69f15d82875..e12e46f94f1 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -281,6 +281,13 @@ static BOOL install_device_driver( DEVICE_OBJECT *device, HDEVINFO set, SP_DEVIN sizeof_multiszW( ids ) * sizeof(WCHAR) ); ExFreePool( ids );
+ if (!get_device_id(device, BusQueryContainerID, &ids) && ids)
Here, too, I don't think the driver should be returning success and NULL. We handle that elsewhere but I'm not sure we should.
+ { + SetupDiSetDeviceRegistryPropertyW( set, sp_device, SPDRP_BASE_CONTAINERID, (BYTE *)ids, + (lstrlenW( ids ) + 1) * sizeof(WCHAR) ); + ExFreePool( ids ); + } + if (!SetupDiBuildDriverInfoList( set, sp_device, SPDIT_COMPATDRIVER )) { ERR("Failed to build compatible driver list, error %#lx.\n", GetLastError());
From: Claire Girka <claire(a)sitedethib.com> --- dlls/ntoskrnl.exe/pnp.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index e12e46f94f1..77d879a07a9 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -880,6 +880,7 @@ NTSTATUS WINAPI IoRegisterDeviceInterface(DEVICE_OBJECT *device, const GUID *cla struct wine_rb_entry *entry; DWORD required; HDEVINFO set; + WCHAR *id; TRACE("device %p, class_guid %s, refstr %s, symbolic_link %p.\n", device, debugstr_guid(class_guid), debugstr_us(refstr), symbolic_link); @@ -913,6 +914,13 @@ NTSTATUS WINAPI IoRegisterDeviceInterface(DEVICE_OBJECT *device, const GUID *cla return STATUS_UNSUCCESSFUL; } + if (!get_device_id(device, BusQueryContainerID, &id) && id) + { + SetupDiSetDeviceRegistryPropertyW( set, &sp_device, SPDRP_BASE_CONTAINERID, (BYTE *)id, + (lstrlenW( id ) + 1) * sizeof(WCHAR) ); + ExFreePool( id ); + } + data->DevicePath[1] = '?'; TRACE("Returning path %s.\n", debugstr_w(data->DevicePath)); RtlCreateUnicodeString( &device_path, data->DevicePath); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/432
On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka <claire(a)sitedethib.com>
--- dlls/ntoskrnl.exe/pnp.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index e12e46f94f1..77d879a07a9 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -880,6 +880,7 @@ NTSTATUS WINAPI IoRegisterDeviceInterface(DEVICE_OBJECT *device, const GUID *cla struct wine_rb_entry *entry; DWORD required; HDEVINFO set; + WCHAR *id;
TRACE("device %p, class_guid %s, refstr %s, symbolic_link %p.\n", device, debugstr_guid(class_guid), debugstr_us(refstr), symbolic_link); @@ -913,6 +914,13 @@ NTSTATUS WINAPI IoRegisterDeviceInterface(DEVICE_OBJECT *device, const GUID *cla return STATUS_UNSUCCESSFUL; }
+ if (!get_device_id(device, BusQueryContainerID, &id) && id) + { + SetupDiSetDeviceRegistryPropertyW( set, &sp_device, SPDRP_BASE_CONTAINERID, (BYTE *)id, + (lstrlenW( id ) + 1) * sizeof(WCHAR) ); + ExFreePool( id ); + } + data->DevicePath[1] = '?'; TRACE("Returning path %s.\n", debugstr_w(data->DevicePath)); RtlCreateUnicodeString( &device_path, data->DevicePath);
Why do we need this?
From: Claire Girka <claire(a)sitedethib.com> --- dlls/ntoskrnl.exe/tests/driver_pnp.c | 5 ++++- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/dlls/ntoskrnl.exe/tests/driver_pnp.c b/dlls/ntoskrnl.exe/tests/driver_pnp.c index 9727292d65d..824c6554693 100644 --- a/dlls/ntoskrnl.exe/tests/driver_pnp.c +++ b/dlls/ntoskrnl.exe/tests/driver_pnp.c @@ -228,7 +228,10 @@ static NTSTATUS query_id(struct device *device, IRP *irp, BUS_QUERY_ID_TYPE type } case BusQueryContainerID: - return STATUS_NOT_SUPPORTED; + if (!(id = ExAllocatePool(PagedPool, 39 * sizeof(WCHAR)))) + return STATUS_NO_MEMORY; + wcscpy(id, L"{12345678-1234-1234-1234-123456789123}"); + break; default: ok(0, "Unexpected ID query type %#x.\n", type); diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index 64debcbebbf..28c65cccf2b 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1460,6 +1460,7 @@ static void test_pnp_devices(void) { static const char expect_hardware_id[] = "winetest_hardware\0winetest_hardware_1\0"; static const char expect_compat_id[] = "winetest_compat\0winetest_compat_1\0"; + static const char expect_container_id[] = "{12345678-1234-1234-1234-123456789123}"; char buffer[200]; SP_DEVICE_INTERFACE_DETAIL_DATA_A *iface_detail = (void *)buffer; @@ -1646,6 +1647,13 @@ static void test_pnp_devices(void) ok(size == sizeof(expect_hardware_id), "got size %lu\n", size); ok(!memcmp(buffer, expect_hardware_id, size), "got hardware IDs %s\n", debugstr_an(buffer, size)); + ret = SetupDiGetDeviceRegistryPropertyA(set, &device, SPDRP_BASE_CONTAINERID, + &type, (BYTE *)buffer, sizeof(buffer), &size); + ok(ret, "got error %#lx\n", GetLastError()); + ok(type == REG_SZ, "got type %lu\n", type); + ok(size == sizeof(expect_container_id), "got size %lu\n", size); + ok(!memcmp(buffer, expect_container_id, size), "got container ID %s\n", debugstr_a(buffer)); + ret = SetupDiGetDeviceRegistryPropertyA(set, &device, SPDRP_COMPATIBLEIDS, &type, (BYTE *)buffer, sizeof(buffer), &size); ok(ret, "got error %#lx\n", GetLastError()); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/432
It is preferred to have test commits come first (with `todo_wine`) and the implementation coming second, since it shows more clearly that the implementation does indeed fix the test failures (todo). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/432#note_3987
On Tue Jul 12 18:43:38 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka <claire(a)sitedethib.com>
Do not assume the underlying driver will return meaningful data, as it may not support BusQueryContainerID which will be queried in a next commit. --- dlls/hidclass.sys/pnp.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c index 873c55c4a67..afc7b6e933d 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -71,7 +71,7 @@ static minidriver *find_minidriver(DRIVER_OBJECT *driver) static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR *id) { IO_STACK_LOCATION *irpsp; - IO_STATUS_BLOCK irp_status; + IO_STATUS_BLOCK irp_status = { .Status = STATUS_NOT_IMPLEMENTED, .Information = 0 }; KEVENT event; IRP *irp;
@@ -87,8 +87,14 @@ static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCH if (IoCallDriver(device, irp) == STATUS_PENDING) KeWaitForSingleObject(&event, Executive, KernelMode, FALSE, NULL);
- wcscpy(id, (WCHAR *)irp_status.Information); - ExFreePool((WCHAR *)irp_status.Information); + if (irp_status.Information && !irp_status.Status) I think "!irp_status.Status" implies "irp_status.Information". + wcscpy(id, (WCHAR *)irp_status.Information); + else + id[0] = 0; + + if (irp_status.Information) + ExFreePool((WCHAR *)irp_status.Information); + return irp_status.Status; }
I don't think it makes sense to return a failure status but still report output buffers. I would instead fill the default "empty" ID in the caller, and just don't modify the output buffer if the call fails. _______________________________________________ wine-gitlab mailing list -- wine-gitlab(a)winehq.org To unsubscribe send an email to wine-gitlab-leave(a)winehq.org ``` `!irp_status.Status` should probably imply `irp_status.Information` but it is not currently the case. For unhandled `IRP_MN_QUERY_ID` types, `winebus` will set the status *based* on the (uninitialized) value of `irp_status.Information`.
This is probably a bug in `winebus`, though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/432#note_3996
On Tue Jul 12 19:45:06 2022 +0000, Claire wrote:
`!irp_status.Status` should probably imply `irp_status.Information` but it is not currently the case. For unhandled `IRP_MN_QUERY_ID` types, `winebus` will set the status *based* on the (uninitialized) value of `irp_status.Information`. This is probably a bug in `winebus`, though. I am actually mistaken, that part of `winebus` is correct, but for some reason it returns `STATUS_SUCCESS` anyway. I have not tracked down why yet.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/432#note_3997
On Tue Jul 12 19:52:49 2022 +0000, Claire wrote:
I am actually mistaken, that part of `winebus` is correct, but for some reason it returns `STATUS_SUCCESS` anyway. I have not tracked down why yet. Ok found it, it's simply because I was setting `irp_status` (output) rather than `irp->IoStatus` (input/state). Will fix in the next version of the patch.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/432#note_4001
On Tue Jul 12 18:45:48 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka <claire(a)sitedethib.com>
Do not assume the underlying driver will return meaningful data, as it may not support BusQueryContainerID which will be queried in a next commit. --- dlls/ntoskrnl.exe/pnp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index 71c03586897..69f15d82875 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -75,7 +75,7 @@ static struct wine_rb_tree device_interfaces = { interface_rb_compare }; static NTSTATUS get_device_id( DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR **id ) { IO_STACK_LOCATION *irpsp; - IO_STATUS_BLOCK irp_status; + IO_STATUS_BLOCK irp_status = { .Information = 0 }; KEVENT event; IRP *irp;
We never interpret Information if the IRP fails, so what's the point? _______________________________________________ wine-gitlab mailing list -- wine-gitlab(a)winehq.org To unsubscribe send an email to wine-gitlab-leave(a)winehq.org ``` No point, that was a misguided attempt at fixing the test failures that I thought were due to memory corruption. I have a better grasp of what is happening and will skip this change in the next patch set.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/432#note_4002
On Tue Jul 12 18:47:51 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka <claire(a)sitedethib.com>
--- dlls/ntoskrnl.exe/pnp.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index 69f15d82875..e12e46f94f1 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -281,6 +281,13 @@ static BOOL install_device_driver( DEVICE_OBJECT *device, HDEVINFO set, SP_DEVIN sizeof_multiszW( ids ) * sizeof(WCHAR) ); ExFreePool( ids );
+ if (!get_device_id(device, BusQueryContainerID, &ids) && ids) Here, too, I don't think the driver should be returning success and NULL. We handle that elsewhere but I'm not sure we should. + { + SetupDiSetDeviceRegistryPropertyW( set, sp_device, SPDRP_BASE_CONTAINERID, (BYTE *)ids, + (lstrlenW( ids ) + 1) * sizeof(WCHAR) ); + ExFreePool( ids ); + } + if (!SetupDiBuildDriverInfoList( set, sp_device, SPDIT_COMPATDRIVER )) { ERR("Failed to build compatible driver list, error %#lx.\n", GetLastError());
wine-gitlab mailing list -- wine-gitlab(a)winehq.org To unsubscribe send an email to wine-gitlab-leave(a)winehq.org ``` Unless you want me to change it, I will keep it this way for consistency with the current code.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/432#note_4003
On Tue Jul 12 18:48:28 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka <claire(a)sitedethib.com>
--- dlls/ntoskrnl.exe/pnp.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index e12e46f94f1..77d879a07a9 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -880,6 +880,7 @@ NTSTATUS WINAPI IoRegisterDeviceInterface(DEVICE_OBJECT *device, const GUID *cla struct wine_rb_entry *entry; DWORD required; HDEVINFO set; + WCHAR *id;
TRACE("device %p, class_guid %s, refstr %s, symbolic_link %p.\n", device, debugstr_guid(class_guid), debugstr_us(refstr), symbolic_link); @@ -913,6 +914,13 @@ NTSTATUS WINAPI IoRegisterDeviceInterface(DEVICE_OBJECT *device, const GUID *cla return STATUS_UNSUCCESSFUL; }
+ if (!get_device_id(device, BusQueryContainerID, &id) && id) + { + SetupDiSetDeviceRegistryPropertyW( set, &sp_device, SPDRP_BASE_CONTAINERID, (BYTE *)id, + (lstrlenW( id ) + 1) * sizeof(WCHAR) ); + ExFreePool( id ); + } + data->DevicePath[1] = '?'; TRACE("Returning path %s.\n", debugstr_w(data->DevicePath)); RtlCreateUnicodeString( &device_path, data->DevicePath); Why do we need this?
wine-gitlab mailing list -- wine-gitlab(a)winehq.org To unsubscribe send an email to wine-gitlab-leave(a)winehq.org ``` I would need to investigate more, but the property isn't properly exposed without that change.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/432#note_4004
On 7/12/22 16:12, Claire (@ClearlyClaire) wrote:
On Tue Jul 12 18:48:28 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka <claire(a)sitedethib.com>
--- dlls/ntoskrnl.exe/pnp.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index e12e46f94f1..77d879a07a9 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -880,6 +880,7 @@ NTSTATUS WINAPI IoRegisterDeviceInterface(DEVICE_OBJECT *device, const GUID *cla struct wine_rb_entry *entry; DWORD required; HDEVINFO set; + WCHAR *id;
TRACE("device %p, class_guid %s, refstr %s, symbolic_link %p.\n", device, debugstr_guid(class_guid), debugstr_us(refstr), symbolic_link); @@ -913,6 +914,13 @@ NTSTATUS WINAPI IoRegisterDeviceInterface(DEVICE_OBJECT *device, const GUID *cla return STATUS_UNSUCCESSFUL; }
+ if (!get_device_id(device, BusQueryContainerID, &id) && id) + { + SetupDiSetDeviceRegistryPropertyW( set, &sp_device, SPDRP_BASE_CONTAINERID, (BYTE *)id, + (lstrlenW( id ) + 1) * sizeof(WCHAR) ); + ExFreePool( id ); + } + data->DevicePath[1] = '?'; TRACE("Returning path %s.\n", debugstr_w(data->DevicePath)); RtlCreateUnicodeString( &device_path, data->DevicePath); Why do we need this?
wine-gitlab mailing list -- wine-gitlab(a)winehq.org To unsubscribe send an email to wine-gitlab-leave(a)winehq.org ``` I would need to investigate more, but the property isn't properly exposed without that change.
Please do; I don't think it should be necessary, and we'd rather fix it the right way upstream if not.
participants (4)
-
Claire (＠ClearlyClaire) -
Claire Girka
-
Jinoh Kang (＠iamahuman)
-
Zebediah Figura