[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu
From: |
Eric Blake |
Subject: |
Re: [PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite |
Date: |
Thu, 9 Apr 2020 14:45:10 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 |
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. 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 <address@hidden>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
- [PATCH for-5.1 4/8] qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily(), (continued)
- [PATCH for-5.1 4/8] qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily(), Markus Armbruster, 2020/04/09
- [PATCH for-5.1 3/8] qemu-option: Fix sloppy recognition of "id=..." after ", , ", Markus Armbruster, 2020/04/09
- [PATCH for-5.1 6/8] test-qemu-opts: Simplify test_has_help_option() after bug fix, Markus Armbruster, 2020/04/09
- [PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite, Markus Armbruster, 2020/04/09
- Re: [PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite,
Eric Blake <=
[PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper, Markus Armbruster, 2020/04/09
Re: [PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper, Kevin Wolf, 2020/04/14