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 <aric(a)codeweavers.com>
> ---
>  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

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

[Aric Stewart]

On 9/7/16 5:08 AM, Sebastian Lackner wrote:
> 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 <aric(a)codeweavers.com>
> > ---
> >  dlls/winebus.sys/Makefile.in      |  5 ++-
> > .
> > .
> > .
> > +    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?

No I personally don't think so.  We are successfully loading just disabled or not doing anything.
> > +    }
> > +
> > +    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?

Same as above.
> > +}
> > +
> > +#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

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

[Alexandre Julliard]

Aric Stewart <aric(a)codeweavers.com> writes:
> On 9/7/16 5:08 AM, Sebastian Lackner wrote:
> > Wouldn't it be better to return an error such that IoCreateDriver() fails?
>
> No I personally don't think so.  We are successfully loading just disabled or not doing anything.

How is this useful? And why do you need a disable option?

-- 
Alexandre Julliard
julliard(a)winehq.org

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

[Aric Stewart]

On 9/7/16 8:14 PM, Alexandre Julliard wrote:
> Aric Stewart <aric(a)codeweavers.com> writes:
> > On 9/7/16 5:08 AM, Sebastian Lackner wrote:
> > > Wouldn't it be better to return an error such that IoCreateDriver() fails?
> >
> > No I personally don't think so.  We are successfully loading just disabled or not doing anything.
>
> How is this useful? And why do you need a disable option?

It mostly comes from when the hidraw bus and linux event bus where separate drivers. Where there would be a real reason to want to be able to disable one or the other. 

There could be future cases as well where multiple drivers are loaded in the base winebus.sys and a user would want to have granularity in their ability to turn individual ones on and off. 

-aric

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

[Alexandre Julliard]

Aric Stewart <aric(a)codeweavers.com> writes:
> On 9/7/16 8:14 PM, Alexandre Julliard wrote:
> > Aric Stewart <aric(a)codeweavers.com> writes:
> > > On 9/7/16 5:08 AM, Sebastian Lackner wrote:
> > > > Wouldn't it be better to return an error such that IoCreateDriver() fails?
> > >
> > > No I personally don't think so.  We are successfully loading just disabled or not doing anything.
> >
> > How is this useful? And why do you need a disable option?
>
> It mostly comes from when the hidraw bus and linux event bus where
> separate drivers. Where there would be a real reason to want to be
> able to disable one or the other.
>
> There could be future cases as well where multiple drivers are loaded
> in the base winebus.sys and a user would want to have granularity in
> their ability to turn individual ones on and off.

If that ever becomes needed, I'd suggest to then split the drivers so
that they can be controlled individually through the standard service
interface. It doesn't seem necessary for the first version though.

-- 
Alexandre Julliard
julliard(a)winehq.org

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

[Aric Stewart]

On 9/7/16 9:05 PM, Alexandre Julliard wrote:
> Aric Stewart <aric(a)codeweavers.com> writes:
> > On 9/7/16 8:14 PM, Alexandre Julliard wrote:
> > > Aric Stewart <aric(a)codeweavers.com> writes:
> > > > On 9/7/16 5:08 AM, Sebastian Lackner wrote:
> > > > > Wouldn't it be better to return an error such that IoCreateDriver() fails?
> > > >
> > > > No I personally don't think so.  We are successfully loading just disabled or not doing anything.
> > >
> > > How is this useful? And why do you need a disable option?
> >
> > It mostly comes from when the hidraw bus and linux event bus where
> > separate drivers. Where there would be a real reason to want to be
> > able to disable one or the other.
> >
> > There could be future cases as well where multiple drivers are loaded
> > in the base winebus.sys and a user would want to have granularity in
> > their ability to turn individual ones on and off.
>
> If that ever becomes needed, I'd suggest to then split the drivers so
> that they can be controlled individually through the standard service
> interface. It doesn't seem necessary for the first version though.

ok v6 patches for the 2 parts that changed sent to remove disabling whole busses

-aric