[prev in list] [next in list] [prev in thread] [next in thread]
List: wine-devel
Subject: Re: [PATCH 1/4] ntoskrnl.exe: Implement IoInvalidateDeviceRelations to initialize device driver stor
From: Sebastian Lackner <sebastian () fds-team ! de>
Date: 2016-08-31 7:10:22
Message-ID: 62b6eb7b-cbd3-2391-ada3-17535b22687b () fds-team ! de
[Download RAW message or body]
On 29.08.2016 20:49, Aric Stewart wrote:
> We are not handling installing drivers just yet, so we just make use of the
> critical device store. These are drivers that, on Windows, are automatically
> loaded on boot every time. This database is maintained in
> [CurrentControlSet/Control/CriticalDeviceDatabase] The keys in this registry key
> represent a HardwareID that match a device. We query the IDs from the bus
> device's BusQueryHardwareIDs and if we find a match (from most specific to most
> general) we look at that registry key. The CriticalDeviceDatabase entry
> specify a [Service] value representing the driver.
>
> Signed-off-by: Aric Stewart <aric@codeweavers.com>
> ---
> dlls/ntoskrnl.exe/Makefile.in | 3 +-
> dlls/ntoskrnl.exe/ntoskrnl.exe.spec | 2 +-
> dlls/ntoskrnl.exe/pnp_manager.c | 185 ++++++++++++++++++++++++++++++++++++
> include/ddk/wdm.h | 1 +
> 4 files changed, 189 insertions(+), 2 deletions(-)
> create mode 100644 dlls/ntoskrnl.exe/pnp_manager.c
>
At some places the patch does not really use a consistent style yet (for example VOID \
<-> void, placement of "*" inbetween of type and variable name, "sz" prefix for \
variables, ...), however I will focus mainly on the technical aspects in this review.
>
>
> 0001-ntoskrnl.exe-Implement-IoInvalidateDeviceRelations-to-.txt
>
>
> diff --git a/dlls/ntoskrnl.exe/Makefile.in b/dlls/ntoskrnl.exe/Makefile.in
> index b459c54..2fa768b 100644
> --- a/dlls/ntoskrnl.exe/Makefile.in
> +++ b/dlls/ntoskrnl.exe/Makefile.in
> @@ -4,6 +4,7 @@ IMPORTS = advapi32
>
> C_SRCS = \
> instr.c \
> - ntoskrnl.c
> + ntoskrnl.c \
> + pnp_manager.c
>
> RC_SRCS = ntoskrnl.rc
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec \
> b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec index 119d406..c94b902 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> @@ -400,7 +400,7 @@
> @ stdcall IoInitializeIrp(ptr long long)
> @ stdcall IoInitializeRemoveLockEx(ptr long long long long)
> @ stdcall IoInitializeTimer(ptr ptr ptr)
> -@ stub IoInvalidateDeviceRelations
> +@ stdcall IoInvalidateDeviceRelations(ptr long)
> @ stub IoInvalidateDeviceState
> @ stub IoIsFileOriginRemote
> @ stub IoIsOperationSynchronous
> diff --git a/dlls/ntoskrnl.exe/pnp_manager.c b/dlls/ntoskrnl.exe/pnp_manager.c
> new file mode 100644
> index 0000000..5b42a0a
> --- /dev/null
> +++ b/dlls/ntoskrnl.exe/pnp_manager.c
> @@ -0,0 +1,185 @@
> +/* HID plug and play Manager like functionality
> + *
> + * Copyright 2016 CodeWeavers, Aric Stewart
> + *
> + * 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
> + */
> +
> +#include <stdarg.h>
> +#include "ntstatus.h"
> +#define WIN32_NO_STATUS
> +#include "windef.h"
> +#include "winbase.h"
> +#include "winternl.h"
> +#include "ddk/wdm.h"
> +#include "cfgmgr32.h"
> +#include "winsvc.h"
I don't think this include is required anymore.
> +#include "winreg.h"
> +#include "wine/debug.h"
> +#include "wine/list.h"
> +
> +WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
> +
> +#define MAX_SERVICE_NAME 260
> +
> +typedef struct _device_driver {
> + struct list entry;
> +
> + BOOL loaded;
> +
> + WCHAR **matching_ids;
> + INT id_count;
> +
> + WCHAR *driver_name;
> +} device_driver;
> +
> +struct list device_drivers = LIST_INIT(device_drivers);
> +static BOOL initialized = FALSE;
> +
> +static CRITICAL_SECTION pnp_cs;
> +static CRITICAL_SECTION_DEBUG critsect_debug =
> +{
> + 0, 0, &pnp_cs,
> + { &critsect_debug.ProcessLocksList, &critsect_debug.ProcessLocksList },
> + 0, 0, { (DWORD_PTR)(__FILE__ ": pnp_cs") }
> +};
> +static CRITICAL_SECTION pnp_cs = { &critsect_debug, -1, 0, 0, 0, 0 };
> +
> +static inline LPWSTR strdupW(LPCWSTR src)
> +{
> + LPWSTR dest;
> +
> + if (!src)
> + return NULL;
> +
> + dest = HeapAlloc(GetProcessHeap(), 0, (lstrlenW(src) + 1) * sizeof(WCHAR));
> + if (dest)
> + lstrcpyW(dest, src);
For new code it would be better to follow the current Wine style guidelines,
which means no LP* types and strlenW/strcpyW without l prefix.
> +
> + return dest;
> +}
> +
> +static device_driver* register_device_driver(const WCHAR *drivername, const WCHAR* \
> match) +{
> + device_driver *ptr;
> +
> + LIST_FOR_EACH_ENTRY(ptr, &device_drivers, device_driver, entry)
> + {
> + if (lstrcmpW(drivername, ptr->driver_name) == 0)
> + {
> + int i;
> + /* Check matching ids */
> + for (i = 0; i < ptr->id_count; i++)
> + if (lstrcmpW(match, ptr->matching_ids[i]) == 0)
> + return ptr;
> + /* Add a new matching id, this should be reletivly rare,
> + no exponental growth needed */
> + ptr->matching_ids = HeapReAlloc(GetProcessHeap(), 0, \
> ptr->matching_ids, sizeof(WCHAR*) * (ptr->id_count+1)); + \
> ptr->matching_ids[ptr->id_count] = strdupW(match); + ptr->id_count++;
If it is very rare, wouldn't it be better to add them as separate driver entries \
then? This would simplify the code and avoid nested loops at some places. The current \
way of organizing the driver db also has the disadvantage that the same "matching id" \
could be added for multiple drivers.
> +
> + return ptr;
> + }
> + }
> +
> + ptr = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*ptr));
> + ptr->driver_name = strdupW(drivername);
> + ptr->loaded = FALSE;
> + ptr->matching_ids = HeapAlloc(GetProcessHeap(), 0, sizeof(WCHAR*));
> + ptr->matching_ids[0] = strdupW(match);
> + ptr->id_count = 1;
> + list_add_tail(&device_drivers, &ptr->entry);
> +
> + return ptr;
> +}
> +
> +static void initialize_driver_store(void)
> +{
> + static const WCHAR criticalW[] =
> + { 'S','y','s','t','e','m','\\',
> + 'C','u','r','r','e','n','t','C','o','n','t','r','o','l','S','e','t','\\',
> + 'C','o','n','t','r','o','l', '\\',
> + 'C','r','i','t','i','c','a','l','D','e','v','i','c','e','D','a','t','a','b','a','s','e', \
> 0}; + static const WCHAR serviceW[] = {'S','e','r','v','i','c','e',0};
> + HKEY hkey_root;
> + WCHAR szMatch[MAX_DEVICE_ID_LEN];
> + int i;
> +
> + if (RegOpenKeyW(HKEY_LOCAL_MACHINE, criticalW, &hkey_root))
> + {
> + TRACE("No drivers found\n");
> + return;
> + }
> +
> + for (i = 0; TRUE; i++)
> + {
> + HKEY hkey_service;
> + DWORD rc;
> + device_driver *driver;
> + DWORD len = 0;
> + WCHAR szService[MAX_SERVICE_NAME];
> +
> + rc = RegEnumKeyW(hkey_root, i, szMatch, MAX_DEVICE_ID_LEN);
> + if (rc == ERROR_NO_MORE_ITEMS)
> + break;
> +
> + if (rc != 0)
> + {
> + ERR("Error %d reading key %d - skipping\n", rc, i);
> + continue;
> + }
> +
> + if (RegOpenKeyW(hkey_root, szMatch, &hkey_service))
> + {
> + ERR("Error opening key %s - skipping\n",debugstr_w(szMatch));
> + continue;
> + }
> +
> + len = sizeof(szService);
> + rc = RegQueryValueExW(hkey_service, serviceW, NULL, NULL, \
> (BYTE*)szService, &len);
It might be useful to check the type of the registry key here.
> + if (rc != ERROR_SUCCESS)
> + {
> + ERR("Error querying service from key %s - \
> skipping\n",debugstr_w(szMatch)); + RegCloseKey(hkey_service);
> + continue;
> + }
> + RegCloseKey(hkey_service);
> +
> + TRACE("Registering service '%s' to match '%s'\n", debugstr_w(szService), \
> debugstr_w(szMatch)); +
> + driver = register_device_driver(szService, szMatch);
> + if (!driver)
> + ERR("failed to load driver %s\n", debugstr_w(szService));
The message is actually a bit misleading. It will only occur for duplicate entries,
which should not happen anyway.
> +
> + }
> +
> + RegCloseKey(hkey_root);
> +}
> +
> +/***********************************************************************
> + * IoInvalidateDeviceRelations(NTOSKRNL.EXE.@)
> + */
> +VOID WINAPI IoInvalidateDeviceRelations(DEVICE_OBJECT *DeviceObject, \
> DEVICE_RELATION_TYPE Type) +{
> + TRACE("(%p %i)\n", DeviceObject, Type);
> +
> + EnterCriticalSection(&pnp_cs);
> + if (!initialized)
> + {
> + initialized = TRUE;
> + initialize_driver_store();
> + }
> + LeaveCriticalSection(&pnp_cs);
> +}
> diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h
> index 68694af..bd3b323 100644
> --- a/include/ddk/wdm.h
> +++ b/include/ddk/wdm.h
> @@ -1233,6 +1233,7 @@ NTSTATUS WINAPI \
> IoGetDeviceProperty(PDEVICE_OBJECT,DEVICE_REGISTRY_PROPERTY,ULO PVOID WINAPI \
> IoGetDriverObjectExtension(PDRIVER_OBJECT,PVOID); PDEVICE_OBJECT WINAPI \
> IoGetRelatedDeviceObject(PFILE_OBJECT); void WINAPI \
> IoInitializeIrp(IRP*,USHORT,CCHAR); +void WINAPI \
> IoInvalidateDeviceRelations(PDEVICE_OBJECT,DEVICE_RELATION_TYPE); VOID WINAPI \
> IoInitializeRemoveLockEx(PIO_REMOVE_LOCK,ULONG,ULONG,ULONG,ULONG); NTSTATUS WINAPI \
> IoWMIRegistrationControl(PDEVICE_OBJECT,ULONG);
>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic