>
> From: Victor Toso <me@xxxxxxxxxxxxxx>
>
> 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 "*".
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.
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.
> Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
> ---
> 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.
Probably pref[0] can't be NULL but it's safer to check it.
> + 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel