Re: [PATCH 02/13] wineplugplay.sys: Initialize device driver store

[Sebastian Lackner]

On 17.08.2016 15:36, 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
>
> dlls/wineplugplay.sys/Makefile.in   |   5 +-
>  dlls/wineplugplay.sys/main.c        |  14 ++++
>  dlls/wineplugplay.sys/pnp.h         |  20 +++++
>  dlls/wineplugplay.sys/pnp_manager.c | 162 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 199 insertions(+), 2 deletions(-)
>  create mode 100644 dlls/wineplugplay.sys/pnp.h
>  create mode 100644 dlls/wineplugplay.sys/pnp_manager.c
>
> 0002-wineplugplay.sys-Initialize-device-driver-store.txt
>
> diff --git a/dlls/wineplugplay.sys/Makefile.in b/dlls/wineplugplay.sys/Makefile.in
> index 4039a8d..618dac7 100644
> --- a/dlls/wineplugplay.sys/Makefile.in
> +++ b/dlls/wineplugplay.sys/Makefile.in
> @@ -1,6 +1,7 @@
>  MODULE    = wineplugplay.sys
> -IMPORTS = ntoskrnl
> +IMPORTS = ntoskrnl advapi32
>  EXTRADLLFLAGS = -Wb,--subsystem,native
>
> C_SRCS = \
> - main.c
> - main.c \
> - pnp_manager.c
>
> diff --git a/dlls/wineplugplay.sys/main.c b/dlls/wineplugplay.sys/main.c
> index 51b5d97..4ce641b 100644
> --- a/dlls/wineplugplay.sys/main.c
> +++ b/dlls/wineplugplay.sys/main.c
> @@ -30,11 +30,25 @@
>  #include "wine/unicode.h"
>  #include "wine/debug.h"
>
> +#include "pnp.h"
> -
>
> WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
>
> +static DWORD CALLBACK Initialize_thread(void *args)
> +{
> - Initialize_DriverStore();
> - return 0;
>
> +}
> -
>
> /* main entry point for the driver */
>  NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path )
>  {
>      TRACE("\n");
> - /* We are doing this in a new thread because we want this DriverEntry
> -
>   to return as immediently as possible. The loader lock will prevent
> -
>   the thread from starting immediently so this service can finish
> -
>   loading before the Initialization start trying to load more
> -
>   services */
> - CreateThread(NULL, 0, Initialize_thread, NULL, 0, NULL);
> return STATUS_SUCCESS;
>
> }
> diff --git a/dlls/wineplugplay.sys/pnp.h b/dlls/wineplugplay.sys/pnp.h
> new file mode 100644
> index 0000000..db18202
> --- /dev/null
> +++ b/dlls/wineplugplay.sys/pnp.h
> @@ -0,0 +1,20 @@
> +/*
> -
> - Copyright 2016 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
> - */
> -
>
> +/* Plug and Play Manager */
> +void Initialize_DriverStore(void) DECLSPEC_HIDDEN;
> diff --git a/dlls/wineplugplay.sys/pnp_manager.c b/dlls/wineplugplay.sys/pnp_manager.c
> new file mode 100644
> index 0000000..0e10240
> --- /dev/null
> +++ b/dlls/wineplugplay.sys/pnp_manager.c
> @@ -0,0 +1,162 @@
> +/*  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"
> +#include "winreg.h"
> +#include "wine/debug.h"
> +#include "wine/list.h"
> -
>
> +#include "pnp.h"
> -
>
> +WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
> -
>
> +#define MAX_SERVICE_NAME 260
> -
>
> +typedef struct _device_driver {
> - struct list entry;
> -
> - WCHAR **matching_ids;
> - INT id_count;
> -
> - WCHAR *driver_name;
>
> +} device_driver;
> -
>
> +struct list device_drivers = LIST_INIT(device_drivers);
> -
>
> +static device_driver* load_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] = HeapAlloc(GetProcessHeap(), 0, sizeof(WCHAR) * (lstrlenW(match)+1));
> -
>        lstrcpyW(ptr->matching_ids[ptr->id_count], match);

Such code would be easier to read with a strdupW() helper function, like in many
other places of Wine. The same also applies to some places below.
> -
>        ptr->id_count++;
> -
> -
>        return ptr;
> -
>    }
> - }
> -
> - ptr = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*ptr));
> - ptr->driver_name = HeapAlloc(GetProcessHeap(), 0, sizeof(WCHAR) * (lstrlenW(drivername)+1));
> - lstrcpyW(ptr->driver_name, drivername);
> - ptr->matching_ids = HeapAlloc(GetProcessHeap(), 0, sizeof(WCHAR*));
> - ptr->matching_ids[0] = HeapAlloc(GetProcessHeap(), 0, sizeof(WCHAR) * (lstrlenW(match)+1));
> - lstrcpyW(ptr->matching_ids[0], match);
> - ptr->id_count = 1;
> - list_add_tail(&device_drivers, &ptr->entry);
> -
> - return ptr;
>
> +}
> -
>
> +void Initialize_DriverStore(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];
> - SC_HANDLE service_manager;
> -
> - int i;
> -
> - if (RegOpenKeyW(HKEY_LOCAL_MACHINE, criticalW, &hkey_root))
> - {
> -
>    TRACE("No drivers found\n");
> -
>    return;
> - }
> -
> - service_manager = OpenSCManagerW(NULL, NULL, GENERIC_READ|GENERIC_EXECUTE);
> -
> - for (i = 0; TRUE; i++)
> - {
> -
>    HKEY hkey_service;
> -
>    DWORD rc;
> -
>    device_driver *driver;
> -
>    DWORD len = 0;
> -
>    WCHAR szService[MAX_SERVICE_NAME];
> -
>    SC_HANDLE service;
> -
> -
>    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);
> -
>    if (rc != ERROR_SUCCESS)
> -
>    {
> -
>        ERR("Error querying service from key %s - skipping\n",debugstr_w(szMatch));
> -
>        RegCloseKey(hkey_service);
> -
>        continue;
> -
>    }
> -
>    RegCloseKey(hkey_service);
> -
> -
>    service = OpenServiceW(service_manager, szService, GENERIC_READ|GENERIC_EXECUTE);
> -
>    if (!service)
> -
>    {
> -
>        ERR("Unable to open service %s - skipping\n",debugstr_w(szService));
> -
>        continue;
> -
>    }
> -
> -
>    TRACE("Loading service '%s' to match '%s'\n", debugstr_w(szService),            debugstr_w(szMatch));

Why are there so many spaces in the middle of the line?
> -
> -
>    driver = load_device_driver(szService, szMatch);
> -
>    if (!driver)
> -
>        ERR("failed to load driver %s\n", debugstr_w(szService));
> -
> -
>    StartServiceW(service, 0, NULL);
> -
>    CloseServiceHandle(service);

The registry parsing code is probably fine, but wouldn't it be easier to handle this similar to
other autostart services in services.exe? Then there would also be no need to spawn a separate
thread for initialization.
> - }
> -
> - CloseServiceHandle(service_manager);
> - RegCloseKey(hkey_root);
>
> +}

[PATCH 02/13] wineplugplay.sys: Initialize device driver store

[Aric Stewart]

Thanks for the review, made most of these changes.  One question.

On 8/18/16 4:35 AM, Sebastian Lackner wrote:
> > -
> > -
> >    driver = load_device_driver(szService, szMatch);
> > -
> >    if (!driver)
> > -
> >        ERR("failed to load driver %s\n", debugstr_w(szService));
> > -
> > -
> >    StartServiceW(service, 0, NULL);
> > -
> >    CloseServiceHandle(service);
>
> The registry parsing code is probably fine, but wouldn't it be easier to handle this similar to
> other autostart services in services.exe? Then there would also be no need to spawn a separate
> thread for initialization.

I don't get what you mean. Instead of using StartServiceW duplicating the code from services.exe? I don't feel like that is the correct plan. Part of the reason for doing all the work in services.exe and winedevice.exe was to prevent code duplication and to be able to do that StartServiceW instead.  If I was going to duplicate code I would duplicate winedevice.exe and just do it that way instead.

-aric

[PATCH 02/13] wineplugplay.sys: Initialize device driver store

[Sebastian Lackner]

On 18.08.2016 15:27, Aric Stewart wrote:
> Thanks for the review, made most of these changes.  One question.
>
> On 8/18/16 4:35 AM, Sebastian Lackner wrote:
> > > -
> > > -
> > >    driver = load_device_driver(szService, szMatch);
> > > -
> > >    if (!driver)
> > > -
> > >        ERR("failed to load driver %s\n", debugstr_w(szService));
> > > -
> > > -
> > >    StartServiceW(service, 0, NULL);
> > > -
> > >    CloseServiceHandle(service);
> >
> > The registry parsing code is probably fine, but wouldn't it be easier to handle this similar to
> > other autostart services in services.exe? Then there would also be no need to spawn a separate
> > thread for initialization.
>
> I don't get what you mean. Instead of using StartServiceW duplicating the code from services.exe? I don't feel like that is the correct plan. Part of the reason for doing all the work in services.exe and winedevice.exe was to prevent code duplication and to be able to do that StartServiceW instead.  If I was going to duplicate code I would duplicate winedevice.exe and just do it that way instead.
>
> -aric

Sorry if I was a bit unclear. My idea was not to duplicate code from services.exe
into your driver, but instead to merge your "autostart" code into services.exe.
You would have to read the database twice then (one time in services.exe to add
autostart services, and a second time in wineplugplay.sys), but a big advantage
would be that you can remove the separate thread in wineplugplay.sys. Also, by
starting all services from one single place it would be easier to ensure that they
are loaded in the correct order. Wouldn't it have advantages to implement the
startup like that, or am I missing something?

BTW: If you have to keep StartServiceW in wineplugplay.sys, it would be good to
check the error code and repeat the call if the database is still locked from the
regular process startup (ERROR_SERVICE_DATABASE_LOCKED).

Regards,
Sebastian