[Qemu-devel] [PATCH v9 36/37] RFC: qapi: Adjust layout of FooList types

[Eric Blake]

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 <address@hidden>

---
v9: no change
v8: rebase to earlier changes
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 41f2da0..0eba901 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, const 
char *name,
             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 8df4ba1..dbbbcdb 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 4eae633..c446726 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -56,11 +56,8 @@
  * created by the qapi generator. */
 typedef struct GenericList
 {
-    union {
-        void *value;
-        uint64_t padding;
-    };
     struct GenericList *next;
+    char padding[];
 } GenericList;

 /**
@@ -130,19 +127,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 address@hidden, or set it to
- * NULL if there are no elements in the list; and output visitors
- * expect address@hidden to point to the start of the list, if any.  On
- * return, if address@hidden is non-NULL, the caller should enter a loop
+ * Input visitors malloc a qapi List struct into address@hidden of size @size,
+ * or set it to NULL if there are no elements in the list; and output
+ * visitors expect address@hidden to point to the start of the list, if any.
+ * On return, if address@hidden 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 address@hidden was allocated; if that happens, and an error
  * occurs any time before the matching visit_end_list(), then the
@@ -150,17 +147,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 38d1e68..28f9a8a 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 0990199..d498f29 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 259e0cb..ed4de71 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 6b4ad68..16c18b3 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -168,7 +168,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);
@@ -184,7 +184,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;
@@ -194,7 +194,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];
@@ -202,7 +202,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 ce592d2..78567d0 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -120,7 +120,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();
@@ -131,7 +132,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 0e4a07c..86bddd1 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 2666802..104f8a4 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -264,7 +264,8 @@ static void print_type_number(Visitor *v, const char *name, 
double *obj,
 }

 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 5cf20c2..47a6523 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 eebb5f7..193a1b6 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -130,7 +130,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, 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, const char *name, 
%(c_name)s **obj, 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.5.0