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

[Dmitry Fleytman <dmitry@xxxxxxxxxx>]

Hi Jonathon,

see below.

On 10 Mar 2016, at 22:20 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:

Sorry, I missed a few more things on this patch the last time I reviewed it :/

On Tue, 2016-03-08 at 16:05 +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 | 48 +++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 41 insertions(+), 7 deletions(-)

diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index 7253ce6..ba9e4ab 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -113,20 +113,54 @@ static void
spice_usbredir_channel_init(SpiceUsbredirChannel *channel)
}

#ifdef USE_USBREDIR
+
+static void _channel_reset_finish(SpiceUsbredirChannel *channel)
+{
+    SpiceUsbredirChannelPrivate *priv = channel->priv;
+
+    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_usbredir_channel_unlock(channel);
+}
+
+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);
+    gboolean migrating = GPOINTER_TO_UINT(user_data);
+    GError *err = NULL;
+
+    _channel_reset_finish(channel);
+
+    SPICE_CHANNEL_CLASS(spice_usbredir_channel_parent_class)
->channel_reset(spice_channel, migrating);
+
+    spice_usbredir_channel_disconnect_device_finish(channel, result, &err);

err is leaked if this call is returns a failure.

Right. Fixing it.

+    g_object_unref(result);

result should not be unreffed here. The only reference should be held by the
async operation that invokes this callback. Presumably it will be released after
this function returns and you will get a crash. If there's no crash, there may
be a leak, but it should be fixed elsewhere.

 

I may be missing something, but according to [1] result is unreferenced explicitly in the callback (see baked_cb sample function).

Please advise.

[1] https://developer.gnome.org/gio/stable/GSimpleAsyncResult.html

Thanks,

Dmitry

+}
+
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));
+        } else {
+            _channel_reset_finish(channel);
+        }
+    } 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