[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 05/10] qapi: Utilize implicit struct visits
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v4 05/10] qapi: Utilize implicit struct visits |
Date: |
Tue, 08 Mar 2016 19:09:34 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.
[...]
[Qemu-devel] [PATCH v4 04/10] qapi: Emit implicit structs in generated C, Eric Blake, 2016/03/05
[Qemu-devel] [PATCH v4 01/10] qapi: Assert in places where variants are not handled, Eric Blake, 2016/03/05