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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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.

This patch actually fixes a similar issue in the qmp_marshal_FOO()
functions.

To keep ignoring it in the qapi_event_send_BAR() functions is okay.
It's fairly easy to fix now, though: split them into two, so that the
outer half does nothing but parameter wrapping.  For instance,

    void qapi_event_send_block_io_error(const char *device, IoOperationType 
operation, BlockErrorAction action, bool has_nospace, bool nospace, const char 
*reason, Error **errp)
    {
        QDict *qmp;
        Error *err = NULL;
        QMPEventFuncEmit emit;
        QmpOutputVisitor *qov;
        Visitor *v;
        QObject *obj;
        _obj_BLOCK_IO_ERROR_arg param = {
            (char *)device, operation, action, has_nospace, nospace, (char 
*)reason
        };

        [do stuff...]
    }

becomes

    static inline void do_event_send_block_io_error(_obj_BLOCK_IO_ERROR_arg 
param, Error **errp)
    {
        QDict *qmp;
        Error *err = NULL;
        QMPEventFuncEmit emit;
        QmpOutputVisitor *qov;
        Visitor *v;
        QObject *obj;

        [do stuff...]
    }

    void qapi_event_send_block_io_error(const char *device, IoOperationType 
operation, BlockErrorAction action, bool has_nospace, bool nospace, const char 
*reason, Error **errp)
    {
        do_event_send_block_io_error((_obj_BLOCK_IO_ERROR_arg){
                (char *)device, operation, action, has_nospace, nospace, (char 
*)reason
            }, errp);
        };
    }

Feel free not to do that now, but mark the spot with a comment then.
Since it's technically wrong, we could even mark it FIXME.

>> 
>>> +    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.

I see.  Const never fails to annoy.

[...]