[Spice-devel] [spice v1 1/3] reds: don't replace video_codecs on failure

[Victor Toso]

From: Victor Toso <me at victortoso.com>

We should replace the video_codecs GArray only after the parsing of
input is done, otherwise we might lose previous configuration.

Tests were updated to match this situation. Input that fails to
replace video_codecs are considerer bad.

Signed-off-by: Victor Toso <victortoso at redhat.com>
---
 server/reds.c                      | 17 +++++++++++++----
 server/tests/test-codecs-parsing.c | 32 ++++++++++++++++++++++++++++----
 2 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 6064a6d..3b30928 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3538,6 +3538,7 @@ static const char* parse_video_codecs(const char *codecs, char **encoder,
 static void reds_set_video_codecs(RedsState *reds, const char *codecs)
 {
     char *encoder_name, *codec_name;
+    GArray *video_codecs;
 
     g_return_if_fail(codecs != NULL);
 
@@ -3545,9 +3546,7 @@ static void reds_set_video_codecs(RedsState *reds, const char *codecs)
         codecs = default_video_codecs;
     }
 
-    /* The video_codecs array is immutable */
-    g_array_unref(reds->config->video_codecs);
-    reds->config->video_codecs = g_array_new(FALSE, FALSE, sizeof(RedVideoCodec));
+    video_codecs = g_array_new(FALSE, FALSE, sizeof(RedVideoCodec));
     const char *c = codecs;
     while ( (c = parse_video_codecs(c, &encoder_name, &codec_name)) ) {
         uint32_t encoder_index, codec_index;
@@ -3568,13 +3567,23 @@ static void reds_set_video_codecs(RedsState *reds, const char *codecs)
             new_codec.create = video_encoder_procs[encoder_index];
             new_codec.type = video_codec_names[codec_index].id;
             new_codec.cap = video_codec_caps[codec_index];
-            g_array_append_val(reds->config->video_codecs, new_codec);
+            g_array_append_val(video_codecs, new_codec);
         }
 
         free(encoder_name);
         free(codec_name);
         codecs = c;
     }
+
+    if (video_codecs->len == 0) {
+        spice_warning("Failed to set video codecs, input string: '%s'", codecs);
+        g_array_unref(video_codecs);
+        return;
+    }
+
+    /* The video_codecs array is immutable */
+    g_array_unref(reds->config->video_codecs);
+    reds->config->video_codecs = video_codecs;
 }
 
 SPICE_GNUC_VISIBLE int spice_server_init(SpiceServer *reds, SpiceCoreInterface *core)
diff --git a/server/tests/test-codecs-parsing.c b/server/tests/test-codecs-parsing.c
index c1b3276..5af2e5d 100644
--- a/server/tests/test-codecs-parsing.c
+++ b/server/tests/test-codecs-parsing.c
@@ -28,9 +28,6 @@ static void codecs_good(void)
 {
     guint i;
     const gchar *codecs[] = {
-        "",
-        ";",
-        ";;;;",
         "spice:mjpeg",
         "spice:mjpeg;;;",
         "spice:mjpeg;;spice:mjpeg;;;",
@@ -62,45 +59,70 @@ static void codecs_bad(void)
         const gchar *codecs;
         const GLogLevelFlags log_level;
         const gchar *error_message;
+        gboolean default_err_message;
     } test_cases[] = {
         {
+            "",
+            G_LOG_LEVEL_WARNING,
+            "*Failed to set video codecs*",
+            FALSE,
+        },{
+            ";",
+            G_LOG_LEVEL_WARNING,
+            "*Failed to set video codecs*",
+            FALSE,
+        },{
+            ";;;;",
+            G_LOG_LEVEL_WARNING,
+            "*Failed to set video codecs*",
+            FALSE,
+        },{
             NULL,
             G_LOG_LEVEL_CRITICAL,
-            "*assertion 'codecs != NULL' failed"
+            "*assertion 'codecs != NULL' failed",
+            FALSE,
         },{
             ";:;",
             G_LOG_LEVEL_WARNING,
             "*spice: invalid encoder:codec value*",
+            TRUE,
         },{
             "::::",
             G_LOG_LEVEL_WARNING,
             "*spice: invalid encoder:codec value*",
+            TRUE,
         },{
             "missingcolon",
             G_LOG_LEVEL_WARNING,
             "*spice: invalid encoder:codec value*",
+            TRUE,
         },{
             ":missing_encoder",
             G_LOG_LEVEL_WARNING,
             "*spice: invalid encoder:codec value*",
+            TRUE,
         },{
             "missing_value:;",
             G_LOG_LEVEL_WARNING,
             "*spice: invalid encoder:codec value*",
+            TRUE,
         },{
             "unknown_encoder:mjpeg",
             G_LOG_LEVEL_WARNING,
             "*spice: unknown video encoder unknown_encoder",
+            TRUE,
         },{
             "spice:unknown_codec",
             G_LOG_LEVEL_WARNING,
             "*spice: unknown video codec unknown_codec",
+            TRUE,
         },
 #if !defined(HAVE_GSTREAMER_1_0) && !defined(HAVE_GSTREAMER_0_10)
         {
             "gstreamer:mjpeg",
             G_LOG_LEVEL_WARNING,
             "*spice: unsupported video encoder gstreamer",
+            TRUE,
         }
 #endif
     };
@@ -111,6 +133,8 @@ static void codecs_bad(void)
 
     for (i = 0; i < G_N_ELEMENTS(test_cases); ++i) {
         g_test_expect_message(G_LOG_DOMAIN, test_cases[i].log_level, test_cases[i].error_message);
+        if (test_cases[i].default_err_message)
+            g_test_expect_message(G_LOG_DOMAIN, G_LOG_LEVEL_WARNING, "*Failed to set video codecs*");
         g_assert_cmpint(spice_server_set_video_codecs(server, test_cases[i].codecs), ==, 0);
         g_test_assert_expected_messages();
     }
-- 
2.9.3