Re: [PATCH v3 05/13] UsbDeviceManager and redirect channel: spawn a different thread for connect device flow

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 03, 2015 at 04:10:45PM +0300, Kirill Moizik wrote:
> From: Kirill Moizik <kmoizik@xxxxxxxxxx>
> 
> ---
>  src/channel-usbredir.c   | 46 +++++++++++++++++++++++---------
>  src/usb-device-manager.c | 69 +++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 90 insertions(+), 25 deletions(-)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 69cb565..7394b81 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -315,6 +315,27 @@ static void spice_usbredir_channel_open_acl_cb(
>  }
>  #endif
>  
> +#ifndef USE_POLKIT
> +static void
> +spice_usbredir_channel_open_device_async(GSimpleAsyncResult *simple,
> +                                         GObject *object,
> +                                         GCancellable *cancellable)

This should be named _open_device_async_cb()

> +{
> +    GError *err = NULL;
> +    SpiceUsbredirChannel *channel= (SpiceUsbredirChannel *)object;

SPICE_USBREDIR_CHANNEL(object) would be better than the direct cast.

> +    SpiceUsbredirChannelPrivate *priv = channel->priv;
> +    g_mutex_lock(priv->flows_mutex);
> +    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;
> +     }
> +    g_mutex_unlock(priv->flows_mutex);
> +}
> +#endif
> +
>  G_GNUC_INTERNAL
>  void spice_usbredir_channel_connect_device_async(
>                                            SpiceUsbredirChannel *channel,
> @@ -324,15 +345,16 @@ void spice_usbredir_channel_connect_device_async(
>                                            GAsyncReadyCallback   callback,
>                                            gpointer              user_data)
>  {
> -    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);
>  
> +    SpiceUsbredirChannelPrivate *priv = channel->priv;
> +    GSimpleAsyncResult *result;
> +#ifndef USE_POLKIT
> +    g_object_ref(channel);
> +#endif

I don't think you need this _ref() here,
g_simple_async_result_new(channel, ...) will take a reference for you
for as long as the GSimpleAsyncResult object is alive.

> +
>      CHANNEL_DEBUG(channel, "connecting usb channel %p", channel);
>  
>      result = g_simple_async_result_new(G_OBJECT(channel), callback, user_data,
> @@ -369,18 +391,18 @@ 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);
> +    g_object_unref(result);
> +    return;
>  #endif
>  
>  done:
>      g_simple_async_result_complete_in_idle(result);
>      g_object_unref(result);
> +    g_object_unref(channel);
>  }
>  
>  G_GNUC_INTERNAL

All the changes below in usb-device-manager.c seems to be more or less
unrelated to the changes above, and aimed at setting 'redirecting' to
TRUE/FALSE when redirection starts/ends, I suspect it would be clearer
to do this in a separate commit.

> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index e58450d..65c6568 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -207,6 +207,11 @@ _spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>                                                 GAsyncReadyCallback callback,
>                                                 gpointer user_data);
>  
> +static
> +void spice_usb_device_manager_connect_device_async_cb(GObject *gobject,
> +                                                      GAsyncResult *channel_res,
> +                                                      gpointer user_data);
> +
>  G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
>                      (GBoxedCopyFunc)spice_usb_device_ref,
>                      (GBoxedFreeFunc)spice_usb_device_unref)
> @@ -1128,8 +1133,16 @@ static void spice_usb_device_manager_channel_connect_cb(
>      }
>      g_simple_async_result_complete(result);
>      g_object_unref(result);
> +    g_object_unref(channel);
>  }
>  
> +typedef struct _connect_cb_data
> +{
> +    SpiceUsbDeviceManager  *self;
> +    GAsyncReadyCallback     callback;
> +    gpointer                user_data;
> +} connect_cb_data;
> +
>  #ifdef G_OS_WIN32
>  
>  typedef struct _UsbInstallCbInfo {
[snip]
> @@ -1603,9 +1625,10 @@ void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>          _spice_usb_device_manager_connect_device_async(self,
>                                                         device,
>                                                         cancellable,
> -                                                       callback,
> -                                                       user_data);
> +                                                       spice_usb_device_manager_connect_device_async_cb,
> +                                                       data);
>      }
> +#endif
>  }
>  
>  gboolean spice_usb_device_manager_connect_device_finish(
> @@ -1623,6 +1646,26 @@ gboolean spice_usb_device_manager_connect_device_finish(
>      return TRUE;
>  }
>  
> +#ifdef USE_USBREDIR
> +static
> +void spice_usb_device_manager_connect_device_async_cb(GObject *gobject,
> +                                                      GAsyncResult *channel_res,
> +                                                      gpointer user_data)
> +{
> +    connect_cb_data    *data    = user_data;
> +    GSimpleAsyncResult *result  = g_simple_async_result_new(G_OBJECT(data->self), data->callback,
> +                                  data->user_data,
> +                                  spice_usb_device_manager_disconnect_device_async);
> +    g_object_set(data->self,"redirecting", FALSE, NULL);
> +#ifndef USE_LIBUSB_HOTPLUG
> +    spice_g_udev_set_redirecting(FALSE);
> +#endif
> +    g_simple_async_result_complete(result);
> +    g_object_unref(result);
> +    g_free(data);
> +}
> +#endif
> +

Can't this be done in spice_usb_device_manager_channel_connect_cb()
rather than introducing an extra async_cb step?

Christophe

Attachment: pgpETQOHwDmhw.pgp
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]