Am 14.04.2020 um 16:34 hat Markus Armbruster geschrieben: > Markus Armbruster <[email protected]> writes: > > > Eric Blake <[email protected]> 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 <[email protected]> > >>> --- > >>> 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
