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

[Marc-André Lureau]

On Mon, Apr 28, 2014 at 6:48 PM, Jonathon Jongsma <jjongsma at redhat.com>wrote:

> ----- 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


that potential race would still exists, with or without the extra ref. Here
the extra ref is useless, since the object was already unref before it was
manipulated.

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
> >
>



-- 
Marc-André Lureau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20140428/1af53c59/attachment.html>