Re: [PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite

[Eric Blake]

On 4/9/20 10:30 AM, Markus Armbruster wrote:
> is_valid_option_list()'s purpose is ensuring qemu-img.c's can safely
> join multiple parameter strings separated by ',' like this:
>
>          g_strdup_printf("%s,%s", params1, params2);
>
> How it does that is anything but obvious.  A close reading of the code
> reveals that it fails exactly when its argument starts with ',' or
> ends with an odd number of ','.  Makes sense, actually, because when
> the argument starts with ',', a separating ',' preceding it would get
> escaped, and when it ends with an odd number of ',', a separating ','
> following it would get escaped.
>
> Move it to qemu-img.c and rewrite it the obvious way.
>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>   include/qemu/option.h |  1 -
>   qemu-img.c            | 26 ++++++++++++++++++++++++++
>   util/qemu-option.c    | 22 ----------------------
>   3 files changed, 26 insertions(+), 23 deletions(-)

> +++ b/qemu-img.c
> @@ -223,6 +223,32 @@ static bool qemu_img_object_print_help(const char *type, 
> QemuOpts *opts)
>       return true;
>   }
>   
> +/*
> + * Is @optarg safe for accumulate_options()?
> + * It is when multiple of them can be joined together separated by ','.
> + * To make that work, @optarg must not start with ',' (or else a
> + * separating ',' preceding it gets escaped), and it must not end with
> + * an odd number of ',' (or else a separating ',' following it gets
> + * escaped).
> + */
> +static bool is_valid_option_list(const char *optarg)
> +{
> +    size_t len = strlen(optarg);
> +    size_t i;
> +
> +    if (optarg[0] == ',') {
> +        return false;
> +    }
> +
> +    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
> +    }
> +    if ((len - i) % 2) {
> +        return false;
> +    }
> +
> +    return true;

Okay, that's easy to read.  Note that is_valid_option_list("") returns 
true.  But proving it replaces the obtuse mess is harder...

> +++ b/util/qemu-option.c
> @@ -165,28 +165,6 @@ void parse_option_size(const char *name, const char *value,
>       *ret = size;
>   }
>   
> -bool is_valid_option_list(const char *p)
> -{
> -    char *value = NULL;
> -    bool result = false;
> -
> -    while (*p) {
> -        p = get_opt_value(p, &value);

Refreshing myself on what get_opt_value() does:

> const char *get_opt_value(const char *p, char **value)
> {
>     size_t capacity = 0, length;
>     const char *offset;
>
>     *value = NULL;

start with p pointing to the tail of a string from which to extract a value,

>     while (1) {
>         offset = qemu_strchrnul(p, ',');

find the potential end of the value: either the first comma (not yet 
sure if it is ',,' or the end of this value), or the end of the string,

>         length = offset - p;
>         if (*offset != '\0' && *(offset + 1) == ',') {
>             length++;
>         }

if we found a comma and it was doubled, we are going to unescape the comma,

>         *value = g_renew(char, *value, capacity + length + 1);
>         strncpy(*value + capacity, p, length);

copy bytes into the tail of value,

>         (*value)[capacity + length] = '\0';
>         capacity += length;
>         if (*offset == '\0' ||
>             *(offset + 1) != ',') {
>             break;
>         }

if we hit the end of the string or the ',' before the next option, we 
are done,

>         p += (offset - p) + 2;

otherwise we unescaped a ',,' and continue appending to the current value.

>     }
>
>     return offset;
> }

and the resulting return value is a substring of p: either the NUL byte 
ending p, or the last ',' in the first odd run of commas (the next byte 
after the return is then NUL if the parser would next see an empty 
option name, or non-NUL if the parser would start processing the next 
option name).

Some interesting cases:
get_opt_value("", &value) returns "" with value set to ""
get_opt_value(",", &value) returns "," with value set to ""
get_opt_value(",,", &value) returns "" with value set to ","
get_opt_value(",,,", &value) returns "," with value set to ","
get_opt_value(",,a", &value) returns "" with value set to ",a"
get_opt_value("a,,", &value) returns "" with value set to "a,"
get_opt_value("a,b", &value) returns ",b" with value set to "a"
get_opt_value("a,,b", &value) returns "" with value set to "a,b"

With that detour out of the way:

> -        if ((*p && !*++p) ||

If *p, then we know '*p' == ','; checking !*++p moves past the comma and 
determines if we have the empty string as the next potential option name 
(if so, we ended in an odd number of commas) or anything else (if so, 
repeat the loop, just past the comma).  Oddly, this means that 
is_valid_option_list("a,,,b") returns true (first pass on "a,,,b" 
returned ",b", second pass on "b" returned "").  But I agree that !*++p 
only fires on the final iteration through our loop of get_opt_value() 
calls, matching your rewrite to check for an odd number of trailing 
commas (regardless of even or odd pairing of commas in the middle).

> -            (!*value || *value == ',')) {

Focusing on "*value == ','", on the first iteration, *value can be 
comma; on later iterations, we are guaranteed that *value will not be 
comma (because later iterations can only occur if the first iteration 
returned p pointing to a final comma of a run, but we always start the 
next iteration beyond that comma).  So this matches your rewrite to 
check for a leading comma.

Focusing on "!*value": the loop is never entered to begin with on 
is_valid_option_list("") (that returns true in the old implementation as 
well as yours); otherwise, value will only be empty if p originally 
started with an unpaired comma (but your new code catches that with its 
check for a leading comma).

Note that is_valid_option_list("") returning true has some odd effects:

$ qemu-img create -f qcow2 -o '' xyz 1
Formatting 'xyz', fmt=qcow2 size=1 cluster_size=65536 lazy_refcounts=off 
refcount_bits=16
$ qemu-img create -f qcow2 -o '' -o '' xyz 1
qemu-img: xyz: Invalid parameter ''
$ qemu-img create -f qcow2 -o 'help' -o '' -o '' xyz 1
qemu-img: xyz: Invalid parameter 'help'
$ qemu-img create -f qcow2 -o 'help' -o '' xyz 1 | head -n1
Supported options:

but as I can't see any substantial differences between the old algorithm 
and your new one, I don't see that changing here.  Perhaps you want 
another patch to make is_valid_option_list() return false for "".


> -            goto out;
> -        }
> -
> -        g_free(value);
> -        value = NULL;
> -    }
> -
> -    result = true;
> -out:
> -    g_free(value);
> -    return result;
> -}

At any rate, your new code is a LOT more efficient and legible than the 
old.  I'll restate the "why" in my own words: is_valid_option_list() is 
only used by our code base to concatenate multiple -o strings into one 
string, where each -o individually parses into one or more options; and 
where the concatenated string must parse back to the same number of 
embedded options.  As both -o ",help" and -o "help," are invalid option 
lists on their own (there is no valid empty option for -o), it makes no 
sense to append that -o to other option lists to produce a single string 
(where the appending would become confusing due to creating escaped ',,' 
that were not present in either original -o).

Reviewed-by: Eric Blake <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org