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

[Hans de Goede]

Hi,

On 01/02/2012 03:10 PM, Christophe Fergeau wrote:
> 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?

Ah, sorry, I thought of that and checked the actual libusb code before sending
of the mail where I claimed this was not an issue, but I guess was not clear:

On device close libusb kills any pending transfer *and* waits for any running transfer
complete callbacks to finish, it does so in a thread safe manner.

libusb has something called an event handler lock, which is held when the callback
is called, and libusb_close takes this lock thus ensuring that any callbacks
already running, when libusb_close gets called, have completed before libusb_close
completes.

Regards,

Hans