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

Submitted by Uri Lublin on April 28, 2014, 12:46 p.m.

Details

Message ID 535E4D96.4010406@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Uri Lublin April 28, 2014, 12:46 p.m.
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.

Thanks,
     Uri.

-----

SPICE_USB_DEVICE_STATE_INSTALLED);
      } else {
@@ -1122,6 +1120,8 @@ static void 
spice_usb_device_manager_drv_install_cb(GObject *gobject,
          g_warning("failed to %s win usb driver (status=0)", opstr);
      }

+    spice_usb_device_unref(device);
+
      if (! is_install) {
          return;
      }

Patch hide | download patch | download mbox

diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
index 1051d10..d8bfdd8 100644
--- a/gtk/usb-device-manager.c
+++ b/gtk/usb-device-manager.c
@@ -1105,8 +1105,6 @@  static void 
spice_usb_device_manager_drv_install_cb(GObject *gobject,

      status = spice_win_usb_driver_install_finish(installer, res, &err);

-    spice_usb_device_unref(device);
-
      if (is_install) {
          spice_usb_device_set_state(device, 

Comments

----- 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?
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.
On Mon, Apr 28, 2014 at 4:42 PM, Uri Lublin <uril@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).
----- Original Message -----
> From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
> To: "Uri Lublin" <uril@redhat.com>
> Cc: "Marc-André Lureau" <mlureau@redhat.com>, spice-devel@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@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 actually free these SpiceUsbDeviceInfo structs.  And if we fix that leak, we'll then start crashing here.  I don't see any downside to holding the ref, even if doesn't seem necessary at the moment.



> 
> --
> Marc-André Lureau
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
On Mon, Apr 28, 2014 at 6:48 PM, Jonathon Jongsma <jjongsma@redhat.com>wrote:

> ----- Original Message -----
> > From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
> > To: "Uri Lublin" <uril@redhat.com>
> > Cc: "Marc-André Lureau" <mlureau@redhat.com>,
> spice-devel@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@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.

actually free these SpiceUsbDeviceInfo structs.  And if we fix that leak,
> we'll then start crashing here.  I don't see any downside to holding the
> ref, even if doesn't seem necessary at the moment.
>
>
>
> >
> > --
> > Marc-André Lureau
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
>
----- Original Message -----
> From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
> To: "Jonathon Jongsma" <jjongsma@redhat.com>
> Cc: "Uri Lublin" <uril@redhat.com>, "Marc-André Lureau" <mlureau@redhat.com>, spice-devel@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@redhat.com>wrote:
> 
> > ----- Original Message -----
> > > From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
> > > To: "Uri Lublin" <uril@redhat.com>
> > > Cc: "Marc-André Lureau" <mlureau@redhat.com>,
> > spice-devel@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@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? 


> 
> actually free these SpiceUsbDeviceInfo structs.  And if we fix that leak,
> > we'll then start crashing here.  I don't see any downside to holding the
> > ref, even if doesn't seem necessary at the moment.
> >
> >
> >
> > >
> > > --
> > > Marc-André Lureau
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> >
> 
> 
> 
> --
> Marc-André Lureau
>
On Mon, Apr 28, 2014 at 7:01 PM, Jonathon Jongsma <jjongsma@redhat.com>wrote:

>
>
> ----- Original Message -----
> > From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
> > To: "Jonathon Jongsma" <jjongsma@redhat.com>
> > Cc: "Uri Lublin" <uril@redhat.com>, "Marc-André Lureau" <
> mlureau@redhat.com>, spice-devel@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@redhat.com
> >wrote:
> >
> > > ----- Original Message -----
> > > > From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
> > > > To: "Uri Lublin" <uril@redhat.com>
> > > > Cc: "Marc-André Lureau" <mlureau@redhat.com>,
> > > spice-devel@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@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).