Eric Blake <ebl...@redhat.com> writes: > 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 <ebl...@redhat.com> > > --- > 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;
Less trickery, I like it. Member padding appears to be unused. > > /** > @@ -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 *@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 > @@ -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); Tighter than size != 0 would be size >= GenericList. Same elsewhere. > + 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) [...] Rest looks good. Didn't look as closely as for the previous patches (getting tired), but so far I like the idea.