On Mon, Apr 28, 2014 at 6:48 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:
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> 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).
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
--
Marc-André Lureau
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel