On Thu, Jun 30, 2016 at 04:54:53PM +0200, Hans de Goede wrote:
> Hi,
>
> The entire series looks good to me now, one remark wrt this patch,
> with that fixed this series is:
>
> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>
>
> On 30-06-16 15:40, Christophe Fergeau wrote:
> > This is a follow-up of the previous commit ('usb-channel: Really stop
> > listening for USB events on disconnection').
> >
> > Since the USB event thread has to be stopped when we destroy the
> > associated SpiceUsbDeviceManager, spice_usb_device_manager_dispose()
> > should force event_thread_run to FALSE even if
> > spice_usb_device_manager_stop_event_listening() was not enough. When
> > this happens, this means that there is a bug in the internal users of
> > spice_usb_device_manager_start_event_listening(), but with this change,
> > we'll at least warn about it, and avoid a thread leak/potential future
> > crash.
> > ---
> > Unchanged since v2 apart from the commit log.
> >
> >
> > src/usb-device-manager.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > index 808ec6c..33818c2 100644
> > --- a/src/usb-device-manager.c
> > +++ b/src/usb-device-manager.c
> > @@ -375,6 +375,15 @@ static void spice_usb_device_manager_dispose(GObject *gobject)
> > #ifdef USE_LIBUSB_HOTPLUG
> > if (priv->hp_handle) {
> > 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);
> > + }
> > /* This also wakes up the libusb_handle_events() in the event_thread */
> > libusb_hotplug_deregister_callback(priv->context, priv->hp_handle);
> > priv->hp_handle = 0;
> >
>
> I would move this to outside the #ifdef USE_LIBUSB_HOTPLUG / if (priv->hp_handle) {}
>
> You will also want the warn_if_reached and g_atomic_int_set(..., FALSE) when
> these are not true.
We need to set event_thread_run to FALSE before calling
libusb_hotplug_deregister_callback() otherwise the thread will not exit
properly libusb_handle_events() will return because
deregister_callback() was called, but event_thread_run is still TRUE, so
the thread does not exit, and we'll be back to waiting on
libusb_handle_events()...
>
> You should also remove the !g_atomic_int_get(&priv->event_thread_run) from the
> if below the #endif since you force that to false now, so that part of the
> if is useless.
However, for the !USE_LIBUSB_HOTPLUG case, we don't need to force set
event_thread_run to FALSE if we remove the event_thread_run test from
the 'if' condition. We can just add a warning there if event_thread_run
was not FALSE. However, if this triggers, the g_thread_join() is likely
to hang as we don't have anything in dispose() which would force
libusb_handle_events() to return.
Proposed change to squash in this patch:
diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index 806af74..aa48a01 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -390,7 +391,8 @@ static void spice_usb_device_manager_dispose(GObject *gobject)
priv->hp_handle = 0;
}
#endif
- if (priv->event_thread && !g_atomic_int_get(&priv->event_thread_run)) {
+ if (priv->event_thread) {
+ g_warn_if_fail(g_atomic_int_get(&priv->event_thread_run) == FALSE);
g_thread_join(priv->event_thread);
priv->event_thread = NULL;
}
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel