Re: [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/17/2016 07:40 AM, Markus Armbruster wrote:
>
>>>>> 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.
>>>>>
>
>>> Also, here we pass 'obj'; visit_type_FOO() had to pass '*obj' (again,
>>> because we have one less level of indirection, and 7/13 reduced the
>>> indirection required in visit_type_FOO_fields()).
>>>
>>>>         // visit_start_union() + switch dropped
>>>>         error_propagate(errp, err);
>>>>         err = NULL;
>>>>         visit_end_struct(v, &err);
>>>>     out:
>>>>         error_propagate(errp, err);
>>>>     }
>>>>
>>>> Why can we drop visit_start_union() + switch?
>>>
>>> visit_start_union() is dropped because its only purpose was to determine
>>> if the dealloc visitor needs to visit the default branch. When we had a
>>> separate allocation, we did not want to visit the branch if the
>>> discriminator was not successfully parsed, because otherwise we would
>>> dereference NULL.  But now that we don't have a second pointer
>>> allocation, we no longer have anything to dealloc, and we can no longer
>>> dereference NULL. Explained better in 12/13, where I delete
>>> visit_start_union() altogether.  But maybe I could keep it in this patch
>>> in the meantime, to minimize confusion.
>> 
>> Perhaps squashing the two patches together could be less confusing.
>
> Yes, I'm closer to posting v11, and in that version, visit_start_union
> is only dropped in a single patch; and this patch just inlines the
> visit_start_struct() allocation directly into the visit_type_ALT()
> rather than creating a new visit_type_alternate_ALT().
>
>> 
>>> Dropped switch, on the other hand, looks to be a genuine bug.  Eeek.
>>> That should absolutely be present, and it proves that our testsuite is
>>> not strong enough for not catching me on it.
>>>
>>> And now that you've made me think about it, maybe I have yet another
>>> idea.  Right now, we've split the visit of local members into
>>> visit_type_FOO_fields(), while leaving the variant members to be visited
>>> in visit_type_FOO()
>> 
>> Yes.  I guess that's to support visiting "implicit" structs.
>
> Up to now, we've forbidden the use of one union as a branch of another
> (but allowed a union as a branch of an alternate), so the types passed
> to visit_struct_FOO_fields() never had variants.  As part of our generic
> "object" work, I _want_ to support one union as a branch of another (as
> long as we can prove there will be no QMP name collisions); and that's
> another reason why visit_struct_FOO_fields() would need to always visit
> variants if present.
>
>
>> Let me get to this result from a different angle.  A "boxed" visit is
>> 
>>     allocate hook
>>     visit the members (common and variant)
>>     cleanup hook
>
> Correct, and we have two choices of allocate hook: visit_start_struct()
> [allocate and consume {}, for visit_type_FOO() in general], and
> visit_start_implicit_struct() [allocate, but don't consume {}, for flat
> union branches prior to this series].
>
>> 
>> An "unboxed" visit is basically the same without the two hooks.
>
> Done anywhere we don't have a separate C struct [base classes prior to
> this series; and then this series is adding unboxed variant visits
> within flat unions and alternates].

Should work for visiting both "inlined" and "unboxed" members, shouldn't
it?

    struct {
      A a;
      B b
    } S;

    struct {
      S *ps;    // boxed member of type S
      S s;      // unboxed member of type S
      A a; B b; // inlined member of type S
    }

>> 
>> "Implicit" is like unboxed, except the members are inlined rather than
>> wrapped in a JSON object.
>> 
>> So the common code to factor out is "visit the members".
>
> Yep, we're on the same wavelength, and it is looking fairly nice for
> what I'm about to post as v11.  And I like 'unboxed' better than
> 'is_member':
>
>>>>> -                     c_type=typ.c_type(),
>>>>> +                     c_type=typ.c_type(is_member=inline),
>> 
>> I don't like the name is_member.  The thing we're dealing with here is a
>> member, regardless of the value of inline.  I think it's boxed
>> vs. unboxed.
>
> Hmm. I have later patches that add a 'box':true QAPI designation to
> commands and events, to let us do qmp_command(Foo *arg, Error **errp)
> instead of qmp_command(type Foo_member1, type Foo_member2 ..., Error
> **errp) (that is, instead of breaking out each parameter, we pass them
> all boxed behind a single pointer).  What we are doing here is in the
> opposite direction - we are taking code that has boxed all the sub-type
> fields behind a single pointer, and unboxing it so that they occur
> inline in the parent type's storage.  Works for me; I'm switching to
> 'is_unboxed' as the case for when we want to omit the pointer
> designation during the member declaration.

Better.  "Unboxed" isn't tied to "member"; we could choose to unbox
elsewhere, too.