[v2] qapi: change QmpInputVisitor to QSLIST

Commit Message

Paolo Bonzini July 7, 2016, 11:27 a.m. UTC

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.

Signed-off-by: Paolo Bonzini <[e-mail]>
---
        v1->v2: leave assert in check_struct [Eric, Markus]

 qapi/qmp-input-visitor.c | 54 ++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

Comments

Markus Armbruster July 7, 2016, 2:25 p.m. UTC | #1

Paolo Bonzini <[e-mail]> writes:

> 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.
>
> Signed-off-by: Paolo Bonzini <[e-mail]>

Does not apply to current master.  Please rebase.

Eric Blake July 7, 2016, 2:33 p.m. UTC | #2

On 07/07/2016 05:27 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.
> 
> Signed-off-by: Paolo Bonzini <[e-mail]>
> ---
>         v1->v2: leave assert in check_struct [Eric, Markus]
> 
>  qapi/qmp-input-visitor.c | 54 ++++++++++++++++++++++++------------------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 

> -static void qmp_input_pop(Visitor *v)
> +static void qmp_input_stack_pop(QmpInputVisitor *qiv)
>  {
> -    QmpInputVisitor *qiv = to_qiv(v);
> -    StackObject *tos = &qiv->stack[qiv->nb_stack - 1];
> -

> +}
> +
> +static void qmp_input_pop(Visitor *v)
> +{
> +    QmpInputVisitor *qiv = to_qiv(v);
> +
> +    qmp_input_stack_pop(qiv);
>  }

I'm not entirely convinced we needed to separate qmp_input_pop() from
qmp_input_stack_pop(), but I'm not opposed to it enough to stop review.

Reviewed-by: Eric Blake <[e-mail]>

Patch

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index aea90a1..ad33a7b 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -29,6 +29,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
@@ -40,8 +42,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;
@@ -60,13 +61,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);
 
@@ -99,17 +100,10 @@  static const QListEntry *qmp_input_push(QmpInputVisitor *qiv, QObject *obj,
                                         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;
-    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);
@@ -119,7 +113,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;
 }
 
@@ -127,10 +121,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) {
@@ -145,22 +138,25 @@  static void qmp_input_check_struct(Visitor *v, Error **errp)
     }
 }
 
-static void qmp_input_pop(Visitor *v)
+static void qmp_input_stack_pop(QmpInputVisitor *qiv)
 {
-    QmpInputVisitor *qiv = to_qiv(v);
-    StackObject *tos = &qiv->stack[qiv->nb_stack - 1];
-
-    assert(qiv->nb_stack > 0);
+    StackObject *tos = QSLIST_FIRST(&qiv->stack);
 
     if (qiv->strict) {
-        GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
-        if (top_ht) {
-            g_hash_table_unref(top_ht);
+        if (tos->h) {
+            g_hash_table_unref(tos->h);
         }
-        tos->h = NULL;
     }
 
-    qiv->nb_stack--;
+    QSLIST_REMOVE_HEAD(&qiv->stack, node);
+    g_free(tos);
+}
+
+static void qmp_input_pop(Visitor *v)
+{
+    QmpInputVisitor *qiv = to_qiv(v);
+
+    qmp_input_stack_pop(qiv);
 }
 
 static void qmp_input_start_struct(Visitor *v, const char *name, void **obj,
@@ -221,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;
@@ -377,6 +373,10 @@  Visitor *qmp_input_get_visitor(QmpInputVisitor *v)
 
 void qmp_input_visitor_cleanup(QmpInputVisitor *v)
 {
+    while (!QSLIST_EMPTY(&v->stack)) {
+        qmp_input_stack_pop(v);
+    }
+
     qobject_decref(v->root);
     g_free(v);
 }