Re: [RFC PATCH 08/14] qemu-options: Add qemu_opts_parse_list_noisily

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Sep 18, 2026 at 07:29:55PM -0300, Fabiano Rosas wrote:
> Add a wrapper similar to qemu_opts_parse_list() with the same
> objective, to free the non _list version,
> i.e. qemu_opts_parse_noisily() to be converted to receive the group
> name.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  block/monitor/block-hmp-cmds.c       |  2 +-
>  include/qemu/option.h                |  2 ++
>  monitor/hmp.c                        |  2 +-
>  net/net.c                            |  2 +-
>  qemu-img.c                           | 10 +++++-----
>  qemu-io-cmds.c                       |  2 +-
>  qemu-io.c                            |  7 ++++---
>  qemu-nbd.c                           |  6 +++---
>  storage-daemon/qemu-storage-daemon.c |  4 ++--
>  system/qdev-monitor.c                |  2 +-
>  system/tpm.c                         |  2 +-
>  system/vl.c                          |  6 +++---
>  tests/unit/test-replication.c        |  6 +++---
>  tools/qemu-vnc/qemu-vnc.c            |  2 +-
>  util/qemu-option.c                   | 14 ++++++++++++++
>  15 files changed, 43 insertions(+), 26 deletions(-)
> 
> diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
> index 7bae4d425c..cbd64166ec 100644
> --- a/block/monitor/block-hmp-cmds.c
> +++ b/block/monitor/block-hmp-cmds.c
> @@ -63,7 +63,7 @@ static void hmp_drive_add_node(MonitorHMP *hmp, const char *optstr)
>      QDict *qdict;
>      Error *err = NULL;
>  
> -    opts = qemu_opts_parse_noisily(&qemu_drive_opts, optstr, false);
> +    opts = qemu_opts_parse_list_noisily(&qemu_drive_opts, optstr, false);
>      if (!opts) {
>          return;
>      }
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index dfe0d9b893..df68da9aa8 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -134,6 +134,8 @@ QemuOpts *qemu_opts_parse(const char *group, const char *params,
>                            bool permit_abbrev, Error **errp);
>  QemuOpts *qemu_opts_parse_list(QemuOptsList *list, const char *params,
>                                 bool permit_abbrev, Error **errp);
> +QemuOpts *qemu_opts_parse_list_noisily(QemuOptsList *list, const char *params,
> +                                       bool permit_abbrev);
>  QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
>                                 Error **errp);
>  QDict *qemu_opts_to_qdict_filtered(QemuOpts *opts, QDict *qdict,


This is adding a new function and not removing an existnig function, but....

> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 488ec23937..fc32cfd1ff 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -897,7 +897,7 @@ static QDict *monitor_parse_arguments(MonitorHMP *mon,
>                  if (get_str(buf, sizeof(buf), &p) < 0) {
>                      goto fail;
>                  }
> -                opts = qemu_opts_parse_noisily(opts_list, buf, true);
> +                opts = qemu_opts_parse_list_noisily(opts_list, buf, true);
>                  if (!opts) {
>                      goto fail;
>                  }

...then the patch changes alot of callers to the new
function, which wasn't mentioned in the commit message.

Did you accidentally squash two patches together ? I'd
suggest the conversion of callers should be separate.

> diff --git a/qemu-img.c b/qemu-img.c
> index 2f63d31141..4996618a4e 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -161,7 +161,7 @@ static OutputFormat parse_output_format(const char *argv0, const char *arg)
>   * an odd number of ',' (or else a separating ',' following it gets
>   * escaped), or be empty (or else a separating ',' preceding it can
>   * escape a separating ',' following it).
> - * 
> + *
>   */
>  static bool is_valid_option_list(const char *list)
>  {

Extraneous change


> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index c8e552823b..c762eb20a3 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -971,6 +971,20 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
>      return qemu_opts_parse_list(list, params, permit_abbrev, NULL);
>  }
>  
> +/**
> + * Create a QemuOpts in @list and with options parsed from @params.
> + * If @permit_abbrev, the first key=value in @params may omit key=,
> + * and is treated as if key was @list->implied_opt_name.
> + * Report errors with error_report_err().  This is inappropriate in
> + * QMP context.  Do not use this function there!
> + * Return the new QemuOpts on success, null pointer on error.
> + */
> +QemuOpts *qemu_opts_parse_list_noisily(QemuOptsList *list, const char *params,
> +                                       bool permit_abbrev)
> +{
> +    return qemu_opts_parse_list(list, params, permit_abbrev, NULL);
> +}

I guess this is why you moved the error reporting in the previous
patch. Personally I'd skip the previous patch and accept the
small duplicate code pattern between the two "noisily" methods.

> +
>  static bool qemu_opts_from_qdict_entry(QemuOpts *opts,
>                                         const QDictEntry *entry,
>                                         Error **errp)
> -- 
> 2.53.0
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|