Re: [PATCH 08/25] hmp: replace "O" parser with keyval

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> HMP is using QemuOpts to parse free-form commands device_add,
> netdev_add and object_add.  However, none of these need QemuOpts
> for validation (these three QemuOptsLists are all of the catch-all
> kind), and keyval is already able to parse into QDict.  So use
> keyval directly, avoiding the detour from
> string to QemuOpts to QDict.
>
> The args_type now stores the implied key.  This arguably makes more
> sense than storing the QemuOptsList name; at least, it _is_ a key
> that might end up in the arguments QDict.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Switching from QemuOpts to keyval changes the accepted language.  We may
change it, because HMP is not a stable interface.  The commit message
should point out the change, though.  Maybe even release notes, not
sure.

Let's recap the differences briefly:

* Boolean sugar: deprecated in QemuOpts, nonexistent in keyval

* QemuOpts accepts a number of more or less crazy corner cases keyval
  rejects: invalid key, long key (silently truncated), first rather than
  last id= wins (unlike other keys), implied key with empty value.

* QemuOpts rejects anti-social ID such as id=666, keyval leaves this to
  the caller, because key "id" is not special in keyval.

  Are these still rejected with your patch?

> ---
>  hmp-commands.hx |  6 +++---
>  monitor/hmp.c   | 20 +++++++++-----------
>  2 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 73e0832ea1..6ee746b53e 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -669,7 +669,7 @@ ERST
>  
>      {
>          .name       = "device_add",
> -        .args_type  = "device:O",
> +        .args_type  = "driver:O",
>          .params     = "driver[,prop=value][,...]",
>          .help       = "add device, like -device on the command line",
>          .cmd        = hmp_device_add,
> @@ -1315,7 +1315,7 @@ ERST
>  
>      {
>          .name       = "netdev_add",
> -        .args_type  = "netdev:O",
> +        .args_type  = "type:O",
>          .params     = 
> "[user|tap|socket|vde|bridge|hubport|netmap|vhost-user],id=str[,prop=value][,...]",
>          .help       = "add host network device",
>          .cmd        = hmp_netdev_add,
> @@ -1343,7 +1343,7 @@ ERST
>  
>      {
>          .name       = "object_add",
> -        .args_type  = "object:O",
> +        .args_type  = "qom-type:O",
>          .params     = "[qom-type=]type,id=str[,prop=value][,...]",
>          .help       = "create QOM object",
>          .cmd        = hmp_object_add,
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 6c0b33a0b1..d2cb886da5 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -744,13 +744,9 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>              break;
>          case 'O':
>              {
> -                QemuOptsList *opts_list;
> -                QemuOpts *opts;
> +                Error *errp;
> +                bool help;
>  
> -                opts_list = qemu_find_opts(key);
> -                if (!opts_list || opts_list->desc->name) {
> -                    goto bad_type;
> -                }
>                  while (qemu_isspace(*p)) {
>                      p++;
>                  }
> @@ -760,12 +756,14 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>                  if (get_str(buf, sizeof(buf), &p) < 0) {
>                      goto fail;
>                  }
> -                opts = qemu_opts_parse_noisily(opts_list, buf, true);
> -                if (!opts) {
> -                    goto fail;
> +                keyval_parse_into(qdict, buf, key, &help, &errp);
> +                if (help) {

Uh...

> +                    if (qdict_haskey(qdict, key)) {

If we parsed a value for the implied key (sugared or not),

> +                        qdict_put_bool(qdict, "help", true);

then encode the help request by mapping key "help" to true,

> +                    } else {
> +                        qdict_put_str(qdict, key, "help");

else by mapping the implied key to "help".

> +                    }

Test cases:

* device_add help

  @qdict before the patch:

    {
        "driver": "help"
    }

  No change.

* device_add e1000,help

  @qdict before the patch:

    {
        "driver": "e1000",
        "help": "on"
    }

  Afterwards:

    {
        "driver": "e1000",
        "help": true
    }

  If this is okay, the commit message should explain it.

* device_add help,e1000

    {
        "e1000": "on",
        "driver": "help"
    }

  Afterwards:
  upstream-qemu: ../util/error.c:59: error_setv: Assertion `*errp == NULL' 
failed.

>                  }
> -                qemu_opts_to_qdict(opts, qdict);
> -                qemu_opts_del(opts);
>              }
>              break;
>          case '/':