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

[Marc-André Lureau <marcandre.lureau@xxxxxxxxx>]

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@xxxxxxxxx>
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@xxxxxxxxx> wrote:

On Mon, Apr 28, 2014 at 7:01 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:

----- Original Message -----
> From: "Marc-André Lureau" <marcandre.lureau@xxxxxxxxx>

> To: "Jonathon Jongsma" <jjongsma@xxxxxxxxxx>
> Cc: "Uri Lublin" <uril@xxxxxxxxxx>, "Marc-André Lureau" <mlureau@xxxxxxxxxx>, spice-devel@xxxxxxxxxxxxxxx
> Sent: Monday, April 28, 2014 11:54:03 AM
> Subject: Re: [PATCH spice-gtk 08/14] usb: remove useless device ref/unref
>
> On Mon, Apr 28, 2014 at 6:48 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx>wrote:
>
> > ----- 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
>
>
> 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

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel