Re: [PATCH v6 10/14] usbredir: Disconnect USB device asynchronously

[Dmitry Fleytman <dmitry@xxxxxxxxxx>]

On 1 Mar 2016, at 21:25 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:

On Sun, 2016-02-28 at 11:54 +0200, Dmitry Fleytman wrote:

From: Kirill Moizik <kmoizik@xxxxxxxxxx>

Signed-off-by: Kirill Moizik <kmoizik@xxxxxxxxxx>
Signed-off-by: Dmitry Fleytman <dfleytma@xxxxxxxxxx>
---
src/channel-usbredir.c | 38 +++++++++++++++++++++++++++++++-------
1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index ebf9fce..3a10823 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -114,20 +114,44 @@ static void
spice_usbredir_channel_init(SpiceUsbredirChannel *channel)
}

#ifdef USE_USBREDIR
+
+static void _channel_reset_cb(GObject *gobject,
+                              GAsyncResult *result,
+                              gpointer user_data)
+{
+    SpiceChannel *spice_channel =  SPICE_CHANNEL(gobject);
+    SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(spice_channel);
+    SpiceUsbredirChannelPrivate *priv = channel->priv;
+    gboolean migrating = GPOINTER_TO_UINT(user_data);
+    GError *err = NULL;
+
+    spice_usbredir_channel_lock(channel);
+
+    usbredirhost_close(priv->host);
+    priv->host = NULL;
+    /* Call set_context to re-create the host */
+    spice_usbredir_channel_set_context(channel, priv->context);
+    SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)
->channel_reset(spice_channel, migrating);
+
+    spice_usbredir_channel_unlock(channel);
+
+    spice_usbredir_channel_disconnect_device_finish(channel, result, &err);
+    g_object_unref(result);
+}
+
static void spice_usbredir_channel_reset(SpiceChannel *c, gboolean migrating)
{
    SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
    SpiceUsbredirChannelPrivate *priv = channel->priv;

    if (priv->host) {
-        if (priv->state == STATE_CONNECTED)
-            spice_usbredir_channel_disconnect_device(channel);
-        usbredirhost_close(priv->host);
-        priv->host = NULL;
-        /* Call set_context to re-create the host */
-        spice_usbredir_channel_set_context(channel, priv->context);
+        if (priv->state == STATE_CONNECTED) {
+            spice_usbredir_channel_disconnect_device_async(channel, NULL,
+                _channel_reset_cb, GUINT_TO_POINTER(migrating));

I don't think I noticed this on the previous review, but it seems there is a
change in behavior introduced here. Previously, if priv->state was not
STATE_CONNECTED, it would omit the call to
spice_usbredir_channel_disconnect_device(), but would proceed to call
usbredirhost_close(), etc. In the new version, these additional calls are done
in the async callback. And the async function is only executed if state is
STATE_CONNECTED. I'm afraid I don't know enough about this code to know whether
that's going to be an issue. Was this change intentional?

You’re right. This change is unintentional, the original behaviour is preserved in the next version.

+        }
+    } else {
+        SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)
->channel_reset(c, migrating);
    }
-    SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)
->channel_reset(c, migrating);
}
#endif

Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>

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