On Thu, 2016-02-04 at 16:40 -0600, Jonathon Jongsma wrote: > On Thu, 2015-10-29 at 17:26 +0200, Dmitry Fleytman wrote: > > Hide rules order UsbDk to avoid showing specific USB > > devices to Windows PnP manager. > > > > Spice-gtk loads hide rules for devices that should be > > automatically redirected on connection to prevent Windows > > from showing "New Hardware Found" wizard window for USB > > devices that do not have driver on the local system. > > > > Signed-off-by: Dmitry Fleytman <dmitry@xxxxxxxxxx> > > --- > > src/usb-device-manager.c | 58 > > ++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 58 insertions(+) > > > > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c > > index dd55276..d5fec8f 100644 > > --- a/src/usb-device-manager.c > > +++ b/src/usb-device-manager.c > > @@ -29,6 +29,10 @@ > > #include <errno.h> > > #include <libusb.h> > > > > +#ifdef G_OS_WIN32 > > +#include "usbdk_api.h" > > +#endif > > + > > #if defined(USE_GUDEV) > > #include <gudev/gudev.h> > > #elif defined(G_OS_WIN32) > > @@ -121,6 +125,8 @@ struct _SpiceUsbDeviceManagerPrivate { > > libusb_hotplug_callback_handle hp_handle; > > #endif > > #ifdef G_OS_WIN32 > > + usbdk_api_wrapper *usbdk_api; > > + HANDLE usbdk_hider_handle; > > SpiceWinUsbDriver *installer; > > #endif > > gboolean use_usbclerk; > > @@ -183,6 +189,9 @@ static void spice_usb_device_unref(SpiceUsbDevice > > *device); > > #ifdef G_OS_WIN32 > > static guint8 spice_usb_device_get_state(SpiceUsbDevice *device); > > static void spice_usb_device_set_state(SpiceUsbDevice *device, guint8 s); > > + > > +static void _usbdk_autoredir_enable(SpiceUsbDeviceManager *manager); > > +static void _usbdk_autoredir_disable(SpiceUsbDeviceManager *manager); > > #endif > > > > static gboolean spice_usb_manager_device_equal_libdev(SpiceUsbDeviceManager > > *manager, > > @@ -364,6 +373,12 @@ static void spice_usb_device_manager_finalize(GObject > > *gobject) > > g_free(priv->auto_connect_filter); > > g_free(priv->redirect_on_connect); > > > > +#ifdef G_OS_WIN32 > > + if (!priv->use_usbclerk) { > > + if(priv->auto_connect) > > + _usbdk_autoredir_disable(self); > > + } > > +#endif Another thing: Here you introduce this code within an #ifdef G_OS_WIN32 block, but in the next commit, you change this to #if defined(G_OS_WIN32) && defined(USE_USBREDIR) If this change is necessary, it should be done in this commit, not in the next one. But it seems that you could simply move it up a few lines to the block where you unref the priv->installer variable, no? Or is there some reason this has to be done after freeing auto_connect_filter and redirect_on_connect? > > /* Chain up to the parent class */ > > if (G_OBJECT_CLASS(spice_usb_device_manager_parent_class)->finalize) > > G_OBJECT_CLASS(spice_usb_device_manager_parent_class) > > ->finalize(gobject); > > @@ -415,6 +430,15 @@ static void > > spice_usb_device_manager_set_property(GObject > > *gobject, > > break; > > case PROP_AUTO_CONNECT: > > priv->auto_connect = g_value_get_boolean(value); > > It does not appear that PROP_AUTO_CONNECT is a construct-only property, so > this > property could theoretically be called multiple times. From looking at the > _usb_autoredir_(en|dis)able() functions below, it doesn't look like a good > idea > to call them multiple times. In addition, it appears racy since > _usbdk_autoredir_enable() depends on redirect_on_connect. If that property is > not set before this one, it will return without doing anything. To avoid the > race, these calls should probably go into a 'constructed' vfunc. > > > +#ifdef G_OS_WIN32 > > + if (!priv->use_usbclerk) { > > + if (priv->auto_connect) { > > + _usbdk_autoredir_enable(self); > > + } else { > > + _usbdk_autoredir_disable(self); > > + } > > + } > > +#endif > > break; > > case PROP_AUTO_CONNECT_FILTER: { > > const gchar *filter = g_value_get_string(value); > > @@ -1856,6 +1880,40 @@ guint8 spice_usb_device_get_state(SpiceUsbDevice > > *device) > > > > return info->state; > > } > > +static > > +void _usbdk_autoredir_enable(SpiceUsbDeviceManager *manager) > > +{ > > + SpiceUsbDeviceManagerPrivate *priv = manager->priv; > > + g_return_if_fail(!priv->use_usbclerk); > > + > > + if (priv->redirect_on_connect == NULL) { > > as I said above, if the 'redirect-on-connect' property is not set before 'auto > -connect', this function will return without doing anythign and print a debug > statement. > > > + SPICE_DEBUG("No autoredirect rules, no hider setup needed"); > > + return; > > + } > > + > > + priv->usbdk_hider_handle = usbdk_create_hider_handle(priv->usbdk_api); > > Also, if this function is called multiple times, I think we'll leak the handle > here. > > > + if (priv->usbdk_hider_handle == NULL) { > > + g_warning("Failed to instantiate UsbDk hider interface"); > > + return; > > + } > > + > > + usbdk_api_set_hide_rules(priv->usbdk_api, > > + priv->usbdk_hider_handle, > > + priv->redirect_on_connect); > > +} > > + > > +static > > +void _usbdk_autoredir_disable(SpiceUsbDeviceManager *manager) > > +{ > > + SpiceUsbDeviceManagerPrivate *priv = manager->priv; > > + g_return_if_fail(!priv->use_usbclerk); > > + > > + if (priv->usbdk_hider_handle != NULL) { > > + usbdk_clear_hide_rules(priv->usbdk_api, priv->usbdk_hider_handle); > > + usbdk_close_hider_handle(priv->usbdk_api, priv > > ->usbdk_hider_handle); > > + priv->usbdk_hider_handle = NULL; > > + } > > +} > > #endif > > > > static SpiceUsbDevice *spice_usb_device_ref(SpiceUsbDevice *device) > > > Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel