[Spice-devel] [PATCH 3/5] Make start redirection asynchronous

Kirill Moizik kirill at daynix.com
Mon Jul 6 05:10:50 PDT 2015


On Fri, Jul 3, 2015 at 3:58 PM, Christophe Fergeau <cfergeau at redhat.com>
wrote:

> On Thu, Jul 02, 2015 at 04:41:33PM +0300, Kirill Moizik wrote:
> > 1)grey out usbredirection widget
> > 2)start async redirection
> > 3)on finish callback update state, update list of devices (we need to do
> it since we cant query device list while redirecting, so we could  miss
> device changes)
> > 4) ungrey widget
>
> I'd tend to move the UI updates in a separate commit
>
> >
> > Signed-off-by: Kirill Moizik <kirill at daynix.com>
> > ---
> >  src/channel-usbredir.c  | 32 ++++++++++++++++++++---------
> >  src/usb-device-widget.c | 54
> ++++++++++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 71 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > index 292b82f..97003dc 100644
> > --- a/src/channel-usbredir.c
> > +++ b/src/channel-usbredir.c
> > @@ -308,6 +308,23 @@ static void spice_usbredir_channel_open_acl_cb(
> >  }
> >  #endif
> >
> > +static void
> > +spice_usbredir_channel_open_device_async(GSimpleAsyncResult *simple,
> > +                                         GObject *object,
> > +                                         GCancellable *cancellable)
> > +{
> > +    GError *err = NULL;
>
> This was wrapped in a #ifdef ! USE_POLKIT, any reason for dropping this?
>
> > +    SpiceUsbredirChannel *channel= (SpiceUsbredirChannel *)object;
> > +    SpiceUsbredirChannelPrivate *priv = channel->priv;
> > +    if (!spice_usbredir_channel_open_device(channel, &err)) {
> > +        g_simple_async_result_take_error(simple, err);
> > +        libusb_unref_device(priv->device);
> > +        priv->device = NULL;
> > +        g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
> > +        priv->spice_device = NULL;
> > +     }
> > +}
>
> This is running in thread context, have you checked that this shared
> context (priv, channel) and all these methods are thread-safe/cannot be
> called concurrently to this code?
>
>
Yes, I tested it. I put an effort trying to  find  all relevant code may
run concurrently. This is how i found critical sections guarded with
redirection_lock now. But if you have any specific scenario you want me to
check  or any ideas how to improve thread safety i will be glad to hear..

> +
> >  G_GNUC_INTERNAL
> >  void spice_usbredir_channel_connect_device_async(
> >                                            SpiceUsbredirChannel *channel,
> > @@ -319,9 +336,6 @@ void spice_usbredir_channel_connect_device_async(
> >  {
> >      SpiceUsbredirChannelPrivate *priv = channel->priv;
> >      GSimpleAsyncResult *result;
> > -#if ! USE_POLKIT
> > -    GError *err = NULL;
> > -#endif
> >
> >      g_return_if_fail(SPICE_IS_USBREDIR_CHANNEL(channel));
> >      g_return_if_fail(device != NULL);
> > @@ -362,13 +376,11 @@ void spice_usbredir_channel_connect_device_async(
> >                                    channel);
> >      return;
> >  #else
> > -    if (!spice_usbredir_channel_open_device(channel, &err)) {
> > -        g_simple_async_result_take_error(result, err);
> > -        libusb_unref_device(priv->device);
> > -        priv->device = NULL;
> > -        g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
> > -        priv->spice_device = NULL;
> > -    }
> > +    g_simple_async_result_run_in_thread(result,
> > +
> spice_usbredir_channel_open_device_async,
> > +                                        G_PRIORITY_DEFAULT,
> > +                                        cancellable);
> > +    return;
> >  #endif
> >
> >  done:
> > diff --git a/src/usb-device-widget.c b/src/usb-device-widget.c
> > index 1ec30e3..4c466ca 100644
> > --- a/src/usb-device-widget.c
> > +++ b/src/usb-device-widget.c
> > @@ -25,6 +25,7 @@
> >  #include "spice-client.h"
> >  #include "spice-marshal.h"
> >  #include "usb-device-widget.h"
> > +#include "win-usb-dev.h"
> >
> >  /**
> >   * SECTION:usb-device-widget
> > @@ -52,6 +53,9 @@ static gboolean
> spice_usb_device_widget_update_status(gpointer user_data);
> >  /* ------------------------------------------------------------------ */
> >  /* gobject glue                                                       */
> >
> > +
> > +static void set_sensitive_all(GtkWidget *widget, gpointer user_data);
> > +
> >  #define SPICE_USB_DEVICE_WIDGET_GET_PRIVATE(obj) \
> >      (G_TYPE_INSTANCE_GET_PRIVATE((obj), SPICE_TYPE_USB_DEVICE_WIDGET, \
> >                                   SpiceUsbDeviceWidgetPrivate))
> > @@ -401,6 +405,10 @@ static gboolean
> spice_usb_device_widget_update_status(gpointer user_data)
> >      SpiceUsbDeviceWidgetPrivate *priv = self->priv;
> >
> >      priv->device_count = 0;
> > +
> > +    if (spice_usb_device_manager_get_redirecting(priv->manager)) {
> > +        return FALSE;
> > +    }
> >      gtk_container_foreach(GTK_CONTAINER(self), check_can_redirect,
> self);
> >
> >      if (priv->err_msg) {
> > @@ -425,6 +433,23 @@ typedef struct _connect_cb_data {
> >      SpiceUsbDeviceWidget *self;
> >  } connect_cb_data;
> >
> > +static void set_redirecting(SpiceUsbDeviceWidget *self, gboolean val)
> > +{
> > +    spice_usb_device_manager_set_redirecting(self->priv->manager , val);
> > +    spice_g_udev_set_redirecting(val);
> > +    gboolean sensitive = !val;
> > +    if (val == TRUE) {
> > +        spice_usb_device_widget_show_info_bar(self, _("Redirecting Usb
> Device"),
> > +                                     GTK_MESSAGE_INFO,
> > +                                     GTK_STOCK_DIALOG_INFO);
> > +    } else {
> > +        spice_g_udev_handle_device_change();
> > +        spice_usb_device_widget_hide_info_bar(self);
> > +    }
> > +    gtk_container_foreach(GTK_CONTAINER(self),
> > +                          set_sensitive_all, (gpointer) &sensitive);
> > +}
> > +
> >  static void connect_cb(GObject *gobject, GAsyncResult *res, gpointer
> user_data)
> >  {
> >      SpiceUsbDeviceManager *manager = SPICE_USB_DEVICE_MANAGER(gobject);
> > @@ -435,6 +460,7 @@ static void connect_cb(GObject *gobject,
> GAsyncResult *res, gpointer user_data)
> >      GError *err = NULL;
> >      gchar *desc;
> >
> > +    set_redirecting (self,FALSE);
> >      spice_usb_device_manager_connect_device_finish(manager, res, &err);
> >      if (err) {
> >          device = g_object_get_data(G_OBJECT(data->check), "usb-device");
> > @@ -448,9 +474,9 @@ static void connect_cb(GObject *gobject,
> GAsyncResult *res, gpointer user_data)
> >          g_error_free(err);
> >
> >          gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(data->check),
> FALSE);
> > -        spice_usb_device_widget_update_status(self);
> > -    }
> >
> > +    }
> > +    spice_usb_device_widget_update_status(self);
> >      g_object_unref(data->check);
> >      g_object_unref(data->self);
> >      g_free(data);
> > @@ -463,11 +489,12 @@ static void checkbox_clicked_cb(GtkWidget *check,
> gpointer user_data)
> >      SpiceUsbDevice *device;
> >
> >      device = g_object_get_data(G_OBJECT(check), "usb-device");
> > +    connect_cb_data *data = g_new(connect_cb_data, 1);
> > +    data->check = g_object_ref(check);
> > +    data->self  = g_object_ref(self);
> > +    set_redirecting(self, TRUE);
> >
> >      if (gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(check))) {
> > -        connect_cb_data *data = g_new(connect_cb_data, 1);
> > -        data->check = g_object_ref(check);
> > -        data->self  = g_object_ref(self);
> >          spice_usb_device_manager_connect_device_async(priv->manager,
> >                                                        device,
> >                                                        NULL,
> > @@ -502,6 +529,10 @@ static void device_added_cb(SpiceUsbDeviceManager
> *manager,
> >                                                       device))
> >          gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(check), TRUE);
> >
> > +
> > +    if (spice_usb_device_manager_get_redirecting(priv->manager)) {
> > +        gtk_widget_set_sensitive(check, FALSE);
> > +    }
> >      g_object_set_data_full(
> >              G_OBJECT(check), "usb-device",
> >              g_boxed_copy(spice_usb_device_get_type(), device),
> > @@ -542,6 +573,19 @@ static void set_inactive_by_usb_device(GtkWidget
> *widget, gpointer user_data)
> >      }
> >  }
> >
> > +static void set_sensitive_all(GtkWidget *widget, gpointer user_data)
> > +{
> > +    gboolean sensitive = *((gboolean *)user_data);
> > +    SpiceUsbDevice *device;
> > +    if (GTK_IS_BIN(widget)) {
> > +        GtkWidget *check = gtk_bin_get_child(GTK_BIN(widget));
> > +        device = get_usb_device(widget);
> > +        if (!device)
> > +            return; /* Non device widget, ie the info_bar */
> > +        gtk_widget_set_sensitive(check, sensitive);
> > +    }
> > +}
> > +
> >  static void device_error_cb(SpiceUsbDeviceManager *manager,
> >      SpiceUsbDevice *device, GError *err, gpointer user_data)
> >  {
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150706/66bea5c7/attachment.html>


More information about the Spice-devel mailing list