[Spice-devel] [PATCH spice-server v5 7/9] reds: drop sscanf() in favour of g_strsplit()

[Victor Toso]

Hi,

On Thu, Jan 19, 2017 at 12:14:06PM +0200, Uri Lublin wrote:
> On 01/06/2017 10:19 AM, Victor Toso wrote:
> > From: Victor Toso <me at victortoso.com>
> >
> > 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 makes much easier to extend the input which is currently a
> > list of tuples encoder:video-codec.
>
> Hi Victor,
>
> Note that the behavior is different.
> For example, this implementation accepts strings like
>   "@#$@34:@%$^$%^"
>   ":"
>   "ddf:"
>   ":fdf"
>
> as valid while previous one rejected them.

The input is accepted but it isn't valid as we check each value to
static array with the valid values. I changed the behavior slightly
here.

All examples above will fail to change the video preference although the
error message might differ.

>
> Also for "a:b:c" -- previous implementation
> returned "a" and "b", this one returns "a" and "b:c"

Again, the input wouldn't be valid or accepted. The reason for that is
to easily extend with the priority per video-codec patch which was sent
as RFC [0], the interesting change would be:

-        pref = g_strsplit(preferences[i], ":", 2);
+        pref = g_strsplit(preferences[i], ":", 3);

[0]
https://lists.freedesktop.org/archives/spice-devel/2017-January/034849.html

I don't see any necessary change for this cases, unless I'm not seeing
something important as most of this are wrong inputs that in both
implementations are wrong.

Maybe improving the commit log?

Thanks for reviewing,
  toso

>
> Regards,
>     Uri.
> 
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  server/reds.c | 62 +++++++++++++++++++++++++++--------------------------------
> >  1 file changed, 28 insertions(+), 34 deletions(-)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index e061e4d..26a8b51 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,28 @@ 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 || pref[1] == NULL) {
> > +            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 +3566,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);
> > 
> 
-------------- 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/20170119/d87db7d0/attachment.sig>