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

[Hans de Goede]

Hi,

On 12/23/2011 06:26 PM, Christophe Fergeau wrote:
> On Mon, Dec 19, 2011 at 12:24:37PM +0100, Hans de Goede wrote:

<snip>

>> @@ -345,26 +351,17 @@ GUsbDevice *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel)
>>       return channel->priv->device;
>>   }
>>
>> -G_GNUC_INTERNAL
>> -void spice_usbredir_channel_do_write(SpiceUsbredirChannel *channel)
>> +/* Note that this function must be re-entrant safe, as it can get called
>> +   from both the main thread as well as from the usb event handling thread */
>> +static void usbredir_write_flush_callback(void *user_data)
>>   {
>> +    SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(user_data);
>>       SpiceUsbredirChannelPrivate *priv = channel->priv;
>>
>> -    /* No recursion allowed! */
>> -    g_return_if_fail(priv->msg_out == NULL);
>> -
>> -    if (priv->state != STATE_CONNECTED ||
>> -            !usbredirhost_has_data_to_write(priv->host))
>> +    if (priv->state != STATE_CONNECTED)
>
> If this function can run from the usb event handling thread,

Yes it can.

> 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.

Which leaves the channel going up scenario. In this scenario the usb event
handling thread will never call the usbredir_write_flush_callback for
*this* channel since no usb packets awaiting completion will have been
submitted for this channel, as they are only submitted in response to
requests received over the channel.

Regards,

Hans