Re: [Qemu-block] [Qemu-devel] [PATCH v14 08/21] qapi: allow QObjectInputVisitor to be created with QemuOpts

2016-10-13 Thread Markus Armbruster
Markus Armbruster  writes:

> "Daniel P. Berrange"  writes:
>
>> Instead of requiring all callers to go through the mutli-step
>
> multi-step
>
>> process of turning QemuOpts into a suitable QObject for visiting,
>> add a new constructor that encapsulates this logic. This will
>> allow QObjectInputVisitor to be a drop-in replacement for the
>> existing OptsVisitor with minimal code changes for callers.
>>
>> NB, at this point it is only supporting opts syntax which
>> explicitly matches the QAPI schema structure, so is not yet
>> a true drop-in replacement for OptsVisitor. The patches that
>> follow will add the special cases requird for full backwards
>> compatibility with OptsVisitor.
>>
>> Signed-off-by: Daniel P. Berrange 
[...]
>> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> index cf41df6..d9269c9 100644
>> --- a/qapi/qobject-input-visitor.c
>> +++ b/qapi/qobject-input-visitor.c
>> @@ -545,3 +545,28 @@ Visitor *qobject_input_visitor_new_autocast(QObject 
>> *obj)
>>  
>>  return >visitor;
>>  }
>> +
>> +
>> +Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts,
>> +Error **errp)
>> +{
>> +QDict *pdict;
>> +QObject *pobj = NULL;
>
> @pdict and @pobj are unusual names.  Let's stick to the more common
> @dict and @obj.
>
>> +Visitor *v = NULL;
>> +
>> +pdict = qemu_opts_to_qdict(opts, NULL);
>> +if (!pdict) {
>> +goto cleanup;

Returns null without setting an error, which is wrong.  Fortunately,
qemu_opts_to_qdict() cannot fail.  Please drop the broken error
handling.

>> +}
>> +
>> +pobj = qdict_crumple(pdict, true, errp);
>> +if (!pobj) {
>> +goto cleanup;
>> +}
>> +
>> +v = qobject_input_visitor_new_autocast(pobj);
>> + cleanup:
>> +qobject_decref(pobj);
>> +QDECREF(pdict);
>> +return v;
>> +}
[...]



Re: [Qemu-block] [Qemu-devel] [PATCH v14 08/21] qapi: allow QObjectInputVisitor to be created with QemuOpts

2016-10-12 Thread Markus Armbruster
"Daniel P. Berrange"  writes:

> Instead of requiring all callers to go through the mutli-step

multi-step

> process of turning QemuOpts into a suitable QObject for visiting,
> add a new constructor that encapsulates this logic. This will
> allow QObjectInputVisitor to be a drop-in replacement for the
> existing OptsVisitor with minimal code changes for callers.
>
> NB, at this point it is only supporting opts syntax which
> explicitly matches the QAPI schema structure, so is not yet
> a true drop-in replacement for OptsVisitor. The patches that
> follow will add the special cases requird for full backwards
> compatibility with OptsVisitor.
>
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qapi/qobject-input-visitor.h | 15 +++
>  include/qemu/option.h|  2 +-
>  qapi/qobject-input-visitor.c | 25 +
>  util/qemu-option.c   |  2 +-
>  4 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index 5022297..f134d90 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -51,4 +51,19 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   */
>  Visitor *qobject_input_visitor_new_autocast(QObject *obj);
>  
> +
> +/**
> + * Create a new input visitor that converts @opts to a QAPI object.
> + *
> + * The QemuOpts will be converted into a QObject using the
> + * qdict_crumple() method to automatically create structs
> + * and lists. The resulting QDict will then be passed to the
> + * qobject_input_visitor_new_autocast() method. See the docs
> + * of that method for further details on processing behaviour.
> + *
> + * The returned input visitor should be released by calling
> + * visit_free() when no longer required.
> + */
> +Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts, Error **errp);
> +
>  #endif
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 2a5266f..29f3f18 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -125,7 +125,7 @@ void qemu_opts_set_defaults(QemuOptsList *list, const 
> char *params,
>  int permit_abbrev);
>  QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> Error **errp);
> -QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
> +QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict);
>  void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
>  
>  typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error 
> **errp);
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index cf41df6..d9269c9 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -545,3 +545,28 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj)
>  
>  return >visitor;
>  }
> +
> +
> +Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts,
> +Error **errp)
> +{
> +QDict *pdict;
> +QObject *pobj = NULL;

@pdict and @pobj are unusual names.  Let's stick to the more common
@dict and @obj.

> +Visitor *v = NULL;
> +
> +pdict = qemu_opts_to_qdict(opts, NULL);
> +if (!pdict) {
> +goto cleanup;
> +}
> +
> +pobj = qdict_crumple(pdict, true, errp);
> +if (!pobj) {
> +goto cleanup;
> +}
> +
> +v = qobject_input_visitor_new_autocast(pobj);
> + cleanup:
> +qobject_decref(pobj);
> +QDECREF(pdict);
> +return v;
> +}
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 41b356c..418cbb6 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -1058,7 +1058,7 @@ void qemu_opts_absorb_qdict(QemuOpts *opts, QDict 
> *qdict, Error **errp)
>   * TODO We'll want to use types appropriate for opt->desc->type, but
>   * this is enough for now.

Implementing this TODO could break users that pass it to
qobject_input_visitor_new_autocast(), because despite its name, the
_autocast visitor expects *only* string scalars.

I guess (but have not checked) that these users have null opt->desc
since their opts->list->desc[] is empty.

We should either drop the TODO (needs review of the other users), or
update it to reflect the new situation.  Perhaps users of
qobject_input_visitor_new_autocast() should additionally assert
opts->list->desc[] is empty.

>   */
> -QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict)
> +QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict)
>  {
>  QemuOpt *opt;
>  QObject *val;