Re: [PATCH 1/4] keyval: Parse help options
Eric Blake writes:
> On 9/29/20 12:26 PM, Kevin Wolf wrote:
>> This adds a new parameter 'help' to keyval_parse() that enables parsing
>> of help options. If NULL is passed, the function behaves the same as
>> before. But if a bool pointer is given, it contains the information
>> whether an option "help" without value was given (which would otherwise
>> either result in an error or be interpreted as the value for an implied
>> key).
>>
>> Signed-off-by: Kevin Wolf
>> ---
>
>> +++ b/util/keyval.c
>
> Might be nice to see this before the testsuite changes by tweaking the
> git orderfile.
>
>> @@ -166,7 +166,7 @@ static QObject *keyval_parse_put(QDict *cur,
>> * On failure, return NULL.
>> */
>> static const char *keyval_parse_one(QDict *qdict, const char *params,
>> -const char *implied_key,
>> +const char *implied_key, bool *help,
>> Error **errp)
>> {
>> const char *key, *key_end, *s, *end;
>> @@ -179,6 +179,16 @@ static const char *keyval_parse_one(QDict *qdict, const
>> char *params,
>>
>> key = params;
>> len = strcspn(params, "=,");
>> +
>> +if (help && key[len] != '=' && !strncmp(key, "help", len)) {
>
> What if the user typed "help,," to get "help," as the value of the
> implied key?
The value of an implied key cannot contain ','. This is intentional.
test-keyval.c:
/* Implied key with empty value (qemu_opts_parse() accepts this) */
qdict = keyval_parse(",", "implied", );
error_free_or_abort();
g_assert(!qdict);
/* Likewise (qemu_opts_parse(): implied key with comma value) */
qdict = keyval_parse(",,,a=1", "implied", );
error_free_or_abort();
g_assert(!qdict);
Grammar:
* val-no-key = / [^=,]* /
Aside: should be + instead of *. Doc bug. I'll fix it.
[...]
Re: [PATCH 1/4] keyval: Parse help options
Am 30.09.2020 um 15:42 hat Eric Blake geschrieben: > On 9/30/20 8:04 AM, Kevin Wolf wrote: > > Am 29.09.2020 um 19:46 hat Eric Blake geschrieben: > >> On 9/29/20 12:26 PM, Kevin Wolf wrote: > >>> This adds a new parameter 'help' to keyval_parse() that enables parsing > >>> of help options. If NULL is passed, the function behaves the same as > >>> before. But if a bool pointer is given, it contains the information > >>> whether an option "help" without value was given (which would otherwise > >>> either result in an error or be interpreted as the value for an implied > >>> key). > >>> > >>> Signed-off-by: Kevin Wolf > >>> --- > >> > >>> +++ b/util/keyval.c > >> > >> Might be nice to see this before the testsuite changes by tweaking the > >> git orderfile. > > > > What does your git orderfile look like? I don't know how to exclude > > tests/ from file type patterns like *.c. > > You can start with scripts/git.orderfile, and temporarily add: > > # decoding tree specification > *.decode > > +# Key files that I want first for this patch > +util/*.c > + > # code > *.c > > or similar. It's not a show-stopper if you don't, and I concede that > remembering to do it (and then to revert back to the usual afterwords) > is not trivial. Ah, I see. I never did per-patch/series orderfiles, I just have my generic one that does things like headers before implementation, and documentation and QAPI schema changes before both. Kevin signature.asc Description: PGP signature
Re: [PATCH 1/4] keyval: Parse help options
On 9/30/20 8:04 AM, Kevin Wolf wrote: > Am 29.09.2020 um 19:46 hat Eric Blake geschrieben: >> On 9/29/20 12:26 PM, Kevin Wolf wrote: >>> This adds a new parameter 'help' to keyval_parse() that enables parsing >>> of help options. If NULL is passed, the function behaves the same as >>> before. But if a bool pointer is given, it contains the information >>> whether an option "help" without value was given (which would otherwise >>> either result in an error or be interpreted as the value for an implied >>> key). >>> >>> Signed-off-by: Kevin Wolf >>> --- >> >>> +++ b/util/keyval.c >> >> Might be nice to see this before the testsuite changes by tweaking the >> git orderfile. > > What does your git orderfile look like? I don't know how to exclude > tests/ from file type patterns like *.c. You can start with scripts/git.orderfile, and temporarily add: # decoding tree specification *.decode +# Key files that I want first for this patch +util/*.c + # code *.c or similar. It's not a show-stopper if you don't, and I concede that remembering to do it (and then to revert back to the usual afterwords) is not trivial. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [PATCH 1/4] keyval: Parse help options
Am 29.09.2020 um 19:46 hat Eric Blake geschrieben: > On 9/29/20 12:26 PM, Kevin Wolf wrote: > > This adds a new parameter 'help' to keyval_parse() that enables parsing > > of help options. If NULL is passed, the function behaves the same as > > before. But if a bool pointer is given, it contains the information > > whether an option "help" without value was given (which would otherwise > > either result in an error or be interpreted as the value for an implied > > key). > > > > Signed-off-by: Kevin Wolf > > --- > > > +++ b/util/keyval.c > > Might be nice to see this before the testsuite changes by tweaking the > git orderfile. What does your git orderfile look like? I don't know how to exclude tests/ from file type patterns like *.c. Kevin signature.asc Description: PGP signature
Re: [PATCH 1/4] keyval: Parse help options
On 9/29/20 12:26 PM, Kevin Wolf wrote:
> This adds a new parameter 'help' to keyval_parse() that enables parsing
> of help options. If NULL is passed, the function behaves the same as
> before. But if a bool pointer is given, it contains the information
> whether an option "help" without value was given (which would otherwise
> either result in an error or be interpreted as the value for an implied
> key).
>
> Signed-off-by: Kevin Wolf
> ---
> +++ b/util/keyval.c
Might be nice to see this before the testsuite changes by tweaking the
git orderfile.
> @@ -166,7 +166,7 @@ static QObject *keyval_parse_put(QDict *cur,
> * On failure, return NULL.
> */
> static const char *keyval_parse_one(QDict *qdict, const char *params,
> -const char *implied_key,
> +const char *implied_key, bool *help,
> Error **errp)
> {
> const char *key, *key_end, *s, *end;
> @@ -179,6 +179,16 @@ static const char *keyval_parse_one(QDict *qdict, const
> char *params,
>
> key = params;
> len = strcspn(params, "=,");
> +
> +if (help && key[len] != '=' && !strncmp(key, "help", len)) {
What if the user typed "help,," to get "help," as the value of the
implied key?
> +*help = true;
> +s = key + len;
> +if (key[len] != '\0') {
> +s++;
> +}
> +return s;
> +}
> +
> if (implied_key && len && key[len] != '=') {
> /* Desugar implied key */
> key = implied_key;
> @@ -388,21 +398,33 @@ static QObject *keyval_listify(QDict *cur, GSList
> *key_of_cur, Error **errp)
>
> /*
> * Parse @params in QEMU's traditional KEY=VALUE,... syntax.
> + *
> * If @implied_key, the first KEY= can be omitted. @implied_key is
> * implied then, and VALUE can't be empty or contain ',' or '='.
> + *
> + * If @help is given, an option "help" without a value isn't added to
> + * the resulting dictionary, but instead sets @help to true. If no
> + * help option is found, @help is false on return. All other options
> + * are parsed and returned normally so that context specific help can
> + * be printed.
> + *
> * On success, return a dictionary of the parsed keys and values.
> * On failure, store an error through @errp and return NULL.
> */
> QDict *keyval_parse(const char *params, const char *implied_key,
> -Error **errp)
> +bool *help, Error **errp)
> {
> QDict *qdict = qdict_new();
> QObject *listified;
> const char *s;
>
> +if (help) {
> +*help = false;
> +}
> +
> s = params;
> while (*s) {
> -s = keyval_parse_one(qdict, s, implied_key, errp);
> +s = keyval_parse_one(qdict, s, implied_key, help, errp);
> if (!s) {
> qobject_unref(qdict);
> return NULL;
>
I like it, but wonder if you are missing one corner case.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
[PATCH 1/4] keyval: Parse help options
This adds a new parameter 'help' to keyval_parse() that enables parsing
of help options. If NULL is passed, the function behaves the same as
before. But if a bool pointer is given, it contains the information
whether an option "help" without value was given (which would otherwise
either result in an error or be interpreted as the value for an implied
key).
Signed-off-by: Kevin Wolf
---
include/qemu/option.h| 2 +-
qapi/qobject-input-visitor.c | 2 +-
storage-daemon/qemu-storage-daemon.c | 2 +-
tests/test-keyval.c | 157 ---
util/keyval.c| 28 -
5 files changed, 123 insertions(+), 68 deletions(-)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 05e8a15c73..ac69352e0e 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -149,6 +149,6 @@ void qemu_opts_free(QemuOptsList *list);
QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
QDict *keyval_parse(const char *params, const char *implied_key,
-Error **errp);
+bool *help, Error **errp);
#endif
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index f918a05e5f..7b184b50a7 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -757,7 +757,7 @@ Visitor *qobject_input_visitor_new_str(const char *str,
assert(args);
v = qobject_input_visitor_new(QOBJECT(args));
} else {
-args = keyval_parse(str, implied_key, errp);
+args = keyval_parse(str, implied_key, NULL, errp);
if (!args) {
return NULL;
}
diff --git a/storage-daemon/qemu-storage-daemon.c
b/storage-daemon/qemu-storage-daemon.c
index e6157ff518..bb9cb740f0 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -278,7 +278,7 @@ static void process_options(int argc, char *argv[])
}
qemu_opts_del(opts);
-args = keyval_parse(optarg, "qom-type", _fatal);
+args = keyval_parse(optarg, "qom-type", NULL, _fatal);
user_creatable_add_dict(args, true, _fatal);
qobject_unref(args);
break;
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index e331a84149..1ac65c371e 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -27,27 +27,28 @@ static void test_keyval_parse(void)
QDict *qdict, *sub_qdict;
char long_key[129];
char *params;
+bool help;
/* Nothing */
-qdict = keyval_parse("", NULL, _abort);
+qdict = keyval_parse("", NULL, NULL, _abort);
g_assert_cmpuint(qdict_size(qdict), ==, 0);
qobject_unref(qdict);
/* Empty key (qemu_opts_parse() accepts this) */
-qdict = keyval_parse("=val", NULL, );
+qdict = keyval_parse("=val", NULL, NULL, );
error_free_or_abort();
g_assert(!qdict);
/* Empty key fragment */
-qdict = keyval_parse(".", NULL, );
+qdict = keyval_parse(".", NULL, NULL, );
error_free_or_abort();
g_assert(!qdict);
-qdict = keyval_parse("key.", NULL, );
+qdict = keyval_parse("key.", NULL, NULL, );
error_free_or_abort();
g_assert(!qdict);
/* Invalid non-empty key (qemu_opts_parse() doesn't care) */
-qdict = keyval_parse("7up=val", NULL, );
+qdict = keyval_parse("7up=val", NULL, NULL, );
error_free_or_abort();
g_assert(!qdict);
@@ -56,25 +57,25 @@ static void test_keyval_parse(void)
long_key[127] = 'z';
long_key[128] = 0;
params = g_strdup_printf("k.%s=v", long_key);
-qdict = keyval_parse(params + 2, NULL, );
+qdict = keyval_parse(params + 2, NULL, NULL, );
error_free_or_abort();
g_assert(!qdict);
/* Overlong key fragment */
-qdict = keyval_parse(params, NULL, );
+qdict = keyval_parse(params, NULL, NULL, );
error_free_or_abort();
g_assert(!qdict);
g_free(params);
/* Long key (qemu_opts_parse() accepts and truncates silently) */
params = g_strdup_printf("k.%s=v", long_key + 1);
-qdict = keyval_parse(params + 2, NULL, _abort);
+qdict = keyval_parse(params + 2, NULL, NULL, _abort);
g_assert_cmpuint(qdict_size(qdict), ==, 1);
g_assert_cmpstr(qdict_get_try_str(qdict, long_key + 1), ==, "v");
qobject_unref(qdict);
/* Long key fragment */
-qdict = keyval_parse(params, NULL, _abort);
+qdict = keyval_parse(params, NULL, NULL, _abort);
g_assert_cmpuint(qdict_size(qdict), ==, 1);
sub_qdict = qdict_get_qdict(qdict, "k");
g_assert(sub_qdict);
@@ -84,25 +85,25 @@ static void test_keyval_parse(void)
g_free(params);
/* Crap after valid key */
-qdict = keyval_parse("key[0]=val", NULL, );
+qdict = keyval_parse("key[0]=val", NULL, NULL, );
error_free_or_abort();
g_assert(!qdict);
/* Multiple keys, last one wins */
-qdict = keyval_parse("a=1,b=2,,x,a=3",
