Re: [Qemu-block] [Qemu-devel] [PATCH v14 09/21] qapi: permit auto-creating single element lists
"Daniel P. Berrange" writes:
> On Wed, Oct 12, 2016 at 11:18:21AM +0200, Markus Armbruster wrote:
>> "Daniel P. Berrange" writes:
>>
>> > When converting QemuOpts to a QObject, there is no information
>> > about compound types available,
>>
>> Yes, that's a drawback of splitting the conversion into a QemuOpts ->
>> QObject part that is oblivious of types, and a QObject -> QAPI object
>> part that knows the types.
>>
>> > so when visiting a list, the
>> > corresponding QObject is not guaranteed to be a QList. We
>> > therefore need to be able to auto-create a single element QList
>> > from whatever type we find.
>> >
>> > This mode should only be enabled if you have compatibility
>> > requirements for
>> >
>> >-arg foo=hello,foo=world
>> >
>> > to be treated as equivalent to the preferred syntax:
>> >
>> >-arg foo.0=hello,foo.1=world
>>
>> Not sure this is "preferred". "More powerfully warty" is probably
>> closer to the truth ;)
>
> Well, I call it "preferred" in the sense that that option syntax
> directly maps to the QAPI syntax in an unambigous manner. ie
> given the arg value alone "foo.0=hello,foo.1=world" you can clearly
> determine that "foo" is a list. With the compat syntax you cannot
> distinguish list from scalar, without knowing the QAPI schema.
>
>> How is "-arg foo=hello,foo=world" treated if this mode isn't enabled?
>
> The default behaviour would be that only the last key is present in
> the dict, eg foo=world, and then if you tried to visit a list, the
> visitor would complain that its got a QString instead of QList for
> the key 'foo'.
>
> This is related to patch 14
>
>> What would be the drawbacks of doing this always instead of only when we
>> "have compatibility requirements"?
>
> Essentially we'd be permanently allowing 2 distinct syntaxes for
> dealing with lists, for all options. I felt it desirable that we
> have only a single syntax and only allow this alt syntax in the
> backcompat cases.
Fair point.
The bolted-on extensions (options visitor's integer list syntax, block
layer's dotted key convention) have only ever worked in the places that
choose to use them. Even the integrated support for repeated keys is
only used for lists in the places that choose to use it that way.
>> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> > index d9269c9..d88e9f9 100644
>> > --- a/qapi/qobject-input-visitor.c
>> > +++ b/qapi/qobject-input-visitor.c
>> > @@ -48,6 +48,10 @@ struct QObjectInputVisitor
>> >
>> > /* True to reject parse in visit_end_struct() if unvisited keys
>> > remain. */
>> > bool strict;
>> > +
>> > +/* Whether we can auto-create single element lists when
>> > + * encountering a non-QList type */
>> > +bool autocreate_list;
>> > };
>> >
>> > static QObjectInputVisitor *to_qiv(Visitor *v)
>> > @@ -108,6 +112,7 @@ static const QListEntry
>> > *qobject_input_push(QObjectInputVisitor *qiv,
>> > assert(obj);
>> > tos->obj = obj;
>> > tos->qapi = qapi;
>> > +qobject_incref(obj);
>> >
>> > if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
>> > h = g_hash_table_new(g_str_hash, g_str_equal);
>> > @@ -147,6 +152,7 @@ static void
>> > qobject_input_stack_object_free(StackObject *tos)
>> > if (tos->h) {
>> > g_hash_table_unref(tos->h);
>> > }
>> > +qobject_decref(tos->obj);
>> >
>> > g_free(tos);
>> > }
>>
>> Can you explain the reference counting change?
>
> Previously the stack stored a borrowed reference, since it didn't
> ever need responsibility for free'ing the object when popping the
> stack. This is no longer the case if you look a few lines later
>
>
>>
>> > @@ -197,7 +203,7 @@ static void qobject_input_start_list(Visitor *v, const
>> > char *name,
>> > QObject *qobj = qobject_input_get_object(qiv, name, true);
>> > const QListEntry *entry;
>> >
>> > -if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
>> > +if (!qobj || (!qiv->autocreate_list && qobject_type(qobj) !=
>> > QTYPE_QLIST)) {
>>
>> Long line, but I believe it'll go away when you rebase for commit
>> 1382d4a.
>>
>> > if (list) {
>> > *list = NULL;
>> > }
>> > @@ -206,7 +212,16 @@ static void qobject_input_start_list(Visitor *v,
>> > const char *name,
>> > return;
>> > }
>> >
>> > -entry = qobject_input_push(qiv, qobj, list, errp);
>> > +if (qobject_type(qobj) != QTYPE_QLIST) {
>> > +QList *tmplist = qlist_new();
>> > +qlist_append_obj(tmplist, qobj);
>> > +qobject_incref(qobj);
>> > +entry = qobject_input_push(qiv, QOBJECT(tmplist), list, errp);
>> > +QDECREF(tmplist);
>
> ... here we are storing the 'qmplist' in the stack, and so when
> popping the stack, we must free that object. We thus need
> the stack to always hold its own reference, so when popping
> it can decref and (potentially) release the last reference.
Re: [Qemu-block] [Qemu-devel] [PATCH v14 09/21] qapi: permit auto-creating single element lists
On Wed, Oct 12, 2016 at 11:18:21AM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange" writes:
>
> > When converting QemuOpts to a QObject, there is no information
> > about compound types available,
>
> Yes, that's a drawback of splitting the conversion into a QemuOpts ->
> QObject part that is oblivious of types, and a QObject -> QAPI object
> part that knows the types.
>
> > so when visiting a list, the
> > corresponding QObject is not guaranteed to be a QList. We
> > therefore need to be able to auto-create a single element QList
> > from whatever type we find.
> >
> > This mode should only be enabled if you have compatibility
> > requirements for
> >
> >-arg foo=hello,foo=world
> >
> > to be treated as equivalent to the preferred syntax:
> >
> >-arg foo.0=hello,foo.1=world
>
> Not sure this is "preferred". "More powerfully warty" is probably
> closer to the truth ;)
Well, I call it "preferred" in the sense that that option syntax
directly maps to the QAPI syntax in an unambigous manner. ie
given the arg value alone "foo.0=hello,foo.1=world" you can clearly
determine that "foo" is a list. With the compat syntax you cannot
distinguish list from scalar, without knowing the QAPI schema.
> How is "-arg foo=hello,foo=world" treated if this mode isn't enabled?
The default behaviour would be that only the last key is present in
the dict, eg foo=world, and then if you tried to visit a list, the
visitor would complain that its got a QString instead of QList for
the key 'foo'.
This is related to patch 14
> What would be the drawbacks of doing this always instead of only when we
> "have compatibility requirements"?
Essentially we'd be permanently allowing 2 distinct syntaxes for
dealing with lists, for all options. I felt it desirable that we
have only a single syntax and only allow this alt syntax in the
backcompat cases.
> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> > index d9269c9..d88e9f9 100644
> > --- a/qapi/qobject-input-visitor.c
> > +++ b/qapi/qobject-input-visitor.c
> > @@ -48,6 +48,10 @@ struct QObjectInputVisitor
> >
> > /* True to reject parse in visit_end_struct() if unvisited keys
> > remain. */
> > bool strict;
> > +
> > +/* Whether we can auto-create single element lists when
> > + * encountering a non-QList type */
> > +bool autocreate_list;
> > };
> >
> > static QObjectInputVisitor *to_qiv(Visitor *v)
> > @@ -108,6 +112,7 @@ static const QListEntry
> > *qobject_input_push(QObjectInputVisitor *qiv,
> > assert(obj);
> > tos->obj = obj;
> > tos->qapi = qapi;
> > +qobject_incref(obj);
> >
> > if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
> > h = g_hash_table_new(g_str_hash, g_str_equal);
> > @@ -147,6 +152,7 @@ static void qobject_input_stack_object_free(StackObject
> > *tos)
> > if (tos->h) {
> > g_hash_table_unref(tos->h);
> > }
> > +qobject_decref(tos->obj);
> >
> > g_free(tos);
> > }
>
> Can you explain the reference counting change?
Previously the stack stored a borrowed reference, since it didn't
ever need responsibility for free'ing the object when popping the
stack. This is no longer the case if you look a few lines later
>
> > @@ -197,7 +203,7 @@ static void qobject_input_start_list(Visitor *v, const
> > char *name,
> > QObject *qobj = qobject_input_get_object(qiv, name, true);
> > const QListEntry *entry;
> >
> > -if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
> > +if (!qobj || (!qiv->autocreate_list && qobject_type(qobj) !=
> > QTYPE_QLIST)) {
>
> Long line, but I believe it'll go away when you rebase for commit
> 1382d4a.
>
> > if (list) {
> > *list = NULL;
> > }
> > @@ -206,7 +212,16 @@ static void qobject_input_start_list(Visitor *v, const
> > char *name,
> > return;
> > }
> >
> > -entry = qobject_input_push(qiv, qobj, list, errp);
> > +if (qobject_type(qobj) != QTYPE_QLIST) {
> > +QList *tmplist = qlist_new();
> > +qlist_append_obj(tmplist, qobj);
> > +qobject_incref(qobj);
> > +entry = qobject_input_push(qiv, QOBJECT(tmplist), list, errp);
> > +QDECREF(tmplist);
... here we are storing the 'qmplist' in the stack, and so when
popping the stack, we must free that object. We thus need
the stack to always hold its own reference, so when popping
it can decref and (potentially) release the last reference.
> > +} else {
> > +entry = qobject_input_push(qiv, qobj, list, errp);
> > +}
> > +
> > if (list) {
> > if (entry) {
> > *list = g_malloc0(size);
>
> Buries autolist behavior in the middle of things. What about doing it
> first, so it's more separate?
I'm not sure I understand what you mean here ?
>
>QObjectInputVisitor *qiv = to_qiv(v);
>QObject *qobj = qobject_input_get_object_(qiv, name, true, err
Re: [Qemu-block] [Qemu-devel] [PATCH v14 09/21] qapi: permit auto-creating single element lists
"Daniel P. Berrange" writes:
> When converting QemuOpts to a QObject, there is no information
> about compound types available,
Yes, that's a drawback of splitting the conversion into a QemuOpts ->
QObject part that is oblivious of types, and a QObject -> QAPI object
part that knows the types.
> so when visiting a list, the
> corresponding QObject is not guaranteed to be a QList. We
> therefore need to be able to auto-create a single element QList
> from whatever type we find.
>
> This mode should only be enabled if you have compatibility
> requirements for
>
>-arg foo=hello,foo=world
>
> to be treated as equivalent to the preferred syntax:
>
>-arg foo.0=hello,foo.1=world
Not sure this is "preferred". "More powerfully warty" is probably
closer to the truth ;)
How is "-arg foo=hello,foo=world" treated if this mode isn't enabled?
What would be the drawbacks of doing this always instead of only when we
"have compatibility requirements"?
> Signed-off-by: Daniel P. Berrange
> ---
> include/qapi/qobject-input-visitor.h | 20 +++-
> qapi/qobject-input-visitor.c | 27 +--
> tests/test-qobject-input-visitor.c | 88
> +++-
> 3 files changed, 117 insertions(+), 18 deletions(-)
>
> diff --git a/include/qapi/qobject-input-visitor.h
> b/include/qapi/qobject-input-visitor.h
> index f134d90..1809f48 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -42,6 +42,19 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool
> strict);
> * represented as strings. i.e. if visiting a boolean, the value should
> * be a QString whose contents represent a valid boolean.
> *
> + * If @autocreate_list is true, then as an alternative to a normal QList,
> + * list values can be stored as a QString or QDict instead, which will
> + * be interpreted as representing single element lists. This should only
> + * by used if compatibility is required with the OptsVisitor which allowed
> + * repeated keys, without list indexes, to represent lists. e.g. set this
> + * to true if you have compatibility requirements for
> + *
> + * -arg foo=hello,foo=world
> + *
> + * to be treated as equivalent to the preferred syntax:
> + *
> + * -arg foo.0=hello,foo.1=world
> + *
> * The visitor always operates in strict mode, requiring all dict keys
> * to be consumed during visitation. An error will be reported if this
> * does not happen.
> @@ -49,7 +62,8 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool
> strict);
> * The returned input visitor should be released by calling
> * visit_free() when no longer required.
> */
> -Visitor *qobject_input_visitor_new_autocast(QObject *obj);
> +Visitor *qobject_input_visitor_new_autocast(QObject *obj,
> +bool autocreate_list);
>
>
> /**
> @@ -64,6 +78,8 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj);
> * 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);
> +Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts,
> +bool autocreate_list,
> +Error **errp);
>
> #endif
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index d9269c9..d88e9f9 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -48,6 +48,10 @@ struct QObjectInputVisitor
>
> /* True to reject parse in visit_end_struct() if unvisited keys remain.
> */
> bool strict;
> +
> +/* Whether we can auto-create single element lists when
> + * encountering a non-QList type */
> +bool autocreate_list;
> };
>
> static QObjectInputVisitor *to_qiv(Visitor *v)
> @@ -108,6 +112,7 @@ static const QListEntry
> *qobject_input_push(QObjectInputVisitor *qiv,
> assert(obj);
> tos->obj = obj;
> tos->qapi = qapi;
> +qobject_incref(obj);
>
> if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
> h = g_hash_table_new(g_str_hash, g_str_equal);
> @@ -147,6 +152,7 @@ static void qobject_input_stack_object_free(StackObject
> *tos)
> if (tos->h) {
> g_hash_table_unref(tos->h);
> }
> +qobject_decref(tos->obj);
>
> g_free(tos);
> }
Can you explain the reference counting change?
> @@ -197,7 +203,7 @@ static void qobject_input_start_list(Visitor *v, const
> char *name,
> QObject *qobj = qobject_input_get_object(qiv, name, true);
> const QListEntry *entry;
>
> -if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
> +if (!qobj || (!qiv->autocreate_list && qobject_type(qobj) !=
> QTYPE_QLIST)) {
Long line, but I believe it'll go away when you rebase for commit
1382d4a.
> if (list) {
> *list = NULL;
> }
> @@ -206,7 +212
