----- Original Message ----- > From: "Marc-André Lureau" <marcandre.lureau@xxxxxxxxx> > To: "Uri Lublin" <uril@xxxxxxxxxx> > Cc: "Marc-André Lureau" <mlureau@xxxxxxxxxx>, spice-devel@xxxxxxxxxxxxxxx > Sent: Monday, April 28, 2014 9:49:59 AM > Subject: Re: [PATCH spice-gtk 08/14] usb: remove useless device ref/unref > > > On Mon, Apr 28, 2014 at 4:42 PM, Uri Lublin < uril@xxxxxxxxxx > wrote: > > > > On 04/28/2014 03:49 PM, Marc-André Lureau wrote: > > > > ----- Original Message ----- > > > On 04/23/2014 09:09 PM, Marc-André Lureau wrote: > > > A code doing an unref() on an object just before manipulating it looks > horribly suspicious... > Suspicious indeed. > > But probably it's better to move the unref below instead of removing the > ref/unref (see below). > > A device is ref'ed when install/uninstall starts and is unref'ed when > install/uninstall finishes. > > Possibly during install the ref is not needed (as there is another ref > for the redir operation), > but I think it is needed during uninstall. > > It wasn't needed until now (or we would have had crashes before), why would > it be now? > > Generally, no crashes does not mean ref/unref are not needed. > > > When I read this code: > > @@ -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); > > It seems pretty clear that this cbinfo ref wasn't involved to keep the device > alive (same fo the install case). Well, it could simply mean that there is a race and the async operation usually completes before it is freed? Or maybe it means there's a leak somewhere else and we never actually free these SpiceUsbDeviceInfo structs. And if we fix that leak, we'll then start crashing here. I don't see any downside to holding the ref, even if doesn't seem necessary at the moment. > > -- > Marc-André Lureau > > _______________________________________________ > 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