[Spice-commits] 7 commits - po/POTFILES.skip src/channel-usbredir.c src/usb-acl-helper.c src/usb-acl-helper.h tests/Makefile.am tests/mock-acl-helper.c tests/usb-acl-helper.c

Jonathon Jongsma jjongsma at kemper.freedesktop.org
Fri Mar 11 16:49:37 UTC 2016


 po/POTFILES.skip        |    1 
 src/channel-usbredir.c  |   15 +--
 src/usb-acl-helper.c    |   34 +++----
 src/usb-acl-helper.h    |   12 +-
 tests/Makefile.am       |   15 ++-
 tests/mock-acl-helper.c |   94 +++++++++++++++++++
 tests/usb-acl-helper.c  |  226 ++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 360 insertions(+), 37 deletions(-)

New commits:
commit 30954552050ff40c9ab24ee6df60fc50f0b1acb8
Author: Jonathon Jongsma <jjongsma at redhat.com>
Date:   Fri Mar 11 10:33:12 2016 -0600

    Update POTFILES.skip
    
    spicy.desktop.in no longer exists.

diff --git a/po/POTFILES.skip b/po/POTFILES.skip
index a7b2365..4da36d0 100644
--- a/po/POTFILES.skip
+++ b/po/POTFILES.skip
@@ -1,4 +1,3 @@
-data/spicy.desktop.in
 spice-common/python_modules/spice_parser.py
 spice-common/spice_codegen.py
 src/glib-compat.c
commit c1683c34638ad1340fb8f6c000a9fc82d3db94a9
Author: Jonathon Jongsma <jjongsma at redhat.com>
Date:   Tue Mar 8 14:55:39 2016 -0600

    Add tests for usb-acl-helper

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 19c02b6..24d45c9 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1,18 +1,21 @@
 NULL =
 
-noinst_PROGRAMS =				\
-	coroutine				\
+TESTS = coroutine				\
 	util					\
 	session					\
+	usb-acl-helper				\
 	$(NULL)
 
 if WITH_PHODAV
-noinst_PROGRAMS += pipe
+TESTS += pipe
 endif
 
-TESTS = $(noinst_PROGRAMS)
+noinst_PROGRAMS = $(TESTS) \
+		  mock-acl-helper \
+		  $(NULL)
 
 AM_CPPFLAGS =					\
+	$(COMMON_CFLAGS)			\
 	$(GIO_CFLAGS)				\
 	-I$(top_srcdir)/src			\
 	-I$(top_builddir)/src			\
@@ -29,6 +32,8 @@ util_SOURCES = util.c
 coroutine_SOURCES = coroutine.c
 session_SOURCES = session.c
 pipe_SOURCES = pipe.c
-
+usb_acl_helper_SOURCES = usb-acl-helper.c
+usb_acl_helper_CFLAGS = -DTESTDIR=\"$(abs_builddir)\"
+mock_acl_helper_SOURCES = mock-acl-helper.c
 
 -include $(top_srcdir)/git.mk
diff --git a/tests/mock-acl-helper.c b/tests/mock-acl-helper.c
new file mode 100644
index 0000000..11268cb
--- /dev/null
+++ b/tests/mock-acl-helper.c
@@ -0,0 +1,94 @@
+/*
+   Copyright (C) 2016 Red Hat, Inc.
+
+   This program is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public License as published
+   by the Free Software Foundation; either version 2 of the License,
+   or (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received a copy of the GNU General Public License along
+   with this program; if not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include "config.h"
+
+#include <stdio.h>
+#include <glib.h>
+#include <gio/gunixinputstream.h>
+
+static int exit_status;
+static int busnum, devnum;
+static char path[PATH_MAX];
+static GMainLoop *loop;
+static GDataInputStream *stdin_stream;
+
+static void cleanup(void)
+{
+    if (loop)
+        g_main_loop_quit(loop);
+}
+
+
+static void stdin_read_complete(GObject *src, GAsyncResult *res, gpointer data)
+{
+    char *s = NULL;
+    const char *response = NULL;
+    GError *err = NULL;
+    gsize len;
+
+    s = g_data_input_stream_read_line_finish(G_DATA_INPUT_STREAM(src), res,
+                                             &len, &err);
+
+    /* exit the program to return an early EOF to the caller */
+    if (g_getenv("TEST_EOF"))
+        goto done;
+
+    /* Don't return any response, but continue running to simulate a
+     * unresponsive binary */
+    if (g_getenv("TEST_NORESPONSE"))
+        return;
+
+    /* specify a particular resonse to be returned to the caller */
+    response = g_getenv("TEST_RESPONSE");
+    if (!response)
+        response = "SUCCESS";
+
+    fprintf(stdout, "%s\n", response);
+    fflush(stdout);
+
+done:
+    g_clear_error(&err);
+    g_free(s);
+    cleanup();
+}
+
+int main(void)
+{
+    GInputStream *stdin_unix_stream;
+
+#if !GLIB_CHECK_VERSION(2,36,0)
+    g_type_init();
+#endif
+
+    loop = g_main_loop_new(NULL, FALSE);
+
+    stdin_unix_stream = g_unix_input_stream_new(STDIN_FILENO, 0);
+    stdin_stream = g_data_input_stream_new(stdin_unix_stream);
+    g_data_input_stream_set_newline_type(stdin_stream,
+                                         G_DATA_STREAM_NEWLINE_TYPE_LF);
+    g_clear_object(&stdin_unix_stream);
+    g_data_input_stream_read_line_async(stdin_stream, G_PRIORITY_DEFAULT, NULL,
+                                        stdin_read_complete, NULL);
+
+    g_main_loop_run(loop);
+
+    g_object_unref(stdin_stream);
+    g_main_loop_unref(loop);
+
+    return exit_status;
+}
diff --git a/tests/usb-acl-helper.c b/tests/usb-acl-helper.c
new file mode 100644
index 0000000..41dda5d
--- /dev/null
+++ b/tests/usb-acl-helper.c
@@ -0,0 +1,226 @@
+/*
+   Copyright (C) 2016 Red Hat, Inc.
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <glib.h>
+#include "usb-acl-helper.h"
+
+typedef struct {
+    SpiceUsbAclHelper *acl_helper;
+    GCancellable *cancellable;
+    GMainLoop *loop;
+    guint timeout_source;
+} Fixture;
+
+gboolean abort_test(gpointer user_data)
+{
+    Fixture *fixture = user_data;
+    g_cancellable_cancel(fixture->cancellable);
+    fixture->timeout_source = 0;
+    return G_SOURCE_REMOVE;
+}
+
+gboolean cancel_test(gpointer user_data)
+{
+    Fixture *fixture = user_data;
+    g_cancellable_cancel(fixture->cancellable);
+    return G_SOURCE_REMOVE;
+}
+
+static void data_setup(Fixture *fixture, gconstpointer user_data)
+{
+    g_setenv("SPICE_USB_ACL_BINARY", TESTDIR"/mock-acl-helper", TRUE);
+    fixture->cancellable = g_cancellable_new();
+    fixture->acl_helper = spice_usb_acl_helper_new();
+    fixture->loop = g_main_loop_new(NULL, FALSE);
+    /* abort test after 2 seconds if it hasn't yet completed */
+    fixture->timeout_source = g_timeout_add_seconds(2, abort_test, fixture);
+}
+
+static void data_teardown(Fixture *fixture, gconstpointer user_data)
+{
+    if (fixture->timeout_source)
+        g_source_remove(fixture->timeout_source);
+    g_object_unref(fixture->cancellable);
+    g_object_unref(fixture->acl_helper);
+    g_main_loop_unref(fixture->loop);
+    g_unsetenv("SPICE_USB_ACL_BINARY");
+}
+
+
+static void success_cb(GObject *source, GAsyncResult *result, gpointer user_data)
+{
+    Fixture *f = user_data;
+    GError *error = NULL;
+    if (!spice_usb_acl_helper_open_acl_finish(SPICE_USB_ACL_HELPER(source), result, &error))
+        g_error("%s", error->message);
+    g_main_loop_quit(f->loop);
+}
+
+static void test_acl_helper_success(Fixture *fixture, gconstpointer user_data)
+{
+    spice_usb_acl_helper_open_acl_async(fixture->acl_helper, 1, 1,
+                                        fixture->cancellable, success_cb, fixture);
+    g_main_loop_run(fixture->loop);
+}
+
+static void spawn_fail_cb(GObject *source, GAsyncResult *result, gpointer user_data)
+{
+    Fixture *f = user_data;
+    GError *error = NULL;
+    gboolean success = spice_usb_acl_helper_open_acl_finish(SPICE_USB_ACL_HELPER(source), result, &error);
+    g_assert(!success);
+    g_assert (error->domain == G_SPAWN_ERROR);
+    g_clear_error(&error);
+    g_main_loop_quit(f->loop);
+}
+
+static void test_acl_helper_spawn_fail(Fixture *fixture, gconstpointer user_data)
+{
+    g_setenv("SPICE_USB_ACL_BINARY", "does-not-exist", TRUE);
+    spice_usb_acl_helper_open_acl_async(fixture->acl_helper, 1, 1,
+                                        fixture->cancellable, spawn_fail_cb,
+                                        fixture);
+    g_main_loop_run(fixture->loop);
+}
+
+static void early_eof_cb(GObject *source, GAsyncResult *result, gpointer user_data)
+{
+    Fixture *f = user_data;
+    GError *error = NULL;
+    gboolean success = spice_usb_acl_helper_open_acl_finish(SPICE_USB_ACL_HELPER(source), result, &error);
+    g_assert(!success);
+    g_assert(error->domain == SPICE_CLIENT_ERROR);
+    g_assert(error->code == SPICE_CLIENT_ERROR_FAILED);
+    g_clear_error(&error);
+    g_main_loop_quit(f->loop);
+}
+
+/* helper sends EOF before sending a response */
+static void test_acl_helper_early_eof(Fixture *fixture, gconstpointer user_data)
+{
+    g_setenv("TEST_EOF", "1", TRUE);
+    spice_usb_acl_helper_open_acl_async(fixture->acl_helper, 1, 1,
+                                        fixture->cancellable, early_eof_cb, fixture);
+    g_main_loop_run(fixture->loop);
+    g_unsetenv("TEST_EOF");
+}
+
+static void helper_canceled_cb(GObject *source, GAsyncResult *result, gpointer user_data)
+{
+    Fixture *f = user_data;
+    GError *error = NULL;
+    gboolean success = spice_usb_acl_helper_open_acl_finish(SPICE_USB_ACL_HELPER(source), result, &error);
+    g_assert(!success);
+    g_assert(error->domain == G_IO_ERROR);
+    g_assert(error->code == G_IO_ERROR_CANCELLED);
+    g_clear_error(&error);
+    g_main_loop_quit(f->loop);
+}
+
+static void test_acl_helper_helper_canceled(Fixture *fixture, gconstpointer user_data)
+{
+    g_setenv("TEST_RESPONSE", "CANCELED", TRUE);
+    spice_usb_acl_helper_open_acl_async(fixture->acl_helper, 1, 1,
+                                        fixture->cancellable, helper_canceled_cb, fixture);
+    g_main_loop_run(fixture->loop);
+    g_unsetenv("TEST_RESPONSE");
+}
+
+static void helper_error_response_cb(GObject *source, GAsyncResult *result, gpointer user_data)
+{
+    Fixture *f = user_data;
+    GError *error = NULL;
+    gboolean success = spice_usb_acl_helper_open_acl_finish(SPICE_USB_ACL_HELPER(source), result, &error);
+    g_assert(!success);
+    g_assert(error->domain == SPICE_CLIENT_ERROR);
+    g_assert(error->code == SPICE_CLIENT_ERROR_FAILED);
+    g_clear_error(&error);
+    g_main_loop_quit(f->loop);
+}
+
+static void test_acl_helper_error_response(Fixture *fixture, gconstpointer user_data)
+{
+    g_setenv("TEST_RESPONSE", "Not authorized", TRUE);
+    spice_usb_acl_helper_open_acl_async(fixture->acl_helper, 1, 1,
+                                        fixture->cancellable, helper_error_response_cb, fixture);
+    g_main_loop_run(fixture->loop);
+    g_unsetenv("TEST_RESPONSE");
+}
+
+static void client_canceled_cb(GObject *source, GAsyncResult *result, gpointer user_data)
+{
+    Fixture *f = user_data;
+    GError *error = NULL;
+    gboolean success = spice_usb_acl_helper_open_acl_finish(SPICE_USB_ACL_HELPER(source), result, &error);
+    g_assert(!success);
+    g_assert(error->domain == G_IO_ERROR);
+    g_assert(error->code == G_IO_ERROR_CANCELLED);
+    g_clear_error(&error);
+    g_main_loop_quit(f->loop);
+}
+
+static void test_acl_helper_client_canceled(Fixture *fixture, gconstpointer user_data)
+{
+    /* ensure that the acl-helper does not have respond, so we can cancel the
+     * task before we get a response from the helper binary */
+    g_setenv("TEST_NORESPONSE", "1", TRUE);
+    spice_usb_acl_helper_open_acl_async(fixture->acl_helper, 1, 1,
+                                        fixture->cancellable, client_canceled_cb, fixture);
+    g_idle_add(cancel_test, fixture);
+    g_main_loop_run(fixture->loop);
+    g_unsetenv("TEST_NORESPONSE");
+}
+
+static void test_acl_helper_no_response(Fixture *fixture, gconstpointer user_data)
+{
+    /* ensure that the acl-helper does not have respond, so we can cancel the
+     * task before we get a response from the helper binary */
+    g_setenv("TEST_NORESPONSE", "1", TRUE);
+    spice_usb_acl_helper_open_acl_async(fixture->acl_helper, 1, 1,
+                                        fixture->cancellable, client_canceled_cb, fixture);
+    g_main_loop_run(fixture->loop);
+    g_unsetenv("TEST_NORESPONSE");
+}
+
+int main(int argc, char* argv[])
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add("/usb-acl-helper/success", Fixture, NULL,
+               data_setup, test_acl_helper_success, data_teardown);
+    g_test_add("/usb-acl-helper/spawn-fail", Fixture, NULL,
+               data_setup, test_acl_helper_spawn_fail, data_teardown);
+    g_test_add("/usb-acl-helper/early-eof", Fixture, NULL,
+               data_setup, test_acl_helper_early_eof, data_teardown);
+    g_test_add("/usb-acl-helper/helper-canceled", Fixture, NULL,
+               data_setup, test_acl_helper_helper_canceled, data_teardown);
+    g_test_add("/usb-acl-helper/helper-error", Fixture, NULL,
+               data_setup, test_acl_helper_error_response, data_teardown);
+    g_test_add("/usb-acl-helper/client-canceled", Fixture, NULL,
+               data_setup, test_acl_helper_client_canceled, data_teardown);
+    g_test_add("/usb-acl-helper/no-response", Fixture, NULL,
+               data_setup, test_acl_helper_no_response, data_teardown);
+    /* additional possible test cases:
+     * - unable to set nonblocking flag on io channel?
+     * - unable to write bus number to helper binary
+     * - unable to flush channel
+     * - read_line from helper binary returns something other than G_IO_STATUS_NORMAL
+     */
+
+    return g_test_run ();
+}
+
commit 17b00a13f2c90a1a1149c632b836c92ba5a765b3
Author: Jonathon Jongsma <jjongsma at redhat.com>
Date:   Wed Mar 9 10:50:18 2016 -0600

    usb-acl-helper: add env var for specifying acl helper binary
    
    Setting SPICE_USB_ACL_BINARY allows us to execute a custom mock acl
    helper binary for testing purposes.

diff --git a/src/usb-acl-helper.c b/src/usb-acl-helper.c
index 20400eb..cc5ce9f 100644
--- a/src/usb-acl-helper.c
+++ b/src/usb-acl-helper.c
@@ -198,7 +198,10 @@ void spice_usb_acl_helper_open_acl_async(SpiceUsbAclHelper *self,
     GIOStatus status;
     GPid helper_pid;
     gsize bytes_written;
-    gchar *argv[] = { (char*) ACL_HELPER_PATH"/spice-client-glib-usb-acl-helper", NULL };
+    const gchar *acl_helper = g_getenv("SPICE_USB_ACL_BINARY");
+    if (acl_helper == NULL)
+        acl_helper = ACL_HELPER_PATH"/spice-client-glib-usb-acl-helper";
+    gchar *argv[] = { (char*)acl_helper, NULL };
     gint in, out;
     gchar buf[128];
 
commit 6eb8d0a3350f6e16af3cbb7d9cf12e59e415537b
Author: Jonathon Jongsma <jjongsma at redhat.com>
Date:   Thu Mar 10 11:03:08 2016 -0600

    Rename spice_usb_acl_helper_open_acl() to _open_acl_async()
    
    Follow established practice and make the function's behavior more
    explicit.

diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index dd9557b..b37f464 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -363,12 +363,12 @@ void spice_usbredir_channel_connect_device_async(
     priv->acl_helper = spice_usb_acl_helper_new();
     g_object_set(spice_channel_get_session(SPICE_CHANNEL(channel)),
                  "inhibit-keyboard-grab", TRUE, NULL);
-    spice_usb_acl_helper_open_acl(priv->acl_helper,
-                                  libusb_get_bus_number(device),
-                                  libusb_get_device_address(device),
-                                  cancellable,
-                                  spice_usbredir_channel_open_acl_cb,
-                                  channel);
+    spice_usb_acl_helper_open_acl_async(priv->acl_helper,
+                                        libusb_get_bus_number(device),
+                                        libusb_get_device_address(device),
+                                        cancellable,
+                                        spice_usbredir_channel_open_acl_cb,
+                                        channel);
     return;
 #else
     if (!spice_usbredir_channel_open_device(channel, &err)) {
diff --git a/src/usb-acl-helper.c b/src/usb-acl-helper.c
index 1cc2c4f..20400eb 100644
--- a/src/usb-acl-helper.c
+++ b/src/usb-acl-helper.c
@@ -184,11 +184,11 @@ SpiceUsbAclHelper *spice_usb_acl_helper_new(void)
 }
 
 G_GNUC_INTERNAL
-void spice_usb_acl_helper_open_acl(SpiceUsbAclHelper *self,
-                                   gint busnum, gint devnum,
-                                   GCancellable *cancellable,
-                                   GAsyncReadyCallback callback,
-                                   gpointer user_data)
+void spice_usb_acl_helper_open_acl_async(SpiceUsbAclHelper *self,
+                                         gint busnum, gint devnum,
+                                         GCancellable *cancellable,
+                                         GAsyncReadyCallback callback,
+                                         gpointer user_data)
 {
     g_return_if_fail(SPICE_IS_USB_ACL_HELPER(self));
 
@@ -203,7 +203,7 @@ void spice_usb_acl_helper_open_acl(SpiceUsbAclHelper *self,
     gchar buf[128];
 
     result = g_simple_async_result_new(G_OBJECT(self), callback, user_data,
-                                       spice_usb_acl_helper_open_acl);
+                                       spice_usb_acl_helper_open_acl_async);
 
     if (priv->out_ch) {
         g_simple_async_result_set_error(result,
@@ -273,7 +273,7 @@ gboolean spice_usb_acl_helper_open_acl_finish(
     GSimpleAsyncResult *result = G_SIMPLE_ASYNC_RESULT(res);
 
     g_return_val_if_fail(g_simple_async_result_is_valid(res, G_OBJECT(self),
-                                               spice_usb_acl_helper_open_acl),
+                                               spice_usb_acl_helper_open_acl_async),
                          FALSE);
 
     if (g_simple_async_result_propagate_error(result, err))
diff --git a/src/usb-acl-helper.h b/src/usb-acl-helper.h
index aa5d3d4..e2e27db 100644
--- a/src/usb-acl-helper.h
+++ b/src/usb-acl-helper.h
@@ -57,11 +57,11 @@ GType spice_usb_acl_helper_get_type(void);
 
 SpiceUsbAclHelper *spice_usb_acl_helper_new(void);
 
-void spice_usb_acl_helper_open_acl(SpiceUsbAclHelper *self,
-                                   gint busnum, gint devnum,
-                                   GCancellable *cancellable,
-                                   GAsyncReadyCallback callback,
-                                   gpointer user_data);
+void spice_usb_acl_helper_open_acl_async(SpiceUsbAclHelper *self,
+                                         gint busnum, gint devnum,
+                                         GCancellable *cancellable,
+                                         GAsyncReadyCallback callback,
+                                         gpointer user_data);
 gboolean spice_usb_acl_helper_open_acl_finish(
     SpiceUsbAclHelper *self, GAsyncResult *res, GError **err);
 
commit da1b2ad771e19e1aa52828ac7dde4fffaab6b406
Author: Jonathon Jongsma <jjongsma at redhat.com>
Date:   Thu Mar 10 10:58:50 2016 -0600

    Remove spice_usb_acl_helper_close_acl()
    
    This function is now only called after the open_acl() task completes,
    and in that scenario it basically does the same thing that the
    SpiceUsbAclHelper destructor does, so it's pointless. Remove it.

diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index 38d5aaf..dd9557b 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -305,7 +305,6 @@ static void spice_usbredir_channel_open_acl_cb(
         priv->state  = STATE_DISCONNECTED;
     }
 
-    spice_usb_acl_helper_close_acl(priv->acl_helper);
     g_clear_object(&priv->acl_helper);
     g_object_set(spice_channel_get_session(SPICE_CHANNEL(channel)),
                  "inhibit-keyboard-grab", FALSE, NULL);
diff --git a/src/usb-acl-helper.c b/src/usb-acl-helper.c
index 9a08fea..1cc2c4f 100644
--- a/src/usb-acl-helper.c
+++ b/src/usb-acl-helper.c
@@ -283,22 +283,6 @@ gboolean spice_usb_acl_helper_open_acl_finish(
 }
 
 G_GNUC_INTERNAL
-void spice_usb_acl_helper_close_acl(SpiceUsbAclHelper *self)
-{
-    g_return_if_fail(SPICE_IS_USB_ACL_HELPER(self));
-
-    SpiceUsbAclHelperPrivate *priv = self->priv;
-
-    /* If the acl open has not completed yet report it as cancelled */
-    if (priv->result) {
-        async_result_set_cancelled(priv->result);
-        g_simple_async_result_complete_in_idle(priv->result);
-    }
-
-    spice_usb_acl_helper_cleanup(self);
-}
-
-G_GNUC_INTERNAL
 void spice_usb_acl_helper_cancel(SpiceUsbAclHelper *self)
 {
     g_return_if_fail(SPICE_IS_USB_ACL_HELPER(self));
diff --git a/src/usb-acl-helper.h b/src/usb-acl-helper.h
index d9a9def..aa5d3d4 100644
--- a/src/usb-acl-helper.h
+++ b/src/usb-acl-helper.h
@@ -65,7 +65,6 @@ void spice_usb_acl_helper_open_acl(SpiceUsbAclHelper *self,
 gboolean spice_usb_acl_helper_open_acl_finish(
     SpiceUsbAclHelper *self, GAsyncResult *res, GError **err);
 
-void spice_usb_acl_helper_close_acl(SpiceUsbAclHelper *self);
 void spice_usb_acl_helper_cancel(SpiceUsbAclHelper *self);
 
 G_END_DECLS
commit 9c8f8982c1b51adf98785d05323c6ab32369a682
Author: Jonathon Jongsma <jjongsma at redhat.com>
Date:   Tue Mar 8 14:28:24 2016 -0600

    usb-acl-helper: Avoid deadlock when cancelled
    
    cancelled_cb() (which is triggered when the GCancellable's "cancelled"
    signal is emitted) called spice_usb_acl_helper_close_acl(), which calls
    spice_usb_acl_helper_cleanup(), which in turn calls
    g_cancellable_disconnect(). Calling g_cancellable_disconnect() from
    within a "cancelled" handler results in a dealock, as mentioned in the
    documentation. Instead of closing the acl here, simply cancel the task
    here. The cleanup() call will happen when the SpiceUsbAclHelper object
    is destroyed.

diff --git a/src/usb-acl-helper.c b/src/usb-acl-helper.c
index 4ed57b9..9a08fea 100644
--- a/src/usb-acl-helper.c
+++ b/src/usb-acl-helper.c
@@ -162,7 +162,7 @@ static void cancelled_cb(GCancellable *cancellable, gpointer user_data)
 {
     SpiceUsbAclHelper *self = SPICE_USB_ACL_HELPER(user_data);
 
-    spice_usb_acl_helper_close_acl(self);
+    spice_usb_acl_helper_cancel(self);
 }
 
 static void helper_child_watch_cb(GPid pid, gint status, gpointer user_data)
commit 3e5004dd5758da27b6f7dab3808769303c30044e
Author: Jonathon Jongsma <jjongsma at redhat.com>
Date:   Thu Mar 10 10:54:11 2016 -0600

    Introduce spice_usb_acl_helper_cancel()
    
    This function explicitly cancels a open_acl() task. It is similar to
    close_acl(), but its behavior is more explicit. This function is very
    similar to the existing close_acl() function but it requires that the
    task has not already been completed. Also, although it releases its
    reference on priv->result to avoid circular references, it doesn't call
    cleanup(), preferring instead to wait for the destructor to clean up.
    This makes the close_acl() function essentially pointless and will be
    removed in the following commit.

diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index f16a30f..38d5aaf 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -419,7 +419,7 @@ void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel)
     case STATE_WAITING_FOR_ACL_HELPER:
         priv->state = STATE_DISCONNECTING;
         /* We're still waiting for the acl helper -> cancel it */
-        spice_usb_acl_helper_close_acl(priv->acl_helper);
+        spice_usb_acl_helper_cancel(priv->acl_helper);
         break;
 #endif
     case STATE_CONNECTED:
diff --git a/src/usb-acl-helper.c b/src/usb-acl-helper.c
index 6a49627..4ed57b9 100644
--- a/src/usb-acl-helper.c
+++ b/src/usb-acl-helper.c
@@ -297,3 +297,16 @@ void spice_usb_acl_helper_close_acl(SpiceUsbAclHelper *self)
 
     spice_usb_acl_helper_cleanup(self);
 }
+
+G_GNUC_INTERNAL
+void spice_usb_acl_helper_cancel(SpiceUsbAclHelper *self)
+{
+    g_return_if_fail(SPICE_IS_USB_ACL_HELPER(self));
+
+    SpiceUsbAclHelperPrivate *priv = self->priv;
+    g_return_if_fail(priv->result != NULL);
+
+    async_result_set_cancelled(priv->result);
+    g_simple_async_result_complete_in_idle(priv->result);
+    g_clear_object(&priv->result);
+}
diff --git a/src/usb-acl-helper.h b/src/usb-acl-helper.h
index 2d41b68..d9a9def 100644
--- a/src/usb-acl-helper.h
+++ b/src/usb-acl-helper.h
@@ -66,6 +66,7 @@ gboolean spice_usb_acl_helper_open_acl_finish(
     SpiceUsbAclHelper *self, GAsyncResult *res, GError **err);
 
 void spice_usb_acl_helper_close_acl(SpiceUsbAclHelper *self);
+void spice_usb_acl_helper_cancel(SpiceUsbAclHelper *self);
 
 G_END_DECLS
 


More information about the Spice-commits mailing list