[Spice-devel] [PATCH spice-gtk 07/10] usbredir: Handle usb events from a thread

[Hans de Goede]

Hi,

On 01/02/2012 02:23 PM, Christophe Fergeau wrote:
> On Mon, Dec 19, 2011 at 12:24:40PM +0100, Hans de Goede wrote:
>> This solves various latency issues with USB handling.
>>
>> Signed-off-by: Hans de Goede<hdegoede at redhat.com>

<snip>

>> +static gpointer spice_usb_device_magager_usb_ev_thread(gpointer user_data)
>
> s/magager/manager
>

Will fix.

>> +{
>> +    SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(user_data);
>> +    SpiceUsbDeviceManagerPrivate *priv = self->priv;
>> +
>> +    while (priv->event_thread_run) {
>> +        libusb_handle_events(_g_usb_context_get_context(priv->context));
>> +    }
>
> Maybe we should log something when libusb_handle_events returns
> LIBUSB_ERROR_* ?
>

That seems like a good idea, will fix.

>> +
>> +    return NULL;
>> +}
>> +
>>   gboolean spice_usb_device_manager_start_event_listening(
>>       SpiceUsbDeviceManager *self, GError **err)
>>   {
>> @@ -491,10 +507,18 @@ gboolean spice_usb_device_manager_start_event_listening(
>>       if (priv->event_listeners>  1)
>>           return TRUE;
>>
>> -    g_return_val_if_fail(priv->source == NULL, FALSE);
>> -
>> -    priv->source = g_usb_source_new(priv->main_context, priv->context, err);
>> -    return priv->source != NULL;
>> +    /* We don't join the thread when we stop event listening, as the
>> +       libusb_handle_events call in the thread won't exit until the
>> +       libusb_close call for the device is made from usbredirhost_close. */
>> +    if (priv->event_thread) {
>> +         g_thread_join(priv->event_thread);
>> +         priv->event_thread = NULL;
>> +    }
>> +    priv->event_thread_run = TRUE;
>> +    priv->event_thread = g_thread_create(
>> +                             spice_usb_device_magager_usb_ev_thread,
>> +                             self, TRUE, err);
>
> Do we need to join threads at all? Not needing to call g_thread_join would
> make the code simpler. Ie the 3rd arg could be FALSE here.

Yes, because the thread must be stopped before we can close the libusb_context,
which we do on usbdevicemanager destriction, which happens when a session is closed.

Is it ok for me to push this with the 2 changes agreed upon above?

Regards,

Hans