Hm, I agree that the existing code does look very suspicious, but in general I think that ensuring that a ref is held for the duration of the async operation is probably a good idea. From a very cursory investigation it looks like the spice_usb_device_manager_remove_dev() call path could be problematic. This ends up calling spice_win_usb_driver_uninstall() and then removing the device (via g_ptr_array_remove()), which could end up dropping the final ref on the object. Then you'd have a dead object in the uninstall_cb callback. Granted, the existing code doesn't appear to be safe in this scenario either... ----- Original Message ----- > From: "Marc-André Lureau" <marcandre.lureau@xxxxxxxxx> > To: spice-devel@xxxxxxxxxxxxxxx > Sent: Wednesday, April 23, 2014 1:09:13 PM > Subject: [PATCH spice-gtk 08/14] usb: remove useless device ref/unref > > A code doing an unref() on an object just before manipulating it looks > horribly suspicious... > --- > gtk/usb-device-manager.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c > index a505c19..af993aa 100644 > --- a/gtk/usb-device-manager.c > +++ b/gtk/usb-device-manager.c > @@ -1094,10 +1094,8 @@ static void > spice_usb_device_manager_drv_install_cb(GObject *gobject, > g_error_free(err); > } > > - spice_usb_device_unref(device); > spice_usb_device_set_state(device, SPICE_USB_DEVICE_STATE_INSTALLED); > > - /* device is already ref'ed */ > _spice_usb_device_manager_connect_device_async(self, > device, > cancellable, > @@ -1122,7 +1120,6 @@ static void > spice_usb_device_manager_drv_uninstall_cb(GObject *gobject, > g_clear_error(&err); > } > > - spice_usb_device_unref(cbinfo->device); > spice_usb_device_set_state(cbinfo->device, SPICE_USB_DEVICE_STATE_NONE); > > g_free(cbinfo); > @@ -1490,7 +1487,7 @@ void > spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self, > installer = self->priv->installer; > cbinfo = g_new0(UsbInstallCbInfo, 1); > cbinfo->manager = self; > - cbinfo->device = spice_usb_device_ref(device); > + cbinfo->device = device; > cbinfo->installer = installer; > cbinfo->cancellable = cancellable; > cbinfo->callback = callback; > @@ -1566,7 +1563,7 @@ void > spice_usb_device_manager_disconnect_device(SpiceUsbDeviceManager *self, > installer = self->priv->installer; > cbinfo = g_new0(UsbInstallCbInfo, 1); > cbinfo->manager = self; > - cbinfo->device = spice_usb_device_ref(device); > + cbinfo->device = device; > cbinfo->installer = installer; > cbinfo->cancellable = NULL; > cbinfo->callback = NULL; > -- > 1.8.5.3 > > _______________________________________________ > 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