[Spice-commits] 14 commits - configure.ac server/tests

Frediano Ziglio fziglio at kemper.freedesktop.org
Thu Sep 7 07:54:55 UTC 2017


 configure.ac                                    |   17 ----
 server/tests/Makefile.am                        |    9 --
 server/tests/README                             |   20 ++---
 server/tests/basic-event-loop.c                 |   11 ++
 server/tests/basic-event-loop.h                 |    1 
 server/tests/test-display-base.c                |   94 ++++++++++++++----------
 server/tests/test-display-base.h                |    1 
 server/tests/test-empty-success.c               |   27 ++++--
 server/tests/test-fail-on-null-core-interface.c |   22 ++++-
 server/tests/test-two-servers.c                 |    2 
 10 files changed, 118 insertions(+), 86 deletions(-)

New commits:
commit e595b18282338b0a9b9b2f8212e6cebab3628ff9
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun Sep 3 19:20:02 2017 +0100

    tests: Make test-two-servers work
    
    This test runs 2 spice server in one program.
    Use two different tcp port to be able to connect to both servers.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/test-display-base.c b/server/tests/test-display-base.c
index e27dcf95..289aa984 100644
--- a/server/tests/test-display-base.c
+++ b/server/tests/test-display-base.c
@@ -900,9 +900,8 @@ void test_set_command_list(Test *test, Command *commands, int num_commands)
 }
 
 
-Test *test_new(SpiceCoreInterface *core)
+Test* test_new_with_port(SpiceCoreInterface* core, int port)
 {
-    int port = 5912;
     Test *test = spice_new0(Test, 1);
     SpiceServer* server = spice_server_new();
 
@@ -926,6 +925,11 @@ Test *test_new(SpiceCoreInterface *core)
     return test;
 }
 
+Test *test_new(SpiceCoreInterface *core)
+{
+    return test_new_with_port(core, 5912);
+}
+
 void test_destroy(Test *test)
 {
     spice_server_destroy(test->server);
diff --git a/server/tests/test-display-base.h b/server/tests/test-display-base.h
index 1a4f20c5..a80f03e7 100644
--- a/server/tests/test-display-base.h
+++ b/server/tests/test-display-base.h
@@ -134,6 +134,7 @@ void test_set_simple_command_list(Test *test, const int *command, int num_comman
 void test_set_command_list(Test *test, Command *command, int num_commands);
 void test_add_display_interface(Test *test);
 void test_add_agent_interface(SpiceServer *server); // TODO - Test *test
+Test* test_new_with_port(SpiceCoreInterface* core, int port);
 Test* test_new(SpiceCoreInterface* core);
 void test_destroy(Test *test);
 
diff --git a/server/tests/test-two-servers.c b/server/tests/test-two-servers.c
index 40a0e571..92935528 100644
--- a/server/tests/test-two-servers.c
+++ b/server/tests/test-two-servers.c
@@ -42,7 +42,7 @@ int main(void)
 
     core = basic_event_loop_init();
     t1 = test_new(core);
-    t2 = test_new(core);
+    t2 = test_new_with_port(core, 5913);
     //spice_server_set_image_compression(server, SPICE_IMAGE_COMPRESSION_OFF);
     test_add_display_interface(t1);
     test_add_display_interface(t2);
commit 8f872043b3c6623a3da005748035ac854028981c
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun Sep 3 08:43:22 2017 +0100

    tests: Make test-empty-success automated
    
    Add some check that something happened during creation/destruction.
    Set as running on "make check".
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
index c35038a8..01e63626 100644
--- a/server/tests/Makefile.am
+++ b/server/tests/Makefile.am
@@ -56,12 +56,12 @@ check_PROGRAMS =				\
 	test-leaks				\
 	test-vdagent				\
 	test-fail-on-null-core-interface	\
+	test-empty-success			\
 	$(NULL)
 
 noinst_PROGRAMS =				\
 	test-display-no-ssl			\
 	test-display-streaming			\
-	test-empty-success			\
 	test-just-sockets-no-ssl		\
 	test-playback				\
 	test-display-resolution-changes		\
diff --git a/server/tests/test-empty-success.c b/server/tests/test-empty-success.c
index 0df551df..1079040b 100644
--- a/server/tests/test-empty-success.c
+++ b/server/tests/test-empty-success.c
@@ -18,36 +18,40 @@
 #include <config.h>
 #include <stdlib.h>
 #include <string.h>
-
+#include <common/log.h>
 #include <spice.h>
 
-struct SpiceTimer {
-    int a,b;
-};
+static unsigned int watches_created = 0;
+static unsigned int timers_created = 0;
+
+static SpiceWatch *const dummy_watch = (SpiceWatch*)(uintptr_t)0xdeadbeef;
+static SpiceTimer *const dummy_timer = (SpiceTimer*)(uintptr_t)0xbeefdead;
 
 static SpiceTimer*
 timer_add(SPICE_GNUC_UNUSED SpiceTimerFunc func,
           SPICE_GNUC_UNUSED void *opaque)
 {
-    static struct SpiceTimer t = {0,};
-
-    return &t;
+    ++timers_created;
+    return dummy_timer;
 }
 
 static void
 timer_start(SPICE_GNUC_UNUSED SpiceTimer *timer,
             SPICE_GNUC_UNUSED uint32_t ms)
 {
+    spice_assert(timer == dummy_timer);
 }
 
 static void
 timer_cancel(SPICE_GNUC_UNUSED SpiceTimer *timer)
 {
+    spice_assert(timer == dummy_timer);
 }
 
 static void
 timer_remove(SPICE_GNUC_UNUSED SpiceTimer *timer)
 {
+    spice_assert(timer == dummy_timer);
 }
 
 static SpiceWatch *
@@ -56,18 +60,21 @@ watch_add(SPICE_GNUC_UNUSED int fd,
           SPICE_GNUC_UNUSED SpiceWatchFunc func,
           SPICE_GNUC_UNUSED void *opaque)
 {
-    return NULL;
+    ++watches_created;
+    return dummy_watch;
 }
 
 static void
 watch_update_mask(SPICE_GNUC_UNUSED SpiceWatch *watch,
                   SPICE_GNUC_UNUSED int event_mask)
 {
+    spice_assert(watch == dummy_watch);
 }
 
 static void
 watch_remove(SPICE_GNUC_UNUSED SpiceWatch *watch)
 {
+    spice_assert(watch == dummy_watch);
 }
 
 static void
@@ -97,5 +104,9 @@ int main(void)
 
     spice_server_destroy(server);
 
+    // should have created some timers and watch
+    spice_assert(watches_created > 0);
+    spice_assert(timers_created > 0);
+
     return 0;
 }
commit 0905a95d3a04a306cf3a6e41808d35aaacfb2061
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun Sep 3 08:26:47 2017 +0100

    tests: Make test-fail-on-null-core-interface an automated test
    
    Update to Glib style to catch warning.
    Initialize properly the structure (invalid) provided.
    Check results of spice_server_init.
    Remove leaks.
    Enable as check.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
index b64add5f..c35038a8 100644
--- a/server/tests/Makefile.am
+++ b/server/tests/Makefile.am
@@ -55,13 +55,13 @@ check_PROGRAMS =				\
 	test-stat-file				\
 	test-leaks				\
 	test-vdagent				\
+	test-fail-on-null-core-interface	\
 	$(NULL)
 
 noinst_PROGRAMS =				\
 	test-display-no-ssl			\
 	test-display-streaming			\
 	test-empty-success			\
-	test-fail-on-null-core-interface	\
 	test-just-sockets-no-ssl		\
 	test-playback				\
 	test-display-resolution-changes		\
@@ -137,6 +137,7 @@ test_vdagent_CPPFLAGS =			\
 	$(NULL)
 test_codecs_parsing_CPPFLAGS = $(test_vdagent_CPPFLAGS)
 test_qxl_parsing_CPPFLAGS = $(test_vdagent_CPPFLAGS)
+test_fail_on_null_core_interface_CPPFLAGS = $(test_vdagent_CPPFLAGS)
 
 if HAVE_GSTREAMER
 test_gst_SOURCES = test-gst.c \
diff --git a/server/tests/test-fail-on-null-core-interface.c b/server/tests/test-fail-on-null-core-interface.c
index 48e92db0..ca96ea01 100644
--- a/server/tests/test-fail-on-null-core-interface.c
+++ b/server/tests/test-fail-on-null-core-interface.c
@@ -18,13 +18,27 @@
 #include <config.h>
 #include <spice.h>
 
-int main(void)
+#include "test-glib-compat.h"
+
+static SpiceCoreInterface core;
+
+static void empty_core(void)
 {
     SpiceServer *server = spice_server_new();
-    SpiceCoreInterface core;
 
-    spice_server_init(server, &core);
+    g_test_expect_message(G_LOG_DOMAIN, G_LOG_LEVEL_WARNING,
+                          "*bad core interface*");
+    int result = spice_server_init(server, &core);
+    g_assert_cmpint(result, ==, -1);
     spice_server_set_port(server, 5911);
+    spice_server_destroy(server);
+}
+
+int main(int argc, char *argv[])
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/server/empty core", empty_core);
 
-    return 0;
+    return g_test_run();
 }
commit 544f5d7ed3ee023764a7abb72fd2392e668d2959
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun Sep 3 08:09:46 2017 +0100

    tests: Update README
    
    Update tests names.
    Remove tetris comments, never available and not planned.
    Update some notes.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/README b/server/tests/README
index 27248537..bd1de131 100644
--- a/server/tests/README
+++ b/server/tests/README
@@ -1,29 +1,29 @@
 What is here
 ============
 
-This directory will contain a testsuite for the server including tetris drawing.
+This directory will contain a testsuite for the server.
 
-Unfortunately tetris and most of the tests are not here right now. You can however run all the tests and use libtool to debug any of them thus:
+You can run all the tests and use libtool to debug any of them:
 
-libtool --mode=execute gdb test_just_sockets_no_ssl
+libtool --mode=execute gdb test-just-sockets-no-ssl
 
 Overview of tests
 =================
 
-test_just_sockets_no_ssl
+test-just-sockets-no-ssl
  A complete server, only provides the main and inputs channels. Doesn't actually produce anything on the channels. Essentially a test of the regular link code (reds.c), good for multiple connect/disconnect tests.
 
-test_empty_success
+test-empty-success
  tests calling
 
-test_fail_on_null_core_interface
+test-fail-on-null-core-interface
  should abort when run (when spice tries to watch_add)
 
-basic_event_loop.c
- used by test_just_sockets_no_ssl, can be used by other tests. very crude event loop. Should probably use libevent for better tests, but this is self contained.
+basic-event-loop.c
+ event loop to provide core interface.
 
 Automated tests
 ===============
 
-test_display_streaming.c
- this test can be used to check regressions. For this, Spice needs to be compiled with --enable-automated-tests and test_display_streaming needs to be called passing --automated-tests as parameter
+test-display-streaming.c
+ this test can be used to check regressions. For this, test-display-streaming needs to be called passing --automated-tests as parameter
commit 6517ea5cbb07b1480319b51a834cfbe9cfc71e69
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun Sep 3 08:08:04 2017 +0100

    test-display-base: Always compile with AUTOMATED_TESTS enabled
    
    There's no need to not compile this feature, it just enable
    a parameters which must be passed in order to change test
    behaviour.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/configure.ac b/configure.ac
index 7d8afd57..483dbfdf 100644
--- a/configure.ac
+++ b/configure.ac
@@ -122,12 +122,6 @@ if test "x$have_gstreamer_0_10" = "xyes" || test "x$have_gstreamer_1_0" = "xyes"
     AC_SUBST(ORC_LIBS)
 fi
 
-AC_ARG_ENABLE([automated_tests],
-              AS_HELP_STRING([--enable-automated-tests], [Enable automated tests using spicy-screenshot (part of spice-gtk)]),,
-              [enable_automated_tests="no"])
-AS_IF([test x"$enable_automated_tests" != "xno"], [enable_automated_tests="yes"])
-AM_CONDITIONAL(HAVE_AUTOMATED_TESTS, test "x$enable_automated_tests" != "xno")
-
 dnl Check for the presence of Valgrind and do the plumbing to allow
 dnl the running of "make check-valgrind".
 AX_VALGRIND_DFLT(memcheck, on)
@@ -214,16 +208,6 @@ AC_SUBST(JPEG_LIBS)
 AC_CHECK_LIB(z, deflate, Z_LIBS='-lz', AC_MSG_ERROR([zlib not found]))
 AC_SUBST(Z_LIBS)
 
-if test "x$enable_automated_tests" = "xyes"; then
-    AC_MSG_CHECKING([for spicy-screenshot])
-    spicy-screenshot --help >/dev/null 2>&1
-    if test $? -ne 0 ; then
-        AC_MSG_RESULT([not found])
-        AC_MSG_ERROR([spicy-screenshot was not found, this module is part of spice-gtk and is required to compile this package])
-    fi
-    AC_MSG_RESULT([found])
-fi
-
 
 AC_ARG_ENABLE([manual],
                AS_HELP_STRING([--enable-manual=@<:@auto/yes/no@:>@],
@@ -327,7 +311,6 @@ AC_MSG_NOTICE([
         Smartcard:                ${have_smartcard}
         GStreamer:                ${enable_gstreamer}
         SASL support:             ${have_sasl}
-        Automated tests:          ${enable_automated_tests}
         Manual:                   ${have_asciidoc}
 
         Now type 'make' to build $PACKAGE
diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
index 17414f65..b64add5f 100644
--- a/server/tests/Makefile.am
+++ b/server/tests/Makefile.am
@@ -24,10 +24,6 @@ AM_CPPFLAGS =					\
 	$(WARN_CFLAGS)				\
 	$(NULL)
 
-if HAVE_AUTOMATED_TESTS
-AM_CPPFLAGS += -DAUTOMATED_TESTS
-endif
-
 noinst_LIBRARIES = libtest.a
 
 libtest_a_SOURCES =				\
diff --git a/server/tests/test-display-base.c b/server/tests/test-display-base.c
index 768b8986..e27dcf95 100644
--- a/server/tests/test-display-base.c
+++ b/server/tests/test-display-base.c
@@ -948,11 +948,7 @@ static void init_automated(void)
 static __attribute__((noreturn))
 void usage(const char *argv0, const int exitcode)
 {
-#ifdef AUTOMATED_TESTS
     const char *autoopt=" [--automated-tests]";
-#else
-    const char *autoopt="";
-#endif
 
     printf("usage: %s%s\n", argv0, autoopt);
     exit(exitcode);
@@ -961,9 +957,7 @@ void usage(const char *argv0, const int exitcode)
 void spice_test_config_parse_args(int argc, char **argv)
 {
     struct option options[] = {
-#ifdef AUTOMATED_TESTS
         {"automated-tests", no_argument, &has_automated_tests, 1},
-#endif
         {NULL, 0, NULL, 0},
     };
     int option_index;
commit 06380a737bff1ae957cd5ed4263082752fdac01d
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat Sep 2 08:01:45 2017 +0100

    tests: Allow to quit the message loop
    
    This allows to end the loop to end some tests.
    Currently different tests enter the loop but never exit from
    them.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/basic-event-loop.c b/server/tests/basic-event-loop.c
index 4cc797bf..46cf3877 100644
--- a/server/tests/basic-event-loop.c
+++ b/server/tests/basic-event-loop.c
@@ -38,6 +38,7 @@ int debug = 0;
 
 static SpiceCoreInterfaceInternal base_core_interface;
 static GMainContext *main_context = NULL;
+static GMainLoop *loop = NULL;
 
 GMainContext *basic_event_loop_get_context(void)
 {
@@ -52,10 +53,18 @@ static void event_loop_channel_event(int event, SpiceChannelEventInfo *info)
 
 void basic_event_loop_mainloop(void)
 {
-    GMainLoop *loop = g_main_loop_new(main_context, FALSE);
+    loop = g_main_loop_new(main_context, FALSE);
 
     g_main_loop_run(loop);
     g_main_loop_unref(loop);
+    loop = NULL;
+}
+
+void basic_event_loop_quit(void)
+{
+    if (loop) {
+        g_main_loop_quit(loop);
+    }
 }
 
 static void ignore_sigpipe(void)
diff --git a/server/tests/basic-event-loop.h b/server/tests/basic-event-loop.h
index 593532b6..097f4433 100644
--- a/server/tests/basic-event-loop.h
+++ b/server/tests/basic-event-loop.h
@@ -25,5 +25,6 @@ GMainContext *basic_event_loop_get_context(void);
 SpiceCoreInterface *basic_event_loop_init(void);
 void basic_event_loop_destroy(void);
 void basic_event_loop_mainloop(void);
+void basic_event_loop_quit(void);
 
 #endif // __BASIC_EVENT_LOOP_H__
commit eb70454de30765e267562cc51f4f9b38d9ef498f
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun Sep 3 04:54:43 2017 +0100

    test-display-base: Do not assume LP64 architecture on 64 bit
    
    In C the sizeof(long) can be different than sizeof(void*),
    use proper type.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/test-display-base.c b/server/tests/test-display-base.c
index ee3cfc75..768b8986 100644
--- a/server/tests/test-display-base.c
+++ b/server/tests/test-display-base.c
@@ -666,7 +666,7 @@ static void do_wakeup(void *opaque)
 static void release_resource(SPICE_GNUC_UNUSED QXLInstance *qin,
                              struct QXLReleaseInfoExt release_info)
 {
-    QXLCommandExt *ext = (QXLCommandExt*)(unsigned long)release_info.info->id;
+    QXLCommandExt *ext = (QXLCommandExt*)(uintptr_t)release_info.info->id;
     //printf("%s\n", __func__);
     spice_assert(release_info.group_id == MEM_SLOT_GROUP_ID);
     switch (ext->cmd.type) {
@@ -677,7 +677,7 @@ static void release_resource(SPICE_GNUC_UNUSED QXLInstance *qin,
             free(ext);
             break;
         case QXL_CMD_CURSOR: {
-            QXLCursorCmd *cmd = (QXLCursorCmd *)(unsigned long)ext->cmd.data;
+            QXLCursorCmd *cmd = (QXLCursorCmd *)(uintptr_t)ext->cmd.data;
             if (cmd->type == QXL_CURSOR_SET || cmd->type == QXL_CURSOR_MOVE) {
                 free(cmd);
             }
@@ -733,14 +733,14 @@ static int get_cursor_command(QXLInstance *qin, struct QXLCommandExt *ext)
     cmd = spice_new0(QXLCommandExt, 1);
     cursor_cmd = spice_new0(QXLCursorCmd, 1);
 
-    cursor_cmd->release_info.id = (unsigned long)cmd;
+    cursor_cmd->release_info.id = (uintptr_t)cmd;
 
     if (set) {
         cursor_cmd->type = QXL_CURSOR_SET;
         cursor_cmd->u.set.position.x = 0;
         cursor_cmd->u.set.position.y = 0;
         cursor_cmd->u.set.visible = TRUE;
-        cursor_cmd->u.set.shape = (unsigned long)&cursor;
+        cursor_cmd->u.set.shape = (uintptr_t)&cursor;
         // Only a white rect (32x32) as cursor
         memset(cursor.data, 255, sizeof(cursor.data));
         set = 0;
@@ -750,7 +750,7 @@ static int get_cursor_command(QXLInstance *qin, struct QXLCommandExt *ext)
         cursor_cmd->u.position.y = y++ % test->primary_height;
     }
 
-    cmd->cmd.data = (unsigned long)cursor_cmd;
+    cmd->cmd.data = (uintptr_t)cursor_cmd;
     cmd->cmd.type = QXL_CMD_CURSOR;
     cmd->group_id = MEM_SLOT_GROUP_ID;
     cmd->flags    = 0;
commit 6745d048c548747a22719678544d18d73bb7449a
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun Sep 3 04:52:30 2017 +0100

    test-display-base: Use spice allocation helpers
    
    Do not use calloc and malloc directly without checking
    the result. Use instead spice functions to get a nice
    error in case of allocation failures.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/test-display-base.c b/server/tests/test-display-base.c
index 80b45a79..ee3cfc75 100644
--- a/server/tests/test-display-base.c
+++ b/server/tests/test-display-base.c
@@ -185,7 +185,7 @@ test_spice_create_update_from_bitmap(uint32_t surface_id,
     bh = bbox.bottom - bbox.top;
     bw = bbox.right - bbox.left;
 
-    update   = calloc(sizeof(*update), 1);
+    update   = spice_new0(SimpleSpiceUpdate, 1);
     update->bitmap = bitmap;
     drawable = &update->drawable;
     image    = &update->image;
@@ -198,7 +198,7 @@ test_spice_create_update_from_bitmap(uint32_t surface_id,
     } else {
         QXLClipRects *cmd_clip;
 
-        cmd_clip = calloc(sizeof(QXLClipRects) + num_clip_rects*sizeof(QXLRect), 1);
+        cmd_clip = spice_malloc0(sizeof(QXLClipRects) + num_clip_rects*sizeof(QXLRect));
         cmd_clip->num_rects = num_clip_rects;
         cmd_clip->chunk.data_size = num_clip_rects*sizeof(QXLRect);
         cmd_clip->chunk.prev_chunk = cmd_clip->chunk.next_chunk = 0;
@@ -247,7 +247,7 @@ static SimpleSpiceUpdate *test_spice_create_update_solid(uint32_t surface_id, QX
     bw = bbox.right - bbox.left;
     bh = bbox.bottom - bbox.top;
 
-    bitmap = malloc(bw * bh * 4);
+    bitmap = spice_malloc(bw * bh * 4);
     dst = (uint32_t *)bitmap;
 
     for (i = 0 ; i < bh * bw ; ++i, ++dst) {
@@ -282,7 +282,7 @@ static SimpleSpiceUpdate *test_spice_create_update_draw(Test *test, uint32_t sur
     bw       = test->primary_width/SINGLE_PART;
     bh       = 48;
 
-    bitmap = dst = malloc(bw * bh * 4);
+    bitmap = dst = spice_malloc(bw * bh * 4);
     //printf("allocated %p\n", dst);
 
     for (i = 0 ; i < bh * bw ; ++i, dst+=4) {
@@ -307,7 +307,7 @@ static SimpleSpiceUpdate *test_spice_create_update_copy_bits(Test *test, uint32_
         .top = 0,
     };
 
-    update   = calloc(sizeof(*update), 1);
+    update   = spice_new0(SimpleSpiceUpdate, 1);
     drawable = &update->drawable;
 
     bw       = test->primary_width/SINGLE_PART;
@@ -352,7 +352,7 @@ static int format_to_bpp(int format)
 
 static SimpleSurfaceCmd *create_surface(int surface_id, int format, int width, int height, uint8_t *data)
 {
-    SimpleSurfaceCmd *simple_cmd = calloc(sizeof(SimpleSurfaceCmd), 1);
+    SimpleSurfaceCmd *simple_cmd = spice_new0(SimpleSurfaceCmd, 1);
     QXLSurfaceCmd *surface_cmd = &simple_cmd->surface_cmd;
     int bpp = format_to_bpp(format);
 
@@ -371,7 +371,7 @@ static SimpleSurfaceCmd *create_surface(int surface_id, int format, int width, i
 
 static SimpleSurfaceCmd *destroy_surface(int surface_id)
 {
-    SimpleSurfaceCmd *simple_cmd = calloc(sizeof(SimpleSurfaceCmd), 1);
+    SimpleSurfaceCmd *simple_cmd = spice_new0(SimpleSurfaceCmd, 1);
     QXLSurfaceCmd *surface_cmd = &simple_cmd->surface_cmd;
 
     set_cmd(&simple_cmd->ext, QXL_CMD_SURFACE, (intptr_t)surface_cmd);
@@ -730,8 +730,8 @@ static int get_cursor_command(QXLInstance *qin, struct QXLCommandExt *ext)
     }
 
     test->cursor_notify--;
-    cmd = calloc(sizeof(QXLCommandExt), 1);
-    cursor_cmd = calloc(sizeof(QXLCursorCmd), 1);
+    cmd = spice_new0(QXLCommandExt, 1);
+    cursor_cmd = spice_new0(QXLCursorCmd, 1);
 
     cursor_cmd->release_info.id = (unsigned long)cmd;
 
@@ -886,8 +886,7 @@ void test_set_simple_command_list(Test *test, const int *simple_commands, int nu
     int i;
 
     free(test->commands);
-    test->commands = malloc(sizeof(*test->commands) * num_commands);
-    memset(test->commands, 0, sizeof(*test->commands) * num_commands);
+    test->commands = spice_new0(Command, num_commands);
     test->num_commands = num_commands;
     for (i = 0 ; i < num_commands; ++i) {
         test->commands[i].command = simple_commands[i];
commit 416c58c3481652b809ceef3b6b7631a353857933
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun Sep 3 04:51:15 2017 +0100

    test-display-base: Avoid cursor move command leaks
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/test-display-base.c b/server/tests/test-display-base.c
index 3f49727e..80b45a79 100644
--- a/server/tests/test-display-base.c
+++ b/server/tests/test-display-base.c
@@ -678,7 +678,7 @@ static void release_resource(SPICE_GNUC_UNUSED QXLInstance *qin,
             break;
         case QXL_CMD_CURSOR: {
             QXLCursorCmd *cmd = (QXLCursorCmd *)(unsigned long)ext->cmd.data;
-            if (cmd->type == QXL_CURSOR_SET) {
+            if (cmd->type == QXL_CURSOR_SET || cmd->type == QXL_CURSOR_MOVE) {
                 free(cmd);
             }
             free(ext);
commit 230dcf82c38148d4451b7952bef80ebe10ef4c4e
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat Sep 2 12:57:42 2017 +0100

    test-display-base: Avoid global buffer overflow
    
    For some reasons (documented in cursor_init) the function
    uses 128 extra bytes of data causing a reading buffer overflow.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/test-display-base.c b/server/tests/test-display-base.c
index abb7d813..3f49727e 100644
--- a/server/tests/test-display-base.c
+++ b/server/tests/test-display-base.c
@@ -694,7 +694,7 @@ static void release_resource(SPICE_GNUC_UNUSED QXLInstance *qin,
 
 static struct {
     QXLCursor cursor;
-    uint8_t data[CURSOR_WIDTH * CURSOR_HEIGHT * 4]; // 32bit per pixel
+    uint8_t data[CURSOR_WIDTH * CURSOR_HEIGHT * 4 + 128]; // 32bit per pixel
 } cursor;
 
 static void cursor_init(void)
commit 8bf7c7803dd9edc9a403f3c58116b54051eb50ce
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat Sep 2 12:56:04 2017 +0100

    test-display-base: Protect the wake up timer start with a mutex
    
    Timers in spice server are supposed to be used in a single thread
    context. To avoid problems, protect the usage from multiple
    thread with a mutex.
    This sometimes caused a crash.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/test-display-base.c b/server/tests/test-display-base.c
index c35eec1d..abb7d813 100644
--- a/server/tests/test-display-base.c
+++ b/server/tests/test-display-base.c
@@ -84,6 +84,10 @@ static int control = 3; //used to know when we can take a screenshot
 static int rects = 16; //number of rects that will be draw
 static int has_automated_tests = 0; //automated test flag
 
+// SPICE implementation is not designed to have a timer shared
+// between multiple threads so use a mutex
+static pthread_mutex_t timer_mutex = PTHREAD_MUTEX_INITIALIZER;
+
 __attribute__((noreturn))
 static void sigchld_handler(SPICE_GNUC_UNUSED int signal_num) // wait for the child process and exit
 {
@@ -637,7 +641,9 @@ static int req_cmd_notification(QXLInstance *qin)
 {
     Test *test = SPICE_CONTAINEROF(qin, Test, qxl_instance);
 
+    pthread_mutex_lock(&timer_mutex);
     test->core->timer_start(test->wakeup_timer, test->wakeup_ms);
+    pthread_mutex_unlock(&timer_mutex);
     return TRUE;
 }
 
@@ -651,7 +657,9 @@ static void do_wakeup(void *opaque)
         produce_command(test);
     }
 
+    pthread_mutex_lock(&timer_mutex);
     test->core->timer_start(test->wakeup_timer, test->wakeup_ms);
+    pthread_mutex_unlock(&timer_mutex);
     spice_qxl_wakeup(&test->qxl_instance);
 }
 
commit 6daf2d115b6e855d15f75a0fbb6eb0910d6d2519
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat Sep 2 12:54:28 2017 +0100

    test-display-base: Avoid usage after free when the wakeup timer is freed
    
    The wakeup timer is used by the worker thread and by the
    main thread.
    Destroying the object before destroying the worker thread
    can lead to use after free.
    Destroying the worker thread first makes sure we don't race.
    This is detected easily when compiling the test with address sanitizer.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/test-display-base.c b/server/tests/test-display-base.c
index 14311dbc..c35eec1d 100644
--- a/server/tests/test-display-base.c
+++ b/server/tests/test-display-base.c
@@ -921,8 +921,10 @@ Test *test_new(SpiceCoreInterface *core)
 
 void test_destroy(Test *test)
 {
-    test->core->timer_remove(test->wakeup_timer);
     spice_server_destroy(test->server);
+    // this timer is used by spice server so
+    // avoid to free it while is running
+    test->core->timer_remove(test->wakeup_timer);
     free(test->commands);
     free(test);
 }
commit 574574e425b6dbf1915347572159c7d021c26749
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat Sep 2 12:52:21 2017 +0100

    test-display-base: Protect command ring with a mutex
    
    The ring is accessed by multiple thread.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/test-display-base.c b/server/tests/test-display-base.c
index 69e0b8d2..14311dbc 100644
--- a/server/tests/test-display-base.c
+++ b/server/tests/test-display-base.c
@@ -26,6 +26,7 @@
 #include <sys/select.h>
 #include <sys/types.h>
 #include <getopt.h>
+#include <pthread.h>
 
 #include "spice.h"
 #include <spice/qxl_dev.h>
@@ -467,37 +468,45 @@ static void get_init_info(SPICE_GNUC_UNUSED QXLInstance *qin,
 static unsigned int commands_end = 0;
 static unsigned int commands_start = 0;
 static struct QXLCommandExt* commands[1024];
+static pthread_mutex_t command_mutex = PTHREAD_MUTEX_INITIALIZER;
 
 #define COMMANDS_SIZE G_N_ELEMENTS(commands)
 
 static void push_command(QXLCommandExt *ext)
 {
-    spice_assert(commands_end - commands_start < (int) COMMANDS_SIZE);
+    pthread_mutex_lock(&command_mutex);
+    spice_assert(commands_end - commands_start < COMMANDS_SIZE);
     commands[commands_end % COMMANDS_SIZE] = ext;
     commands_end++;
+    pthread_mutex_unlock(&command_mutex);
 }
 
-static struct QXLCommandExt *get_simple_command(void)
+static int get_num_commands(void)
 {
-    struct QXLCommandExt *ret = commands[commands_start % COMMANDS_SIZE];
-    spice_assert(commands_start < commands_end);
-    commands_start++;
-    return ret;
+    return commands_end - commands_start;
 }
 
-static int get_num_commands(void)
+static struct QXLCommandExt *get_simple_command(void)
 {
-    return commands_end - commands_start;
+    pthread_mutex_lock(&command_mutex);
+    struct QXLCommandExt *ret = NULL;
+    if (get_num_commands() > 0) {
+        ret = commands[commands_start % COMMANDS_SIZE];
+        commands_start++;
+    }
+    pthread_mutex_unlock(&command_mutex);
+    return ret;
 }
 
 // called from spice_server thread (i.e. red_worker thread)
 static int get_command(SPICE_GNUC_UNUSED QXLInstance *qin,
                        struct QXLCommandExt *ext)
 {
-    if (get_num_commands() == 0) {
+    struct QXLCommandExt *cmd = get_simple_command();
+    if (!cmd) {
         return FALSE;
     }
-    *ext = *get_simple_command();
+    *ext = *cmd;
     return TRUE;
 }
 
commit f73fbdcae527ef2a98e2b3e3800b242cd4f93a54
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat Sep 2 12:51:12 2017 +0100

    test-display-base: Use unsigned numbers for command ring indexes
    
    As the indexes are used to compute the index inside an array
    using modulo operation when a signed value overflows, the
    modulo becames negative, causing a buffer underflow.
    Unlikely to happens (take lot of time) but is safer that way.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/tests/test-display-base.c b/server/tests/test-display-base.c
index 26fe7c8a..69e0b8d2 100644
--- a/server/tests/test-display-base.c
+++ b/server/tests/test-display-base.c
@@ -464,8 +464,8 @@ static void get_init_info(SPICE_GNUC_UNUSED QXLInstance *qin,
 // which cannot be done from red_worker context (not via dispatcher,
 // since you get a deadlock, and it isn't designed to be done
 // any other way, so no point testing that).
-static int commands_end = 0;
-static int commands_start = 0;
+static unsigned int commands_end = 0;
+static unsigned int commands_start = 0;
 static struct QXLCommandExt* commands[1024];
 
 #define COMMANDS_SIZE G_N_ELEMENTS(commands)


More information about the Spice-commits mailing list