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

[Marc-André Lureau]

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

>
>
> ----- Original Message -----
> > From: "Marc-André Lureau" <marcandre.lureau at gmail.com>
> > To: "Jonathon Jongsma" <jjongsma at redhat.com>
> > Cc: "Uri Lublin" <uril at redhat.com>, "Marc-André Lureau" <
> mlureau at redhat.com>, spice-devel at freedesktop.org
> > Sent: Monday, April 28, 2014 11:54:03 AM
> > Subject: Re: [Spice-devel] [PATCH spice-gtk 08/14] usb: remove useless
> device ref/unref
> >
> > 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.
>
>
> Not if you move the unref after the line where you manipulate it... I
> agree with you that the *current* code is pointless.  But if you move the
> unref to the very end of the function, the ref ceases to be pointless.
>  right?
>
>
>
If it was useless until now, why add it? Just because we are not sure?
Sigh, I guess I have to figure out to convince ourself the code was safe
(or not).


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