[Qemu-devel] [PATCH v7 31/31] RFC: qapi: Adjust layout of FooList types

[Eric Blake <eblake@redhat.com>]

By sticking the next pointer first, we don't need a union with
64-bit padding for smaller types.  On 32-bit platforms, this
can reduce the size of uint8List from 16 bytes (or 12, depending
on whether 64-bit ints can tolerate 4-byte alignment) down to 8.
It has no effect on 64-bit platforms (where alignment still
dictates a 16-byte struct); but fewer anonymous unions is still
a win in my book.

However, this requires visit_start_list() and visit_next_list()
to gain a size parameter, to know what size element to allocate.

I debated about going one step further, to allow for fewer casts,
by doing:
    typedef GenericList GenericList;
    struct GenericList {
        GenericList *next;
    };
    struct FooList {
        GenericList base;
        Foo value;
    };
so that you convert to 'GenericList *' by '&foolist->base', and
back by 'container_of(generic, GenericList, base)' (as opposed to
the existing '(GenericList *)foolist' and '(FooList *)generic').
But doing that would require hoisting the declaration of
GenericList prior to inclusion of qapi-types.h, rather than its
current spot in visitor.h; it also makes iteration a bit more
verbose through 'foolist->base.next' instead of 'foolist->next'.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: new patch; probably too invasive to be worth it, especially if
we can't prove that our current size for uint8List is a bottleneck
---
 hw/ppc/spapr_drc.c           |  2 +-
 include/qapi/visitor-impl.h  |  5 +++--
 include/qapi/visitor.h       | 39 +++++++++++++++++++--------------------
 qapi/opts-visitor.c          |  9 +++++----
 qapi/qapi-dealloc-visitor.c  |  5 +++--
 qapi/qapi-visit-core.c       | 14 +++++++++-----
 qapi/qmp-input-visitor.c     |  8 ++++----
 qapi/qmp-output-visitor.c    |  5 +++--
 qapi/string-input-visitor.c  |  9 +++++----
 qapi/string-output-visitor.c |  5 +++--
 scripts/qapi-types.py        |  5 +----
 scripts/qapi-visit.py        |  4 ++--
 12 files changed, 58 insertions(+), 52 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index f5ea3e0..6d07393 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -299,7 +299,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
             int i;
             prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
             name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
-            visit_start_list(v, name, NULL, &err);
+            visit_start_list(v, name, NULL, 0, &err);
             if (err) {
                 error_propagate(errp, err);
                 return;
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index ccb8b19..71313a9 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -41,9 +41,10 @@ struct Visitor

     /* Must be set */
     bool (*start_list)(Visitor *v, const char *name, GenericList **list,
-                       Error **errp);
+                       size_t size, Error **errp);
     /* Must be set */
-    GenericList *(*next_list)(Visitor *v, GenericList *element, Error **errp);
+    GenericList *(*next_list)(Visitor *v, GenericList *element, size_t size,
+                              Error **errp);
     /* Must be set */
     void (*end_list)(Visitor *v);

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index e9bb811..febe5da 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -55,11 +55,8 @@
  * created by the qapi generator. */
 typedef struct GenericList
 {
-    union {
-        void *value;
-        uint64_t padding;
-    };
     struct GenericList *next;
+    char padding[];
 } GenericList;

 /**
@@ -129,19 +126,19 @@ void visit_end_implicit_struct(Visitor *v);
 /**
  * Prepare to visit a list tied to an object key @name.
  * @name will be NULL if this is visited as part of another list.
- * Input visitors malloc a qapi List struct into *@list, or set it to
- * NULL if there are no elements in the list; and output visitors
- * expect *@list to point to the start of the list, if any.  On
- * return, if *@list is non-NULL, the caller should enter a loop
+ * Input visitors malloc a qapi List struct into *@list of size @size,
+ * or set it to NULL if there are no elements in the list; and output
+ * visitors expect *@list to point to the start of the list, if any.
+ * On return, if *@list is non-NULL, the caller should enter a loop
  * visiting the current element, then using visit_next_list() to
  * advance to the next element, until that returns NULL; then
  * visit_end_list() must be used to complete the visit.
  *
- * If supported by a visitor, @list can be NULL to indicate that there
- * is no qapi List struct, and that the upcoming visit calls are
- * parsing input to or creating output from some other representation;
- * in this case, visit_next_list() will not be needed, but
- * visit_end_list() is still mandatory.
+ * If supported by a visitor, @list can be NULL (and @size 0) to
+ * indicate that there is no qapi List struct, and that the upcoming
+ * visit calls are parsing input to or creating output from some other
+ * representation; in this case, visit_next_list() will not be needed,
+ * but visit_end_list() is still mandatory.
  *
  * Returns true if *@list was allocated; if that happens, and an error
  * occurs any time before the matching visit_end_list(), then the
@@ -149,17 +146,19 @@ void visit_end_implicit_struct(Visitor *v);
  * allocation before returning control further.
  */
 bool visit_start_list(Visitor *v, const char *name, GenericList **list,
-                      Error **errp);
+                      size_t size, Error **errp);
 /**
  * Iterate over a GenericList during a list visit.
  * Before calling this function, the caller should use the appropriate
- * visit_type_FOO() for the current list element at @element->value, and
- * check for errors. @element must not be NULL; on the first iteration,
- * it should be the value in *list after visit_start_list(); on other
- * calls it should be the previous return value.  This function
- * returns NULL once there are no further list elements.
+ * visit_type_FOO() for the current list element at @element->value,
+ * and check for errors. @element must not be NULL; on the first
+ * iteration, it should be the value in *list after
+ * visit_start_list(); on other calls it should be the previous return
+ * value.  @size should be the same as for visit_start_list().  This
+ * function returns NULL once there are no further list elements.
  */
-GenericList *visit_next_list(Visitor *v, GenericList *element, Error **errp);
+GenericList *visit_next_list(Visitor *v, GenericList *element, size_t size,
+                             Error **errp);
 /**
  * Complete the list started earlier.
  * Must be called after any successful use of visit_start_list(),
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 6dd22e2..573249a 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -214,7 +214,8 @@ lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)


 static bool
-opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
+opts_start_list(Visitor *v, const char *name, GenericList **list, size_t size,
+                Error **errp)
 {
     OptsVisitor *ov = to_ov(v);

@@ -225,7 +226,7 @@ opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
     ov->repeated_opts = lookup_distinct(ov, name, errp);
     if (ov->repeated_opts) {
         ov->list_mode = LM_IN_PROGRESS;
-        *list = g_new0(GenericList, 1);
+        *list = g_malloc0(size);
         return true;
     } else {
         *list = NULL;
@@ -235,7 +236,7 @@ opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp)


 static GenericList *
-opts_next_list(Visitor *v, GenericList *list, Error **errp)
+opts_next_list(Visitor *v, GenericList *list, size_t size, Error **errp)
 {
     OptsVisitor *ov = to_ov(v);

@@ -269,7 +270,7 @@ opts_next_list(Visitor *v, GenericList *list, Error **errp)
         abort();
     }

-    list->next = g_new0(GenericList, 1);
+    list->next = g_malloc0(size);
     return list->next;
 }

diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 42f366a..add38b5 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -90,7 +90,8 @@ static void qapi_dealloc_end_implicit_struct(Visitor *v)
 }

 static bool qapi_dealloc_start_list(Visitor *v, const char *name,
-                                    GenericList **list, Error **errp)
+                                    GenericList **list, size_t size,
+                                    Error **errp)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     qapi_dealloc_push(qov, NULL);
@@ -98,7 +99,7 @@ static bool qapi_dealloc_start_list(Visitor *v, const char *name,
 }

 static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *list,
-                                           Error **errp)
+                                           size_t size, Error **errp)
 {
     GenericList *next = list->next;
     g_free(list);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index c703002..07865e4 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -80,19 +80,23 @@ void visit_end_implicit_struct(Visitor *v)
 }

 bool visit_start_list(Visitor *v, const char *name, GenericList **list,
-                      Error **errp)
+                      size_t size, Error **errp)
 {
-    bool result = v->start_list(v, name, list, errp);
+    bool result;
+
+    assert(list ? size : !size);
+    result = v->start_list(v, name, list, size, errp);
     if (result) {
         assert(list && *list);
     }
     return result;
 }

-GenericList *visit_next_list(Visitor *v, GenericList *list, Error **errp)
+GenericList *visit_next_list(Visitor *v, GenericList *list, size_t size,
+                             Error **errp)
 {
-    assert(list);
-    return v->next_list(v, list, errp);
+    assert(list && size);
+    return v->next_list(v, list, size, errp);
 }

 void visit_end_list(Visitor *v)
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 263ac97..c56ff3d 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -166,7 +166,7 @@ static bool qmp_input_start_implicit_struct(Visitor *v, void **obj,
 }

 static bool qmp_input_start_list(Visitor *v, const char *name,
-                                 GenericList **list, Error **errp)
+                                 GenericList **list, size_t size, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
     QObject *qobj = qmp_input_get_object(qiv, name, true);
@@ -182,7 +182,7 @@ static bool qmp_input_start_list(Visitor *v, const char *name,
     qmp_input_push(qiv, qobj, entry, errp);
     if (list) {
         if (entry) {
-            *list = g_new0(GenericList, 1);
+            *list = g_malloc0(size);
             return true;
         } else {
             *list = NULL;
@@ -192,7 +192,7 @@ static bool qmp_input_start_list(Visitor *v, const char *name,
 }

 static GenericList *qmp_input_next_list(Visitor *v, GenericList *list,
-                                        Error **errp)
+                                        size_t size, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
     StackObject *so = &qiv->stack[qiv->nb_stack - 1];
@@ -200,7 +200,7 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList *list,
     if (!so->entry) {
         return NULL;
     }
-    list->next = g_new0(GenericList, 1);
+    list->next = g_malloc0(size);
     return list->next;
 }

diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index dd08a61..2a7b239 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -121,7 +121,8 @@ static void qmp_output_end_struct(Visitor *v)
 }

 static bool qmp_output_start_list(Visitor *v, const char *name,
-                                  GenericList **listp, Error **errp)
+                                  GenericList **listp, size_t size,
+                                  Error **errp)
 {
     QmpOutputVisitor *qov = to_qov(v);
     QList *list = qlist_new();
@@ -132,7 +133,7 @@ static bool qmp_output_start_list(Visitor *v, const char *name,
 }

 static GenericList *qmp_output_next_list(Visitor *v, GenericList *list,
-                                         Error **errp)
+                                         size_t size, Error **errp)
 {
     return list->next;
 }
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 4b8f1a5..8a9e385 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -121,7 +121,8 @@ error:
 }

 static bool
-start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
+start_list(Visitor *v, const char *name, GenericList **list, size_t size,
+           Error **errp)
 {
     StringInputVisitor *siv = to_siv(v);
     Error *err = NULL;
@@ -141,7 +142,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
         if (r) {
             siv->cur = r->begin;
         }
-        *list = g_new0(GenericList, 1);
+        *list = g_malloc0(size);
         return true;
     } else {
         *list = NULL;
@@ -150,7 +151,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
 }

 static GenericList *
-next_list(Visitor *v, GenericList *list, Error **errp)
+next_list(Visitor *v, GenericList *list, size_t size, Error **errp)
 {
     StringInputVisitor *siv = to_siv(v);
     Range *r;
@@ -176,7 +177,7 @@ next_list(Visitor *v, GenericList *list, Error **errp)
         siv->cur = r->begin;
     }

-    list->next = g_new0(GenericList, 1);
+    list->next = g_malloc0(size);
     return list->next;
 }

diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index 29b8f50..0611c6d 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -264,7 +264,8 @@ static void print_type_number(Visitor *v, double *obj, const char *name,
 }

 static bool
-start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
+start_list(Visitor *v, const char *name, GenericList **list, size_t size,
+           Error **errp)
 {
     StringOutputVisitor *sov = to_sov(v);

@@ -280,7 +281,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
 }

 static GenericList *
-next_list(Visitor *v, GenericList *list, Error **errp)
+next_list(Visitor *v, GenericList *list, size_t size, Error **errp)
 {
     StringOutputVisitor *sov = to_sov(v);
     GenericList *ret = list->next;
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 91c5ae0..7c1afba 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -26,11 +26,8 @@ def gen_array(name, element_type):
     return mcgen('''

 struct %(c_name)s {
-    union {
-        %(c_type)s value;
-        uint64_t padding;
-    };
     %(c_name)s *next;
+    %(c_type)s value;
 };
 ''',
                  c_name=c_name(name), c_type=element_type.c_type())
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 9e6e7a6..108823d 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -130,7 +130,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     %(c_name)s *elt;
     bool allocated;

-    allocated = visit_start_list(v, name, (GenericList **)obj, &err);
+    allocated = visit_start_list(v, name, (GenericList **)obj, sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
@@ -140,7 +140,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
         if (err) {
             break;
         }
-        elt = (%(c_name)s *)visit_next_list(v, (GenericList *)elt, &err);
+        elt = (%(c_name)s *)visit_next_list(v, (GenericList *)elt, sizeof(%(c_name)s), &err);
         if (err) {
             break;
         }
-- 
2.4.3