Re: [Qemu-devel] [PATCH v11 12/15] qapi: Don't box struct branch of alternate

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> There's no reason to do two malloc's for an alternate type visiting
> a QAPI struct; let's just inline the struct directly as the C union
> branch of the struct.
>
> Surprisingly, no clients were actually using the struct member prior
> to this patch outside of the testsuite; an earlier patch in the series
> added some testsuite coverage to make the effect of this patch more
> obvious.
>
> In qapi.py, c_type() gains a new is_unboxed flag to control when we
> are emitting a C struct unboxed within the context of an outer
> struct (different from our other two modes of usage with no flags
> for normal local variable declarations, and with is_param for adding
> 'const' in a parameter list).  I don't know if there is any more
> pythonic way of collapsing the two flags into a single parameter,
> as we never have a caller setting both flags at once.
>
> Ultimately, we want to also unbox branches for QAPI unions, but as
> that touches a lot more client code, it is better as separate
> patches.  But since unions and alternates share gen_variants(), I
> had to hack in a way to test if we are visiting an alternate type
> for setting the is_unboxed flag: look for a non-object branch.
> This works because alternates have at least two branches, with at
> most one object branch, while unions have only object branches.
> The hack will go away in a later patch.
>
> The generated code difference to qapi-types.h is relatively small:
>
> | struct BlockdevRef {
> |     QType type;
> |     union { /* union tag is @type */
> |         void *data;
> |-        BlockdevOptions *definition;
> |+        BlockdevOptions definition;
> |         char *reference;
> |     } u;
> | };
>
> meanwhile, in qapi-visit.h, we trade the previous visit_type_FOO(obj)

Meanwhile

> (which allocates a pointer, handles a layer of {} from the JSON stream,
> and visits the fields) with an inline call to visit_type_FOO(NULL) (to
> consume the {} without allocation) and a direct visit of the fields:

I don't see a call to visit_type_FOO(NULL).

Suggest not to abbreviate argument lists, it's mildly confusing.

>
> |     switch ((*obj)->type) {
> |     case QTYPE_QDICT:
> |-        visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
> |+        visit_start_struct(v, name, NULL, 0, &err);
> |+        if (err) {
> |+            break;
> |+        }
> |+        visit_type_BlockdevOptions_fields(v, &(*obj)->u.definition, &err);
> |+        error_propagate(errp, err);
> |+        err = NULL;
> |+        visit_end_struct(v, &err);
> |         break;
> |     case QTYPE_QSTRING:
> |         visit_type_str(v, name, &(*obj)->u.reference, &err);
>
> The visit of non-object fields is unchanged.
>
> Signed-off-by: Eric Blake <address@hidden>

Patch looks good.