Re: [Qemu-devel] [PATCH v4 05/10] qapi: Utilize implicit struct visits

[Eric Blake]

On 03/08/2016 08:10 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Rather than generate inline per-member visits, take advantage
>> of the 'visit_type_FOO_members()' function for both event and
>> command marshalling.  This is possible now that implicit
>> structs can be visited like any other.
>>
>> Generated code shrinks accordingly; events initialize a struct
>> based on parameters, such as:
>>

>>
>> And command marshalling generates call arguments from a stack-allocated
>> struct:
> 
> I see qmp-marshal.c shrink from ~5700 lines to ~4300.  Neat!

Yeah, it nicely balances out the .h getting so much larger, except that
the .h gets parsed a lot more by the compiler.


>>
>> +# Declare and initialize an object 'qapi' using parameters from gen_params()
>> +def gen_struct_init(typ):
> 
> It's not just a "struct init", it's a variable declaration with an
> initializer.  gen_param_var()?
> 
> Name the variable param rather than qapi?

Sure, I'm not tied to a specific name.  I will point out that we have a
potential collision point - any local variable we create here can
collide with members of the QAPI struct passed to the event.  We haven't
hit the collision on any events we've created so far, and it's easy to
rename our local variables at such time if we do run into the collision
down the road, so I won't worry about it now.

> 
>> +    assert not typ.variants
>> +    ret = mcgen('''
>> +    %(c_name)s qapi = {
>> +''',
>> +                c_name=typ.c_name())
>> +    sep = '        '
>> +    for memb in typ.members:
>> +        ret += sep
>> +        sep = ', '
>> +        if memb.optional:
>> +            ret += 'has_' + c_name(memb.name) + sep
>> +        if memb.type.name == 'str':
>> +            # Cast away const added in gen_params()
>> +            ret += '(char *)'
> 
> Why is that useful?

The compiler complains if you try to initialize a 'char *' member of a
QAPI C struct with a 'const char *' parameter.  It's no different than
the fact that the gen_visit_members() call we are getting rid of also
has to cast away const.

> 
>> +        ret += c_name(memb.name)
>> +    ret += mcgen('''
>> +
>> +    };
>> +''')
>> +    return ret
> 
> Odd: you use this only in qapi-event.py, not in qapi-commands.py.
> Should it live in qapi-event.py then?

Yeah, I guess so.  I originally put it in qapi.py thinking that I could
reuse it in qapi-commands.py, then realized that the two are opposite
(event collects parameters into a struct, commands parses a QObject into
parameters), so I couldn't share it after all.


>> @@ -107,17 +96,24 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
>>      qdv = qapi_dealloc_visitor_new();
>>      v = qapi_dealloc_get_visitor(qdv);
>>  ''')
>> +        errp = 'NULL'
>>      else:
>>          ret += mcgen('''
>>      v = qmp_input_get_visitor(qiv);
>>  ''')
>> +        errp = '&err'
>>
>> -    ret += gen_visit_members(arg_type.members, skiperr=dealloc)
> 
> Unless I'm mistaken, this is the only use of skiperr.  Follow up with a
> patch to drop the parameter and simplify?

Oh, nice.  I noticed some cleanups in patch 6/10, but missed this one as
a reasonable improvement.

In fact, gen_visit_members() is now only used in qapi-visits.py, so
maybe I can move it back there (it used to live there before commit
82ca8e469 moved it for sharing with the two files simplified here).


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

signature.asc
Description: OpenPGP digital signature