| Message ID | 20200409153041.17576-9-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-option: Fix corner cases and clean up | expand |
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. 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 <eblake@redhat.com>
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. > 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). Yes. > 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" QemuOpts is a language without syntax errors. > 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 > "". I do. >> - 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; Correct. > and where the concatenated string must parse > back to the same number of embedded options. We want the concatenated string to parse into the concatenation of the parse of its parts. Whether "same number" implies that is an interesting puzzle, but not one worth solving today :) > As both -o ",help" and > -o "help," are invalid option lists on their own (there is no valid > empty option for -o), Yes, it's *semantically* invalid. It parses as sugar for "help=on,=on", except "help=on" doesn't get you help, only "help" does. > 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). Yes. If the empty option existed, concatenation would not do. It doesn't now, and I expect it to stay that way. > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks for your thorough review!
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? [...]
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
Kevin Wolf <kwolf@redhat.com> writes: > 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. Thanks!
diff --git a/include/qemu/option.h b/include/qemu/option.h index 844587cab3..eb4097889d 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -33,7 +33,6 @@ const char *get_opt_value(const char *p, char **value); void parse_option_size(const char *name, const char *value, uint64_t *ret, Error **errp); bool has_help_option(const char *param); -bool is_valid_option_list(const char *param); enum QemuOptType { QEMU_OPT_STRING = 0, /* no parsing (use string as-is) */ diff --git a/qemu-img.c b/qemu-img.c index 28b27b738a..d8ca8a73e2 100644 --- a/qemu-img.c +++ 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; +} + static int accumulate_options(char **options, char *optarg) { char *new_options; diff --git a/util/qemu-option.c b/util/qemu-option.c index 279f1b3fb3..6f80807738 100644 --- a/util/qemu-option.c +++ 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); - if ((*p && !*++p) || - (!*value || *value == ',')) { - goto out; - } - - g_free(value); - value = NULL; - } - - result = true; -out: - g_free(value); - return result; -} - static const char *opt_type_to_string(enum QemuOptType type) { switch (type) {
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(-)