| Message ID | 20210118163113.780171-6-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-option, keyval, vl: switch -object/-M/-accel to keyval parsing | expand |
Paolo Bonzini <pbonzini@redhat.com> writes: > Now that the key is NULL terminated, we can remove some of the contortions > that were done to operate on possibly '='-terminated strings in > keyval_parse_one. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Alright, I'm now ready to discuss the third argument: nicer code. I think there is improvement, and I think it comes mostly from working on a copy. Could be done without the syntax change. I feel further improvement is possible, but that's no reason to delay this series. Second argument: better error messages. We discussed that in review of a prior post, but you've since improved your error reporting further, so let's have another look. I'm testing with $ qemu-storage-daemon --nbd $ARG because that one doesn't have an implied key, which permits slightly simpler $ARG. * Empty key --nbd , master: Invalid parameter '' your patch: Expected parameter before ',' Improvement. --nbd key=val,=,fob= master: Invalid parameter '' your patch: Expected parameter before '=,fob=' Improvement. * Empty key fragment --nbd key..= master: Invalid parameter 'key..' your patch: same Could use some love. Not your patch's fault. --nbd .key= master: Invalid parameter '..key' your patch: same Likweise. If I omit the '=', your patch's message changes to No implicit parameter name for value 'key..' I consider that worse than before, because it's talking about something outside the user's control (lack of an implict parameter name) where it should instead tell the user what needs fixing in the input. * Missing value --nbd key master: Expected '=' after parameter 'key' your patch: No implicit parameter name for value 'key' Same criticism as above. * Invalid key fragment --nbd _= master: Invalid parameter '_' your patch: same --nbd key.1a.b= master: Invalid parameter 'key.1a.b' your patch: same Could perhaps use some love. Not your patch's fault. --ndb anti,,social,,key= master: Expected '=' after parameter 'anti' your patch: Invalid parameter 'anti,social,key' The new error message shows the *unescaped* string. Okay. Your patch also adds an "Expected parameter at end of string" error. Can you tell me how to trigger it? I think there is improvement, and I think it could also be done without the syntax change. There also is one regression: "No implicit parameter name..." is no good. Looks fixable to me. I feel further improvement is possible, but that's no reason to delay this series. Now let me put the three arguments together. Nicer code and better error reporting could be had with or without the syntax change. With is a bit easier, because we already have your patch. The syntax change is a choice we can make. I'm reluctant to mess with the syntax, but if you want the change, I'm not going to block it. Hmm, bartering opportunity... May I have your support for me eliminating anti-social device names in exchange? ;) I believe your grammar is ambiguous. Your code seems to pick the sane alternative. If I'm wrong, you need to enlighten me. If I'm right, you need to fix your grammar. > --- > util/keyval.c | 27 ++++++++++----------------- > 1 file changed, 10 insertions(+), 17 deletions(-) > > diff --git a/util/keyval.c b/util/keyval.c > index eb9b9c55ec..e7f708cd1e 100644 > --- a/util/keyval.c > +++ b/util/keyval.c > @@ -170,11 +170,10 @@ static QObject *keyval_parse_put(QDict *cur, > * > * On return: > * - either NUL or the separator (comma or equal sign) is returned. > - * - the length of the string is stored in @len. > * - @start is advanced to either the NUL or the first character past the > * separator. > */ > -static char keyval_fetch_string(char **start, size_t *len, bool key) > +static char keyval_fetch_string(char **start, bool key) > { > char sep; > char *p, *unescaped; > @@ -197,7 +196,6 @@ static char keyval_fetch_string(char **start, size_t *len, bool key) > } > > *unescaped = 0; > - *len = unescaped - *start; > *start = p; > return sep; > } > @@ -219,7 +217,7 @@ static char *keyval_parse_one(QDict *qdict, char *params, > const char *implied_key, bool *help, > Error **errp) > { > - const char *key, *key_end, *s, *end; > + const char *key, *s, *end; > const char *val = NULL; > char sep; > size_t len; > @@ -229,8 +227,8 @@ static char *keyval_parse_one(QDict *qdict, char *params, > QObject *next; > > key = params; > - sep = keyval_fetch_string(¶ms, &len, true); > - if (!len) { > + sep = keyval_fetch_string(¶ms, true); > + if (!*key) { > if (sep) { > error_setg(errp, "Expected parameter before '%c%s'", sep, params); > } else { > @@ -247,13 +245,11 @@ static char *keyval_parse_one(QDict *qdict, char *params, > /* Desugar implied key */ > val = key; > key = implied_key; > - len = strlen(implied_key); > } else { > error_setg(errp, "No implicit parameter name for value '%s'", key); > return NULL; > } > } > - key_end = key + len; > > /* > * Loop over key fragments: @s points to current fragment, it > @@ -269,24 +265,21 @@ static char *keyval_parse_one(QDict *qdict, char *params, > ret = parse_qapi_name(s, false); > len = ret < 0 ? 0 : ret; > } > - assert(s + len <= key_end); > - if (!len || (s + len < key_end && s[len] != '.')) { > + if (!len || (s[len] != '\0' && s[len] != '.')) { > assert(key != implied_key); > - error_setg(errp, "Invalid parameter '%.*s'", > - (int)(key_end - key), key); > + error_setg(errp, "Invalid parameter '%s'", key); > return NULL; > } > if (len >= sizeof(key_in_cur)) { > assert(key != implied_key); > error_setg(errp, "Parameter%s '%.*s' is too long", > - s != key || s + len != key_end ? " fragment" : "", > + s != key || s[len] == '.' ? " fragment" : "", > (int)len, s); > return NULL; > } > > if (s != key) { > - next = keyval_parse_put(cur, key_in_cur, NULL, > - key, s - 1, errp); > + next = keyval_parse_put(cur, key_in_cur, NULL, key, s - 1, errp); Unrelated line join. > if (!next) { > return NULL; > } > @@ -301,9 +294,9 @@ static char *keyval_parse_one(QDict *qdict, char *params, > > if (key != implied_key) { > val = params; > - keyval_fetch_string(¶ms, &len, false); > + keyval_fetch_string(¶ms, false); > } > - if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) { > + if (!keyval_parse_put(cur, key_in_cur, val, key, s - 1, errp)) { > return NULL; > } > return params;
On 22/01/21 14:48, Markus Armbruster wrote: > --nbd .key= > > master: Invalid parameter '..key' > your patch: same > > Likweise. > > If I omit the '=', your patch's message changes to > > No implicit parameter name for value 'key..' > > I consider that worse than before, because it's talking about > something outside the user's control (lack of an implict parameter > name) where it should instead tell the user what needs fixing in the > input. I think whether it's better or worse depends on the specific erroneous command line (think "--nbd /path/to/file.qcow2"), but I can certainly change it. > Your patch also adds an "Expected parameter at end of string" error. > Can you tell me how to trigger it? It is meant for "--nbd ''" but it is effectively dead code due to the "while (*s)" in the caller. Possibilities: 1) leave it in as dead code 2) replace it with an assert 3) change the caller to use a do...while in such a way that it triggers it (and be careful not to change the grammar). > I believe your grammar is ambiguous. Your code seems to pick the sane > alternative. If I'm wrong, you need to enlighten me. If I'm right, you > need to fix your grammar. Will do. Can I change the EBNF to use "+" and "*" for simplicity and clarity? > Hmm, bartering opportunity... May I have your support for me > eliminating anti-social device names in exchange? I do not mind removing them. What's the barter exactly like? :) Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 22/01/21 14:48, Markus Armbruster wrote: >> --nbd .key= >> master: Invalid parameter '..key' >> your patch: same >> Likweise. >> If I omit the '=', your patch's message changes to >> No implicit parameter name for value 'key..' >> I consider that worse than before, because it's talking about >> something outside the user's control (lack of an implict parameter >> name) where it should instead tell the user what needs fixing in the >> input. > > I think whether it's better or worse depends on the specific erroneous > command line (think "--nbd /path/to/file.qcow2"), but I can certainly > change it. Quoting myself: Error messages based on guesses what the user has in mind can be quite confusing when we guess wrong. A strictly factual syntax error style like "I expected FOO instead of BAR here" may not be great, but has a relatively low risk of being confusing. >> Your patch also adds an "Expected parameter at end of string" error. >> Can you tell me how to trigger it? > > It is meant for "--nbd ''" but it is effectively dead code due to the > "while (*s)" in the caller. Possibilities: > > 1) leave it in as dead code I'd prefer not to. > 2) replace it with an assert Works for me. > 3) change the caller to use a do...while in such a way that it > triggers it (and be careful not to change the grammar). Only if it's not too hard, and results in better error messages. >> I believe your grammar is ambiguous. Your code seems to pick the sane >> alternative. If I'm wrong, you need to enlighten me. If I'm right, you >> need to fix your grammar. > > Will do. Can I change the EBNF to use "+" and "*" for simplicity and > clarity? I tried to stick to ISO 14977 as described in <https://en.wikipedia.org/wiki/Extended_Backus%E2%80%93Naur_form>, less its foolish ',' for concatenation. I'm not at all enamored with it. All I want is something that is widely understood, and preferably not invented here. Switch to <https://www.w3.org/TR/xml/#sec-notation> perhaps? [...]
diff --git a/util/keyval.c b/util/keyval.c index eb9b9c55ec..e7f708cd1e 100644 --- a/util/keyval.c +++ b/util/keyval.c @@ -170,11 +170,10 @@ static QObject *keyval_parse_put(QDict *cur, * * On return: * - either NUL or the separator (comma or equal sign) is returned. - * - the length of the string is stored in @len. * - @start is advanced to either the NUL or the first character past the * separator. */ -static char keyval_fetch_string(char **start, size_t *len, bool key) +static char keyval_fetch_string(char **start, bool key) { char sep; char *p, *unescaped; @@ -197,7 +196,6 @@ static char keyval_fetch_string(char **start, size_t *len, bool key) } *unescaped = 0; - *len = unescaped - *start; *start = p; return sep; } @@ -219,7 +217,7 @@ static char *keyval_parse_one(QDict *qdict, char *params, const char *implied_key, bool *help, Error **errp) { - const char *key, *key_end, *s, *end; + const char *key, *s, *end; const char *val = NULL; char sep; size_t len; @@ -229,8 +227,8 @@ static char *keyval_parse_one(QDict *qdict, char *params, QObject *next; key = params; - sep = keyval_fetch_string(¶ms, &len, true); - if (!len) { + sep = keyval_fetch_string(¶ms, true); + if (!*key) { if (sep) { error_setg(errp, "Expected parameter before '%c%s'", sep, params); } else { @@ -247,13 +245,11 @@ static char *keyval_parse_one(QDict *qdict, char *params, /* Desugar implied key */ val = key; key = implied_key; - len = strlen(implied_key); } else { error_setg(errp, "No implicit parameter name for value '%s'", key); return NULL; } } - key_end = key + len; /* * Loop over key fragments: @s points to current fragment, it @@ -269,24 +265,21 @@ static char *keyval_parse_one(QDict *qdict, char *params, ret = parse_qapi_name(s, false); len = ret < 0 ? 0 : ret; } - assert(s + len <= key_end); - if (!len || (s + len < key_end && s[len] != '.')) { + if (!len || (s[len] != '\0' && s[len] != '.')) { assert(key != implied_key); - error_setg(errp, "Invalid parameter '%.*s'", - (int)(key_end - key), key); + error_setg(errp, "Invalid parameter '%s'", key); return NULL; } if (len >= sizeof(key_in_cur)) { assert(key != implied_key); error_setg(errp, "Parameter%s '%.*s' is too long", - s != key || s + len != key_end ? " fragment" : "", + s != key || s[len] == '.' ? " fragment" : "", (int)len, s); return NULL; } if (s != key) { - next = keyval_parse_put(cur, key_in_cur, NULL, - key, s - 1, errp); + next = keyval_parse_put(cur, key_in_cur, NULL, key, s - 1, errp); if (!next) { return NULL; } @@ -301,9 +294,9 @@ static char *keyval_parse_one(QDict *qdict, char *params, if (key != implied_key) { val = params; - keyval_fetch_string(¶ms, &len, false); + keyval_fetch_string(¶ms, false); } - if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) { + if (!keyval_parse_put(cur, key_in_cur, val, key, s - 1, errp)) { return NULL; } return params;
Now that the key is NULL terminated, we can remove some of the contortions that were done to operate on possibly '='-terminated strings in keyval_parse_one. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- util/keyval.c | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-)