[Spice-devel] [PATCH spice-gtk 04/10] usbredir: Use new libusbredirhost write flush callback

[Christophe Fergeau]

On Mon, Jan 02, 2012 at 03:01:39PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 01/02/2012 01:59 PM, Christophe Fergeau wrote:
> >On Sat, Dec 24, 2011 at 10:27:40AM +0100, Hans de Goede wrote:
> >>On 12/23/2011 06:26 PM, Christophe Fergeau wrote:
> >>>is there
> >>>something preventing the check on priv->state from racing with code running
> >>>in the main thread?
> >>
> >>Oh, good one I must admit I did not think about this. I believe this is not a
> >>problem though:
> >>
> >>The write flush callback causes libusbredirhost to write out writes queued
> >>inside libusbredirhost (so in essence they are moved from a libusbredirhost
> >>internal queue to the channel's xmit queue).
> >>
> >>There are 2 cases where the state can change, the channel going up (so going
> >>to state == CONNECTED) and the channel going down. Going down is not really
> >>interesting, because once the channel is disconnected it does not matter if
> >>writes are buffered inside libusbredirhost or inside the channel's xmit
> >>queue, as the channel is tore down they will get discarded either way.
> >
> >I looked a bit more at the code, and I'm still worried by
> >spice_usbredir_channel_disconnect running while
> >usbredir_write_flush_callback executes, ie:
> >
> >flush_callback checks priv->state != STATE_CONNECTED
> >
> >disconnect is called from the main thread (eg from dispose) and calls
> >usbredirhost_close(priv->host) which frees priv->host and then is preempted
> >
> >flush_callback resumes and runs usbredirhost_write_guest_data(priv->host);
> >which has been freed but not set to NULL.
> >
> >
> >I'm not exactly sure at what point the flush callback stops being called by
> >usbredir, so I prefer to mention this here and get explained why this can't
> >happen :)
> 
> This cannot happen because usbredirhost_close() calls libusb_close on the device
> handle for the device for this channel, and this will cleanup (kill) any
> not yet completed transfers. libusb guarantees that after a libusb_close() no
> packet complete callbacks will get called for the closed device. So after
> libusb_close() has completed the usbredir_write_flush_callback will never get
> called for this channel again.

Yes, but in the example I gave, the flush callback starts executing before any
call to libusb_close, and then got preempted by the main thread calling
_disconnect. There will be no more invokations of the flush callback after
the call to libusb_close, but the flush callback execution that got
preempted won't magically get cancelled, will it?

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20120102/f8080840/attachment.pgp>