[Spice-devel] [spice-gtk v3 3/4] usb-redir: move USB events handling to USB backend
Yuri Benditovich
yuri.benditovich at daynix.com
Tue Jul 23 15:13:48 UTC 2019
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 at daynix.com>
---
src/channel-usbredir.c | 28 -------------
src/usb-backend.c | 49 +++++++++++++++-------
src/usb-backend.h | 2 -
src/usb-device-manager-priv.h | 6 ---
src/usb-device-manager.c | 79 -----------------------------------
5 files changed, 34 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 2a2f9fa..7c9b544 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;
@@ -410,28 +413,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)
@@ -442,6 +442,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,
@@ -465,6 +471,11 @@ 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);
return TRUE;
}
@@ -472,6 +483,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
More information about the Spice-devel
mailing list