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

[Eric Blake <eblake@redhat.com>]

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

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.


> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  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.

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 <eblake@redhat.com>

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

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