[Spice-commits] 3 commits - src/channel-usbredir.c src/usb-device-manager.c
Christophe Fergau
teuf at kemper.freedesktop.org
Fri Jul 1 14:23:50 UTC 2016
src/channel-usbredir.c | 26 +++++++++++++++-----------
src/usb-device-manager.c | 10 +++++-----
2 files changed, 20 insertions(+), 16 deletions(-)
New commits:
commit 4a777ec0a6153e8229c406ff9b1589e30195afc2
Author: Christophe Fergeau <cfergeau at redhat.com>
Date: Wed Jun 29 16:57:36 2016 +0200
usb-channel: Really stop listening for USB events on disconnection
When using USB redirection, it's fairly easy to leak the thread handling
USB events, which will eventually cause problems in long lived apps.
In particular, in virt-manager, one can:
- start a VM
- connect to it with SPICE
- open the USB redirection window
- redirect a device
- close the SPICE window
-> the SpiceUsbDeviceManager instance will be destroyed (including the
USB context it owns), but the associated event thread will keep running.
Since it's running a loop blocking on libusb_handle_events(priv->context),
the loop will eventually try to use the USB context we just destroyed
causing a crash.
We can get in this situation when redirecting a USB device because we
will call spice_usb_device_manager_start_event_listening() in
spice_usbredir_channel_open_device(). The matching
spice_usb_device_manager_stop_event_listening() call is supposed to
happen in spice_usbredir_channel_disconnect_device(), however by the
time it's called in the scenario described above, the session associated
with the channel will already have been set to NULL in
spice_session_channel_destroy().
The session is only needed in order to get the SpiceUsbDeviceManager
instance we need to call spice_usb_device_manager_stop_event_listening()
on. This patch stores it in SpiceChannelUsbredir instead, this way we
don't need SpiceChannel::session to be non-NULL during device
disconnection.
This should fix the issues described in
https://bugzilla.redhat.com/show_bug.cgi?id=1217202
(virt-manager) and most
likely https://bugzilla.redhat.com/show_bug.cgi?id=1337007 (gnome-boxes)
as well.
diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index df18be2..6595508 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -79,6 +79,7 @@ struct _SpiceUsbredirChannelPrivate {
SpiceUsbAclHelper *acl_helper;
#endif
GMutex device_connect_mutex;
+ SpiceUsbDeviceManager *usb_device_manager;
};
static void channel_set_handlers(SpiceChannelClass *klass);
@@ -188,6 +189,11 @@ 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)
@@ -275,6 +281,7 @@ static gboolean spice_usbredir_channel_open_device(
SpiceUsbredirChannel *channel, GError **err)
{
SpiceUsbredirChannelPrivate *priv = channel->priv;
+ SpiceSession *session;
libusb_device_handle *handle = NULL;
int rc, status;
@@ -300,10 +307,9 @@ static gboolean spice_usbredir_channel_open_device(
return FALSE;
}
- if (!spice_usb_device_manager_start_event_listening(
- spice_usb_device_manager_get(
- spice_channel_get_session(SPICE_CHANNEL(channel)), NULL),
- err)) {
+ session = spice_channel_get_session(SPICE_CHANNEL(channel));
+ priv->usb_device_manager = g_object_ref(spice_usb_device_manager_get(session, NULL));
+ if (!spice_usb_device_manager_start_event_listening(priv->usb_device_manager, err)) {
usbredirhost_set_device(priv->host, NULL);
return FALSE;
}
@@ -481,12 +487,10 @@ void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel)
* usbredirhost_set_device NULL will interrupt the
* libusb_handle_events call in the thread.
*/
- {
- SpiceSession *session = spice_channel_get_session(SPICE_CHANNEL(channel));
- if (session != NULL)
- spice_usb_device_manager_stop_event_listening(
- spice_usb_device_manager_get(session, NULL));
- }
+ 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 */
usbredirhost_set_device(priv->host, NULL);
g_clear_pointer(&priv->device, libusb_unref_device);
commit 76ad9f2682b37746afe9834705a2799ad3d7ce2e
Author: Christophe Fergeau <cfergeau at redhat.com>
Date: Thu Jun 30 15:21:15 2016 +0200
usb: Update outdated GSimpleAsyncResult comment
It's still true after the switch to GTask.
diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index b22cec5..df18be2 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -205,7 +205,7 @@ static void spice_usbredir_channel_dispose(GObject *obj)
* Now the last one may seem like an issue, since what will happen if
* spice_usbredir_channel_open_acl_cb will run after finalization?
*
- * This will never happens since the GSimpleAsyncResult created before we
+ * This will never happens since the GTask created before we
* get into the STATE_WAITING_FOR_ACL_HELPER takes a reference to its
* source object, which is our SpiceUsbredirChannel object, so
* the finalize won't hapen until spice_usbredir_channel_open_acl_cb runs,
commit eaededfa67fe6363c26d2b14e604ad17eeb4d4df
Author: Christophe Fergeau <cfergeau at redhat.com>
Date: Wed Jun 29 16:48:18 2016 +0200
usbredir: Use atomic for UsbDeviceManager::event_thread_run
This variable is accessed from 2 different threads (main thread and USB
event thread), so some care must be taken to read/write it.
diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index 1fc8fc1..808ec6c 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -112,7 +112,7 @@ struct _SpiceUsbDeviceManagerPrivate {
libusb_context *context;
int event_listeners;
GThread *event_thread;
- gboolean event_thread_run;
+ gint event_thread_run;
struct usbredirfilter_rule *auto_conn_filter_rules;
struct usbredirfilter_rule *redirect_on_connect_rules;
int auto_conn_filter_rules_count;
@@ -380,7 +380,7 @@ static void spice_usb_device_manager_dispose(GObject *gobject)
priv->hp_handle = 0;
}
#endif
- if (priv->event_thread && !priv->event_thread_run) {
+ if (priv->event_thread && !g_atomic_int_get(&priv->event_thread_run)) {
g_thread_join(priv->event_thread);
priv->event_thread = NULL;
}
@@ -1280,7 +1280,7 @@ static gpointer spice_usb_device_manager_usb_ev_thread(gpointer user_data)
SpiceUsbDeviceManagerPrivate *priv = self->priv;
int rc;
- while (priv->event_thread_run) {
+ while (g_atomic_int_get(&priv->event_thread_run)) {
rc = libusb_handle_events(priv->context);
if (rc && rc != LIBUSB_ERROR_INTERRUPTED) {
const char *desc = spice_usbutil_libusb_strerror(rc);
@@ -1310,7 +1310,7 @@ gboolean spice_usb_device_manager_start_event_listening(
g_thread_join(priv->event_thread);
priv->event_thread = NULL;
}
- priv->event_thread_run = TRUE;
+ 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);
@@ -1326,7 +1326,7 @@ void spice_usb_device_manager_stop_event_listening(
priv->event_listeners--;
if (priv->event_listeners == 0)
- priv->event_thread_run = FALSE;
+ g_atomic_int_set(&priv->event_thread_run, FALSE);
}
static void spice_usb_device_manager_check_redir_on_connect(
More information about the Spice-commits
mailing list