On Fri, 2016-02-05 at 10:21 -0600, Jonathon Jongsma wrote:
> 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.
... And now I notice that priv->usbdk_api has not actually been initialized in
this patch. It's not until patch 10 that usbdk_api_load() is called. So this
call will segfault unless these patches are re-ordered so that the api is loaded
before it's used.
> >
> > > + 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel