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

[Frediano Ziglio]

> 
> 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.
> 

What I'm trying to say is that current code don't have any issue.
If sscanf(... "%m[0-9a-zA-Z_]:%m[0-9a-zA-Z_]%n"...) returns 2 means
that the last item is parsed and %n came just next to it so it must
be initialized. As you said above the uninitialized came when
you tried to expand it.

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

I would just remove. The complexity of unneeded testing (as
stated below) is the right and correct rationale.
Or you can say that extending sscanf to support more items
is prone to have n uninitialized but it's just an extension
to the last paragraph of the commit message.

> >
> > > 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