[PATCH v14 0/4] MR6315: plugplay: Rework how PnP notifications are sent over RPC to support DBT_DEVTYP_HANDLE events.
PnP device notifications/events are currently sent and received from the `plugplay` service as the raw-bytes for the `DBT_DEVTYP_DEVICEINTERFACE` struct that needs to be sent from the PnP code in `ntoskrnl.exe`. While simple, it does not support sending `DBT_DEVTYP_HANDLE` events, as they also include a `HANDLE` to the device the event comes from, which cannot simply be sent as raw bytes. This MR reworks how notifications are sent and received from the `plugplay` service, by:
1. Introducing new structs `DEVICE_BROADCAST_HANDLE` and `DEVICE_BROADCAST_DEVICEINTERFACE` in `plugplay.idl`. The structs are similar to their counterparts in `dbt.h` without the `devicetype` and `size` fields added for the header. 2. Re-write `plugplay_{send,get}_event` to use the newly added union `DEVICE_BROADCAST`, which uses the `devicetype` value as the discriminant. A deep copy of this union is then appended to the event queue of every registered listener. 3. Re-write `I_ScRegisterDeviceNotification` and `device_notify_proc` in `sechost/service.c` to use the newly added types instead. Additionally, add support for registering for `DBT_DEVTYPE_HANDLE` notifications, for which the `OBJECT_NAME_INFORMATION` value for the device `HANDLE` is used to filter notifications in order to correctly dispatch them to their registered callbacks. 4. Re-write the callbacks in `user32/input.c` to accept the `DEVICE_BROADCAST` union instead, and construct the appropriate `DEV_BROADCAST_*` struct from it, which gets sent to `SendMessageTimeoutW` as usual.
Support for `DBT_DEVTYP_HANDLE` is needed for the Bluetooth driver stack I have been working on recently, which uses `DBT_DEVTYP_HANDLE` events to notify userspace about newly found Bluetooth devices during discovery and incoming authentication requests, as documented by MS [here](https://learn.microsoft.com/en-us/windows/win32/bluetooth/bluetooth-and-wm-d...).
-- v14: plugplay: Broadcast PnP events on a dedicated thread.
From: Vibhav Pant vibhavp@gmail.com
Instead of sending and receiving events from plugplay as single array of bytes, introduce a new union, DEVICE_BROADCAST. The union currently supports the types DEVICE_BROADCAST_HANDLE and DEVICE_BROADCAST_DEVICEINTERFACE for the DBT_DEVTYP_HANDLE and DBT_DEVTYP_DEVICEINTERFACE event codes respectively, and define individual fields corresponding to their equivalent structs in dbt.h.
While this introduces some additional complexity while converting from a DEVICE_BROADCAST to a DEV_BROADCAST_* type, it also allows support for DBT_DEVTYP_HANDLE events to be sent, which was not supported with the previous raw-bytes method, as the DEV_BROADCAST_HANDLE struct was identified with a HANDLE to the originating device file. --- include/wine/plugplay.idl | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/include/wine/plugplay.idl b/include/wine/plugplay.idl index a3e7b04bf30..d5aaebe388e 100644 --- a/include/wine/plugplay.idl +++ b/include/wine/plugplay.idl @@ -1,5 +1,6 @@ /* * Copyright (C) 2020 Zebediah Figura for CodeWeavers + * Copyright (C) 2024 Vibhav Pant * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -18,6 +19,42 @@
import "wtypes.idl";
+typedef enum _broadcast_device_type +{ + /* DBT_DEVTYP_DEVICEINTERFACE */ + BROADCAST_DEVICE_TYPE_DEVICEINTERFACE = 5, + /* DBT_DEVTYP_HANDLE */ + BROADCAST_DEVICE_TYPE_HANDLE = 6 +} broadcast_device_type; + +typedef struct _device_broadcast_header +{ + DWORD reserved; +} device_broadcast_header; + +typedef struct _device_broadcast_deviceinterface +{ + device_broadcast_header header; + GUID class_guid; + [string] LPWSTR name; +} device_broadcast_deviceinterface; + +typedef struct _device_broadcast_handle +{ + device_broadcast_header header; + LPWSTR handle_file_path; + GUID event_guid; + [string] LPWSTR name; + DWORD data_size; + [unique, size_is(data_size)] BYTE *data; +} device_broadcast_handle; + +typedef union _device_broadcast switch (broadcast_device_type devicetype) event +{ + case BROADCAST_DEVICE_TYPE_DEVICEINTERFACE: device_broadcast_deviceinterface device_interface; + case BROADCAST_DEVICE_TYPE_HANDLE: device_broadcast_handle handle; +} device_broadcast; + [ uuid(57c680ac-7bce-4f39-97fd-ffea566754d5), endpoint("ncacn_np:[\pipe\wine_plugplay]"),
From: Vibhav Pant vibhavp@gmail.com
wine/dbt.h defines types used for registering and filter device notifications. --- include/Makefile.in | 1 + include/wine/dbt.h | 98 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 include/wine/dbt.h
diff --git a/include/Makefile.in b/include/Makefile.in index bbf28cfc87e..c26b0ed18da 100644 --- a/include/Makefile.in +++ b/include/Makefile.in @@ -900,6 +900,7 @@ SOURCES = \ wine/asm.h \ wine/atsvc.idl \ wine/condrv.h \ + wine/dbt.h \ wine/dcetypes.idl \ wine/debug.h \ wine/dplaysp.h \ diff --git a/include/wine/dbt.h b/include/wine/dbt.h new file mode 100644 index 00000000000..d6955c1d9d7 --- /dev/null +++ b/include/wine/dbt.h @@ -0,0 +1,98 @@ +/* + * Definitions for registering device notifications + * + * Copyright 2024 Vibhav Pant + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#ifndef __WINE_WINE_DBT_H_ +#define __WINE_WINE_DBT_H_ + +#include <wine/debug.h> + +struct device_notification_details +{ + DWORD (CALLBACK *cb)(HANDLE handle, HDEVNOTIFY notify, DWORD flags, device_broadcast *data, HANDLE device_handle); + HANDLE handle; + DWORD devicetype; + union + { + struct + { + /* Used to implement DEVICE_NOTIFY_ALL_INTERFACE_CLASSES. If true, the class field below + * should be ignored. */ + BOOL all_classes; + GUID class; + } deviceinterface; + struct + { + /* The path for the device file the notification is originating from. */ + OBJECT_NAME_INFORMATION *name_info; + /* A handle to the device file. It is passed as is to the callback. */ + HANDLE device; + } device; + } filter; +}; + +extern HDEVNOTIFY WINAPI I_ScRegisterDeviceNotification( struct device_notification_details *details, + void *filter, DWORD flags ); +extern BOOL WINAPI I_ScUnregisterDeviceNotification( HDEVNOTIFY handle ); + +static inline const char *debugstr_device_broadcast(const device_broadcast *data) +{ + if (!data) return "(null)"; + + switch (data->devicetype) + { + case DBT_DEVTYP_DEVICEINTERFACE: + return wine_dbg_sprintf( "{%#lx DBT_DEVTYP_DEVICEINTERFACE %s %s}", + data->event.device_interface.header.reserved, + debugstr_guid( &data->event.device_interface.class_guid ), + debugstr_w( data->event.device_interface.name ) ); + case DBT_DEVTYP_HANDLE: + return wine_dbg_sprintf( "{%#lx DBT_DEVTYP_HANDLE %s %s %s %d %p}", + data->event.device_interface.header.reserved, + debugstr_w( data->event.handle.handle_file_path ), + debugstr_guid( &data->event.handle.event_guid ), + debugstr_w( data->event.handle.name ), + data->event.handle.data_size, data->event.handle.data ); + default: + return wine_dbg_sprintf( "{type=%#lx %p}\n", data->devicetype, data ); + } +} + +static inline const char * +debugstr_device_notification_details( const struct device_notification_details *details ) +{ + if (!details) return "(null)"; + switch (details->devicetype) + { + case DBT_DEVTYP_DEVICEINTERFACE: + if (details->filter.deviceinterface.all_classes) + return wine_dbg_sprintf( "{%p %p DBT_DEVTYP_DEVICEINTERFACE {all_classes=1}}", + details->cb, details->handle ); + return wine_dbg_sprintf( "{%p %p DBT_DEVTYP_DEVICEINTERFACE {class=%s}}", details->cb, + details->handle, + debugstr_guid( &details->filter.deviceinterface.class ) ); + case DBT_DEVTYP_HANDLE: + return wine_dbg_sprintf( "{%p %p DBT_DEVTYP_HANDLE {%p %p}}", details->cb, details->handle, + details->filter.device.name_info, details->filter.device.device ); + default: + return wine_dbg_sprintf( "{%p %p (unknown %#lx)}", details->cb, details->handle, details->devicetype); + } +} + +#endif /* __WINE_WINE_DBT_H_ */
From: Vibhav Pant vibhavp@gmail.com
--- dlls/ntoskrnl.exe/pnp.c | 26 +++--- dlls/sechost/service.c | 79 ++++++++++-------- dlls/user32/Makefile.in | 1 + dlls/user32/input.c | 169 +++++++++++++++++++++++++++++--------- dlls/user32/plugplay.idl | 2 + include/wine/dbt.h | 4 +- include/wine/plugplay.idl | 4 +- programs/plugplay/main.c | 128 +++++++++++++++++++++++------ 8 files changed, 299 insertions(+), 114 deletions(-) create mode 100644 dlls/user32/plugplay.idl
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index 7444b81823c..8127c410941 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -695,11 +695,11 @@ static LONG WINAPI rpc_filter( EXCEPTION_POINTERS *eptr ) return I_RpcExceptionFilter( eptr->ExceptionRecord->ExceptionCode ); }
-static void send_devicechange( DWORD code, void *data, unsigned int size ) +static void send_devicechange( DWORD code, device_broadcast *data ) { __TRY { - plugplay_send_event( code, data, size ); + plugplay_send_event( code, data ); } __EXCEPT(rpc_filter) { @@ -722,7 +722,7 @@ NTSTATUS WINAPI IoSetDeviceInterfaceState( UNICODE_STRING *name, BOOLEAN enable static const WCHAR hashW[] = {'#',0};
size_t namelen = name->Length / sizeof(WCHAR); - DEV_BROADCAST_DEVICEINTERFACE_W *broadcast; + device_broadcast broadcast = {0}; struct device_interface *iface; HANDLE iface_key, control_key; OBJECT_ATTRIBUTES attr = {0}; @@ -805,18 +805,16 @@ NTSTATUS WINAPI IoSetDeviceInterfaceState( UNICODE_STRING *name, BOOLEAN enable
iface->enabled = enable;
- len = offsetof(DEV_BROADCAST_DEVICEINTERFACE_W, dbcc_name[namelen + 1]); - - if ((broadcast = heap_alloc( len ))) + broadcast.devicetype = DBT_DEVTYP_DEVICEINTERFACE; + broadcast.event.device_interface.name = heap_alloc( name->Length + 1 ); + if ( broadcast.event.device_interface.name ) { - broadcast->dbcc_size = len; - broadcast->dbcc_devicetype = DBT_DEVTYP_DEVICEINTERFACE; - broadcast->dbcc_reserved = 0; - broadcast->dbcc_classguid = iface->interface_class; - lstrcpynW( broadcast->dbcc_name, name->Buffer, namelen + 1 ); - if (namelen > 1) broadcast->dbcc_name[1] = '\'; - send_devicechange( enable ? DBT_DEVICEARRIVAL : DBT_DEVICEREMOVECOMPLETE, broadcast, len ); - heap_free( broadcast ); + broadcast.event.device_interface.class_guid = iface->interface_class; + lstrcpyW( broadcast.event.device_interface.name, name->Buffer ); + if ( namelen > 1 ) broadcast.event.device_interface.name[1] = '\'; + send_devicechange( enable ? DBT_DEVICEARRIVAL : DBT_DEVICEREMOVECOMPLETE, + &broadcast ); + heap_free( broadcast.event.device_interface.name ); } return ret; } diff --git a/dlls/sechost/service.c b/dlls/sechost/service.c index 915b0b4ebe0..9de6383972a 100644 --- a/dlls/sechost/service.c +++ b/dlls/sechost/service.c @@ -36,6 +36,8 @@ #include "svcctl.h" #include "plugplay.h"
+#include "wine/dbt.h" + WINE_DEFAULT_DEBUG_CHANNEL(service);
struct notify_data @@ -1973,17 +1975,6 @@ BOOL WINAPI DECLSPEC_HOTPATCH StartServiceCtrlDispatcherW( const SERVICE_TABLE_E return service_run_main_thread(); }
-struct device_notification_details -{ - DWORD (CALLBACK *cb)(HANDLE handle, DWORD flags, DEV_BROADCAST_HDR *header); - HANDLE handle; - union - { - DEV_BROADCAST_HDR header; - DEV_BROADCAST_DEVICEINTERFACE_W iface; - } filter; -}; - static HANDLE device_notify_thread; static struct list device_notify_list = LIST_INIT(device_notify_list);
@@ -1993,23 +1984,27 @@ struct device_notify_registration struct device_notification_details details; };
-static BOOL notification_filter_matches( DEV_BROADCAST_HDR *filter, DEV_BROADCAST_HDR *event ) +static BOOL notification_filter_matches( struct device_notification_details *details, + device_broadcast *data ) { - if (!filter->dbch_devicetype) return TRUE; - if (filter->dbch_devicetype != event->dbch_devicetype) return FALSE; + if (!details->devicetype) return TRUE; + if (details->devicetype != data->devicetype) return FALSE;
- if (filter->dbch_devicetype == DBT_DEVTYP_DEVICEINTERFACE) + if (details->devicetype == DBT_DEVTYP_DEVICEINTERFACE) { - DEV_BROADCAST_DEVICEINTERFACE_W *filter_iface = (DEV_BROADCAST_DEVICEINTERFACE_W *)filter; - DEV_BROADCAST_DEVICEINTERFACE_W *event_iface = (DEV_BROADCAST_DEVICEINTERFACE_W *)event; - if (filter_iface->dbcc_size == offsetof(DEV_BROADCAST_DEVICEINTERFACE_W, dbcc_classguid)) return TRUE; - return IsEqualGUID( &filter_iface->dbcc_classguid, &event_iface->dbcc_classguid ); + if (details->filter.deviceinterface.all_classes) return TRUE; + return IsEqualGUID( &details->filter.deviceinterface.class, &data->event.device_interface.class_guid ); }
- FIXME( "Filter dbch_devicetype %lu not implemented\n", filter->dbch_devicetype ); - return TRUE; + return !lstrcmpiW( details->filter.device.name_info->Name.Buffer, data->event.handle.handle_file_path ); }
+struct device_notification_details_with_reg +{ + HDEVNOTIFY notify_handle; + struct device_notification_details details; +}; + static DWORD WINAPI device_notify_proc( void *arg ) { WCHAR endpoint[] = L"\pipe\wine_plugplay"; @@ -2017,12 +2012,11 @@ static DWORD WINAPI device_notify_proc( void *arg ) RPC_WSTR binding_str; DWORD err = ERROR_SUCCESS; struct device_notify_registration *registration; - struct device_notification_details *details_copy; + struct device_notification_details_with_reg *details_copy; unsigned int details_copy_nelems, details_copy_size; plugplay_rpc_handle handle = NULL; DWORD code = 0; - unsigned int i, size; - BYTE *buf; + unsigned int i;
SetThreadDescription( GetCurrentThread(), L"wine_sechost_device_notify" );
@@ -2060,10 +2054,12 @@ static DWORD WINAPI device_notify_proc( void *arg )
for (;;) { - buf = NULL; + device_broadcast broadcast_data = { 0 }; + WCHAR *str; + __TRY { - code = plugplay_get_event( handle, &buf, &size ); + code = plugplay_get_event( handle, &broadcast_data ); err = ERROR_SUCCESS; } __EXCEPT(rpc_filter) @@ -2084,7 +2080,8 @@ static DWORD WINAPI device_notify_proc( void *arg ) EnterCriticalSection( &service_cs ); LIST_FOR_EACH_ENTRY(registration, &device_notify_list, struct device_notify_registration, entry) { - details_copy[i++] = registration->details; + details_copy[i].details = registration->details; + details_copy[i++].notify_handle = registration; details_copy_nelems++; if (i == details_copy_size) { @@ -2092,14 +2089,30 @@ static DWORD WINAPI device_notify_proc( void *arg ) details_copy = realloc( details_copy, details_copy_size * sizeof(*details_copy) ); } } - LeaveCriticalSection(&service_cs); + LeaveCriticalSection( &service_cs );
for (i = 0; i < details_copy_nelems; i++) { - if (!notification_filter_matches( &details_copy[i].filter.header, (DEV_BROADCAST_HDR *)buf )) continue; - details_copy[i].cb( details_copy[i].handle, code, (DEV_BROADCAST_HDR *)buf ); + struct device_notification_details *details = &details_copy[i].details; + HANDLE device = code == DBT_DEVTYP_HANDLE ? details->filter.device.device : NULL; + + if (!notification_filter_matches( details, &broadcast_data )) continue; + details->cb( details->handle, details_copy[i].notify_handle, code, &broadcast_data, device ); + } + switch (broadcast_data.devicetype) + { + case DBT_DEVTYP_DEVICEINTERFACE: + str = broadcast_data.event.device_interface.name; + break; + case DBT_DEVTYP_HANDLE: + str = broadcast_data.event.handle.name; + MIDL_user_free( broadcast_data.event.handle.handle_file_path ); + if (broadcast_data.event.handle.name) + MIDL_user_free( broadcast_data.event.handle.name ); + if (broadcast_data.event.handle.data_size != 0) + MIDL_user_free( broadcast_data.event.handle.data ); } - MIDL_user_free(buf); + if (str) MIDL_user_free( str ); }
__TRY @@ -2124,7 +2137,7 @@ HDEVNOTIFY WINAPI I_ScRegisterDeviceNotification( struct device_notification_det { struct device_notify_registration *registration;
- TRACE("callback %p, handle %p, filter %p, flags %#lx\n", details->cb, details->handle, filter, flags); + TRACE("details %s, filter %p, flags %#lx\n", debugstr_device_notification_details(details), filter, flags);
if (!(registration = malloc( sizeof(struct device_notify_registration) ))) { @@ -2160,6 +2173,8 @@ BOOL WINAPI I_ScUnregisterDeviceNotification( HDEVNOTIFY handle ) EnterCriticalSection( &service_cs ); list_remove( ®istration->entry ); LeaveCriticalSection(&service_cs); + if (registration->details.devicetype == DBT_DEVTYP_HANDLE) + free( registration->details.filter.device.name_info ); free( registration ); return TRUE; } diff --git a/dlls/user32/Makefile.in b/dlls/user32/Makefile.in index 5f624559931..5b1c775d201 100644 --- a/dlls/user32/Makefile.in +++ b/dlls/user32/Makefile.in @@ -31,6 +31,7 @@ SOURCES = \ misc.c \ msgbox.c \ nonclient.c \ + plugplay.idl \ property.c \ resource.c \ resources/ocr_appstarting.svg \ diff --git a/dlls/user32/input.c b/dlls/user32/input.c index 108592f018f..68a113101a4 100644 --- a/dlls/user32/input.c +++ b/dlls/user32/input.c @@ -24,11 +24,16 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */
+#include "ntstatus.h" +#define WIN32_NO_STATUS #include "user_private.h" #include "dbt.h" #include "wine/server.h" #include "wine/debug.h"
+#include "plugplay.h" +#include "wine/dbt.h" + WINE_DEFAULT_DEBUG_CHANNEL(win); WINE_DECLARE_DEBUG_CHANNEL(keyboard);
@@ -497,74 +502,137 @@ BOOL WINAPI UnloadKeyboardLayout( HKL layout ) return FALSE; }
- -static DWORD CALLBACK devnotify_window_callbackW(HANDLE handle, DWORD flags, DEV_BROADCAST_HDR *header) +static DWORD CALLBACK devnotify_window_callbackW( HANDLE handle, HDEVNOTIFY notify, DWORD flags, + device_broadcast *data, HANDLE device_handle ) { - SendMessageTimeoutW(handle, WM_DEVICECHANGE, flags, (LPARAM)header, SMTO_ABORTIFHUNG, 2000, NULL); + TRACE( "(%p, %p, %#lx, %s)\n", handle, notify, flags, debugstr_device_broadcast( data ) ); + + switch (data->devicetype) + { + case DBT_DEVTYP_DEVICEINTERFACE: + { + DEV_BROADCAST_DEVICEINTERFACE_W *iface; + size_t len = wcslen( data->event.device_interface.name ); + DWORD dbcc_size = offsetof( DEV_BROADCAST_DEVICEINTERFACE_W, dbcc_name[len + 1] ); + + iface = malloc( dbcc_size ); + if (!iface) + return 0; + wcscpy( iface->dbcc_name, data->event.device_interface.name ); + iface->dbcc_size = dbcc_size; + iface->dbcc_devicetype = data->devicetype; + iface->dbcc_reserved = data->event.device_interface.header.reserved; + iface->dbcc_classguid = data->event.device_interface.class_guid; + SendMessageTimeoutW( handle, WM_DEVICECHANGE, flags, (LPARAM)iface, SMTO_ABORTIFHUNG, 2000, + NULL ); + free( iface ); + break; + } + case DBT_DEVTYP_HANDLE: + { + DEV_BROADCAST_HANDLE *bc_handle; + DWORD data_size = data->event.handle.data_size; + DWORD name_size = data->event.handle.name ? wcslen( data->event.handle.name ) + 1 : 0; + DWORD dbch_size = offsetof( DEV_BROADCAST_HANDLE, + dbch_data[name_size + data_size] ); + bc_handle = malloc( dbch_size ); + if (!bc_handle) + return 0; + bc_handle->dbch_size = dbch_size; + bc_handle->dbch_devicetype = DBT_DEVTYP_HANDLE; + bc_handle->dbch_reserved = data->event.handle.header.reserved; + bc_handle->dbch_handle = device_handle; + bc_handle->dbch_hdevnotify = notify; + bc_handle->dbch_eventguid = data->event.handle.event_guid; + bc_handle->dbch_nameoffset = name_size ? data_size : -1; + memcpy( bc_handle->dbch_data, data->event.handle.data, data_size ); + memcpy( &bc_handle->dbch_data[data_size], data->event.handle.name, name_size ); + SendMessageTimeoutW( handle, WM_DEVICECHANGE, flags, (LPARAM)bc_handle, SMTO_ABORTIFHUNG, + 2000, NULL ); + free( bc_handle ); + break; + } + default: + FIXME("unimplemented devicetype %#x\n", data->devicetype); + break; + } + return 0; }
-static DWORD CALLBACK devnotify_window_callbackA(HANDLE handle, DWORD flags, DEV_BROADCAST_HDR *header) +static DWORD CALLBACK devnotify_window_callbackA( HANDLE handle, HDEVNOTIFY notify, DWORD flags, + device_broadcast *data, HANDLE device_handle ) { + TRACE("(%p, %p, %#lx, %s)\n", handle, notify, flags, debugstr_device_broadcast( data )); + if (flags & 0x8000) { - switch (header->dbch_devicetype) + switch (data->devicetype) { case DBT_DEVTYP_DEVICEINTERFACE: { - const DEV_BROADCAST_DEVICEINTERFACE_W *ifaceW = (const DEV_BROADCAST_DEVICEINTERFACE_W *)header; - size_t lenW = wcslen( ifaceW->dbcc_name ); + size_t lenW = wcslen( data->event.device_interface.name ); DEV_BROADCAST_DEVICEINTERFACE_A *ifaceA; DWORD lenA;
if (!(ifaceA = malloc( offsetof(DEV_BROADCAST_DEVICEINTERFACE_A, dbcc_name[lenW * 3 + 1]) ))) return 0; - lenA = WideCharToMultiByte( CP_ACP, 0, ifaceW->dbcc_name, lenW + 1, + lenA = WideCharToMultiByte( CP_ACP, 0, data->event.device_interface.name, lenW + 1, ifaceA->dbcc_name, lenW * 3 + 1, NULL, NULL );
ifaceA->dbcc_size = offsetof(DEV_BROADCAST_DEVICEINTERFACE_A, dbcc_name[lenA + 1]); - ifaceA->dbcc_devicetype = ifaceW->dbcc_devicetype; - ifaceA->dbcc_reserved = ifaceW->dbcc_reserved; - ifaceA->dbcc_classguid = ifaceW->dbcc_classguid; + ifaceA->dbcc_devicetype = data->devicetype; + ifaceA->dbcc_reserved = data->event.device_interface.header.reserved; + ifaceA->dbcc_classguid = data->event.device_interface.class_guid; SendMessageTimeoutA( handle, WM_DEVICECHANGE, flags, (LPARAM)ifaceA, SMTO_ABORTIFHUNG, 2000, NULL ); free( ifaceA ); - return 0; + break; }
- default: - FIXME( "unimplemented W to A mapping for %#lx\n", header->dbch_devicetype ); - /* fall through */ case DBT_DEVTYP_HANDLE: - case DBT_DEVTYP_OEM: + { + DEV_BROADCAST_HANDLE *bc_handle; + size_t lenW = data->event.handle.name ? wcslen( data->event.handle.name ) + 1: 0; + DWORD data_size = data->event.handle.data_size; + DWORD lenA = 0; + + bc_handle = malloc( offsetof( DEV_BROADCAST_HANDLE, dbch_data[lenW * 3 + 1 + data_size] ) ); + if (!bc_handle) + return 0; + + if (lenW) + lenA = WideCharToMultiByte( CP_ACP, 0, data->event.handle.name, lenW + 1, + (char *)&bc_handle->dbch_data[data_size], lenW * 3 + 1, + NULL, NULL ); + bc_handle->dbch_nameoffset = lenW ? data_size : -1; + bc_handle->dbch_size = offsetof( DEV_BROADCAST_HANDLE, dbch_data[lenA + 1] ); + bc_handle->dbch_devicetype = DBT_DEVTYP_HANDLE; + bc_handle->dbch_reserved = data->event.handle.header.reserved; + bc_handle->dbch_handle = device_handle; + bc_handle->dbch_hdevnotify = notify; + bc_handle->dbch_eventguid = data->event.handle.event_guid; + memcpy( bc_handle->dbch_data, data->event.handle.data, data_size ); + SendMessageTimeoutA( handle, WM_DEVICECHANGE, flags, (LPARAM)bc_handle, + SMTO_ABORTIFHUNG, 200, NULL ); + free( bc_handle ); + break; + } + default: + FIXME( "unimplemented W to A mapping for %#x\n", data->devicetype ); break; } }
- SendMessageTimeoutA( handle, WM_DEVICECHANGE, flags, (LPARAM)header, SMTO_ABORTIFHUNG, 2000, NULL ); return 0; }
-static DWORD CALLBACK devnotify_service_callback(HANDLE handle, DWORD flags, DEV_BROADCAST_HDR *header) +static DWORD CALLBACK devnotify_service_callback( HANDLE handle, HDEVNOTIFY notify, DWORD flags, + device_broadcast *data, HANDLE device_handle ) { FIXME("Support for service handles is not yet implemented!\n"); return 0; }
-struct device_notification_details -{ - DWORD (CALLBACK *cb)(HANDLE handle, DWORD flags, DEV_BROADCAST_HDR *header); - HANDLE handle; - union - { - DEV_BROADCAST_HDR header; - DEV_BROADCAST_DEVICEINTERFACE_W iface; - } filter; -}; - -extern HDEVNOTIFY WINAPI I_ScRegisterDeviceNotification( struct device_notification_details *details, - void *filter, DWORD flags ); -extern BOOL WINAPI I_ScUnregisterDeviceNotification( HDEVNOTIFY handle ); - /*********************************************************************** * RegisterDeviceNotificationA (USER32.@) * @@ -580,7 +648,7 @@ HDEVNOTIFY WINAPI RegisterDeviceNotificationA( HANDLE handle, void *filter, DWOR */ HDEVNOTIFY WINAPI RegisterDeviceNotificationW( HANDLE handle, void *filter, DWORD flags ) { - struct device_notification_details details; + struct device_notification_details details = {0}; DEV_BROADCAST_HDR *header = filter;
TRACE("handle %p, filter %p, flags %#lx\n", handle, filter, flags); @@ -597,21 +665,42 @@ HDEVNOTIFY WINAPI RegisterDeviceNotificationW( HANDLE handle, void *filter, DWOR return NULL; }
- if (!header) details.filter.header.dbch_devicetype = 0; + if (!header) details.devicetype = 0; else if (header->dbch_devicetype == DBT_DEVTYP_DEVICEINTERFACE) { DEV_BROADCAST_DEVICEINTERFACE_W *iface = (DEV_BROADCAST_DEVICEINTERFACE_W *)header; - details.filter.iface = *iface;
+ details.devicetype = DBT_DEVTYP_DEVICEINTERFACE; if (flags & DEVICE_NOTIFY_ALL_INTERFACE_CLASSES) - details.filter.iface.dbcc_size = offsetof( DEV_BROADCAST_DEVICEINTERFACE_W, dbcc_classguid ); + details.filter.deviceinterface.all_classes = TRUE; else - details.filter.iface.dbcc_size = offsetof( DEV_BROADCAST_DEVICEINTERFACE_W, dbcc_name ); + details.filter.deviceinterface.class = iface->dbcc_classguid; } else if (header->dbch_devicetype == DBT_DEVTYP_HANDLE) { - FIXME( "DBT_DEVTYP_HANDLE filter type not implemented\n" ); - details.filter.header.dbch_devicetype = 0; + OBJECT_NAME_INFORMATION *name_info; + DEV_BROADCAST_HANDLE *handle = (DEV_BROADCAST_HANDLE *)header; + HANDLE device = handle->dbch_handle; + NTSTATUS status; + DWORD size; + + details.devicetype = DBT_DEVTYP_HANDLE; + status = NtQueryObject( device, ObjectNameInformation, NULL, 0, &size ); + if (status != STATUS_INFO_LENGTH_MISMATCH) + { + SetLastError( RtlNtStatusToDosError( status ) ); + return NULL; + } + + name_info = malloc( size ); + if (!name_info) + { + SetLastError( ERROR_NOT_ENOUGH_MEMORY ); + return NULL; + } + + details.filter.device.device = device; + details.filter.device.name_info = name_info; } else { diff --git a/dlls/user32/plugplay.idl b/dlls/user32/plugplay.idl new file mode 100644 index 00000000000..6b60a7efa75 --- /dev/null +++ b/dlls/user32/plugplay.idl @@ -0,0 +1,2 @@ +#pragma makedep header +#include "wine/plugplay.idl" diff --git a/include/wine/dbt.h b/include/wine/dbt.h index d6955c1d9d7..7ae6c04a666 100644 --- a/include/wine/dbt.h +++ b/include/wine/dbt.h @@ -68,9 +68,9 @@ static inline const char *debugstr_device_broadcast(const device_broadcast *data debugstr_w( data->event.handle.handle_file_path ), debugstr_guid( &data->event.handle.event_guid ), debugstr_w( data->event.handle.name ), - data->event.handle.data_size, data->event.handle.data ); + (int)data->event.handle.data_size, data->event.handle.data ); default: - return wine_dbg_sprintf( "{type=%#lx %p}\n", data->devicetype, data ); + return wine_dbg_sprintf( "{type=%#x %p}\n", data->devicetype, data ); } }
diff --git a/include/wine/plugplay.idl b/include/wine/plugplay.idl index d5aaebe388e..97740f51c6e 100644 --- a/include/wine/plugplay.idl +++ b/include/wine/plugplay.idl @@ -65,7 +65,7 @@ interface plugplay typedef [context_handle] void *plugplay_rpc_handle;
plugplay_rpc_handle plugplay_register_listener(); - DWORD plugplay_get_event([in] plugplay_rpc_handle handle, [out, size_is(,*size)] BYTE **data, [out] unsigned int *size); void plugplay_unregister_listener([in] plugplay_rpc_handle handle); - void plugplay_send_event([in] DWORD event_code, [in, size_is(size)] const BYTE *data, [in] unsigned int size); + void plugplay_send_event([in] DWORD event_code, [in] const device_broadcast *broadcast); + DWORD plugplay_get_event([in] plugplay_rpc_handle handle, [out] device_broadcast *broadcast); } diff --git a/programs/plugplay/main.c b/programs/plugplay/main.c index 8426f2204a3..ea1978493f8 100644 --- a/programs/plugplay/main.c +++ b/programs/plugplay/main.c @@ -19,12 +19,15 @@ #define WIN32_LEAN_AND_MEAN
#include <windows.h> +#include <winternl.h> #include <dbt.h> #include "winsvc.h" #include "wine/debug.h" #include "wine/list.h" #include "plugplay.h"
+#include "wine/dbt.h" + WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
static WCHAR plugplayW[] = L"PlugPlay"; @@ -64,11 +67,9 @@ struct event { struct list entry; DWORD code; - BYTE *data; - unsigned int size; + device_broadcast data; };
- static void destroy_listener( struct listener *listener ) { struct event *event, *next; @@ -79,7 +80,18 @@ static void destroy_listener( struct listener *listener )
LIST_FOR_EACH_ENTRY_SAFE(event, next, &listener->events, struct event, entry) { - MIDL_user_free( event->data ); + switch (event->data.devicetype) + { + case DBT_DEVTYP_HANDLE: + MIDL_user_free( event->data.event.handle.name ); + MIDL_user_free( event->data.event.handle.handle_file_path ); + MIDL_user_free( event->data.event.handle.data ); + break; + case DBT_DEVTYP_DEVICEINTERFACE: + MIDL_user_free( event->data.event.device_interface.name ); + break; + } + list_remove( &event->entry ); free( event ); } @@ -108,7 +120,7 @@ plugplay_rpc_handle __cdecl plugplay_register_listener(void) return listener; }
-DWORD __cdecl plugplay_get_event( plugplay_rpc_handle handle, BYTE **data, unsigned int *size ) +DWORD __cdecl plugplay_get_event( plugplay_rpc_handle handle, device_broadcast *data ) { struct listener *listener = handle; struct event *event; @@ -117,17 +129,16 @@ DWORD __cdecl plugplay_get_event( plugplay_rpc_handle handle, BYTE **data, unsig
EnterCriticalSection( &plugplay_cs );
- while (!(entry = list_head( &listener->events ))) + while(!(entry = list_head( &listener->events ))) SleepConditionVariableCS( &listener->cv, &plugplay_cs, INFINITE );
- event = LIST_ENTRY(entry, struct event, entry); - list_remove( &event->entry ); + event = LIST_ENTRY( entry, struct event, entry ); + list_remove(&event->entry);
LeaveCriticalSection( &plugplay_cs );
ret = event->code; *data = event->data; - *size = event->size; free( event ); return ret; } @@ -137,35 +148,104 @@ void __cdecl plugplay_unregister_listener( plugplay_rpc_handle handle ) destroy_listener( handle ); }
-void __cdecl plugplay_send_event( DWORD code, const BYTE *data, unsigned int size ) +void __cdecl plugplay_send_event( DWORD event_code, const device_broadcast *data ) { struct listener *listener; - struct event *event; + const WCHAR *name_src = NULL; + DWORD str_size = 0;
- BroadcastSystemMessageW( 0, NULL, WM_DEVICECHANGE, code, (LPARAM)data ); - BroadcastSystemMessageW( 0, NULL, WM_DEVICECHANGE, DBT_DEVNODES_CHANGED, 0 ); + TRACE( "(%#lx, %s)\n", event_code, debugstr_device_broadcast( data ) );
- EnterCriticalSection( &plugplay_cs ); + switch(data->devicetype) + { + case DBT_DEVTYP_HANDLE: + name_src = data->event.handle.name; + break; + case DBT_DEVTYP_DEVICEINTERFACE: + { + const device_broadcast_deviceinterface *event = &data->event.device_interface; + DWORD name_len = wcslen( event->name ) + 1; + DWORD dbcc_size = offsetof( DEV_BROADCAST_DEVICEINTERFACE_W, dbcc_name[name_len] ); + DEV_BROADCAST_DEVICEINTERFACE_W *iface; + + name_src = event->name; + + iface = malloc( dbcc_size ); + if (!iface) break; + iface->dbcc_size = dbcc_size; + iface->dbcc_devicetype = DBT_DEVTYP_DEVICEINTERFACE; + iface->dbcc_reserved = event->header.reserved; + iface->dbcc_classguid = event->class_guid; + memcpy( iface->dbcc_name, event->name, name_len ); + BroadcastSystemMessageW( 0, NULL, WM_DEVICECHANGE, event_code, (LPARAM)iface ); + BroadcastSystemMessageW( 0, NULL, WM_DEVICECHANGE, DBT_DEVNODES_CHANGED, 0 ); + free( iface ); + break; + } + default: + ERR("unknown devicetype value: %#x\n", data->devicetype); + return; + }
+ str_size = name_src ? (lstrlenW(name_src) + 1) * sizeof(WCHAR) : 0; + + EnterCriticalSection( &plugplay_cs ); LIST_FOR_EACH_ENTRY(listener, &listener_list, struct listener, entry) { - if (!(event = malloc( sizeof(*event) ))) - break; + struct event *event = calloc( 1, sizeof(*event)); + if (!event) break;
- if (!(event->data = malloc( size ))) + event->code = event_code; + + event->data = *data; + if (data->devicetype == DBT_DEVTYP_HANDLE) { - free( event ); - break; + DWORD size = sizeof(WCHAR) * ( lstrlenW( data->event.handle.handle_file_path ) + 1 ); + event->data.event.handle.handle_file_path = malloc( size ); + if (!event->data.event.handle.handle_file_path) + { + free( event ); + break; + } + memcpy( event->data.event.handle.handle_file_path, data->event.handle.handle_file_path, size ); + + if (data->event.handle.data_size != 0) + { + event->data.event.handle.data = malloc( data->event.handle.data_size ); + if (!event->data.event.handle.data) + { + free( event->data.event.handle.handle_file_path ); + free( event ); + break; + } + memcpy( event->data.event.handle.data, data->event.handle.data, data->event.handle.data_size); + } + } + if (str_size != 0) + { + WCHAR *str = malloc( str_size ); + if (!str) + { + if (data->devicetype == DBT_DEVTYP_HANDLE) + { + free( event->data.event.handle.handle_file_path ); + free( event->data.event.handle.data ); + } + free( event ); + break; + } + memcpy( str, name_src, str_size ); + + if (data->devicetype == DBT_DEVTYP_HANDLE) + event->data.event.handle.name = str; + else + event->data.event.device_interface.name = str; } - - event->code = code; - memcpy( event->data, data, size ); - event->size = size; list_add_tail( &listener->events, &event->entry ); WakeConditionVariable( &listener->cv ); } - LeaveCriticalSection( &plugplay_cs ); + return; }
static DWORD WINAPI service_handler( DWORD ctrl, DWORD event_type, LPVOID event_data, LPVOID context )
From: Vibhav Pant vibhavp@gmail.com
As incoming events in plugplay_send_event are additionally broadcast asynchronously, free-ing them immediately after the call to BroadcastSystemMessageW would result in receiving threads potentially accessing invalid memory.
To prevent this, perform these broadcasts synchronously on a dedicated thread with its own listener and event queue, which is spawned during service startup. For events of type DBT_DEVTYP_DEVICEINTERFACE, plugplay_send_event will stick an extra copy of the event into the listener's event queue, which then gets broadcast synchronously (and then freed) in the "broadcast listener" thread. --- programs/plugplay/main.c | 250 ++++++++++++++++++++++++++------------- 1 file changed, 166 insertions(+), 84 deletions(-)
diff --git a/programs/plugplay/main.c b/programs/plugplay/main.c index ea1978493f8..e70e9b91c83 100644 --- a/programs/plugplay/main.c +++ b/programs/plugplay/main.c @@ -1,5 +1,6 @@ /* * Copyright 2011 Hans Leidekker for CodeWeavers + * Copyright 2024 Vibhav Pant * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -63,6 +64,12 @@ struct listener CONDITION_VARIABLE cv; };
+/* A separate listener used to broadcast DBT_DEVTYP_DEVICEINTERFACE events received + * in plugplay_send_event. */ +static struct listener broadcast_listener; +#define EVENT_CODE_SHUTDOWN (~0) +static HANDLE broadcast_listener_thread; + struct event { struct list entry; @@ -70,15 +77,11 @@ struct event device_broadcast data; };
-static void destroy_listener( struct listener *listener ) +static void listener_free_events( struct list *events ) { struct event *event, *next;
- EnterCriticalSection( &plugplay_cs ); - list_remove( &listener->entry ); - LeaveCriticalSection( &plugplay_cs ); - - LIST_FOR_EACH_ENTRY_SAFE(event, next, &listener->events, struct event, entry) + LIST_FOR_EACH_ENTRY_SAFE(event, next, events, struct event, entry) { switch (event->data.devicetype) { @@ -95,6 +98,59 @@ static void destroy_listener( struct listener *listener ) list_remove( &event->entry ); free( event ); } +} + +static DWORD WINAPI broadcast_listener_proc( void *arg ) +{ + while (TRUE) + { + device_broadcast broadcast = {0}; + const device_broadcast_deviceinterface *event; + DEV_BROADCAST_DEVICEINTERFACE_W *iface; + DWORD name_len; + DWORD dbcc_size; + DWORD code; + + code = plugplay_get_event( &broadcast_listener, &broadcast ); + if (code == EVENT_CODE_SHUTDOWN) break; + + event = &broadcast.event.device_interface; + name_len = wcslen( event->name ) + 1; + dbcc_size = offsetof( DEV_BROADCAST_DEVICEINTERFACE_W, dbcc_name[name_len] ); + + iface = malloc( dbcc_size ); + if (!iface) continue; + + iface->dbcc_size = dbcc_size; + iface->dbcc_devicetype = DBT_DEVTYP_DEVICEINTERFACE; + iface->dbcc_reserved = event->header.reserved; + iface->dbcc_classguid = event->class_guid; + memcpy( iface->dbcc_name, event->name, name_len ); + TRACE( "sending %s\n", debugstr_device_broadcast( &broadcast ) ); + BroadcastSystemMessageW( 0, NULL, BSF_FORCEIFHUNG | BSF_QUERY, code, (LPARAM)iface ); + BroadcastSystemMessageW( 0, NULL, WM_DEVICECHANGE, DBT_DEVNODES_CHANGED, 0 ); + free( iface ); + free( event->name ); + } + + EnterCriticalSection(&plugplay_cs); + /* Since broacast_listener is accessed outside of listener_list (and therefore can still be + * accessed by plugplay_send_event), we need to hold plugplay_cs while freeing any remaining + * events. */ + listener_free_events( &broadcast_listener.events ); + LeaveCriticalSection( &plugplay_cs ); + + TRACE( "shutting down the broadcast listener thread\n" ); + return 0; +} + +static void destroy_listener( struct listener *listener ) +{ + EnterCriticalSection( &plugplay_cs ); + list_remove( &listener->entry ); + LeaveCriticalSection( &plugplay_cs ); + + listener_free_events( &listener->events ); free( listener ); }
@@ -103,15 +159,22 @@ void __RPC_USER plugplay_rpc_handle_rundown( plugplay_rpc_handle handle ) destroy_listener( handle ); }
+static plugplay_rpc_handle plugplay_init_listener( struct listener *listener ) +{ + list_init( &listener->events ); + InitializeConditionVariable( &listener->cv ); + + return listener; +} + plugplay_rpc_handle __cdecl plugplay_register_listener(void) { struct listener *listener;
- if (!(listener = calloc( 1, sizeof(*listener) ))) + if (!(listener = calloc( 1, sizeof(*listener)))) return NULL;
- list_init( &listener->events ); - InitializeConditionVariable( &listener->cv ); + plugplay_init_listener( listener );
EnterCriticalSection( &plugplay_cs ); list_add_tail( &listener_list, &listener->entry ); @@ -148,99 +211,99 @@ void __cdecl plugplay_unregister_listener( plugplay_rpc_handle handle ) destroy_listener( handle ); }
-void __cdecl plugplay_send_event( DWORD event_code, const device_broadcast *data ) +static struct event *new_event( DWORD event_code, const device_broadcast *src ) { - struct listener *listener; const WCHAR *name_src = NULL; - DWORD str_size = 0; + WCHAR *name_copy = NULL; + struct event *event;
- TRACE( "(%#lx, %s)\n", event_code, debugstr_device_broadcast( data ) ); + event = calloc( 1, sizeof( *event ) ); + if (!event) return NULL;
- switch(data->devicetype) + name_src = src->devicetype == DBT_DEVTYP_HANDLE ? src->event.handle.name + : src->event.device_interface.name; + if (name_src) { - case DBT_DEVTYP_HANDLE: - name_src = data->event.handle.name; - break; - case DBT_DEVTYP_DEVICEINTERFACE: - { - const device_broadcast_deviceinterface *event = &data->event.device_interface; - DWORD name_len = wcslen( event->name ) + 1; - DWORD dbcc_size = offsetof( DEV_BROADCAST_DEVICEINTERFACE_W, dbcc_name[name_len] ); - DEV_BROADCAST_DEVICEINTERFACE_W *iface; + SIZE_T str_size = (lstrlenW(name_src) + 1) * sizeof(WCHAR); + name_copy = malloc( str_size ); + if (!name_copy) + { + free( event ); + return NULL; + } + memcpy( name_copy, name_src, str_size ); + }
- name_src = event->name; + event->code = event_code; + event->data = *src; + if (src->devicetype == DBT_DEVTYP_HANDLE) + { + const device_broadcast_handle *event_handle = &src->event.handle; + DWORD size = sizeof(WCHAR) * (lstrlenW( event_handle->handle_file_path ) + 1); + WCHAR *path = malloc( size ); + if (!path) + { + if (name_copy) free( name_copy ); + free( event ); + return NULL; + } + memcpy( path, event_handle->handle_file_path, size ); + event->data.event.handle.handle_file_path = path;
- iface = malloc( dbcc_size ); - if (!iface) break; - iface->dbcc_size = dbcc_size; - iface->dbcc_devicetype = DBT_DEVTYP_DEVICEINTERFACE; - iface->dbcc_reserved = event->header.reserved; - iface->dbcc_classguid = event->class_guid; - memcpy( iface->dbcc_name, event->name, name_len ); - BroadcastSystemMessageW( 0, NULL, WM_DEVICECHANGE, event_code, (LPARAM)iface ); - BroadcastSystemMessageW( 0, NULL, WM_DEVICECHANGE, DBT_DEVNODES_CHANGED, 0 ); - free( iface ); - break; + if (event_handle->data_size != 0) + { + void *data = malloc( event_handle->data_size ); + if (!data) + { + if (name_copy) free( name_copy ); + free( path ); + free( event ); + return NULL; + } + memcpy( data, event_handle->data, event_handle->data_size ); + event->data.event.handle.data = data; + } + event->data.event.handle.name = name_copy; } - default: + else /* DBT_DEVTYP_DEVICEINTERFACE */ + { + event->data.event.device_interface.name = name_copy; + } + + return event; +} + +void __cdecl plugplay_send_event( DWORD event_code, const device_broadcast *data ) +{ + struct listener *listener; + + TRACE( "(%#lx, %s)\n", event_code, debugstr_device_broadcast( data ) ); + + if (data->devicetype != DBT_DEVTYP_HANDLE && data->devicetype != DBT_DEVTYP_DEVICEINTERFACE) + { ERR("unknown devicetype value: %#x\n", data->devicetype); return; }
- str_size = name_src ? (lstrlenW(name_src) + 1) * sizeof(WCHAR) : 0;
EnterCriticalSection( &plugplay_cs ); - LIST_FOR_EACH_ENTRY(listener, &listener_list, struct listener, entry) + /* For DBT_DEVTYP_DEVICEINTERFACE events, stick an extra copy of the event into + * broadcast_listener's queue. */ + if (data->devicetype == DBT_DEVTYP_DEVICEINTERFACE) { - struct event *event = calloc( 1, sizeof(*event)); - if (!event) break; - - event->code = event_code; - - event->data = *data; - if (data->devicetype == DBT_DEVTYP_HANDLE) + struct event *event = new_event( event_code, data ); + if (event) { - DWORD size = sizeof(WCHAR) * ( lstrlenW( data->event.handle.handle_file_path ) + 1 ); - event->data.event.handle.handle_file_path = malloc( size ); - if (!event->data.event.handle.handle_file_path) - { - free( event ); - break; - } - memcpy( event->data.event.handle.handle_file_path, data->event.handle.handle_file_path, size ); - - if (data->event.handle.data_size != 0) - { - event->data.event.handle.data = malloc( data->event.handle.data_size ); - if (!event->data.event.handle.data) - { - free( event->data.event.handle.handle_file_path ); - free( event ); - break; - } - memcpy( event->data.event.handle.data, data->event.handle.data, data->event.handle.data_size); - } + list_add_tail( &broadcast_listener.events, &event->entry ); + WakeConditionVariable( &broadcast_listener.cv ); } - if (str_size != 0) - { - WCHAR *str = malloc( str_size ); - if (!str) - { - if (data->devicetype == DBT_DEVTYP_HANDLE) - { - free( event->data.event.handle.handle_file_path ); - free( event->data.event.handle.data ); - } - free( event ); - break; - } - memcpy( str, name_src, str_size ); + } + + LIST_FOR_EACH_ENTRY(listener, &listener_list, struct listener, entry) + { + struct event *event = new_event( event_code, data ); + if (!event) break;
- if (data->devicetype == DBT_DEVTYP_HANDLE) - event->data.event.handle.name = str; - else - event->data.event.device_interface.name = str; - } list_add_tail( &listener->events, &event->entry ); WakeConditionVariable( &listener->cv ); } @@ -283,6 +346,7 @@ static void WINAPI ServiceMain( DWORD argc, LPWSTR *argv ) unsigned char protseq[] = "ncacn_np"; SERVICE_STATUS status; RPC_STATUS err; + struct event *shutdown_event;
WINE_TRACE( "starting service\n" );
@@ -301,9 +365,19 @@ static void WINAPI ServiceMain( DWORD argc, LPWSTR *argv ) ERR("RpcServerListen() failed, error %lu\n", err); return; } + if (!(shutdown_event = calloc(1, sizeof(*shutdown_event)))) + { + ERR("Failed to allocate memory for shutdown event\n"); + return; + } + shutdown_event->code = EVENT_CODE_SHUTDOWN;
stop_event = CreateEventW( NULL, TRUE, FALSE, NULL );
+ plugplay_init_listener(&broadcast_listener); + broadcast_listener_thread = + CreateThread( NULL, 0, broadcast_listener_proc, NULL, 0, NULL ); + service_handle = RegisterServiceCtrlHandlerExW( plugplayW, service_handler, NULL ); if (!service_handle) return; @@ -319,6 +393,14 @@ static void WINAPI ServiceMain( DWORD argc, LPWSTR *argv )
WaitForSingleObject( stop_event, INFINITE );
+ /* First, shutdown our broadcast listener. */ + EnterCriticalSection( &plugplay_cs ); + list_add_tail( &broadcast_listener.events, &shutdown_event->entry ); + LeaveCriticalSection( &plugplay_cs ); + + WaitForSingleObject( broadcast_listener_thread, INFINITE ); + CloseHandle( broadcast_listener_thread ); + RpcMgmtStopServerListening( NULL ); RpcServerUnregisterIf( plugplay_v0_0_s_ifspec, NULL, TRUE ); RpcMgmtWaitServerListen();
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=147872
Your paranoid android.
=== debian11b (64 bit WoW report) ===
ddraw: ddraw7.c:3822: Test failed: Expected (0,0)-(640,480), got (0,0)-(1024,768). ddraw7.c:3847: Test failed: Expected (0,0)-(640,480), got (0,0)-(1024,768).
Elizabeth Figura (@zfigura) commented about dlls/user32/plugplay.idl:
+#pragma makedep header +#include "wine/plugplay.idl"
That shouldn't be necessary? If we're only using it for the header, we should be able to include "wine/plugplay.h" directly in user32.
Elizabeth Figura (@zfigura) commented about include/wine/dbt.h:
debugstr_w( data->event.handle.handle_file_path ), debugstr_guid( &data->event.handle.event_guid ), debugstr_w( data->event.handle.name ),
data->event.handle.data_size, data->event.handle.data );
(int)data->event.handle.data_size, data->event.handle.data );
These hunks should be fixed up to the commit that introduces them.
Note that if you're using DWORD (and you could just as easily use "unsigned int" instead) you probably want "%lu" rather than casting to int.
Elizabeth Figura (@zfigura) commented about programs/plugplay/main.c:
if (data->devicetype == DBT_DEVTYP_HANDLE)event->data.event.handle.name = str;elseevent->data.event.device_interface.name = str; }
event->code = code;memcpy( event->data, data, size );event->size = size; list_add_tail( &listener->events, &event->entry ); WakeConditionVariable( &listener->cv );- LeaveCriticalSection( &plugplay_cs );
- return;
This return can go away.
Elizabeth Figura (@zfigura) commented about programs/plugplay/main.c:
EnterCriticalSection( &plugplay_cs );
- while (!(entry = list_head( &listener->events )))
- while(!(entry = list_head( &listener->events ))) SleepConditionVariableCS( &listener->cv, &plugplay_cs, INFINITE );
- event = LIST_ENTRY(entry, struct event, entry);
- list_remove( &event->entry );
- event = LIST_ENTRY( entry, struct event, entry );
- list_remove(&event->entry);
There's a lot of unnecessary whitespace changes here, and they run contrary to the usual style in Wine. We use a space after if/while/switch, (at least in user32 and plugplay) a space inside function calls and function-like macros (but no space inside if/while/switch conditions).
Some other code in this merge request also has inconsistent spacing.
Elizabeth Figura (@zfigura) commented about programs/plugplay/main.c:
destroy_listener( handle );}
-void __cdecl plugplay_send_event( DWORD code, const BYTE *data, unsigned int size ) +void __cdecl plugplay_send_event( DWORD event_code, const device_broadcast *data )
The changes to this function are not the easiest to review, since it both refactors the existing code (to use the new RPC structures) and adds handle support at the same time. It would be nice if these could be split into separate commits.
There are also unnecessary changes like "code" -> "event_code".
Elizabeth Figura (@zfigura) commented about programs/plugplay/main.c:
+void __cdecl plugplay_send_event( DWORD event_code, const device_broadcast *data ) { struct listener *listener;
- struct event *event;
- const WCHAR *name_src = NULL;
- DWORD str_size = 0;
- BroadcastSystemMessageW( 0, NULL, WM_DEVICECHANGE, code, (LPARAM)data );
- BroadcastSystemMessageW( 0, NULL, WM_DEVICECHANGE, DBT_DEVNODES_CHANGED, 0 );
- TRACE( "(%#lx, %s)\n", event_code, debugstr_device_broadcast( data ) );
- EnterCriticalSection( &plugplay_cs );
- switch(data->devicetype)
- {
- case DBT_DEVTYP_HANDLE:
name_src = data->event.handle.name;
The logic surrounding "name_src" in this function seems awkwardly structured. The point is apparently to deep-copy the structure contents, but the two arms have different fields, and trying to unify one of the strings is unidiomatic and I don't think really saves you anything. Rather, I think this function should
(1) convert to DEV_BROADCAST_DEVICEINTERFACE_W and broadcast (2) deep-copy the struct (3) queue the copied struct
as three separate and subsequent operations. (2) should probably also be a helper function.
Elizabeth Figura (@zfigura) commented about include/wine/plugplay.idl:
import "wtypes.idl";
+typedef enum _broadcast_device_type +{
- /* DBT_DEVTYP_DEVICEINTERFACE */
- BROADCAST_DEVICE_TYPE_DEVICEINTERFACE = 5,
- /* DBT_DEVTYP_HANDLE */
- BROADCAST_DEVICE_TYPE_HANDLE = 6
+} broadcast_device_type;
Do we actually need this enum? Can we just use the DBT_DEVTYP_* values directly? They're not in an enum, but IIRC RPC doesn't care.
Elizabeth Figura (@zfigura) commented about include/wine/plugplay.idl:
import "wtypes.idl";
+typedef enum _broadcast_device_type +{
- /* DBT_DEVTYP_DEVICEINTERFACE */
- BROADCAST_DEVICE_TYPE_DEVICEINTERFACE = 5,
- /* DBT_DEVTYP_HANDLE */
- BROADCAST_DEVICE_TYPE_HANDLE = 6
+} broadcast_device_type;
+typedef struct _device_broadcast_header +{
- DWORD reserved;
+} device_broadcast_header;
We don't currently fill this, and the tests suggest it's just always zero. I think we can just get rid of it in our internal structs.
Elizabeth Figura (@zfigura) commented about include/wine/plugplay.idl:
import "wtypes.idl";
+typedef enum _broadcast_device_type +{
- /* DBT_DEVTYP_DEVICEINTERFACE */
- BROADCAST_DEVICE_TYPE_DEVICEINTERFACE = 5,
- /* DBT_DEVTYP_HANDLE */
- BROADCAST_DEVICE_TYPE_HANDLE = 6
+} broadcast_device_type;
+typedef struct _device_broadcast_header +{
- DWORD reserved;
+} device_broadcast_header;
+typedef struct _device_broadcast_deviceinterface
I would get rid of the typedef as well. Generally I think 'struct X' is better by virtue of being more explicit.
Elizabeth Figura (@zfigura) commented about include/wine/dbt.h:
BOOL all_classes;GUID class;} deviceinterface;struct{/* The path for the device file the notification is originating from. */OBJECT_NAME_INFORMATION *name_info;/* A handle to the device file. It is passed as is to the callback. */HANDLE device;} device;- } filter;
+};
+extern HDEVNOTIFY WINAPI I_ScRegisterDeviceNotification( struct device_notification_details *details,
void *filter, DWORD flags );+extern BOOL WINAPI I_ScUnregisterDeviceNotification( HDEVNOTIFY handle );
If you're going to add these, you should use them in the same commit.
Elizabeth Figura (@zfigura) commented about include/wine/dbt.h:
debugstr_guid( &data->event.device_interface.class_guid ),debugstr_w( data->event.device_interface.name ) );case DBT_DEVTYP_HANDLE:return wine_dbg_sprintf( "{%#lx DBT_DEVTYP_HANDLE %s %s %s %d %p}",data->event.device_interface.header.reserved,debugstr_w( data->event.handle.handle_file_path ),debugstr_guid( &data->event.handle.event_guid ),debugstr_w( data->event.handle.name ),data->event.handle.data_size, data->event.handle.data );default:return wine_dbg_sprintf( "{type=%#lx %p}\n", data->devicetype, data );- }
+}
+static inline const char * +debugstr_device_notification_details( const struct device_notification_details *details )
This is only used in sechost. As such it could be moved there, and even inlined.
Elizabeth Figura (@zfigura) commented about include/wine/dbt.h:
} deviceinterface;struct{/* The path for the device file the notification is originating from. */OBJECT_NAME_INFORMATION *name_info;/* A handle to the device file. It is passed as is to the callback. */HANDLE device;} device;- } filter;
+};
+extern HDEVNOTIFY WINAPI I_ScRegisterDeviceNotification( struct device_notification_details *details,
void *filter, DWORD flags );+extern BOOL WINAPI I_ScUnregisterDeviceNotification( HDEVNOTIFY handle );
+static inline const char *debugstr_device_broadcast(const device_broadcast *data)
Same here—if you're going to add this it shouldn't be dead code; you should use it in the same commit.
Elizabeth Figura (@zfigura) commented about dlls/sechost/service.c:
- if (details->devicetype != data->devicetype) return FALSE;
- if (filter->dbch_devicetype == DBT_DEVTYP_DEVICEINTERFACE)
- if (details->devicetype == DBT_DEVTYP_DEVICEINTERFACE) {
DEV_BROADCAST_DEVICEINTERFACE_W *filter_iface = (DEV_BROADCAST_DEVICEINTERFACE_W *)filter;DEV_BROADCAST_DEVICEINTERFACE_W *event_iface = (DEV_BROADCAST_DEVICEINTERFACE_W *)event;if (filter_iface->dbcc_size == offsetof(DEV_BROADCAST_DEVICEINTERFACE_W, dbcc_classguid)) return TRUE;return IsEqualGUID( &filter_iface->dbcc_classguid, &event_iface->dbcc_classguid );
if (details->filter.deviceinterface.all_classes) return TRUE;return IsEqualGUID( &details->filter.deviceinterface.class, &data->event.device_interface.class_guid );
- FIXME( "Filter dbch_devicetype %lu not implemented\n", filter->dbch_devicetype );
- return TRUE;
- return !lstrcmpiW( details->filter.device.name_info->Name.Buffer, data->event.handle.handle_file_path );
Shouldn't we be explicitly checking for DBT_DEVTYP_* here?
Also, I'd prefer the wcs* family to the nonstandard lstr* family in new code.
Elizabeth Figura (@zfigura) commented about dlls/sechost/service.c:
- if (details->devicetype == DBT_DEVTYP_DEVICEINTERFACE) {
DEV_BROADCAST_DEVICEINTERFACE_W *filter_iface = (DEV_BROADCAST_DEVICEINTERFACE_W *)filter;DEV_BROADCAST_DEVICEINTERFACE_W *event_iface = (DEV_BROADCAST_DEVICEINTERFACE_W *)event;if (filter_iface->dbcc_size == offsetof(DEV_BROADCAST_DEVICEINTERFACE_W, dbcc_classguid)) return TRUE;return IsEqualGUID( &filter_iface->dbcc_classguid, &event_iface->dbcc_classguid );
if (details->filter.deviceinterface.all_classes) return TRUE;return IsEqualGUID( &details->filter.deviceinterface.class, &data->event.device_interface.class_guid );
- FIXME( "Filter dbch_devicetype %lu not implemented\n", filter->dbch_devicetype );
- return TRUE;
- return !lstrcmpiW( details->filter.device.name_info->Name.Buffer, data->event.handle.handle_file_path );
}
+struct device_notification_details_with_reg
"with_reg" is not very communicative; "with_handle" would be better. Or maybe "..._details_copy"?
I haven't looked at 4/4 yet, but I've given feedback for the first parts at least.
1/4 should also be squashed into the first commit that actually uses those structs.
On Tue Aug 20 22:57:46 2024 +0000, Elizabeth Figura wrote:
That shouldn't be necessary? If we're only using it for the header, we should be able to include "wine/plugplay.h" directly in user32.
`wine/plugplay.h` doesn't seem to get generated as part of the build. Is it because it's missing a `makedep header` pragma directive? I was afraid adding it to wine/plugplay.idl would break it somewhere else, so I chose to do it separately for user32.
On Wed Aug 21 12:39:31 2024 +0000, Vibhav Pant wrote:
`wine/plugplay.h` doesn't seem to get generated as part of the build. Is it because it's missing a `makedep header` pragma directive? I was afraid adding it to wine/plugplay.idl would break it somewhere else, so I chose to do it separately for user32.
Never mind, adding the directive fixes it.
Note that if you're using DWORD (and you could just as easily use "unsigned int" instead) you probably want "%lu" rather than casting to int.
Yeah, I was a little confused if there's a format specification convention used by Wine for unsigned type variables used for storing size. Except for `%#Ix` used to `SIZE_T`, I could only see `ULONG`s being cast to int and then being printed with `%d`, so that's what I went with to be safe. But yeah, `%lu` makes more sense.
-
Elizabeth Figura (＠zfigura) -
Marvin -
Vibhav Pant
-
Vibhav Pant (＠vibhavp)