<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Apr 28, 2014 at 6:48 PM, Jonathon Jongsma <span dir="ltr"><<a href="mailto:jjongsma@redhat.com" target="_blank">jjongsma@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">----- Original Message -----<br>
> From: "Marc-André Lureau" <<a href="mailto:marcandre.lureau@gmail.com">marcandre.lureau@gmail.com</a>><br>
</div><div><div class="h5">> To: "Uri Lublin" <<a href="mailto:uril@redhat.com">uril@redhat.com</a>><br>
> Cc: "Marc-André Lureau" <<a href="mailto:mlureau@redhat.com">mlureau@redhat.com</a>>, <a href="mailto:spice-devel@freedesktop.org">spice-devel@freedesktop.org</a><br>
> Sent: Monday, April 28, 2014 9:49:59 AM<br>
> Subject: Re: [Spice-devel] [PATCH spice-gtk 08/14] usb: remove useless        device ref/unref<br>
><br>
><br>
> On Mon, Apr 28, 2014 at 4:42 PM, Uri Lublin < <a href="mailto:uril@redhat.com">uril@redhat.com</a> > wrote:<br>
><br>
><br>
><br>
> On 04/28/2014 03:49 PM, Marc-André Lureau wrote:<br>
><br>
><br>
><br>
> ----- Original Message -----<br>
><br>
><br>
> On 04/23/2014 09:09 PM, Marc-André Lureau wrote:<br>
><br>
><br>
> A code doing an unref() on an object just before manipulating it looks<br>
> horribly suspicious...<br>
> Suspicious indeed.<br>
><br>
> But probably it's better to move the unref below instead of removing the<br>
> ref/unref (see below).<br>
><br>
> A device is ref'ed when install/uninstall starts and is unref'ed when<br>
> install/uninstall finishes.<br>
><br>
> Possibly during install the ref is not needed (as there is another ref<br>
> for the redir operation),<br>
> but I think it is needed during uninstall.<br>
><br>
> It wasn't needed until now (or we would have had crashes before), why would<br>
> it be now?<br>
><br>
> Generally, no crashes does not mean ref/unref are not needed.<br>
><br>
><br>
> When I read this code:<br>
><br>
> @@ -1122,7 +1120,6 @@ static void spice_usb_device_manager_drv_<br>
> uninstall_cb(GObject *gobject,<br>
> g_clear_error(&err);<br>
> }<br>
><br>
> - spice_usb_device_unref(cbinfo->device);<br>
> spice_usb_device_set_state(cbinfo->device, SPICE_USB_DEVICE_STATE_NONE);<br>
><br>
> It seems pretty clear that this cbinfo ref wasn't involved to keep the device<br>
> alive (same fo the install case).<br>
<br>
<br>
</div></div>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 </blockquote><div><br></div><div>
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.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<span class="HOEnZb"><font color="#888888"><br>
<br>
<br>
><br>
> --<br>
> Marc-André Lureau<br>
</font></span><div class="HOEnZb"><div class="h5">><br>
> _______________________________________________<br>
> Spice-devel mailing list<br>
> <a href="mailto:Spice-devel@lists.freedesktop.org">Spice-devel@lists.freedesktop.org</a><br>
> <a href="http://lists.freedesktop.org/mailman/listinfo/spice-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/spice-devel</a><br>
><br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br>Marc-André Lureau
</div></div>