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

[Jonathon Jongsma]

ACK

----- Original Message -----
> From: "Marc-André Lureau" <marcandre.lureau at gmail.com>
> To: "Jonathon Jongsma" <jjongsma at redhat.com>
> Cc: "spice-devel" <spice-devel at lists.freedesktop.org>
> Sent: Monday, April 28, 2014 12:40:20 PM
> Subject: Re: [Spice-devel] [PATCH spice-gtk 08/14] usb: remove useless device ref/unref
> 
> Since the device can be removed from devices, while the async is till being
> performed, I changed the commit that way:
> 
> commit 1542b1103c4a8df27eadd281501b39d2678117f1
> Author: Marc-André Lureau <marcandre.lureau at gmail.com>
> Date:   Wed Apr 23 17:24:09 2014 +0200
> 
>     usb: unref the device when it is no longer needed
> 
>     The current code unref() the device too early, it must be unref after it
>     is no longer needed.
> 
> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
> index a505c19..fc776e9 100644
> --- a/gtk/usb-device-manager.c
> +++ b/gtk/usb-device-manager.c
> @@ -1094,7 +1094,6 @@ 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 */
> @@ -1104,6 +1103,7 @@ static void
> spice_usb_device_manager_drv_install_cb(GObject *gobject,
>                                                     callback,
>                                                     user_data);
> 
> +    spice_usb_device_unref(device);
>  }
> 
>  static void spice_usb_device_manager_drv_uninstall_cb(GObject *gobject,
> @@ -1122,9 +1122,9 @@ 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);
> 
> +    spice_usb_device_unref(cbinfo->device);
>      g_free(cbinfo);
>  }
> 
> 
> 
> On Mon, Apr 28, 2014 at 7:03 PM, Marc-André Lureau <
> marcandre.lureau at gmail.com> wrote:
> 
> >
> >
> >
> > 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
> >
> 
> 
> 
> --
> Marc-André Lureau
>