ACK ----- Original Message ----- > From: "Marc-André Lureau" <marcandre.lureau@xxxxxxxxx> > To: "Jonathon Jongsma" <jjongsma@xxxxxxxxxx> > Cc: "spice-devel" <spice-devel@xxxxxxxxxxxxxxxxxxxxx> > Sent: Monday, April 28, 2014 12:40:20 PM > Subject: Re: [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@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