Re: [Qemu-devel] [PATCH 2/3] qapi-visit: Expose visit_type_FOO_fields()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4971 bytes --]

On 02/24/2016 05:28 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Dan Berrange reported a case where he needs to work with a
>> QCryptoBlockOptions union type using the OptsVisitor, but only
>> visit one of the branches of that type (the discriminator is not
>> visited directly, but learned externally).  When things were
>> boxed, it was easy: just visit the variant directly, which took
>> care of both allocating the variant and visiting its fields.  But
>> now that things are unboxed, we need a way to visit the fields
>> without allocation, done by exposing visit_type_FOO_fields() to
>> the user.  Of course, this should only be done for objects, not
>> lists, so we need another flag to gen_visit_decl().
>>
>> Since the function is now public, we no longer need to preserve
>> topological ordering via struct_fields_seen.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> Minor conflicts with pending series "qapi implicit types"; I can
>> rebase whichever series gets reviewed second.

Sounds like you want this in first; I'll respin a single series with
both sets of patches, with this at the front.

>> ---
>>  scripts/qapi-visit.py | 47 +++++++++++++----------------------------------
>>  1 file changed, 13 insertions(+), 34 deletions(-)
>>
> 
> Let me review how this thing works for an object type FOO before your
> patch.
> 
> gen_visit_object() generates visit_type_FOO() with external linkage, and
> gen_visit_struct_fields() generates visit_type_FOO_fields() with
> internal linkage.
> 
> gen_visit_decl() generates a declaration of visit_type_FOO(), and
> gen_visit_fields_decl() generates one for visit_type_FOO_fields() unless
> it's already in scope.
> 
> visit_type_FOO_fields() is always called by visit_type_FOO(), and
> sometimes called elsewhere.
> 
> We generate visit_type_FOO_fields() right before visit_type_FOO(), so
> it's in scope there.  Anything that generates uses elsewhere must call
> gen_visit_fields_decl().
> 
> Your patch generates visit_type_FOO_fields() declarations into the
> header, which renders the "if already in scope" logic useless, along
> with the need to call gen_visit_fields_decl() before generating
> visit_type_FOO_fields() uses outside visit_type_FOO().

Correct. I'll try and incorporate some of this text in the commit
message for better justification.

>> +def gen_visit_decl(name, scalar=False, list=False):
>> +    ret = ''
>>      c_type = c_name(name) + ' *'
>>      if not scalar:
>> +        if not list:
>> +            ret += mcgen('''
>> +void visit_type_%(c_name)s_fields(Visitor *v, %(c_type)sobj, Error **errp);
>> +''',
>> +                         c_name=c_name(name), c_type=c_type)

> 
> This folds gen_visit_fields_decl() into gen_visit_decl() with the
> necessary changes: drop the "is already in scope" conditional, switch to
> external linkage.
> 
> Since gen_visit_decl() is called for non-object types as well, it gets a
> new optional parameter to suppress generation of
> visit_type_FOO_fields().  The parameter is named @list, which makes no
> sense to me.  See more below.
> 
> I don't like do-everything functions with suppressor flags much.  Can we
> structure this as a set of do-one-thing functions?  Possibly with
> convenience functions collecting common sets.

Sure; gen_visit_decl() stays unchanged, and gen_visit_decl_fields()
would be new and called only for objects.


> 
>        def visit_alternate_type(self, name, info, variants):
>            self.decl += gen_visit_decl(name)
> 
> Bug: here too.

Indeed - the declaration doesn't hurt, but there is no implementation to
back up the declaration, so it's better if I fix things to not add the
declaration.  v2 will fix it.

> 
>     $ grep _BlockdevRef_fields q*[ch]
>     qapi-visit.h:void visit_type_BlockdevRef_fields(Visitor *v, BlockdevRef *obj, Error **errp);
> 
>            self.defn += gen_visit_alternate(name, variants)
> 
> Your choice of an optional argument to keep things unchanged effectively
> hid the places that changed.  Failed to fool me today, but don't expect
> to remain lucky that way :)
> 
> One more thing: I never liked the name _fields.  C struct and union
> types don't have fields, they have members.  Likewiese, JSON objects.
> We shouldn't gratitously invent terminology.  We've done that quite a
> bit around QMP (JSON object vs. C QDict, JSON array vs. QList, QFLoat is
> really a double, ...).  Cleaning that up completely is probably hopeless
> by now.  But we can rename visit_type_FOO_fields() to something more
> sensible, say visit_type_FOO_members(), or even
> visit_type_FOO_unboxed().

visit_type_FOO_members() sounds slightly nicer to me.  I'll use that
terminology in v2 to see how things look.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]