Thanks, Christophe, We’ll modify the patch according to your comments. > On May 28, 2015, at 18:09 PM, Christophe Fergeau <cferg...@redhat.com> wrote: > > On Thu, May 28, 2015 at 01:24:02PM +0300, Kirill Moizik wrote: >> From: Pavel Gurvich <pa...@daynix.com <mailto:pa...@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 <pa...@daynix.com> >> Signed-off-by: Dmitry Fleytman <dmi...@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
_______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
