Hi, On Mon, Jul 22, 2019 at 06:31:24PM +0300, Yuri Benditovich wrote: > On Mon, Jul 22, 2019 at 6:00 PM Victor Toso <[email protected]> wrote: > > > > Hi, > > > > On Mon, Jul 22, 2019 at 04:34:48PM +0300, Yuri Benditovich wrote: > > > Ping > > > > Sorry, missed it. > > > > Patch seems fine and again, code is getting nicer thanks to > > usb-backend.[ch]. Thanks. Only cosmetic changes for v2. > > > > > On Sun, Jul 14, 2019 at 5:07 PM Yuri Benditovich > > > <[email protected]> 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 as 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 and 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. > > > > Log is fine, I'd remove the indentation space and add empty line > > before 'Current commit', not important. > > > > > > Signed-off-by: Yuri Benditovich <[email protected]> > > > > --- > > > > src/channel-usbredir.c | 28 ------------- > > > > src/usb-backend.c | 48 ++++++++++++++------- > > > > src/usb-backend.h | 2 - > > > > src/usb-device-manager-priv.h | 6 --- > > > > src/usb-device-manager.c | 79 ----------------------------------- > > > > 5 files changed, 33 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 829d81d..37db951 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 = (SpiceUsbBackend *)user_data; > > > > I don't think you need to cast from gpointer. > > > > > > 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_try_new("usb_ev_thread", > > > > + handle_libusb_events, > > > > + be, error); > > > > It was g_thread_new() before, this changed it, why do you think > > it is important? I'd do that in a separate patch with a proper > > rationale for it. > > From the beginning it should be g_thread_try_new but this was > not available in old glib. > g_thread_try_new does not abort the application on error when > g_thread_new does.
Yeah, I'm fine with the change but I'd still do it in a separate
patch titled something along the line "do not abort if can't
create a thread" instead of "move USB events handling to USB
backend"
> > > > + if (!be->event_thread) {
> > > > + g_warning("Error starting event thread");
> > > > + spice_usb_backend_deregister_hotplug(be);
> > > > + return FALSE;
> > > > + }
> > > > return TRUE;
> > > > }
> > > >
> > > > @@ -468,6 +484,8 @@ void spice_usb_backend_delete(SpiceUsbBackend *be)
> > > > {
> > > > g_return_if_fail(be != NULL);
> > > > SPICE_DEBUG("%s >>", __FUNCTION__);
> > > > + /* just to be on the safe side if not deregistered */
> > > > + spice_usb_backend_deregister_hotplug(be);
> >
> > Not sure if I follow the comment, should a warn be added in case
> > spice_usb_backend_delete() is called in some unexpected situation?
Thanks,
> >
> > > > 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 7105ff1..25fac04 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;
> > > > @@ -262,9 +259,6 @@ static gboolean
> > > > spice_usb_device_manager_initable_init(GInitable *initable,
> > > > spice_usb_backend_delete(priv->context);
> > > > 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);
> > > > @@ -286,27 +280,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 */
> > > > @@ -324,7 +299,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);
> > > > }
> > > > @@ -916,59 +890,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
> > > [email protected]
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/spice-devel
