Kevin Wolf <kw...@redhat.com> writes:
> Am 14.04.2020 um 16:34 hat Markus Armbruster geschrieben:
>> Markus Armbruster <arm...@redhat.com> writes:
>>
>> > Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com>
>> >>> ---
>> >>> 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?
>
> In other words, the choice is between reporting an error and ignoring it
> silently. I think reporting an error makes more sense.
Thanks!
