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

[Kevin Wolf <kwolf@redhat.com>]

Am 14.04.2020 um 16:34 hat Markus Armbruster geschrieben:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Eric Blake <eblake@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 <armbru@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.

Kevin