Re: [PATCH v7 1/4] monitor/hmp: add support for flag argument with value

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Stefan Reiter (s.reiter@proxmox.com) wrote:
> Adds support for the "-xV" parameter type, where "-x" denotes a flag
> name and the "V" suffix indicates that this flag is supposed to take an
> arbitrary string parameter.
> 
> These parameters are always optional, the entry in the qdict will be
> omitted if the flag is not given.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
> 
> v6:
> It wasn't possible to pass the 'connected' parameter to set_password, since the
> code to handle optional parameters couldn't live with a different param (not
> starting with '-') coming up instead - fix that by advancing over the 'value
> flag' modifier in case `*p != '-'`.
> 
> Also change the modifier to 'V' instead of 'S' so it can be distinguished from
> an actual trailing 'S' type param.
> 
> Discovered in testing. I dropped Eric's R-b due to the code change.
> 
>  monitor/hmp.c              | 19 ++++++++++++++++++-
>  monitor/monitor-internal.h |  3 ++-
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index d50c3124e1..899e0c990f 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -980,6 +980,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>              {
>                  const char *tmp = p;
>                  int skip_key = 0;
> +                int ret;
>                  /* option */
>  
>                  c = *typestr++;
> @@ -1002,11 +1003,27 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>                      }
>                      if (skip_key) {
>                          p = tmp;
> +                    } else if (*typestr == 'V') {
> +                        /* has option with string value */
> +                        typestr++;
> +                        tmp = p++;
> +                        while (qemu_isspace(*p)) {
> +                            p++;
> +                        }
> +                        ret = get_str(buf, sizeof(buf), &p);
> +                        if (ret < 0) {
> +                            monitor_printf(mon, "%s: value expected for -%c\n",
> +                                           cmd->name, *tmp);
> +                            goto fail;
> +                        }
> +                        qdict_put_str(qdict, key, buf);
>                      } else {
> -                        /* has option */
> +                        /* has boolean option */
>                          p++;
>                          qdict_put_bool(qdict, key, true);
>                      }
> +                } else if (*typestr == 'V') {
> +                    typestr++;
>                  }
>              }
>              break;
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 9c3a09cb01..9e708b329d 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -63,7 +63,8 @@
>   * '.'          other form of optional type (for 'i' and 'l')
>   * 'b'          boolean
>   *              user mode accepts "on" or "off"
> - * '-'          optional parameter (eg. '-f')
> + * '-'          optional parameter (eg. '-f'); if followed by an 'V', it
> + *              specifies an optional string param (e.g. '-fV' allows '-f foo')
>   *
>   */
>  
> -- 
> 2.30.2
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK