Re: [PATCH v4 7/20] winebus.sys: Add Linux udev bus

[Sebastian Lackner]

On 06.09.2016 17:14, Aric Stewart wrote:
> v2: Style changes
> v4: Updated with corrections from Thomas Faber
>
> Includes a common function to check if a bus is enabled and a base common
> HID IRP_MJ_PNP handler
> Signed-off-by: Aric Stewart
>
> dlls/winebus.sys/Makefile.in      |  5 ++-
>  dlls/winebus.sys/bus.h            | 24 +++++++++++
>  dlls/winebus.sys/bus_hid_common.c | 89 +++++++++++++++++++++++++++++++++++++++
>  dlls/winebus.sys/bus_udev.c       | 78 ++++++++++++++++++++++++++++++++++
>  dlls/winebus.sys/main.c           |  8 +++-
>  loader/wine.inf.in                | 15 +++++--
>  6 files changed, 213 insertions(+), 6 deletions(-)
>  create mode 100644 dlls/winebus.sys/bus.h
>  create mode 100644 dlls/winebus.sys/bus_hid_common.c
>  create mode 100644 dlls/winebus.sys/bus_udev.c
>
> v4-0008-winebus.sys-Add-Linux-udev-bus.txt
>
> diff --git a/dlls/winebus.sys/Makefile.in b/dlls/winebus.sys/Makefile.in
> index c3e31cc..7e4e97f 100644
> --- a/dlls/winebus.sys/Makefile.in
> +++ b/dlls/winebus.sys/Makefile.in
> @@ -1,5 +1,8 @@
>  MODULE    = winebus.sys
> +IMPORTS   = ntoskrnl
>  EXTRADLLFLAGS = -Wb,--subsystem,native
>
> C_SRCS = \
> - main.c
> - main.c \
> - bus_hid_common.c \
> - bus_udev.c \
>
> diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
> new file mode 100644
> index 0000000..a6f08b2
> --- /dev/null
> +++ b/dlls/winebus.sys/bus.h
> @@ -0,0 +1,24 @@
> +/*
> -
> - 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
> - */
> -
>
> +/* Busses */
> +NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry_path) DECLSPEC_HIDDEN;
> -
>
> +/* HID Plug and Play Bus */
> +NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
> +DWORD check_bus_disabled(UNICODE_STRING *registry_path) DECLSPEC_HIDDEN;
> diff --git a/dlls/winebus.sys/bus_hid_common.c b/dlls/winebus.sys/bus_hid_common.c
> new file mode 100644
> index 0000000..c98a0dd
> --- /dev/null
> +++ b/dlls/winebus.sys/bus_hid_common.c

It looks like main.c remains almost completely empty after your series.
What about adding all common helper functions there?
> @@ -0,0 +1,89 @@
> +/*  plug and play Bus like functionality for HID busses
> -
> -
> -
> - 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
> - */
> -
>
> +#define NONAMELESSUNION
> +#include <stdarg.h>
> +#include "ntstatus.h"
> +#define WIN32_NO_STATUS
> +#include "windef.h"
> +#include "winbase.h"
> +#include "winuser.h"
> +#include "winternl.h"
> +#include "winioctl.h"
> +#include "ddk/wdm.h"
> +#include "winreg.h"
> +#include "setupapi.h"

Some of the headers are not really required yet. I would suggest to move
them to the patches where they are actually needed.
> +#include "regstr.h"
> +#include "cfgmgr32.h"
> +#include "wine/debug.h"
> +#include "wine/unicode.h"
> +#include "wine/list.h"
> -
>
> +#include "bus.h"
> -
>
> +WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
> -
>
> +NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp)
> +{
> - NTSTATUS rc = irp->IoStatus.u.Status;
> - IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
> -
> - switch(irpsp->MinorFunction)
> - {
> -
>    case IRP_MN_QUERY_DEVICE_RELATIONS:
> -
>        TRACE("IRP_MN_QUERY_DEVICE_RELATIONS\n");
> -
>        break;
> -
>    case IRP_MN_QUERY_ID:
> -
>    {
> -
>        TRACE("IRP_MN_QUERY_ID\n");
> -
>        break;
> -
>    }
> -
>    case IRP_MN_QUERY_CAPABILITIES:
> -
>        TRACE("IRP_MN_QUERY_CAPABILITIES\n");
> -
>        break;
> - }
> - IoCompleteRequest( irp, IO_NO_INCREMENT );
> - return rc;
>
> +}
> -
>
> +DWORD check_bus_disabled(UNICODE_STRING *registry_path)
> +{
> - OBJECT_ATTRIBUTES attr;
> - HANDLE key;
> - DWORD disabled = 0;
> - static const WCHAR disabledW[] = {'D','i','s','a','b','l','e','d',0};
> - static const UNICODE_STRING disabled_str = {sizeof(disabledW) - sizeof(WCHAR), sizeof(disabledW), (WCHAR*)disabledW};
> -
> - InitializeObjectAttributes(&attr, registry_path, OBJ_CASE_INSENSITIVE | OBJ_KERNEL_HANDLE, NULL, NULL);
> - if (ZwOpenKey(&key, KEY_ALL_ACCESS, &attr) == STATUS_SUCCESS)
> - {
> -
>    DWORD size;
> -
>    char buffer[sizeof(KEY_VALUE_PARTIAL_INFORMATION) + sizeof(DWORD)];

Not a critical issue, but the buffer size will be one byte too big. For a correct size
computation you will have to use something like:

char buffer[FIELD_OFFSET( KEY_VALUE_PARTIAL_INFORMATION, Data[sizeof(DWORD] )];
> -
>    KEY_VALUE_PARTIAL_INFORMATION *info = (KEY_VALUE_PARTIAL_INFORMATION*)buffer;
> -
> -
>    if (ZwQueryValueKey(key, &disabled_str, KeyValuePartialInformation, info, sizeof(buffer), &size) == STATUS_SUCCESS)
> -
>    {

Please also check the info->Type here.
> -
>        disabled = *(DWORD*)info->Data;
> -
>    }
> -
> -
>    ZwClose(key);
> - }
> -
> - return disabled;
>
> +}
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> new file mode 100644
> index 0000000..101bdeb
> --- /dev/null
> +++ b/dlls/winebus.sys/bus_udev.c
> @@ -0,0 +1,78 @@
> +/*  Plug and Play support for hid devices found through udev
> -
> -
> -
> - 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 "config.h"
> +#include "wine/port.h"
> +#include <stdarg.h>
> -
>
> +#ifdef HAVE_SYS_IOCTL_H
> +# include <sys/ioctl.h>
> +#endif
> -
>
> +#include <errno.h>
> -
>
> +#define NONAMELESSUNION
> -
>
> +#include "ntstatus.h"
> +#define WIN32_NO_STATUS
> +#include "windef.h"
> +#include "winbase.h"
> +#include "winternl.h"
> +#include "winioctl.h"
> +#include "ddk/wdm.h"
> +#include "winnls.h"
> +#include "wine/debug.h"
> +#include "bus.h"
> -
>
> +WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
> +#ifdef SONAME_LIBUDEV
> -
>
> +static DRIVER_OBJECT *udev_driver_obj = NULL;
> -
>
> +static VOID WINAPI unload(DRIVER_OBJECT *driver)
> +{
> - TRACE("Linux udev Driver Unload\n");
>
> +}
> -
>
> +NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry_path)
> +{
> - TRACE("Linux udev Driver Init\n");
> -
> - udev_driver_obj = driver;
> - driver->MajorFunction[IRP_MJ_PNP] = common_pnp_dispatch;
> - driver->DriverUnload = unload;
> -
> - if (check_bus_disabled(registry_path))
> - {
> -
>    TRACE("UDEV plug and play bus disabled in registry\n");
> -
>    return STATUS_SUCCESS;

Wouldn't it be better to return an error such that IoCreateDriver() fails?
> - }
> -
> - return STATUS_SUCCESS;
>
> +}
> -
>
> +#else
> -
>
> +NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry_path)
> +{
> - TRACE("Dummy Linux udev Driver Init\n");
> - return STATUS_SUCCESS;

Similar to above, wouldn't it be better to return an error?
> +}
> -
>
> +#endif /* SONAME_LIBUDEV */
> diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
> index d495cd9..4690baf 100644
> --- a/dlls/winebus.sys/main.c
> +++ b/dlls/winebus.sys/main.c
> @@ -28,11 +28,17 @@
>  #include "ddk/wdm.h"
>  #include "wine/debug.h"
>
> +#include "bus.h"
> -
>
> WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
>
> NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path )
>  {
> - TRACE( "(%p, %s)\n", driver, debugstr_w(path->Buffer) );
> -
>
> static const WCHAR udevW[] = {'\','D','r','i','v','e','r','\','U','D','E','V',0};
> -
>
> static UNICODE_STRING udev = {sizeof(udevW) - sizeof(WCHAR), sizeof(udevW), (WCHAR*)udevW};
> -
> -
>
> TRACE( "Wine Platform Bus(%p, %s)\n", driver, debugstr_w(path->Buffer) );
> -
>
> IoCreateDriver(&udev, udev_driver_init);
>
> return STATUS_SUCCESS;
>
> }
> diff --git a/loader/wine.inf.in b/loader/wine.inf.in
> index 85dbef1..b595693 100644
> --- a/loader/wine.inf.in
> +++ b/loader/wine.inf.in
> @@ -48,7 +48,8 @@ AddReg=\
>      SessionMgr,\
>      Tapi,\
>      Timezones,\
> - LicenseInformation
> - LicenseInformation,\
> - PlatformBus
>
> [DefaultInstall.NT]
>  RegisterDlls=RegisterDllsSection
> @@ -73,7 +74,8 @@ AddReg=\
>      Tapi,\
>      Timezones,\
>      VersionInfo,\
> - LicenseInformation
> - LicenseInformation,\
> - PlatformBus
>
> [DefaultInstall.ntamd64]
>  RegisterDlls=RegisterDllsSection
> @@ -100,7 +102,8 @@ AddReg=\
>      Tapi,\
>      Timezones,\
>      VersionInfo.ntamd64,\
> - LicenseInformation
> - LicenseInformation,\
> - PlatformBus
>
> [Wow64Install]
>  RegisterDlls=RegisterDllsSection
> @@ -115,7 +118,8 @@ AddReg=\
>      Misc,\
>      Tapi,\
>      VersionInfo.ntamd64,\
> - LicenseInformation
> - LicenseInformation,\
> - PlatformBus
>
> [DefaultInstall.Services]
>  AddService=BITS,0,BITSService
> @@ -3397,3 +3401,6 @@ HKLM,Software\Wine\LicenseInformation,"Shell-InBoxGames-Shanghai-EnableGame",0x1
>  HKLM,Software\Wine\LicenseInformation,"Shell-InBoxGames-Solitaire-EnableGame",0x10001,0x00000001
>  HKLM,Software\Wine\LicenseInformation,"Shell-InBoxGames-SpiderSolitaire-EnableGame",0x10001,0x00000001
>  HKLM,Software\Wine\LicenseInformation,"Shell-PremiumInBoxGames-Chess-EnableGame",0x10001,0x00000001
> -
>
> +[PlatformBus]
> +HKLM,System\CurrentControlSet\Services\UDEV,"Disabled",0x10001,0x00000000