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