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

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> Eric Blake <address@hidden> writes:
>
>> 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.
>
> Hmm, that's a bug:
>
>     $ qemu-img create -f qcow2 -o backing_file=a -o "" -o 
> backing_fmt=raw,size=1M new.qcow2
>     qemu-img: warning: Could not verify backing image. This may become an 
> error in future versions.
>     Could not open 'a,backing_fmt=raw': No such file or directory
>     Formatting 'new.qcow2', fmt=qcow2 size=1048576 
> backing_file=a,,backing_fmt=raw cluster_size=65536 lazy_refcounts=off 
> refcount_bits=16
>     $ qemu-img info new.qcow2 
>     image: new.qcow2
>     file format: qcow2
>     virtual size: 1 MiB (1048576 bytes)
>     disk size: 196 KiB
>     cluster_size: 65536
> --> backing file: a,backing_fmt=raw
>     Format specific information:
>         compat: 1.1
>         lazy refcounts: false
>         refcount bits: 16
>         corrupt: false
>
> My rewrite preserves this bug.  Will fix in v2.

Kevin, two obvious fixes:

* Make is_valid_option_list() reject -o ""

* Make accumulate_options(options, "") return options.

Got a preference?

[...]