[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
From: Claire Girka claire@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
On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka claire@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@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; }
On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka claire@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@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;
From: Claire Girka claire@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;
From: Claire Girka claire@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);
From: Claire Girka claire@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;
On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka claire@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@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());
On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka claire@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@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);
On 7/12/22 13:01, Claire Girka wrote:
From: Claire Girka claire@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@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());
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).
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@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@winehq.org To unsubscribe send an email to wine-gitlab-leave@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.
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.
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.
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@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@winehq.org To unsubscribe send an email to wine-gitlab-leave@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.
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@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@winehq.org To unsubscribe send an email to wine-gitlab-leave@winehq.org
Unless you want me to change it, I will keep it this way for consistency with the current code.
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@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@winehq.org To unsubscribe send an email to wine-gitlab-leave@winehq.org
I would need to investigate more, but the property isn't properly exposed without that change.
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@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@winehq.org To unsubscribe send an email to wine-gitlab-leave@winehq.orgI 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.
-
Claire (＠ClearlyClaire) -
Claire Girka
-
Jinoh Kang (＠iamahuman)
-
Zebediah Figura