[Spice-devel] [spice v1 1/3] reds: don't replace video_codecs on failure
Victor Toso
victortoso at redhat.com
Fri Dec 9 12:26:07 UTC 2016
Hi,
On Fri, Dec 09, 2016 at 10:42:15AM +0100, Pavel Grunt wrote:
> On Thu, 2016-12-08 at 23:27 +0100, Victor Toso wrote:
> > 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[] = {
> > - "",
> > - ";",
> > - ";;;;",
> Why do you remove these ?
I moved those to bad, as mentioned in the commit log:
"Tests were updated to match this situation. Input that fails to
replace video_codecs are considerer bad."
The rationale is that host is calling an API to update video-codecs but
it did not, so it failed and a warning is now triggered about it.
>
> > "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,
> > + },{
^ here they are
toso
> > 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();
> > }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161209/d15f2d25/attachment.sig>
More information about the Spice-devel
mailing list