[Spice-devel] [spice v1 3/3] reds: drop sscanf() in favour of g_strsplit()

[Frediano Ziglio]

> 
> On Fri, Dec 09, 2016 at 08:52:00AM -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 initialized integer.
> > >
> > > While working on extending the codecs string, this seems a bit risky
> > > although I did not find any real example that could trigger that path.
> > >
> >
> > This rationale looks like "I don't understand sscanf so I changed to
> > g_strsplit". I don't think there's much wrong with sscanf beside
> > possibly some syntax like "gstreamer:vp8*garbage" which cause
> > "gstreamer:vp8" to be accepted and "*garbage" to give a warning;
> > this as last "%m[0-9a-zA-Z_]" will stop at "*".
> 
> Maybe I did not make myself clear about the issue. I have a wip patch
> similar to [0] that introduce a rank value for that pair of
> encoder/codec. It is another value to be parsed and introduce more
> possibilities, bigger the change of issues.
> 
> That, combined with the `return codecs + n` that could have
> uninitialised n value, seems a bit dangerous. g_strplit() is simpler
> yes, but why should we make this complex?
> 

Your rationale mentioned about an "initialised integer", not UNinitialised.
I think the complexity of extending sscanf is a much better rationale than
the usage of an initialised integer. But then this patch should be included
in a series with the other change.

> > Worth noticing that sscanf did some check on syntax however the code
> > also check encoders/codecs again fixed list so it does not change
> > much at the end, just you have a syntax error for "££%$:!^%" while
> > now you'll get an invalid encoder error.
> 
> I really don't see an issue with that. As you said bellow, we will check
> them in a fixed list and it will be warned.
> 

But from the user prospective the error message will change.
Agreed it's not an issue.

> > OT: personally I ignored sscanf for a long time till I came to
> > have a look at micro_httpd code and I discovered that this
> > function looks like sprintf for reading instead of writing
> > but it's really powerful although difficult to master.
> > IMHO it's really worth having a look at it.
> 
> I just did not see the point in trying to fix it. We need to iterate in

That's nothing broken in the current code, just complicate to extend.

> every pair between ';' and get every value between ':'. As we do more
> checks with each value we get, I'm fine using something simpler.
> 

Agreed.

> >
> > > 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], "")) {
> > 
> > Here I would avoid the checks for empty as the values will be checked
> > against fixed lists.
> 
> Indeed.
> 
> > Probably pref[0] can't be NULL but it's safer to check it.
> 
> Thanks for reviewing.
>   toso
> 
> >
> > > +            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