Re: [Qemu-devel] [PATCH RFC v2 32/47] qapi-event: Convert to QAPISchemaVisitor, fixing data with base

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/01/2015 02:22 PM, Markus Armbruster wrote:
>> Fixes events whose data is struct with base to include the struct's
>> base members.  Test case is qapi-schema-test.json's event
>> __org.qemu_x-command:
>> 
>>     { 'event': '__ORG.QEMU_X-EVENT', 'data': '__org.qemu_x-Struct' }
>> 
>>     { 'struct': '__org.qemu_x-Struct', 'base': '__org.qemu_x-Base',
>>       'data': { '__org.qemu_x-member2': 'str' } }
>> 
>>     { 'struct': '__org.qemu_x-Base',
>>       'data': { '__org.qemu_x-member1': '__org.qemu_x-Enum' } }
>
> No change to generated code in qemu proper, so this is a corner case we
> are not yet exploiting. But good to have it fixed :)
>
>> 
>> Patch's effect on generated qapi_event_send___org_qemu_x_event():
>> 
>>     -void qapi_event_send___org_qemu_x_event(const char 
>> *__org_qemu_x_member2,
>>     +void qapi_event_send___org_qemu_x_event(__org_qemu_x_Enum 
>> __org_qemu_x_member1,
>>     +                                        const char 
>> *__org_qemu_x_member2,
>>                                              Error **errp)
>
> Ouch - I think we have a bug in qapi.py:check_event().  There, we call
> check_type(... allow_metas=['union', 'struct']) - but it looks like the
> generated signature requires that we have no variants, which means we
> cannot have:
>
> { 'union': 'Un', 'data': ... }
> { 'event': 'EV', 'data': 'Un' }
>
> because it would fail C generation. Sounds like you should add a check
> to that in 14/47, and a fix for it in 15/47.

Will do.

>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  scripts/qapi-event.py | 87 ++++++++++++++++-----------------
>>  tests/qapi-schema/qapi-schema-test.json |  3 --
>>  2 files changed, 43 insertions(+), 47 deletions(-)
>> 
>> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
>> index 537da17..456e590 100644
>> --- a/scripts/qapi-event.py
>> +++ b/scripts/qapi-event.py
>> @@ -2,28 +2,29 @@
>>  # QAPI event generator
>>  #
>>  # Copyright (c) 2014 Wenchao Xia
>> +# Copyright (c) 2013-2015 Red Hat Inc.
>
> Umm - the file didn't exist until 2014; and to my knowledge, you aren't
> adding anything to it that was copied from some other file dating back
> to 2013.  Using the range 2014-2015 might be better.

Pasto, will fix.

> But that's minor.  And assuming that you reject union types for events
> in an earlier patch, the rest of this is fine:
> Reviewed-by: Eric Blake <address@hidden>
>
>> @@ -89,15 +90,15 @@ def generate_event_implement(api_name, event_name, 
>> params):
>>  """,
>>                  event_name = event_name)
>>  
>> -        for argname, argentry, optional in parse_args(params):
>> -            if optional:
>> +        for memb in params.members:
>> +            if memb.optional:
>>                  ret += mcgen("""
>>      if (has_%(var)s) {
>>  """,
>> -                             var = c_name(argname))
>> +                             var=c_name(memb.name))
>>                  push_indent()
>>  
>> -            if argentry == "str":
>> +            if memb.type.name == "str":
>>                  var_type = "(char **)"
>
> Our visitors require us to cast away const.  Is that something we should
> consider reworking, so that we don't need to do that?  But it's a
> question for another day.

Yes, it's annoying, and yes, we need to leave it for later.

Thanks!