<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Apr 28, 2014 at 7:01 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=""><br>
<br>
----- 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: "Jonathon Jongsma" <<a href="mailto:jjongsma@redhat.com">jjongsma@redhat.com</a>><br>
> Cc: "Uri Lublin" <<a href="mailto:uril@redhat.com">uril@redhat.com</a>>, "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 11:54:03 AM<br>
> Subject: Re: [Spice-devel] [PATCH spice-gtk 08/14] usb: remove useless device ref/unref<br>
><br>
> On Mon, Apr 28, 2014 at 6:48 PM, Jonathon Jongsma <<a href="mailto:jjongsma@redhat.com">jjongsma@redhat.com</a>>wrote:<br>
><br>
> > ----- Original Message -----<br>
> > > From: "Marc-André Lureau" <<a href="mailto:marcandre.lureau@gmail.com">marcandre.lureau@gmail.com</a>><br>
> > > 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>>,<br>
> > <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<br>
> >      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<br>
> > 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<br>
> > device<br>
> > > alive (same fo the install case).<br>
> ><br>
> ><br>
> > Well, it could simply mean that there is a race and the async operation<br>
> > usually completes before it is freed? Or maybe it means there's a leak<br>
> > somewhere else and we never<br>
><br>
><br>
> that potential race would still exists, with or without the extra ref. Here<br>
> the extra ref is useless, since the object was already unref before it was<br>
> manipulated.<br>
<br>
<br>
</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?<br>

<div class="HOEnZb"><div class="h5"><br>
<br></div></div></blockquote><div><br></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).<br></div></div><br clear="all">
<br>-- <br>Marc-André Lureau
</div></div>