Re: [Qemu-devel] [PATCH V6 03/29] qapi script: add event support

[Eric Blake <eblake@redhat.com>]

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

On 06/05/2014 06:21 AM, Wenchao Xia wrote:
> qapi-event.py will parse the schema and generate qapi-event.c, then
> the API in qapi-event.c can be used to handle event in qemu code.

s/event in/events in/

> All API have prefix "qapi_event".
> 
> The script mainly includes two parts: generate API for each event
> define, generate an enum type for all defined events.
> 
> Since in some cases the real emit behavior may change, for example,
> qemu-img would not send a event, a callback layer is used to
> control the behavior. As a result, the stubs at compile time
> can be saved, the binding of block layer code and monitor code
> will become looser.
> 
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
> ---
>  Makefile                                 |   11 +-
>  Makefile.objs                            |    2 +-
>  docs/qapi-code-gen.txt                   |   18 ++
>  scripts/qapi-event.py                    |  369 ++++++++++++++++++++++++++++++
>  scripts/qapi.py                          |   12 +
>  tests/Makefile                           |    2 +-
>  tests/qapi-schema/event-nest-struct.err  |    1 +
>  tests/qapi-schema/event-nest-struct.exit |    1 +
>  tests/qapi-schema/event-nest-struct.json |    2 +
>  9 files changed, 413 insertions(+), 5 deletions(-)
>  create mode 100644 scripts/qapi-event.py
>  create mode 100644 tests/qapi-schema/event-nest-struct.err
>  create mode 100644 tests/qapi-schema/event-nest-struct.exit
>  create mode 100644 tests/qapi-schema/event-nest-struct.json
>  create mode 100644 tests/qapi-schema/event-nest-struct.out

My python is weak, but I did apply this patch (or rather, used Paolo's
rebase of this patch) to test the generated files.

At this point, I'm interested in seeing the series go in sooner rather
than later, and my findings below are minor enough that I'm okay fixing
them in a followup patch rather than waiting for a respin of the whole
series.

Your patch fails to update .gitignore for the new generated files:

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	qapi-event.c
	qapi-event.h

>  
> +=== Events ===
> +
> +Events are defined with key word 'event'.  When 'data' is also specified,
> +additional info will be carried on.  Finally there will be C API generated

s/carried on/included in the event/

> +++ b/scripts/qapi-event.py
> +
> +def _generate_event_api_name(event_name, params):
> +    api_name = "void qapi_event_send_%s(" % c_fun(event_name).lower();

As of this commit, no events using qapi_event_send are generated yet.
I'm trusting that this code works, but I reserve the right to revisit
this patch later once I look at later patches and see what the generated
code is doing there.  But for now, the generated code has merely:

const char *QAPIEvent_lookup[] = {
    NULL,
};

which looks correct, even if a bit sparse :)

> +++ b/scripts/qapi.py
> @@ -248,6 +248,16 @@ def discriminator_find_enum_define(expr):
>  
>      return find_enum(discriminator_type)
>  
> +def check_event(expr, expr_info):
> +    params = expr.get('data')
> +    if params:
> +        for argname, argentry, optional, structured in parse_args(params):
> +            if structured:
> +                raise QAPIExprError(expr_info,
> +                                    "Nested structure define in event is not "
> +                                    "supported now, event '%s', argname '%s'"

s/ now// - we don't EVER want to support nested structures in events
(for that matter, I've been arguing in other threads that we want to
completely ditch support for nested structures to simplify the generator
code-base and make it possible to cleanly support argument defaults).

> +++ b/tests/qapi-schema/event-nest-struct.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/event-nest-struct.json:1: Nested structure define in event is not supported now, event 'EVENT_A', argname 'a'

Of course, if you tweak the message above, you'll also have to tweak
this test.  But thanks for adding a test!

Since I'm okay saving the cleanups mentioned above for a followup-patch,
and assuming I don't revisit this file:

Reviewed-by: Eric Blake <eblake@redhat.com>

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