Re: [spice-gtk v2 8/9] usb-redir: move USB events handling to USB backend

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jul 23, 2019 at 10:27:07AM +0300, Yuri Benditovich wrote:
> Before this commit:
> usb-device-manager starts thread for handling libusb events:
> on Linux - from the beginning (as it is needed for hotplug
> callbacks), on Windows - starts it on first redirection and
> stops when there are no redirections (it can't keep the thread
> when there are no redirections as with libusb < 1.0.21 it will
> not be able to force the thread to exit if there are no events).
> 
> Current commit moves the event thread and handling events
> completely to usb backend; usb-device-manager and other
> are not aware of libusb and should not assume what it needs
> to work. We start the event thread from the beginning on both
> Linux and Windows. On Linux it works only for hotplug callbacks,
> on Windows - just waits until device redirection starts.
> On dispose of usb-device-manager (when hotplug callbacks are
> deregistered), we interrupt the thread once to stop it.
> 
> This removes many lines of code and also removes all the
> differences between Linux and Windows in usb-device-manager.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@xxxxxxxxxx>

Acked-by: Victor Toso <victortoso@xxxxxxxxxx>

> ---
>  src/channel-usbredir.c        | 28 -------------
>  src/usb-backend.c             | 54 +++++++++++++++++-------
>  src/usb-backend.h             |  2 -
>  src/usb-device-manager-priv.h |  6 ---
>  src/usb-device-manager.c      | 79 -----------------------------------
>  5 files changed, 39 insertions(+), 130 deletions(-)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 04acf0b..8d4cd66 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -72,7 +72,6 @@ struct _SpiceUsbredirChannelPrivate {
>      SpiceUsbAclHelper *acl_helper;
>  #endif
>      GMutex device_connect_mutex;
> -    SpiceUsbDeviceManager *usb_device_manager;
>  };
>  
>  static void channel_set_handlers(SpiceChannelClass *klass);
> @@ -169,11 +168,6 @@ static void spice_usbredir_channel_dispose(GObject *obj)
>      SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(obj);
>  
>      spice_usbredir_channel_disconnect_device(channel);
> -    /* This should have been set to NULL during device disconnection,
> -     * but better not to leak it if this does not happen for some reason
> -     */
> -    g_warn_if_fail(channel->priv->usb_device_manager == NULL);
> -    g_clear_object(&channel->priv->usb_device_manager);
>  
>      /* Chain up to the parent class */
>      if (G_OBJECT_CLASS(spice_usbredir_channel_parent_class)->dispose)
> @@ -248,8 +242,6 @@ static gboolean spice_usbredir_channel_open_device(
>      SpiceUsbredirChannel *channel, GError **err)
>  {
>      SpiceUsbredirChannelPrivate *priv = channel->priv;
> -    SpiceSession *session;
> -    SpiceUsbDeviceManager *manager;
>  
>      g_return_val_if_fail(priv->state == STATE_DISCONNECTED
>  #ifdef USE_POLKIT
> @@ -265,16 +257,6 @@ static gboolean spice_usbredir_channel_open_device(
>          return FALSE;
>      }
>  
> -    session = spice_channel_get_session(SPICE_CHANNEL(channel));
> -    manager = spice_usb_device_manager_get(session, NULL);
> -    g_return_val_if_fail(manager != NULL, FALSE);
> -
> -    priv->usb_device_manager = g_object_ref(manager);
> -    if (!spice_usb_device_manager_start_event_listening(priv->usb_device_manager, err)) {
> -        spice_usb_backend_channel_detach(priv->host);
> -        return FALSE;
> -    }
> -
>      priv->state = STATE_CONNECTED;
>  
>      return TRUE;
> @@ -445,16 +427,6 @@ void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel)
>          break;
>  #endif
>      case STATE_CONNECTED:
> -        /*
> -         * This sets the usb event thread run condition to FALSE, therefor
> -         * it must be done before usbredirhost_set_device NULL, as
> -         * usbredirhost_set_device NULL will interrupt the
> -         * libusb_handle_events call in the thread.
> -         */
> -        g_warn_if_fail(priv->usb_device_manager != NULL);
> -        spice_usb_device_manager_stop_event_listening(priv->usb_device_manager);
> -        g_clear_object(&priv->usb_device_manager);
> -
>          /* This also closes the libusb handle we passed from open_device */
>          spice_usb_backend_channel_detach(priv->host);
>          g_clear_pointer(&priv->device, spice_usb_backend_device_unref);
> diff --git a/src/usb-backend.c b/src/usb-backend.c
> index 486875e..bbd3b67 100644
> --- a/src/usb-backend.c
> +++ b/src/usb-backend.c
> @@ -58,6 +58,9 @@ struct _SpiceUsbBackend
>      usb_hot_plug_callback hotplug_callback;
>      void *hotplug_user_data;
>      libusb_hotplug_callback_handle hotplug_handle;
> +    GThread *event_thread;
> +    gint event_thread_run;
> +
>  #ifdef G_OS_WIN32
>      HANDLE hWnd;
>      libusb_device **libusb_device_list;
> @@ -406,28 +409,25 @@ SpiceUsbBackend *spice_usb_backend_new(GError **error)
>      return be;
>  }
>  
> -gboolean spice_usb_backend_handle_events(SpiceUsbBackend *be)
> +static gpointer handle_libusb_events(gpointer user_data)
>  {
> +    SpiceUsbBackend *be = user_data;
>      SPICE_DEBUG("%s >>", __FUNCTION__);
> -    gboolean ok = FALSE;
> -    if (be->libusb_context) {
> -        int res = libusb_handle_events(be->libusb_context);
> -        ok = res == 0;
> +    int res = 0;
> +    const char *desc = "";
> +    while (g_atomic_int_get(&be->event_thread_run)) {
> +        res = libusb_handle_events(be->libusb_context);
>          if (res && res != LIBUSB_ERROR_INTERRUPTED) {
> -            const char *desc = libusb_strerror(res);
> +            desc = libusb_strerror(res);
>              g_warning("Error handling USB events: %s [%i]", desc, res);
> -            ok = FALSE;
> +            break;
>          }
>      }
> -    SPICE_DEBUG("%s << %d", __FUNCTION__, ok);
> -    return ok;
> -}
> -
> -void spice_usb_backend_interrupt_event_handler(SpiceUsbBackend *be)
> -{
> -    if (be->libusb_context) {
> -        libusb_interrupt_event_handler(be->libusb_context);
> +    if (be->event_thread_run) {
> +        SPICE_DEBUG("%s: the thread aborted, %s(%d)", __FUNCTION__, desc, res);
>      }
> +    SPICE_DEBUG("%s <<", __FUNCTION__);
> +    return NULL;
>  }
>  
>  void spice_usb_backend_deregister_hotplug(SpiceUsbBackend *be)
> @@ -438,6 +438,12 @@ void spice_usb_backend_deregister_hotplug(SpiceUsbBackend *be)
>          be->hotplug_handle = 0;
>      }
>      be->hotplug_callback = NULL;
> +    g_atomic_int_set(&be->event_thread_run, FALSE);
> +    if (be->event_thread) {
> +        libusb_interrupt_event_handler(be->libusb_context);
> +        g_thread_join(be->event_thread);
> +        be->event_thread = NULL;
> +    }
>  }
>  
>  gboolean spice_usb_backend_register_hotplug(SpiceUsbBackend *be,
> @@ -461,6 +467,16 @@ gboolean spice_usb_backend_register_hotplug(SpiceUsbBackend *be,
>                      "Error on USB hotplug detection: %s [%i]", desc, rc);
>          return FALSE;
>      }
> +
> +    g_atomic_int_set(&be->event_thread_run, TRUE);
> +    be->event_thread = g_thread_new("usb_ev_thread",
> +                                    handle_libusb_events,
> +                                    be);
> +    if (!be->event_thread) {
> +        g_warning("Error starting event thread");
> +        spice_usb_backend_deregister_hotplug(be);
> +        return FALSE;
> +    }
>      return TRUE;
>  }
>  
> @@ -468,6 +484,14 @@ void spice_usb_backend_delete(SpiceUsbBackend *be)
>  {
>      g_return_if_fail(be != NULL);
>      SPICE_DEBUG("%s >>", __FUNCTION__);
> +    /*
> +        we expect hotplug callbacks are deregistered
> +        and the event thread is terminated. If not,
> +        we do that anyway before delete the backend
> +    */
> +    g_warn_if_fail(be->hotplug_handle == 0);
> +    g_warn_if_fail(be->event_thread == NULL);
> +    spice_usb_backend_deregister_hotplug(be);
>      if (be->libusb_context) {
>          libusb_exit(be->libusb_context);
>      }
> diff --git a/src/usb-backend.h b/src/usb-backend.h
> index 814da46..69a490b 100644
> --- a/src/usb-backend.h
> +++ b/src/usb-backend.h
> @@ -56,8 +56,6 @@ enum {
>  SpiceUsbBackend *spice_usb_backend_new(GError **error);
>  void spice_usb_backend_delete(SpiceUsbBackend *context);
>  
> -gboolean spice_usb_backend_handle_events(SpiceUsbBackend *be);
> -void spice_usb_backend_interrupt_event_handler(SpiceUsbBackend *be);
>  gboolean spice_usb_backend_register_hotplug(SpiceUsbBackend *be,
>                                              void *user_data,
>                                              usb_hot_plug_callback proc,
> diff --git a/src/usb-device-manager-priv.h b/src/usb-device-manager-priv.h
> index 39aaf2f..66acf6d 100644
> --- a/src/usb-device-manager-priv.h
> +++ b/src/usb-device-manager-priv.h
> @@ -25,12 +25,6 @@
>  
>  G_BEGIN_DECLS
>  
> -gboolean spice_usb_device_manager_start_event_listening(
> -    SpiceUsbDeviceManager *manager, GError **err);
> -
> -void spice_usb_device_manager_stop_event_listening(
> -    SpiceUsbDeviceManager *manager);
> -
>  #ifdef USE_USBREDIR
>  void spice_usb_device_manager_device_error(
>      SpiceUsbDeviceManager *manager, SpiceUsbDevice *device, GError *err);
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 857d057..a530be9 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -93,9 +93,6 @@ struct _SpiceUsbDeviceManagerPrivate {
>      gchar *redirect_on_connect;
>  #ifdef USE_USBREDIR
>      SpiceUsbBackend *context;
> -    int event_listeners;
> -    GThread *event_thread;
> -    gint event_thread_run;
>      struct usbredirfilter_rule *auto_conn_filter_rules;
>      struct usbredirfilter_rule *redirect_on_connect_rules;
>      int auto_conn_filter_rules_count;
> @@ -261,9 +258,6 @@ static gboolean spice_usb_device_manager_initable_init(GInitable  *initable,
>                                              err)) {
>          return FALSE;
>      }
> -#ifndef G_OS_WIN32
> -    spice_usb_device_manager_start_event_listening(self, NULL);
> -#endif
>  
>      /* Start listening for usb channels connect/disconnect */
>      spice_g_signal_connect_object(priv->session, "channel-new", G_CALLBACK(channel_new), self, G_CONNECT_AFTER);
> @@ -285,27 +279,8 @@ static void spice_usb_device_manager_dispose(GObject *gobject)
>      SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(gobject);
>      SpiceUsbDeviceManagerPrivate *priv = self->priv;
>  
> -#ifndef G_OS_WIN32
> -    spice_usb_device_manager_stop_event_listening(self);
> -    if (g_atomic_int_get(&priv->event_thread_run)) {
> -        /* Force termination of the event thread even if there were some
> -         * mismatched spice_usb_device_manager_{start,stop}_event_listening
> -         * calls. Otherwise, the usb event thread will be leaked, and will
> -         * try to use the libusb context we destroy in finalize(), which would
> -         * cause a crash */
> -        g_warn_if_reached();
> -        g_atomic_int_set(&priv->event_thread_run, FALSE);
> -    }
> -#endif
>      spice_usb_backend_deregister_hotplug(priv->context);
>  
> -    if (priv->event_thread) {
> -        g_warn_if_fail(g_atomic_int_get(&priv->event_thread_run) == FALSE);
> -        g_atomic_int_set(&priv->event_thread_run, FALSE);
> -        spice_usb_backend_interrupt_event_handler(priv->context);
> -        g_thread_join(priv->event_thread);
> -        priv->event_thread = NULL;
> -    }
>  #endif
>  
>      /* Chain up to the parent class */
> @@ -323,7 +298,6 @@ static void spice_usb_device_manager_finalize(GObject *gobject)
>      if (priv->devices) {
>          g_ptr_array_unref(priv->devices);
>      }
> -    g_return_if_fail(priv->event_thread == NULL);
>      if (priv->context) {
>          spice_usb_backend_delete(priv->context);
>      }
> @@ -915,59 +889,6 @@ static void spice_usb_device_manager_channel_connect_cb(
>  /* ------------------------------------------------------------------ */
>  /* private api                                                        */
>  
> -static gpointer spice_usb_device_manager_usb_ev_thread(gpointer user_data)
> -{
> -    SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(user_data);
> -    SpiceUsbDeviceManagerPrivate *priv = self->priv;
> -
> -    while (g_atomic_int_get(&priv->event_thread_run)) {
> -        if (!spice_usb_backend_handle_events(priv->context)) {
> -            break;
> -        }
> -    }
> -
> -    return NULL;
> -}
> -
> -gboolean spice_usb_device_manager_start_event_listening(
> -    SpiceUsbDeviceManager *self, GError **err)
> -{
> -    SpiceUsbDeviceManagerPrivate *priv = self->priv;
> -
> -    g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
> -
> -    priv->event_listeners++;
> -    if (priv->event_listeners > 1)
> -        return TRUE;
> -
> -    /* 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_atomic_int_set(&priv->event_thread_run, FALSE);
> -        spice_usb_backend_interrupt_event_handler(priv->context);
> -         g_thread_join(priv->event_thread);
> -         priv->event_thread = NULL;
> -    }
> -    g_atomic_int_set(&priv->event_thread_run, TRUE);
> -    priv->event_thread = g_thread_new("usb_ev_thread",
> -                                      spice_usb_device_manager_usb_ev_thread,
> -                                      self);
> -    return priv->event_thread != NULL;
> -}
> -
> -void spice_usb_device_manager_stop_event_listening(
> -    SpiceUsbDeviceManager *self)
> -{
> -    SpiceUsbDeviceManagerPrivate *priv = self->priv;
> -
> -    g_return_if_fail(priv->event_listeners > 0);
> -
> -    priv->event_listeners--;
> -    if (priv->event_listeners == 0)
> -        g_atomic_int_set(&priv->event_thread_run, FALSE);
> -}
> -
>  static void spice_usb_device_manager_check_redir_on_connect(
>      SpiceUsbDeviceManager *self, SpiceChannel *channel)
>  {
> -- 
> 2.17.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]