<div dir="ltr">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 <<a href="mailto:marcandre.lureau@gmail.com">marcandre.lureau@gmail.com</a>> 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); } </div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Apr 28, 2014 at 7:03 PM, Marc-André Lureau <<a href="mailto:marcandre.lureau@gmail.com" target="_blank">marcandre.lureau@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote"><div><div class="h5">On Mon, Apr 28, 2014 at 7:01 PM, Jonathon Jongsma <<a href="mailto:jjongsma@redhat.com" target="_blank">jjongsma@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div> ----- Original Message ----- > From: "Marc-André Lureau" <<a href="mailto:marcandre.lureau@gmail.com" target="_blank">marcandre.lureau@gmail.com</a>> </div><div><div>> To: "Jonathon Jongsma" <<a href="mailto:jjongsma@redhat.com" target="_blank">jjongsma@redhat.com</a>> > Cc: "Uri Lublin" <<a href="mailto:uril@redhat.com" target="_blank">uril@redhat.com</a>>, "Marc-André Lureau" <<a href="mailto:mlureau@redhat.com" target="_blank">mlureau@redhat.com</a>>, <a href="mailto:spice-devel@freedesktop.org" target="_blank">spice-devel@freedesktop.org</a> > 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 <<a href="mailto:jjongsma@redhat.com" target="_blank">jjongsma@redhat.com</a>>wrote: > > > ----- Original Message ----- > > > From: "Marc-André Lureau" <<a href="mailto:marcandre.lureau@gmail.com" target="_blank">marcandre.lureau@gmail.com</a>> > > > To: "Uri Lublin" <<a href="mailto:uril@redhat.com" target="_blank">uril@redhat.com</a>> > > > Cc: "Marc-André Lureau" <<a href="mailto:mlureau@redhat.com" target="_blank">mlureau@redhat.com</a>>, > > <a href="mailto:spice-devel@freedesktop.org" target="_blank">spice-devel@freedesktop.org</a> > > > 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 < <a href="mailto:uril@redhat.com" target="_blank">uril@redhat.com</a> > 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. </div></div>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? <div><div> </div></div></blockquote><div> </div></div></div><div>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). </div></div> -- Marc-André Lureau </div></div> </blockquote></div> -- Marc-André Lureau </div>