[Spice-devel] [PATCH spice-gtk 08/14] usb: remove useless device ref/unref

[Jonathon Jongsma]

----- Original Message -----
> From: "Marc-André Lureau" <marcandre.lureau at gmail.com>
> To: "Uri Lublin" <uril at redhat.com>
> Cc: "Marc-André Lureau" <mlureau at redhat.com>, spice-devel at freedesktop.org
> Sent: Monday, April 28, 2014 9:49:59 AM
> Subject: Re: [Spice-devel] [PATCH spice-gtk 08/14] usb: remove useless	device ref/unref
> 
> 
> On Mon, Apr 28, 2014 at 4:42 PM, Uri Lublin < uril at redhat.com > 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>