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

[Marc-André Lureau]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20140428/f77f6732/attachment.html>