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

[Hans de Goede]

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.

Regards,

Hans