Re: [PATCH 3/5] usbdk: make backend selection dynamic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 28, 2015 at 01:24:02PM +0300, Kirill Moizik wrote:
> From: Pavel Gurvich <pavel@xxxxxxxxxx>
> 
> introducing use_usbdk global variable providing functionality of dynamic backend switching

This really should not be a global variable, but a member of
SpiceUsbDeviceManager. This means passing it to a few more functions, or
moving the if (use_usbdk) checks from an inner function to its caller.

> 
> Signed-off-by: Pavel Gurvich <pavel@xxxxxxxxxx>
> Signed-off-by: Dmitry Fleytman <dmitry@xxxxxxxxxx>
> ---
>  gtk/usb-device-manager.c | 264 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 188 insertions(+), 76 deletions(-)
> 
> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
> index 7739337..841e3a4 100644
> --- a/gtk/usb-device-manager.c
> +++ b/gtk/usb-device-manager.c
> @@ -182,6 +182,7 @@ static SpiceUsbDevice *spice_usb_device_ref(SpiceUsbDevice *device);
>  static void spice_usb_device_unref(SpiceUsbDevice *device);
>  
>  #ifdef USE_WINUSB
> +gboolean use_usbdk;
>  static guint8 spice_usb_device_get_state(SpiceUsbDevice *device);
>  static void  spice_usb_device_set_state(SpiceUsbDevice *device, guint8 s);
>  #endif
> @@ -222,8 +223,33 @@ static guint signals[LAST_SIGNAL] = { 0, };
>  G_DEFINE_TYPE_WITH_CODE(SpiceUsbDeviceManager, spice_usb_device_manager, G_TYPE_OBJECT,
>       G_IMPLEMENT_INTERFACE (G_TYPE_INITABLE, spice_usb_device_manager_initable_iface_init));
>  
> +#ifdef USE_WINUSB
> +static gboolean is_usbdk_driver_installed(void)
> +{
> +    gboolean usbdk_installed = FALSE;
> +
> +    SC_HANDLE managerHandle = OpenSCManager(NULL, NULL, SC_MANAGER_CONNECT);
> +    if (managerHandle)
> +    {
> +        SC_HANDLE serviceHandle = OpenService(managerHandle, TEXT("UsbDk"), GENERIC_READ);
> +        if (serviceHandle)
> +        {
> +            SPICE_DEBUG("UsbDk driver is installed.");
> +            usbdk_installed = TRUE;
> +            CloseServiceHandle(serviceHandle);
> +        }
> +        CloseServiceHandle(managerHandle);
> +    }
> +    return usbdk_installed;
> +}
> +#endif
> +
>  static void spice_usb_device_manager_init(SpiceUsbDeviceManager *self)
>  {
> +#ifdef USE_WINUSB
> +    use_usbdk = is_usbdk_driver_installed();
> +#endif
> +
>      SpiceUsbDeviceManagerPrivate *priv;
>  
>      priv = SPICE_USB_DEVICE_MANAGER_GET_PRIVATE(self);
> @@ -374,8 +400,11 @@ static void spice_usb_device_manager_finalize(GObject *gobject)
>      free(priv->auto_conn_filter_rules);
>      free(priv->redirect_on_connect_rules);
>  #ifdef USE_WINUSB
> -    if (priv->installer)
> -        g_object_unref(priv->installer);
> +    if(! use_usbdk)

no space between '!' and 'use_usbdk', and { on the same line.

> +    {
> +        if (priv->installer)
> +            g_object_unref(priv->installer);
> +    }

In _finalize, you wantn to unref installer when it's set even if it's
not supposed to be set. Add a g_warn_if_fail(!use_usbdk) if you want.

>  #endif
>  #endif
>  
> @@ -672,8 +701,16 @@ static gboolean spice_usb_device_manager_get_udev_bus_n_address(
>      bus_str = g_udev_device_get_property(udev, "BUSNUM");
>      address_str = g_udev_device_get_property(udev, "DEVNUM");
>  #else /* WinUSB -- request vid:pid instead */
> -    bus_str = g_udev_device_get_property(udev, "VID");
> -    address_str = g_udev_device_get_property(udev, "PID");
> +    if(! use_usbdk)
> +    {
> +        bus_str = g_udev_device_get_property(udev, "VID");
> +        address_str = g_udev_device_get_property(udev, "PID");
> +    }
> +    else
> +    {
> +        bus_str = g_udev_device_get_property(udev, "BUSNUM");
> +        address_str = g_udev_device_get_property(udev, "DEVNUM");
> +    }


So here, the if (use_usbdk) block is the same as the linux code, which
is in a different #ifdef. This happens in other hunks down that file.
Should we change 'use_usbdk' to 'use_usbclerk' or such, which would be
FALSE on linux, and when usbdk is used, and remove the #ifdef so that we
don't copy & paste the code between linux and windows/usbdk?

>  #endif
>      if (bus_str)
>          *bus = atoi(bus_str);
> @@ -835,20 +872,36 @@ static gboolean
>  spice_usb_device_manager_device_match(SpiceUsbDevice *device,
>                                        const int vid, const int pid)
>  {
> -    return (spice_usb_device_get_vid(device) == vid &&
> +    if(! use_usbdk)
> +    {
> +        return (spice_usb_device_get_vid(device) == vid &&
>              spice_usb_device_get_pid(device) == pid);
> +    }
> +    else
> +    {
> +        return (spice_usb_device_get_busnum(device) == vid &&
> +            spice_usb_device_get_devaddr(device) == pid);
> +    }
>  }
>  
>  static gboolean
>  spice_usb_device_manager_libdev_match(libusb_device *libdev,
>                                        const int vid, const int pid)
>  {
> -    int vid2, pid2;
> +    if(! use_usbdk)
> +    {
> +        int vid2, pid2;
>  
> -    if (!spice_usb_device_manager_get_libdev_vid_pid(libdev, &vid2, &pid2)) {
> -        return FALSE;
> +        if (!spice_usb_device_manager_get_libdev_vid_pid(libdev, &vid2, &pid2)) {
> +            return FALSE;
> +        }
> +        return (vid == vid2 && pid == pid2);
> +    }
> +    else
> +    {
> +        return (libusb_get_bus_number(libdev) == vid &&
> +            libusb_get_device_address(libdev) == pid);
>      }
> -    return (vid == vid2 && pid == pid2);
>  }
>  #endif /* of Win32 -- match functions */
>  
> @@ -926,12 +979,15 @@ static void spice_usb_device_manager_remove_dev(SpiceUsbDeviceManager *self,
>      }
>  
>  #ifdef USE_WINUSB
> -    const guint8 state = spice_usb_device_get_state(device);
> -    if ((state == SPICE_USB_DEVICE_STATE_INSTALLING) ||
> -        (state == SPICE_USB_DEVICE_STATE_UNINSTALLING)) {
> -        SPICE_DEBUG("skipping " DEV_ID_FMT ". It is un/installing its driver",
> -                    bus, address);
> -        return;
> +    if(! use_usbdk)
> +    {
> +        const guint8 state = spice_usb_device_get_state(device);
> +        if ((state == SPICE_USB_DEVICE_STATE_INSTALLING) ||
> +            (state == SPICE_USB_DEVICE_STATE_UNINSTALLING)) {
> +            SPICE_DEBUG("skipping " DEV_ID_FMT ". It is un/installing its driver",
> +                bus, address);
> +            return;
> +        }
>      }
>  #endif
>  
> @@ -1109,6 +1165,11 @@ static void spice_usb_device_manager_drv_install_cb(GObject *gobject,
>                                                      GAsyncResult *res,
>                                                      gpointer user_data)
>  {
> +    if (use_usbdk)
> +    {
> +        return;
> +    }

This one should never be called when use_usbdk is set as
spice_win_usb_driver_install() calls using this callback are already
protected by a if (use_usbdk) test, so this early return can be a
g_return_if_fail(!use_usbdk);

> +
>      SpiceUsbDeviceManager *self;
>      SpiceWinUsbDriver *installer;
>      gint status;
> @@ -1484,33 +1545,45 @@ done:
>  
>  
>  void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
> -                                             SpiceUsbDevice *device,
> -                                             GCancellable *cancellable,
> -                                             GAsyncReadyCallback callback,
> -                                             gpointer user_data)
> +    SpiceUsbDevice *device,
> +    GCancellable *cancellable,
> +    GAsyncReadyCallback callback,
> +    gpointer user_data)
>  {
>  
>  #if defined(USE_USBREDIR) && defined(USE_WINUSB)
> -    SpiceWinUsbDriver *installer;
> -    UsbInstallCbInfo *cbinfo;
>  
> -    spice_usb_device_set_state(device, SPICE_USB_DEVICE_STATE_INSTALLING);
> -    if (! self->priv->installer) {
> -        self->priv->installer = spice_win_usb_driver_new();
> +    if (! use_usbdk)
> +    {
> +        SpiceWinUsbDriver *installer;
> +        UsbInstallCbInfo *cbinfo;
> +
> +        spice_usb_device_set_state(device, SPICE_USB_DEVICE_STATE_INSTALLING);
> +        if (! self->priv->installer) {
> +            self->priv->installer = spice_win_usb_driver_new();
> +        }
> +        installer = self->priv->installer;
> +        cbinfo = g_new0(UsbInstallCbInfo, 1);
> +        cbinfo->manager     = self;
> +        cbinfo->device      = spice_usb_device_ref(device);
> +        cbinfo->installer   = installer;
> +        cbinfo->cancellable = cancellable;
> +        cbinfo->callback    = callback;
> +        cbinfo->user_data   = user_data;
> +        cbinfo->is_install  = TRUE;
> +
> +        spice_win_usb_driver_install(installer, device, cancellable,
> +            spice_usb_device_manager_drv_install_cb,
> +            cbinfo);
> +    }
> +    else
> +    {
> +        _spice_usb_device_manager_connect_device_async(self,
> +            device,
> +            cancellable,
> +            callback,
> +            user_data);
>      }
> -    installer = self->priv->installer;
> -    cbinfo = g_new0(UsbInstallCbInfo, 1);
> -    cbinfo->manager     = self;
> -    cbinfo->device      = spice_usb_device_ref(device);
> -    cbinfo->installer   = installer;
> -    cbinfo->cancellable = cancellable;
> -    cbinfo->callback    = callback;
> -    cbinfo->user_data   = user_data;
> -    cbinfo->is_install  = TRUE;
> -
> -    spice_win_usb_driver_install(installer, device, cancellable,
> -                                 spice_usb_device_manager_drv_install_cb,
> -                                 cbinfo);
>  #else
>      _spice_usb_device_manager_connect_device_async(self,
>                                                     device,
> @@ -1558,36 +1631,39 @@ void spice_usb_device_manager_disconnect_device(SpiceUsbDeviceManager *self,
>          spice_usbredir_channel_disconnect_device(channel);
>  
>  #ifdef USE_WINUSB
> -    SpiceWinUsbDriver *installer;
> -    UsbInstallCbInfo *cbinfo;
> -    guint8 state;
> -
> -    g_warn_if_fail(device != NULL);
> -    g_warn_if_fail(self->priv->installer != NULL);
> -
> -    state = spice_usb_device_get_state(device);
> -    if ((state != SPICE_USB_DEVICE_STATE_INSTALLED) &&
> -        (state != SPICE_USB_DEVICE_STATE_CONNECTED)) {
> -        return;
> -    }
> +    if (! use_usbdk)
> +    {
> +        SpiceWinUsbDriver *installer;
> +        UsbInstallCbInfo *cbinfo;
> +        guint8 state;
> +
> +        g_warn_if_fail(device != NULL);
> +        g_warn_if_fail(self->priv->installer != NULL);
> +
> +        state = spice_usb_device_get_state(device);
> +        if ((state != SPICE_USB_DEVICE_STATE_INSTALLED) &&
> +            (state != SPICE_USB_DEVICE_STATE_CONNECTED)) {
> +            return;
> +        }
>  
> -    spice_usb_device_set_state(device, SPICE_USB_DEVICE_STATE_UNINSTALLING);
> -    if (! self->priv->installer) {
> -        self->priv->installer = spice_win_usb_driver_new();
> +        spice_usb_device_set_state(device, SPICE_USB_DEVICE_STATE_UNINSTALLING);
> +        if (! self->priv->installer) {
> +            self->priv->installer = spice_win_usb_driver_new();
> +        }
> +        installer = self->priv->installer;
> +        cbinfo = g_new0(UsbInstallCbInfo, 1);
> +        cbinfo->manager     = self;
> +        cbinfo->device      = spice_usb_device_ref(device);
> +        cbinfo->installer   = installer;
> +        cbinfo->cancellable = NULL;
> +        cbinfo->callback    = NULL;
> +        cbinfo->user_data   = NULL;
> +        cbinfo->is_install  = FALSE;
> +
> +        spice_win_usb_driver_uninstall(installer, device, NULL,
> +                                       spice_usb_device_manager_drv_install_cb,
> +                                       cbinfo);
>      }
> -    installer = self->priv->installer;
> -    cbinfo = g_new0(UsbInstallCbInfo, 1);
> -    cbinfo->manager     = self;
> -    cbinfo->device      = spice_usb_device_ref(device);
> -    cbinfo->installer   = installer;
> -    cbinfo->cancellable = NULL;
> -    cbinfo->callback    = NULL;
> -    cbinfo->user_data   = NULL;
> -    cbinfo->is_install  = FALSE;
> -
> -    spice_win_usb_driver_uninstall(installer, device, NULL,
> -                                   spice_usb_device_manager_drv_install_cb,
> -                                   cbinfo);
>  #endif
>  
>  #endif
> @@ -1805,6 +1881,11 @@ guint16 spice_usb_device_get_pid(const SpiceUsbDevice *device)
>  #ifdef USE_WINUSB
>  void spice_usb_device_set_state(SpiceUsbDevice *device, guint8 state)
>  {
> +    if (use_usbdk)
> +    {
> +        return;
> +    }
> +

Can be a g_return_if_fail too

>      SpiceUsbDeviceInfo *info = (SpiceUsbDeviceInfo *)device;
>  
>      g_return_if_fail(info != NULL);
> @@ -1814,6 +1895,11 @@ void spice_usb_device_set_state(SpiceUsbDevice *device, guint8 state)
>  
>  guint8 spice_usb_device_get_state(SpiceUsbDevice *device)
>  {
> +    if (use_usbdk)
> +    {
> +        return 0;
> +    }
> +

Can be a g_return_if_fail too

Christophe

Attachment: pgpIJEqUuQze4.pgp
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]