[Spice-devel] [PATCH 3/5] usbdk: make backend selection dynamic
Dmitry Fleytman
dmitry at daynix.com
Sun May 31 02:19:48 PDT 2015
Thanks, Christophe,
We’ll modify the patch according to your comments.
> On May 28, 2015, at 18:09 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
>
> On Thu, May 28, 2015 at 01:24:02PM +0300, Kirill Moizik wrote:
>> From: Pavel Gurvich <pavel at daynix.com <mailto:pavel at daynix.com>>
>>
>> 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 at daynix.com>
>> Signed-off-by: Dmitry Fleytman <dmitry at daynix.com>
>> ---
>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150531/6fd3ef4b/attachment-0001.html>
More information about the Spice-devel
mailing list