[Qemu-devel] [PATCH v3 0/2] qapi: change QObject visitors to QSLIST

[Qemu-devel] [PATCH v3 0/2] qapi: change QObject visitors to QSLIST

[Paolo Bonzini]

Rebased patches, grouped in a single series for simplicity since
I had to resend both of them.

Paolo Bonzini (2):
  qapi: change QmpOutputVisitor to QSLIST
  qapi: change QmpInputVisitor to QSLIST

 qapi/qmp-input-visitor.c  | 59 +++++++++++++++++++++++------------------------
 qapi/qmp-output-visitor.c | 24 +++++++++----------
 2 files changed, 40 insertions(+), 43 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v3 1/2] qapi: change QmpOutputVisitor to QSLIST

[Paolo Bonzini]

This saves a little memory compared to the doubly-linked QTAILQ.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi/qmp-output-visitor.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 0452056..64aa42c 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -23,15 +23,13 @@ typedef struct QStackEntry
 {
     QObject *value;
     void *qapi; /* sanity check that caller uses same pointer */
-    QTAILQ_ENTRY(QStackEntry) node;
+    QSLIST_ENTRY(QStackEntry) node;
 } QStackEntry;
 
-typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
-
 struct QmpOutputVisitor
 {
     Visitor visitor;
-    QStack stack; /* Stack of containers that haven't yet been finished */
+    QSLIST_HEAD(, QStackEntry) stack; /* Stack of containers that haven't yet been finished */
     QObject *root; /* Root of the output visit */
     QObject **result; /* User's storage location for result */
 };
@@ -56,18 +54,18 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value,
     assert(value);
     e->value = value;
     e->qapi = qapi;
-    QTAILQ_INSERT_HEAD(&qov->stack, e, node);
+    QSLIST_INSERT_HEAD(&qov->stack, e, node);
 }
 
 /* Pop a value off the stack of QObjects being built, and return it. */
 static QObject *qmp_output_pop(QmpOutputVisitor *qov, void *qapi)
 {
-    QStackEntry *e = QTAILQ_FIRST(&qov->stack);
+    QStackEntry *e = QSLIST_FIRST(&qov->stack);
     QObject *value;
 
     assert(e);
     assert(e->qapi == qapi);
-    QTAILQ_REMOVE(&qov->stack, e, node);
+    QSLIST_REMOVE_HEAD(&qov->stack, node);
     value = e->value;
     assert(value);
     g_free(e);
@@ -80,7 +78,7 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov, void *qapi)
 static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
                                QObject *value)
 {
-    QStackEntry *e = QTAILQ_FIRST(&qov->stack);
+    QStackEntry *e = QSLIST_FIRST(&qov->stack);
     QObject *cur = e ? e->value : NULL;
 
     if (!cur) {
@@ -206,7 +204,7 @@ static void qmp_output_complete(Visitor *v, void *opaque)
     QmpOutputVisitor *qov = to_qov(v);
 
     /* A visit must have occurred, with each start paired with end.  */
-    assert(qov->root && QTAILQ_EMPTY(&qov->stack));
+    assert(qov->root && QSLIST_EMPTY(&qov->stack));
     assert(opaque == qov->result);
 
     qobject_incref(qov->root);
@@ -217,10 +215,11 @@ static void qmp_output_complete(Visitor *v, void *opaque)
 static void qmp_output_free(Visitor *v)
 {
     QmpOutputVisitor *qov = to_qov(v);
-    QStackEntry *e, *tmp;
+    QStackEntry *e;
 
-    QTAILQ_FOREACH_SAFE(e, &qov->stack, node, tmp) {
-        QTAILQ_REMOVE(&qov->stack, e, node);
+    while (!QSLIST_EMPTY(&qov->stack)) {
+        e = QSLIST_FIRST(&qov->stack);
+        QSLIST_REMOVE_HEAD(&qov->stack, node);
         g_free(e);
     }
 
@@ -250,7 +249,6 @@ Visitor *qmp_output_visitor_new(QObject **result)
     v->visitor.complete = qmp_output_complete;
     v->visitor.free = qmp_output_free;
 
-    QTAILQ_INIT(&v->stack);
     *result = NULL;
     v->result = result;
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 1/2] qapi: change QmpOutputVisitor to QSLIST

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This saves a little memory compared to the doubly-linked QTAILQ.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qapi/qmp-output-visitor.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index 0452056..64aa42c 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -23,15 +23,13 @@ typedef struct QStackEntry
>  {
>      QObject *value;
>      void *qapi; /* sanity check that caller uses same pointer */
> -    QTAILQ_ENTRY(QStackEntry) node;
> +    QSLIST_ENTRY(QStackEntry) node;
>  } QStackEntry;
>  
> -typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
> -
>  struct QmpOutputVisitor
>  {
>      Visitor visitor;
> -    QStack stack; /* Stack of containers that haven't yet been finished */
> +    QSLIST_HEAD(, QStackEntry) stack; /* Stack of containers that haven't yet been finished */

Long line.  I'll squash in Eric's proposed fix on commit if you don't
mind:

       QSLIST_HEAD(...);           /* Stack of unfinished containers */

>      QObject *root; /* Root of the output visit */
>      QObject **result; /* User's storage location for result */
>  };
[...]

Re: [Qemu-devel] [PATCH v3 1/2] qapi: change QmpOutputVisitor to QSLIST

[Paolo Bonzini]

> >  struct QmpOutputVisitor
> >  {
> >      Visitor visitor;
> > -    QStack stack; /* Stack of containers that haven't yet been finished */
> > +    QSLIST_HEAD(, QStackEntry) stack; /* Stack of containers that haven't
> > yet been finished */
> 
> Long line.  I'll squash in Eric's proposed fix on commit if you don't
> mind:
> 
>        QSLIST_HEAD(...);           /* Stack of unfinished containers */

Yes, of course, sorry for forgetting about it.

Paolo

Re: [Qemu-devel] [PATCH v3 1/2] qapi: change QmpOutputVisitor to QSLIST

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

>> >  struct QmpOutputVisitor
>> >  {
>> >      Visitor visitor;
>> > -    QStack stack; /* Stack of containers that haven't yet been finished */
>> > +    QSLIST_HEAD(, QStackEntry) stack; /* Stack of containers that haven't
>> > yet been finished */
>> 
>> Long line.  I'll squash in Eric's proposed fix on commit if you don't
>> mind:
>> 
>>        QSLIST_HEAD(...);           /* Stack of unfinished containers */
>
> Yes, of course, sorry for forgetting about it.

No problem at all :)

[Qemu-devel] [PATCH v3 2/2] qapi: change QmpInputVisitor to QSLIST

[Paolo Bonzini]

This saves a lot of memory compared to a statically-sized array,
or at least 24kb could be considered a lot on an Atari ST.
It also makes the code more similar to QmpOutputVisitor.

This removes the limit on the depth of a QObject that can be processed
into a QAPI tree.  This is not a problem because QObjects can be
considered truested; the text received on the QMP wire is untrusted
input, but the JSON parser already takes pains to limit the QObject tree
it creates.  We don't need the QMP input visitor to limit it again.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi/qmp-input-visitor.c | 59 ++++++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 21edb39..64dd392 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -30,6 +30,8 @@ typedef struct StackObject
 
     GHashTable *h;           /* If obj is dict: unvisited keys */
     const QListEntry *entry; /* If obj is list: unvisited tail */
+
+    QSLIST_ENTRY(StackObject) node;
 } StackObject;
 
 struct QmpInputVisitor
@@ -41,8 +43,7 @@ struct QmpInputVisitor
 
     /* Stack of objects being visited (all entries will be either
      * QDict or QList). */
-    StackObject stack[QIV_STACK_SIZE];
-    int nb_stack;
+    QSLIST_HEAD(, StackObject) stack;
 
     /* True to reject parse in visit_end_struct() if unvisited keys remain. */
     bool strict;
@@ -61,13 +62,13 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
     QObject *qobj;
     QObject *ret;
 
-    if (!qiv->nb_stack) {
+    if (QSLIST_EMPTY(&qiv->stack)) {
         /* Starting at root, name is ignored. */
         return qiv->root;
     }
 
     /* We are in a container; find the next element. */
-    tos = &qiv->stack[qiv->nb_stack - 1];
+    tos = QSLIST_FIRST(&qiv->stack);
     qobj = tos->obj;
     assert(qobj);
 
@@ -100,18 +101,11 @@ static const QListEntry *qmp_input_push(QmpInputVisitor *qiv, QObject *obj,
                                         void *qapi, Error **errp)
 {
     GHashTable *h;
-    StackObject *tos = &qiv->stack[qiv->nb_stack];
+    StackObject *tos = g_new0(StackObject, 1);
 
     assert(obj);
-    if (qiv->nb_stack >= QIV_STACK_SIZE) {
-        error_setg(errp, "An internal buffer overran");
-        return NULL;
-    }
-
     tos->obj = obj;
     tos->qapi = qapi;
-    assert(!tos->h);
-    assert(!tos->entry);
 
     if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
         h = g_hash_table_new(g_str_hash, g_str_equal);
@@ -121,7 +115,7 @@ static const QListEntry *qmp_input_push(QmpInputVisitor *qiv, QObject *obj,
         tos->entry = qlist_first(qobject_to_qlist(obj));
     }
 
-    qiv->nb_stack++;
+    QSLIST_INSERT_HEAD(&qiv->stack, tos, node);
     return tos->entry;
 }
 
@@ -129,10 +123,9 @@ static const QListEntry *qmp_input_push(QmpInputVisitor *qiv, QObject *obj,
 static void qmp_input_check_struct(Visitor *v, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    StackObject *tos = &qiv->stack[qiv->nb_stack - 1];
-
-    assert(qiv->nb_stack > 0);
+    StackObject *tos = QSLIST_FIRST(&qiv->stack);
 
+    assert(tos && !tos->entry);
     if (qiv->strict) {
         GHashTable *const top_ht = tos->h;
         if (top_ht) {
@@ -147,23 +140,23 @@ static void qmp_input_check_struct(Visitor *v, Error **errp)
     }
 }
 
-static void qmp_input_pop(Visitor *v, void **obj)
+static void qmp_input_stack_object_free(StackObject *tos)
 {
-    QmpInputVisitor *qiv = to_qiv(v);
-    StackObject *tos = &qiv->stack[qiv->nb_stack - 1];
+    if (tos->h) {
+        g_hash_table_unref(tos->h);
+    }
 
-    assert(qiv->nb_stack > 0);
-    assert(tos->qapi == obj);
+    g_free(tos);
+}
 
-    if (qiv->strict) {
-        GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
-        if (top_ht) {
-            g_hash_table_unref(top_ht);
-        }
-        tos->h = NULL;
-    }
+static void qmp_input_pop(Visitor *v, void **obj)
+{
+    QmpInputVisitor *qiv = to_qiv(v);
+    StackObject *tos = QSLIST_FIRST(&qiv->stack);
 
-    qiv->nb_stack--;
+    assert(tos && tos->qapi == obj);
+    QSLIST_REMOVE_HEAD(&qiv->stack, node);
+    qmp_input_stack_object_free(tos);
 }
 
 static void qmp_input_start_struct(Visitor *v, const char *name, void **obj,
@@ -224,7 +217,7 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList *tail,
                                         size_t size)
 {
     QmpInputVisitor *qiv = to_qiv(v);
-    StackObject *so = &qiv->stack[qiv->nb_stack - 1];
+    StackObject *so = QSLIST_FIRST(&qiv->stack);
 
     if (!so->entry) {
         return NULL;
@@ -376,6 +369,12 @@ static void qmp_input_optional(Visitor *v, const char *name, bool *present)
 static void qmp_input_free(Visitor *v)
 {
     QmpInputVisitor *qiv = to_qiv(v);
+    while (!QSLIST_EMPTY(&qiv->stack)) {
+        StackObject *tos = QSLIST_FIRST(&qiv->stack);
+
+        QSLIST_REMOVE_HEAD(&qiv->stack, node);
+        qmp_input_stack_object_free(tos);
+    }
 
     qobject_decref(qiv->root);
     g_free(qiv);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 2/2] qapi: change QmpInputVisitor to QSLIST

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

On 07/07/2016 09:53 AM, Paolo Bonzini wrote:
> This saves a lot of memory compared to a statically-sized array,
> or at least 24kb could be considered a lot on an Atari ST.
> It also makes the code more similar to QmpOutputVisitor.
> 
> This removes the limit on the depth of a QObject that can be processed
> into a QAPI tree.  This is not a problem because QObjects can be
> considered truested; the text received on the QMP wire is untrusted

s/truested/trusted/

> input, but the JSON parser already takes pains to limit the QObject tree
> it creates.  We don't need the QMP input visitor to limit it again.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qapi/qmp-input-visitor.c | 59 ++++++++++++++++++++++++------------------------
>  1 file changed, 29 insertions(+), 30 deletions(-)
> 

With commit message tweak here and comment tweak in 1/2,
Series:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 0/2] qapi: change QObject visitors to QSLIST

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Rebased patches, grouped in a single series for simplicity since
> I had to resend both of them.

Series:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

I'll take this through qapi-next, but first Eric gets a chance to chime
in.

Re: [Qemu-devel] [PATCH v3 0/2] qapi: change QObject visitors to QSLIST

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Rebased patches, grouped in a single series for simplicity since
>> I had to resend both of them.
>
> Series:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> I'll take this through qapi-next, but first Eric gets a chance to chime
> in.

Applied to qapi-next with the touch-ups suggested by Eric.  Thanks!