diff mbox series

[v2,1/4] keyval: Parse help options

Message ID 20200930124557.51835-2-kwolf@redhat.com
State New
Headers show
Series qemu-storage-daemon: Remove QemuOpts from --object parser | expand

Commit Message

Kevin Wolf Sept. 30, 2020, 12:45 p.m. UTC 
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 <kwolf@redhat.com>
---
 include/qemu/option.h                |   2 +-
 qapi/qobject-input-visitor.c         |   2 +-
 storage-daemon/qemu-storage-daemon.c |   2 +-
 tests/test-keyval.c                  | 205 +++++++++++++++++++--------
 util/keyval.c                        |  38 ++++-
 5 files changed, 179 insertions(+), 70 deletions(-)

Comments

Eric Blake Sept. 30, 2020, 1:35 p.m. UTC | #1 
On 9/30/20 7:45 AM, 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 <kwolf@redhat.com>

> ---


> +

> +    /* "help" is only a help option if it has no value */

> +    qdict = keyval_parse("help=on", NULL, &help, &error_abort);

> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);

> +    g_assert_cmpstr(qdict_get_try_str(qdict, "help"), ==, "on");

> +    g_assert_false(help);

> +    qobject_unref(qdict);

> +

> +    /* Double comma after "help" in an implied key is not a help option */

> +    qdict = keyval_parse("help,,abc", "implied", &help, &error_abort);

> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);

> +    g_assert_false(help);

> +    qobject_unref(qdict);


Worth checking qdict_get_try_str(qdict, "implied") for "help,abc"?

> +

> +    /* Without implied key and without value, it's an error */

> +    qdict = keyval_parse("help,,abc", NULL, &help, &err);

> +    error_free_or_abort(&err);

> +    g_assert(!qdict);

> +

> +    /* "help" as the only option */

> +    qdict = keyval_parse("help", NULL, &help, &error_abort);

> +    g_assert_cmpuint(qdict_size(qdict), ==, 0);

> +    g_assert_true(help);

> +    qobject_unref(qdict);

> +

> +    /* "help" as the first part of the key */

> +    qdict = keyval_parse("help.abc", NULL, &help, &err);

> +    error_free_or_abort(&err);

> +    g_assert(!qdict);


Worth checking qdict_get_try_str(qdict, "help.abc") for "on"? (at least,
that's my guess as what happened)

> +

> +    /* "help" as the last part of the key */

> +    qdict = keyval_parse("abc.help", NULL, &help, &err);

> +    error_free_or_abort(&err);

> +    g_assert(!qdict);


[1]

> +

> +    /* "help" is not a value for the implied key if &help is given */

> +    qdict = keyval_parse("help", "implied", &help, &error_abort);

> +    g_assert_cmpuint(qdict_size(qdict), ==, 0);

> +    g_assert_true(help);

> +    qobject_unref(qdict);


Worth checking that the qdict does not contain "implied"?  Perhaps by
checking qdict_size() == 0?

> +

> +    /* "help" is a value for the implied key when passing NULL for help */

> +    qdict = keyval_parse("help", "implied", NULL, &error_abort);

> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);

> +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help");

> +    qobject_unref(qdict);

> +

> +    /* "help.abc" is a value for the implied key */

> +    qdict = keyval_parse("help.abc", "implied", &help, &err);

> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);

> +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help.abc");

> +    g_assert_false(help);

> +    qobject_unref(qdict);

> +

> +    /* "abc.help" is a value for the implied key */

> +    qdict = keyval_parse("abc.help", "implied", &help, &err);

> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);

> +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "abc.help");

> +    g_assert_false(help);

> +    qobject_unref(qdict);

> +

> +    /* "help" as the last part of the key */

> +    qdict = keyval_parse("abc.help", NULL, &help, &err);

> +    error_free_or_abort(&err);

> +    g_assert(!qdict);


duplicates [1]

> +

> +    /* Other keys before and after help are still parsed normally */

> +    qdict = keyval_parse("number=42,help,foo=bar", NULL, &help, &error_abort);

> +    g_assert_cmpuint(qdict_size(qdict), ==, 2);

> +    g_assert_cmpstr(qdict_get_try_str(qdict, "number"), ==, "42");

> +    g_assert_cmpstr(qdict_get_try_str(qdict, "foo"), ==, "bar");

> +    g_assert_true(help);

> +    qobject_unref(qdict);

> +

> +    /* ...even with an implied key */

> +    qdict = keyval_parse("val,help,foo=bar", "implied", &help, &error_abort);

> +    g_assert_cmpuint(qdict_size(qdict), ==, 2);

> +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "val");

> +    g_assert_cmpstr(qdict_get_try_str(qdict, "foo"), ==, "bar");

> +    g_assert_true(help);

> +    qobject_unref(qdict);

>  }

>  


Overall a nice set of additions.  You could tweak it further, but I'm no
longer seeing a hole like last time.

> +++ b/util/keyval.c

> @@ -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;

> @@ -238,13 +238,20 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,

>      if (key == implied_key) {

>          assert(!*s);

>          s = params;

> +    } else if (*s == '=') {

> +        s++;

>      } else {

> -        if (*s != '=') {

> +        if (help && !strncmp(key, "help", s - key)) {


Should this use is_help_option() to also accept "?", or are we okay
demanding exactly "help"?

> +            *help = true;

> +            if (*s) {

> +                s++;

> +            }

> +            return s;

> +        } else {

>              error_setg(errp, "Expected '=' after parameter '%.*s'",

>                         (int)(s - key), key);

>              return NULL;

>          }

> -        s++;

>      }


Assuming you can touch up the testsuite before the pull request, and
assuming we don't care about "?",

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org
Kevin Wolf Sept. 30, 2020, 3:10 p.m. UTC | #2 
Am 30.09.2020 um 15:35 hat Eric Blake geschrieben:
> On 9/30/20 7:45 AM, 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 <kwolf@redhat.com>

> > ---

> 

> > +

> > +    /* "help" is only a help option if it has no value */

> > +    qdict = keyval_parse("help=on", NULL, &help, &error_abort);

> > +    g_assert_cmpuint(qdict_size(qdict), ==, 1);

> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "help"), ==, "on");

> > +    g_assert_false(help);

> > +    qobject_unref(qdict);

> > +

> > +    /* Double comma after "help" in an implied key is not a help option */

> > +    qdict = keyval_parse("help,,abc", "implied", &help, &error_abort);

> > +    g_assert_cmpuint(qdict_size(qdict), ==, 1);

> > +    g_assert_false(help);

> > +    qobject_unref(qdict);

> 

> Worth checking qdict_get_try_str(qdict, "implied") for "help,abc"?


Yes, this makes sense.

> > +

> > +    /* Without implied key and without value, it's an error */

> > +    qdict = keyval_parse("help,,abc", NULL, &help, &err);

> > +    error_free_or_abort(&err);

> > +    g_assert(!qdict);

> > +

> > +    /* "help" as the only option */

> > +    qdict = keyval_parse("help", NULL, &help, &error_abort);

> > +    g_assert_cmpuint(qdict_size(qdict), ==, 0);

> > +    g_assert_true(help);

> > +    qobject_unref(qdict);

> > +

> > +    /* "help" as the first part of the key */

> > +    qdict = keyval_parse("help.abc", NULL, &help, &err);

> > +    error_free_or_abort(&err);

> > +    g_assert(!qdict);

> 

> Worth checking qdict_get_try_str(qdict, "help.abc") for "on"? (at least,

> that's my guess as what happened)


The keyval parser doesn't support boolean options like this (I think
this is intentional because it would be ambiguous with implied keys).

This is an error case, and I don't think the QDict has defined content
then. But if anything, we would want to check that it's empty.

> > +

> > +    /* "help" as the last part of the key */

> > +    qdict = keyval_parse("abc.help", NULL, &help, &err);

> > +    error_free_or_abort(&err);

> > +    g_assert(!qdict);

> 

> [1]

> 

> > +

> > +    /* "help" is not a value for the implied key if &help is given */

> > +    qdict = keyval_parse("help", "implied", &help, &error_abort);

> > +    g_assert_cmpuint(qdict_size(qdict), ==, 0);

> > +    g_assert_true(help);

> > +    qobject_unref(qdict);

> 

> Worth checking that the qdict does not contain "implied"?  Perhaps by

> checking qdict_size() == 0?


Already there, the first line after keyval_parse(). :-)

> > +

> > +    /* "help" is a value for the implied key when passing NULL for help */

> > +    qdict = keyval_parse("help", "implied", NULL, &error_abort);

> > +    g_assert_cmpuint(qdict_size(qdict), ==, 1);

> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help");

> > +    qobject_unref(qdict);

> > +

> > +    /* "help.abc" is a value for the implied key */

> > +    qdict = keyval_parse("help.abc", "implied", &help, &err);

> > +    g_assert_cmpuint(qdict_size(qdict), ==, 1);

> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help.abc");

> > +    g_assert_false(help);

> > +    qobject_unref(qdict);

> > +

> > +    /* "abc.help" is a value for the implied key */

> > +    qdict = keyval_parse("abc.help", "implied", &help, &err);

> > +    g_assert_cmpuint(qdict_size(qdict), ==, 1);

> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "abc.help");

> > +    g_assert_false(help);

> > +    qobject_unref(qdict);

> > +

> > +    /* "help" as the last part of the key */

> > +    qdict = keyval_parse("abc.help", NULL, &help, &err);

> > +    error_free_or_abort(&err);

> > +    g_assert(!qdict);

> 

> duplicates [1]


So we can be extra sure!

Somehow I suspect I wanted to test another case here, but I'm not sure
which one. Maybe the same with an implied key? Or I can just remove it.

> > +

> > +    /* Other keys before and after help are still parsed normally */

> > +    qdict = keyval_parse("number=42,help,foo=bar", NULL, &help, &error_abort);

> > +    g_assert_cmpuint(qdict_size(qdict), ==, 2);

> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "number"), ==, "42");

> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "foo"), ==, "bar");

> > +    g_assert_true(help);

> > +    qobject_unref(qdict);

> > +

> > +    /* ...even with an implied key */

> > +    qdict = keyval_parse("val,help,foo=bar", "implied", &help, &error_abort);

> > +    g_assert_cmpuint(qdict_size(qdict), ==, 2);

> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "val");

> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "foo"), ==, "bar");

> > +    g_assert_true(help);

> > +    qobject_unref(qdict);

> >  }

> >  

> 

> Overall a nice set of additions.  You could tweak it further, but I'm no

> longer seeing a hole like last time.

> 

> > +++ b/util/keyval.c

> > @@ -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;

> > @@ -238,13 +238,20 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,

> >      if (key == implied_key) {

> >          assert(!*s);

> >          s = params;

> > +    } else if (*s == '=') {

> > +        s++;

> >      } else {

> > -        if (*s != '=') {

> > +        if (help && !strncmp(key, "help", s - key)) {

> 

> Should this use is_help_option() to also accept "?", or are we okay

> demanding exactly "help"?


The comment for is_help_option() calls "?" deprecated, so I think we
don't want to enable it in a new parser.

> > +            *help = true;

> > +            if (*s) {

> > +                s++;

> > +            }

> > +            return s;

> > +        } else {

> >              error_setg(errp, "Expected '=' after parameter '%.*s'",

> >                         (int)(s - key), key);

> >              return NULL;

> >          }

> > -        s++;

> >      }

> 

> Assuming you can touch up the testsuite before the pull request, and

> assuming we don't care about "?",

> 

> Reviewed-by: Eric Blake <eblake@redhat.com>


Thanks!

Kevin
Markus Armbruster Oct. 1, 2020, 10:34 a.m. UTC | #3 
Kevin Wolf <kwolf@redhat.com> writes:

> Am 30.09.2020 um 15:35 hat Eric Blake geschrieben:

>> On 9/30/20 7:45 AM, 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 <kwolf@redhat.com>

[...]
>> > +++ b/util/keyval.c

>> > @@ -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;

>> > @@ -238,13 +238,20 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,

>> >      if (key == implied_key) {

>> >          assert(!*s);

>> >          s = params;

>> > +    } else if (*s == '=') {

>> > +        s++;

>> >      } else {

>> > -        if (*s != '=') {

>> > +        if (help && !strncmp(key, "help", s - key)) {

>> 

>> Should this use is_help_option() to also accept "?", or are we okay

>> demanding exactly "help"?

>

> The comment for is_help_option() calls "?" deprecated, so I think we

> don't want to enable it in a new parser.


Valid point.

But do we really want comparisons for "help" inline everywhere we want
to check for non-deprecated help requests?  Would a common helper
function be better?

On the deprecation of "?": the comment is more than eight years old
(commit c8057f951d6).  We didn't have a deprecation process back then,
but we did purge '?' from the documentation.  Can we finally drop it?
I'm going to ask that in a new thread.

[...]
Kevin Wolf Oct. 1, 2020, 11:33 a.m. UTC | #4 
Am 01.10.2020 um 12:34 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:

> 

> > Am 30.09.2020 um 15:35 hat Eric Blake geschrieben:

> >> On 9/30/20 7:45 AM, 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 <kwolf@redhat.com>

> [...]

> >> > +++ b/util/keyval.c

> >> > @@ -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;

> >> > @@ -238,13 +238,20 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,

> >> >      if (key == implied_key) {

> >> >          assert(!*s);

> >> >          s = params;

> >> > +    } else if (*s == '=') {

> >> > +        s++;

> >> >      } else {

> >> > -        if (*s != '=') {

> >> > +        if (help && !strncmp(key, "help", s - key)) {

> >> 

> >> Should this use is_help_option() to also accept "?", or are we okay

> >> demanding exactly "help"?

> >

> > The comment for is_help_option() calls "?" deprecated, so I think we

> > don't want to enable it in a new parser.

> 

> Valid point.

> 

> But do we really want comparisons for "help" inline everywhere we want

> to check for non-deprecated help requests?  Would a common helper

> function be better?


How many more parsers are we going to add? I thought we intended the
keyval parser to be the one canoncial place in the long term?

If you prefer, I can make this a second static inline function in
include/qemu/help_option.h (that would have to work both as strcmp and
as strncmp), but I don't see a second user that would make it a "common"
helper.

> On the deprecation of "?": the comment is more than eight years old

> (commit c8057f951d6).  We didn't have a deprecation process back then,

> but we did purge '?' from the documentation.  Can we finally drop it?

> I'm going to ask that in a new thread.


I guess we can.

Kevin
Markus Armbruster Oct. 1, 2020, 3:46 p.m. UTC | #5 
Fried brain today, I have to go through this real slow.  I apologize in
advance for being denser and more error-prone than usual.

Kevin Wolf <kwolf@redhat.com> writes:

> 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 <kwolf@redhat.com>


You change the parser, but neglected to update the grammar in the big
file comment.

I made the mistake of attempting to review the parser change without
fixing up the grammar first, and got lost in the weeds.  In part because
today is not my day, but also because doing parsers without a grammar
never works out well for me.

Grammar from the big file comment:

 * KEY=VALUE,... syntax:
 *
 *   key-vals     = [ key-val { ',' key-val } [ ',' ] ]
 *   key-val      = key '=' val
 *   key          = key-fragment { '.' key-fragment }
 *   key-fragment = / [^=,.]* /
 *   val          = { / [^,]* / | ',,' }

and

 * Additional syntax for use with an implied key:
 *
 *   key-vals-ik  = val-no-key [ ',' key-vals ]
 *   val-no-key   = / [^=,]* /
 *
 * where no-key is syntactic sugar for implied-key=val-no-key.

According to the commit message, we want to recognize "help" in place of
key-val and val-no-key, but only for callers that opt in.

This is more complex than recognizing it in place of just key-vals.  The
commit message doesn't say why the extra complexity is wanted.  Peeking
ahead in the series, I can see it's for supporting

    -object TYPE,help

in addition to

    -object help

I see.

Making help support opt-in complicates things.  Is there a genuine use
for not supporting help?  Or is just to keep the users that don't
support help yet (but should) working without change?  Mind, I'm not
asking you to make them work, I'm only asking whether you can think of a
genuine case where help should not work.

What are the existing users that don't support help?  Let's see... many
in tests/test-keyval.c (ignore), and qobject_input_visitor_new_str().
That one's used for qemu-system-FOO -audiodev, -display, -blockdev, and
for qemu-storage-daemon --blockdev, --export, --monitor, --nbd-server.

Attempting to get help for them fails like this:

    $ bld-x86/storage-daemon/qemu-storage-daemon --blockdev help
    qemu-storage-daemon: Invalid parameter 'help'
    $ bld-x86/storage-daemon/qemu-storage-daemon --blockdev file,help
    qemu-storage-daemon: Expected '=' after parameter 'help'

I believe making them fail like

    qemu-storage-daemon: no help available

would actually be an improvement.

If we get rid of the case "help is not supported", the grammar update
isn't too bad, something like

 *   key-val      = 'help' | key '=' val

and

 *   key-vals-ik  = 'help' | val-no-key [ ',' key-vals ]

Done for today, hope my brain unfries itself overnight.

[...]
Markus Armbruster Oct. 5, 2020, 12:08 p.m. UTC | #6 
Markus Armbruster <armbru@redhat.com> writes:

[...]
> Making help support opt-in complicates things.  Is there a genuine use

> for not supporting help?  Or is just to keep the users that don't

> support help yet (but should) working without change?  Mind, I'm not

> asking you to make them work, I'm only asking whether you can think of a

> genuine case where help should not work.

>

> What are the existing users that don't support help?  Let's see... many

> in tests/test-keyval.c (ignore), and qobject_input_visitor_new_str().

> That one's used for qemu-system-FOO -audiodev, -display, -blockdev, and

> for qemu-storage-daemon --blockdev, --export, --monitor, --nbd-server.

>

> Attempting to get help for them fails like this:

>

>     $ bld-x86/storage-daemon/qemu-storage-daemon --blockdev help

>     qemu-storage-daemon: Invalid parameter 'help'

>     $ bld-x86/storage-daemon/qemu-storage-daemon --blockdev file,help

>     qemu-storage-daemon: Expected '=' after parameter 'help'

>

> I believe making them fail like

>

>     qemu-storage-daemon: no help available

>

> would actually be an improvement.


Potential issue: if an option's implied key may have the value "help",
then option argument "help" can be either parsed as "give me help", or
as "implied-key=help".

None of the existing options have this issue:

* audiodev: union Audiodev, implied key "driver" is enum AudiodevDriver,
   "help" is not among its values

* display: union DisplayOptions, implied key "type" is enum
   DisplayType, "help" is not among its values

* blockdev: union BlockdevOptions, implied key "driver is enum
   BlockdevDriver, "help" is not among its values

* export: union BlockExport, implied key "type" is enum BlockExportType,
  "help" is not among its values

* monitor: struct MonitorOptions, implied key "mode"is enum MonitorMode,
  "help" is not among its values

* nbd-server: struct NbdServerOptions, no implied key.

We're good.

What's the risk of not staying good?

The implied keys above are all QAPI enums.

The only existing QAPI enum with a value 'help' is QKeyCode.

The only existing C enum with a name that ends in _HELP is the one
defining Mac keycodes in hw/input/adb-keys.h.

For completeness, I double-checked non of the existing occurences of
string "help" are used as values of option parameters.

We'll almost certainly stay good.

What if we manage to mess this up against all odds?

In my opinion, consistency in getting help is more important than
consistency within a badly designed option: option argument "help"
should give you help even when that means you can't omit the implied key
when its value is "help".

[...]
diff mbox series

Patch

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", &error_fatal);
+                args = keyval_parse(optarg, "qom-type", NULL, &error_fatal);
                 user_creatable_add_dict(args, true, &error_fatal);
                 qobject_unref(args);
                 break;
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index e331a84149..83b65f04f7 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, &error_abort);
+    qdict = keyval_parse("", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 0);
     qobject_unref(qdict);
 
     /* Empty key (qemu_opts_parse() accepts this) */
-    qdict = keyval_parse("=val", NULL, &err);
+    qdict = keyval_parse("=val", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Empty key fragment */
-    qdict = keyval_parse(".", NULL, &err);
+    qdict = keyval_parse(".", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
-    qdict = keyval_parse("key.", NULL, &err);
+    qdict = keyval_parse("key.", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Invalid non-empty key (qemu_opts_parse() doesn't care) */
-    qdict = keyval_parse("7up=val", NULL, &err);
+    qdict = keyval_parse("7up=val", NULL, NULL, &err);
     error_free_or_abort(&err);
     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, &err);
+    qdict = keyval_parse(params + 2, NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Overlong key fragment */
-    qdict = keyval_parse(params, NULL, &err);
+    qdict = keyval_parse(params, NULL, NULL, &err);
     error_free_or_abort(&err);
     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, &error_abort);
+    qdict = keyval_parse(params + 2, NULL, NULL, &error_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, &error_abort);
+    qdict = keyval_parse(params, NULL, NULL, &error_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, &err);
+    qdict = keyval_parse("key[0]=val", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Multiple keys, last one wins */
-    qdict = keyval_parse("a=1,b=2,,x,a=3", NULL, &error_abort);
+    qdict = keyval_parse("a=1,b=2,,x,a=3", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 2);
     g_assert_cmpstr(qdict_get_try_str(qdict, "a"), ==, "3");
     g_assert_cmpstr(qdict_get_try_str(qdict, "b"), ==, "2,x");
     qobject_unref(qdict);
 
     /* Even when it doesn't in qemu_opts_parse() */
-    qdict = keyval_parse("id=foo,id=bar", NULL, &error_abort);
+    qdict = keyval_parse("id=foo,id=bar", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 1);
     g_assert_cmpstr(qdict_get_try_str(qdict, "id"), ==, "bar");
     qobject_unref(qdict);
 
     /* Dotted keys */
-    qdict = keyval_parse("a.b.c=1,a.b.c=2,d=3", NULL, &error_abort);
+    qdict = keyval_parse("a.b.c=1,a.b.c=2,d=3", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 2);
     sub_qdict = qdict_get_qdict(qdict, "a");
     g_assert(sub_qdict);
@@ -115,48 +116,48 @@  static void test_keyval_parse(void)
     qobject_unref(qdict);
 
     /* Inconsistent dotted keys */
-    qdict = keyval_parse("a.b=1,a=2", NULL, &err);
+    qdict = keyval_parse("a.b=1,a=2", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
-    qdict = keyval_parse("a.b=1,a.b.c=2", NULL, &err);
+    qdict = keyval_parse("a.b=1,a.b.c=2", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Trailing comma is ignored */
-    qdict = keyval_parse("x=y,", NULL, &error_abort);
+    qdict = keyval_parse("x=y,", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 1);
     g_assert_cmpstr(qdict_get_try_str(qdict, "x"), ==, "y");
     qobject_unref(qdict);
 
     /* Except when it isn't */
-    qdict = keyval_parse(",", NULL, &err);
+    qdict = keyval_parse(",", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Value containing ,id= not misinterpreted as qemu_opts_parse() does */
-    qdict = keyval_parse("x=,,id=bar", NULL, &error_abort);
+    qdict = keyval_parse("x=,,id=bar", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 1);
     g_assert_cmpstr(qdict_get_try_str(qdict, "x"), ==, ",id=bar");
     qobject_unref(qdict);
 
     /* Anti-social ID is left to caller (qemu_opts_parse() rejects it) */
-    qdict = keyval_parse("id=666", NULL, &error_abort);
+    qdict = keyval_parse("id=666", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 1);
     g_assert_cmpstr(qdict_get_try_str(qdict, "id"), ==, "666");
     qobject_unref(qdict);
 
     /* Implied value not supported (unlike qemu_opts_parse()) */
-    qdict = keyval_parse("an,noaus,noaus=", NULL, &err);
+    qdict = keyval_parse("an,noaus,noaus=", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Implied value, key "no" (qemu_opts_parse(): negated empty key) */
-    qdict = keyval_parse("no", NULL, &err);
+    qdict = keyval_parse("no", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Implied key */
-    qdict = keyval_parse("an,aus=off,noaus=", "implied", &error_abort);
+    qdict = keyval_parse("an,aus=off,noaus=", "implied", NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 3);
     g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "an");
     g_assert_cmpstr(qdict_get_try_str(qdict, "aus"), ==, "off");
@@ -164,7 +165,7 @@  static void test_keyval_parse(void)
     qobject_unref(qdict);
 
     /* Implied dotted key */
-    qdict = keyval_parse("val", "eins.zwei", &error_abort);
+    qdict = keyval_parse("val", "eins.zwei", NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 1);
     sub_qdict = qdict_get_qdict(qdict, "eins");
     g_assert(sub_qdict);
@@ -173,19 +174,100 @@  static void test_keyval_parse(void)
     qobject_unref(qdict);
 
     /* Implied key with empty value (qemu_opts_parse() accepts this) */
-    qdict = keyval_parse(",", "implied", &err);
+    qdict = keyval_parse(",", "implied", NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Likewise (qemu_opts_parse(): implied key with comma value) */
-    qdict = keyval_parse(",,,a=1", "implied", &err);
+    qdict = keyval_parse(",,,a=1", "implied", NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Empty key is not an implied key */
-    qdict = keyval_parse("=val", "implied", &err);
+    qdict = keyval_parse("=val", "implied", NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
+
+    /* "help" is only a help option if it has no value */
+    qdict = keyval_parse("help=on", NULL, &help, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "help"), ==, "on");
+    g_assert_false(help);
+    qobject_unref(qdict);
+
+    /* Double comma after "help" in an implied key is not a help option */
+    qdict = keyval_parse("help,,abc", "implied", &help, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_false(help);
+    qobject_unref(qdict);
+
+    /* Without implied key and without value, it's an error */
+    qdict = keyval_parse("help,,abc", NULL, &help, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* "help" as the only option */
+    qdict = keyval_parse("help", NULL, &help, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 0);
+    g_assert_true(help);
+    qobject_unref(qdict);
+
+    /* "help" as the first part of the key */
+    qdict = keyval_parse("help.abc", NULL, &help, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* "help" as the last part of the key */
+    qdict = keyval_parse("abc.help", NULL, &help, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* "help" is not a value for the implied key if &help is given */
+    qdict = keyval_parse("help", "implied", &help, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 0);
+    g_assert_true(help);
+    qobject_unref(qdict);
+
+    /* "help" is a value for the implied key when passing NULL for help */
+    qdict = keyval_parse("help", "implied", NULL, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help");
+    qobject_unref(qdict);
+
+    /* "help.abc" is a value for the implied key */
+    qdict = keyval_parse("help.abc", "implied", &help, &err);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help.abc");
+    g_assert_false(help);
+    qobject_unref(qdict);
+
+    /* "abc.help" is a value for the implied key */
+    qdict = keyval_parse("abc.help", "implied", &help, &err);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "abc.help");
+    g_assert_false(help);
+    qobject_unref(qdict);
+
+    /* "help" as the last part of the key */
+    qdict = keyval_parse("abc.help", NULL, &help, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Other keys before and after help are still parsed normally */
+    qdict = keyval_parse("number=42,help,foo=bar", NULL, &help, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 2);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "number"), ==, "42");
+    g_assert_cmpstr(qdict_get_try_str(qdict, "foo"), ==, "bar");
+    g_assert_true(help);
+    qobject_unref(qdict);
+
+    /* ...even with an implied key */
+    qdict = keyval_parse("val,help,foo=bar", "implied", &help, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 2);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "val");
+    g_assert_cmpstr(qdict_get_try_str(qdict, "foo"), ==, "bar");
+    g_assert_true(help);
+    qobject_unref(qdict);
 }
 
 static void check_list012(QList *qlist)
@@ -210,26 +292,26 @@  static void test_keyval_parse_list(void)
     QDict *qdict, *sub_qdict;
 
     /* Root can't be a list */
-    qdict = keyval_parse("0=1", NULL, &err);
+    qdict = keyval_parse("0=1", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* List elements need not be in order */
-    qdict = keyval_parse("list.0=null,list.2=zwei,list.1=eins",
-                         NULL, &error_abort);
+    qdict = keyval_parse("list.0=null,list.2=zwei,list.1=eins", NULL, NULL,
+                         &error_abort);
     g_assert_cmpint(qdict_size(qdict), ==, 1);
     check_list012(qdict_get_qlist(qdict, "list"));
     qobject_unref(qdict);
 
     /* Multiple indexes, last one wins */
     qdict = keyval_parse("list.1=goner,list.0=null,list.01=eins,list.2=zwei",
-                         NULL, &error_abort);
+                         NULL, NULL, &error_abort);
     g_assert_cmpint(qdict_size(qdict), ==, 1);
     check_list012(qdict_get_qlist(qdict, "list"));
     qobject_unref(qdict);
 
     /* List at deeper nesting */
-    qdict = keyval_parse("a.list.1=eins,a.list.00=null,a.list.2=zwei",
+    qdict = keyval_parse("a.list.1=eins,a.list.00=null,a.list.2=zwei", NULL,
                          NULL, &error_abort);
     g_assert_cmpint(qdict_size(qdict), ==, 1);
     sub_qdict = qdict_get_qdict(qdict, "a");
@@ -238,18 +320,19 @@  static void test_keyval_parse_list(void)
     qobject_unref(qdict);
 
     /* Inconsistent dotted keys: both list and dictionary */
-    qdict = keyval_parse("a.b.c=1,a.b.0=2", NULL, &err);
+    qdict = keyval_parse("a.b.c=1,a.b.0=2", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
-    qdict = keyval_parse("a.0.c=1,a.b.c=2", NULL, &err);
+    qdict = keyval_parse("a.0.c=1,a.b.c=2", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Missing list indexes */
-    qdict = keyval_parse("list.1=lonely", NULL, &err);
+    qdict = keyval_parse("list.1=lonely", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
-    qdict = keyval_parse("list.0=null,list.2=eins,list.02=zwei", NULL, &err);
+    qdict = keyval_parse("list.0=null,list.2=eins,list.02=zwei", NULL, NULL,
+                         &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 }
@@ -261,7 +344,7 @@  static void test_keyval_visit_bool(void)
     QDict *qdict;
     bool b;
 
-    qdict = keyval_parse("bool1=on,bool2=off", NULL, &error_abort);
+    qdict = keyval_parse("bool1=on,bool2=off", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -273,7 +356,7 @@  static void test_keyval_visit_bool(void)
     visit_end_struct(v, NULL);
     visit_free(v);
 
-    qdict = keyval_parse("bool1=offer", NULL, &error_abort);
+    qdict = keyval_parse("bool1=offer", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -291,7 +374,7 @@  static void test_keyval_visit_number(void)
     uint64_t u;
 
     /* Lower limit zero */
-    qdict = keyval_parse("number1=0", NULL, &error_abort);
+    qdict = keyval_parse("number1=0", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -302,7 +385,7 @@  static void test_keyval_visit_number(void)
     visit_free(v);
 
     /* Upper limit 2^64-1 */
-    qdict = keyval_parse("number1=18446744073709551615,number2=-1",
+    qdict = keyval_parse("number1=18446744073709551615,number2=-1", NULL,
                          NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
@@ -316,8 +399,8 @@  static void test_keyval_visit_number(void)
     visit_free(v);
 
     /* Above upper limit */
-    qdict = keyval_parse("number1=18446744073709551616",
-                         NULL, &error_abort);
+    qdict = keyval_parse("number1=18446744073709551616", NULL, NULL,
+                         &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -327,8 +410,8 @@  static void test_keyval_visit_number(void)
     visit_free(v);
 
     /* Below lower limit */
-    qdict = keyval_parse("number1=-18446744073709551616",
-                         NULL, &error_abort);
+    qdict = keyval_parse("number1=-18446744073709551616", NULL, NULL,
+                         &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -338,8 +421,7 @@  static void test_keyval_visit_number(void)
     visit_free(v);
 
     /* Hex and octal */
-    qdict = keyval_parse("number1=0x2a,number2=052",
-                         NULL, &error_abort);
+    qdict = keyval_parse("number1=0x2a,number2=052", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -352,8 +434,7 @@  static void test_keyval_visit_number(void)
     visit_free(v);
 
     /* Trailing crap */
-    qdict = keyval_parse("number1=3.14,number2=08",
-                         NULL, &error_abort);
+    qdict = keyval_parse("number1=3.14,number2=08", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -373,7 +454,7 @@  static void test_keyval_visit_size(void)
     uint64_t sz;
 
     /* Lower limit zero */
-    qdict = keyval_parse("sz1=0", NULL, &error_abort);
+    qdict = keyval_parse("sz1=0", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -389,7 +470,7 @@  static void test_keyval_visit_size(void)
     qdict = keyval_parse("sz1=9007199254740991,"
                          "sz2=9007199254740992,"
                          "sz3=9007199254740993",
-                         NULL, &error_abort);
+                         NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -406,7 +487,7 @@  static void test_keyval_visit_size(void)
     /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
     qdict = keyval_parse("sz1=9223372036854774784," /* 7ffffffffffffc00 */
                          "sz2=9223372036854775295", /* 7ffffffffffffdff */
-                         NULL, &error_abort);
+                         NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -421,7 +502,7 @@  static void test_keyval_visit_size(void)
     /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */
     qdict = keyval_parse("sz1=18446744073709549568," /* fffffffffffff800 */
                          "sz2=18446744073709550591", /* fffffffffffffbff */
-                         NULL, &error_abort);
+                         NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -436,7 +517,7 @@  static void test_keyval_visit_size(void)
     /* Beyond limits */
     qdict = keyval_parse("sz1=-1,"
                          "sz2=18446744073709550592", /* fffffffffffffc00 */
-                         NULL, &error_abort);
+                         NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -449,7 +530,7 @@  static void test_keyval_visit_size(void)
 
     /* Suffixes */
     qdict = keyval_parse("sz1=8b,sz2=1.5k,sz3=2M,sz4=0.1G,sz5=16777215T",
-                         NULL, &error_abort);
+                         NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -468,7 +549,7 @@  static void test_keyval_visit_size(void)
     visit_free(v);
 
     /* Beyond limit with suffix */
-    qdict = keyval_parse("sz1=16777216T", NULL, &error_abort);
+    qdict = keyval_parse("sz1=16777216T", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -478,7 +559,7 @@  static void test_keyval_visit_size(void)
     visit_free(v);
 
     /* Trailing crap */
-    qdict = keyval_parse("sz1=0Z,sz2=16Gi", NULL, &error_abort);
+    qdict = keyval_parse("sz1=0Z,sz2=16Gi", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -497,7 +578,7 @@  static void test_keyval_visit_dict(void)
     QDict *qdict;
     int64_t i;
 
-    qdict = keyval_parse("a.b.c=1,a.b.c=2,d=3", NULL, &error_abort);
+    qdict = keyval_parse("a.b.c=1,a.b.c=2,d=3", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -515,7 +596,7 @@  static void test_keyval_visit_dict(void)
     visit_end_struct(v, NULL);
     visit_free(v);
 
-    qdict = keyval_parse("a.b=", NULL, &error_abort);
+    qdict = keyval_parse("a.b=", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -537,7 +618,7 @@  static void test_keyval_visit_list(void)
     QDict *qdict;
     char *s;
 
-    qdict = keyval_parse("a.0=,a.1=I,a.2.0=II", NULL, &error_abort);
+    qdict = keyval_parse("a.0=,a.1=I,a.2.0=II", NULL, NULL, &error_abort);
     /* TODO empty list */
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
@@ -561,7 +642,7 @@  static void test_keyval_visit_list(void)
     visit_end_struct(v, NULL);
     visit_free(v);
 
-    qdict = keyval_parse("a.0=,b.0.0=head", NULL, &error_abort);
+    qdict = keyval_parse("a.0=,b.0.0=head", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -590,7 +671,7 @@  static void test_keyval_visit_optional(void)
     bool present;
     int64_t i;
 
-    qdict = keyval_parse("a.b=1", NULL, &error_abort);
+    qdict = keyval_parse("a.b=1", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -626,7 +707,7 @@  static void test_keyval_visit_alternate(void)
      * the string variant if there is one, else an error.
      * TODO make it work for unambiguous cases like AltEnumBool below
      */
-    qdict = keyval_parse("a=1,b=2,c=on", NULL, &error_abort);
+    qdict = keyval_parse("a=1,b=2,c=on", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -650,7 +731,7 @@  static void test_keyval_visit_any(void)
     QList *qlist;
     QString *qstr;
 
-    qdict = keyval_parse("a.0=null,a.1=1", NULL, &error_abort);
+    qdict = keyval_parse("a.0=null,a.1=1", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
diff --git a/util/keyval.c b/util/keyval.c
index 13def4af54..c8ce3bb6c9 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -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;
@@ -238,13 +238,20 @@  static const char *keyval_parse_one(QDict *qdict, const char *params,
     if (key == implied_key) {
         assert(!*s);
         s = params;
+    } else if (*s == '=') {
+        s++;
     } else {
-        if (*s != '=') {
+        if (help && !strncmp(key, "help", s - key)) {
+            *help = true;
+            if (*s) {
+                s++;
+            }
+            return s;
+        } else {
             error_setg(errp, "Expected '=' after parameter '%.*s'",
                        (int)(s - key), key);
             return NULL;
         }
-        s++;
     }
 
     val = qstring_new();
@@ -260,6 +267,15 @@  static const char *keyval_parse_one(QDict *qdict, const char *params,
         qstring_append_chr(val, *s++);
     }
 
+    if (help && key == implied_key) {
+        const char *str_val = qstring_get_str(val);
+        if (!strcmp(str_val, "help")) {
+            *help = true;
+            qobject_unref(val);
+            return s;
+        }
+    }
+
     if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) {
         return NULL;
     }
@@ -388,21 +404,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;