[Spice-devel] [spice-server v3 06/11] reds: don't replace video_codecs on failure

[Frediano Ziglio]

> 
> 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 considered bad.
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>

Acked-by: Frediano Ziglio <fziglio 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();
>      }

Frediano