[Spice-devel] [spice-server v3 08/11] reds: drop sscanf() in favour of g_strsplit()

[Victor Toso]

Hi,

On Thu, Dec 15, 2016 at 06:27:21AM -0500, Frediano Ziglio wrote:
>
> > From: Victor Toso <me at victortoso.com>
> >
> > In case of errors in sscanf(), we were returning (codecs + n) being n
> > an uninitialized variable.  That should be avoided in any circumstance.
> >
>
> These lines are wrong. n is always used initialized.

Even in case of errors? The situation that I had with n being
uninitialized was on error while expanding the parser using sscanf.

I don't mind to change/remove it, I just want to be sure.

>
> > As there is a need to iterate over every encoder:codec pair and we do
> > a check for every encoder and every codec, g_strsplit() is less
> > complex and easier to follow.
> > 
> > This change also makes the following patch which introduces an
> > optional value for each encoder:codec pair easier to understand.
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  server/reds.c | 63
> >  +++++++++++++++++++++++++++--------------------------------
> >  1 file changed, 29 insertions(+), 34 deletions(-)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index e061e4d..eaa18d9 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3512,34 +3512,15 @@ static const int video_codec_caps[] = {
> >  };
> >  
> >  
> > -/* Expected string:  encoder:codec;encoder:codec */
> > -static const char* parse_video_codecs(const char *codecs, char **encoder,
> > -                                      char **codec)
> > -{
> > -    if (!codecs) {
> > -        return NULL;
> > -    }
> > -    while (*codecs == ';') {
> > -        codecs++;
> > -    }
> > -    if (!*codecs) {
> > -        return NULL;
> > -    }
> > -    int n;
> > -    *encoder = *codec = NULL;
> > -    if (sscanf(codecs, "%m[0-9a-zA-Z_]:%m[0-9a-zA-Z_]%n", encoder, codec,
> > &n) != 2) {
> > -        while (*codecs != '\0' && *codecs != ';') {
> > -            codecs++;
> > -        }
> > -        return codecs;
> > -    }
> > -    return codecs + n;
> > -}
> > -
> > +/*
> > + * @codecs should be described in a semi-colon separated list of
> > encoder:codec.
> > + * e.g: "gstreamer:vp8;spice:mjpeg"
> > + */
> >  static void reds_set_video_codecs_from_string(RedsState *reds, const char
> >  *codecs)
> >  {
> > -    char *encoder_name, *codec_name;
> >      GArray *video_codecs;
> > +    gchar **preferences;
> > +    gint i;
> >  
> >      g_return_if_fail(codecs != NULL);
> >  
> > @@ -3548,13 +3529,29 @@ static void
> > reds_set_video_codecs_from_string(RedsState *reds, const char *codec
> >      }
> >  
> >      video_codecs = g_array_new(FALSE, FALSE, sizeof(RedVideoCodec));
> > -    const char *c = codecs;
> > -    while ( (c = parse_video_codecs(c, &encoder_name, &codec_name)) ) {
> > +    preferences = g_strsplit(codecs, ";", -1);
> > +    for (i = 0; preferences[i] != NULL; i++) {
> > +        const gchar *encoder_name, *codec_name;
> >          uint32_t encoder_index, codec_index;
> > -        if (!encoder_name || !codec_name) {
> > -            spice_warning("spice: invalid encoder:codec value at %s",
> > codecs);
> > +        gchar **pref;
> > +
> > +        /* Don't bother with empty strings that would be an extra ';'
> > somewhere */
> > +        if (g_str_equal(preferences[i], ""))
> > +            continue;
> > +
> > +        pref = g_strsplit(preferences[i], ":", 2);
> >  
> > -        } else if (!get_name_index(video_encoder_names, encoder_name,
> > &encoder_index)){
> > +        if (pref[0] == NULL || g_str_equal(pref[0], "") ||
> > +                pref[1] == NULL || g_str_equal(pref[1], "")) {
>
> If I remember I commented out that the check for empty are
> redundant (they just change warning message). Did you
> decide to just retain this message?

No, I forgot to commit this change. I'll do soon.
Sorry about that.

> 
> > +            spice_warning("spice: invalid encoder:codec value at '%s'",
> > preferences[i]);
> > +            g_strfreev(pref);
> > +            continue;
> > +        }
> > +
> > +        encoder_name = pref[0];
> > +        codec_name = pref[1];
> > +
> > +        if (!get_name_index(video_encoder_names, encoder_name,
> > &encoder_index)){
> >              spice_warning("spice: unknown video encoder %s", encoder_name);
> >  
> >          } else if (!get_name_index(video_codec_names, codec_name,
> >          &codec_index)) {
> > @@ -3570,11 +3567,9 @@ static void
> > reds_set_video_codecs_from_string(RedsState *reds, const char *codec
> >              new_codec.cap = video_codec_caps[codec_index];
> >              g_array_append_val(video_codecs, new_codec);
> >          }
> > -
> > -        free(encoder_name);
> > -        free(codec_name);
> > -        codecs = c;
> > +        g_strfreev(pref);
> >      }
> > +    g_strfreev(preferences);
> >  
> >      if (video_codecs->len == 0) {
> >          spice_warning("Failed to set video codecs, input string: '%s'",
> >          codecs);
> 
> Frediano
-------------- 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/20161215/debfdb81/attachment.sig>