Re: [Qemu-block] [Qemu-devel] [PATCH v14 14/21] qapi: allow repeated opts with qobject_input_visitor_new_opts

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> The qobject_input_visitor_new_opts() method gains a new
> parameter to control whether it allows repeated option
> keys in the input QemuOpts or not.
>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  include/qapi/qobject-input-visitor.h | 6 ++++++
>  qapi/qobject-input-visitor.c         | 5 ++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index 242b767..bc5062a 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -112,6 +112,11 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj,
>   * qobject_input_visitor_new_autocast() method. See the docs
>   * of that method for further details on processing behaviour.
>   *
> + * If the @permit_repeated_opts parameter is true, then the input
> + * @opts is allowed to contain repeated keys and they will be
> + * turned into a QList. If it is false, then repeated keys will
> + * result in an error being reported.
> + *
>   * The returned input visitor should be released by calling
>   * visit_free() when no longer required.
>   */
> @@ -119,6 +124,7 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts 
> *opts,
>                                          bool autocreate_list,
>                                          size_t autocreate_struct_levels,
>                                          bool permit_int_ranges,
> +                                        bool permit_repeated_opts,
>                                          Error **errp);
>  
>  #endif
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 2287d11..5a3872c 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -753,6 +753,7 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts 
> *opts,
>                                          bool autocreate_list,
>                                          size_t autocreate_struct_levels,
>                                          bool permit_int_ranges,
> +                                        bool permit_repeated_opts,
>                                          Error **errp)
>  {
>      QDict *pdict;
> @@ -760,7 +761,9 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts 
> *opts,
>      Visitor *v = NULL;
>  
>      pdict = qemu_opts_to_qdict(opts, NULL,
> -                               QEMU_OPTS_REPEAT_POLICY_LAST,
> +                               permit_repeated_opts ?
> +                               QEMU_OPTS_REPEAT_POLICY_ALL :
> +                               QEMU_OPTS_REPEAT_POLICY_ERROR,
>                                 errp);
>      if (!pdict) {
>          goto cleanup;

@permit_repeated_opts applies to the whole @opts.  However, the "repeat
to build a list" feature is actually used more selectively.  For
instance, -spice wants it for "tls-channel" and "plaintext-channel", but
not for its other keys.  There, the code uses qemu_opt_get() or similar,
which gets the last value.

The options visitor works the same.  Internally, it stores only lists.
When the visit is for a scalar, it gets the last value from the list.
When the visit is for a list, it sets up things so that the list element
visits get the values in order.

As long as no key is list-valued, you can keep @permit_repeated_opts
false, and have duplicate keys rejected, for whatever that's worth.

As soon as you have a list-valued key, you need to set
@permit_repeated_opts to true.  But then *no* duplicate keys are
rejected.  If consistency with @permit_repeated_opts = false is needed,
then something else needs to detect unwanted lists and reject them.

What use cases for @permit_repeated_opts = false do you envisage?

To serve as replacement for the options visitor, this needs to treat
duplicates the same.  As we saw in PATCH 12, QEMU_OPTS_REPEAT_POLICY_ALL
makes a list when the key is repeated, while the options visitor makes a
list always.  I guess some other patch hides this difference.