| Message ID | 1435782155-31412-33-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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> 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 <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. 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 <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. Yes, it's annoying, and yes, we need to leave it for later. Thanks!
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. # # Authors: # Wenchao Xia <wenchaoqemu@gmail.com> +# Markus Armbruster <armbru@redhat.com> # # This work is licensed under the terms of the GNU GPL, version 2. # See the COPYING file in the top-level directory. -from ordereddict import OrderedDict from qapi import * -def _generate_event_api_name(event_name, params): +def _generate_event_api_name(event_name, data): api_name = "void qapi_event_send_%s(" % c_name(event_name).lower(); l = len(api_name) - if params: - for argname, argentry, optional in parse_args(params): - if optional: - api_name += "bool has_%s,\n" % c_name(argname) + if data: + for m in data.members: + if m.optional: + api_name += "bool has_%s,\n" % c_name(m.name) api_name += "".ljust(l) - api_name += "%s %s,\n" % (c_type(argentry, is_param=True), - c_name(argname)) + api_name += "%s %s,\n" % (m.type.c_type(is_param=True), + c_name(m.name)) api_name += "".ljust(l) api_name += "Error **errp)" @@ -51,7 +52,7 @@ def generate_event_implement(api_name, event_name, params): """, api_name = api_name) - if params: + if params and params.members: ret += mcgen(""" QmpOutputVisitor *qov; Visitor *v; @@ -72,7 +73,7 @@ def generate_event_implement(api_name, event_name, params): event_name = event_name) # step 3: visit the params if params != None - if params: + if params and params.members: ret += mcgen(""" qov = qmp_output_visitor_new(); g_assert(qov); @@ -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 **)" else: var_type = "" @@ -109,11 +110,11 @@ def generate_event_implement(api_name, event_name, params): } """, var_type = var_type, - var = c_name(argname), - type = type_name(argentry), - name = argname) + var=c_name(memb.name), + type=memb.type.c_name(), + name=memb.name) - if optional: + if memb.optional: pop_indent() ret += mcgen(""" } @@ -140,7 +141,7 @@ def generate_event_implement(api_name, event_name, params): event_enum_value = c_enum_const(event_enum_name, event_name)) # step 5: clean up - if params: + if params and params.members: ret += mcgen(""" clean: qmp_output_visitor_cleanup(qov); @@ -153,6 +154,24 @@ def generate_event_implement(api_name, event_name, params): return ret +class QAPISchemaGenEventVisitor(QAPISchemaVisitor): + def __init__(self): + self.decl = None + self.defn = None + self.event_names = None + def visit_begin(self): + self.decl = '' + self.defn = '' + self.event_names = [] + def visit_end(self): + self.decl += generate_enum(event_enum_name, self.event_names) + self.defn += generate_enum_lookup(event_enum_name, self.event_names) + def visit_event(self, name, info, data): + api_name = _generate_event_api_name(name, data) + self.decl += generate_event_declaration(api_name) + self.defn += generate_event_implement(api_name, name, data) + self.event_names.append(name) + (input_file, output_dir, do_c, do_h, prefix, dummy) = parse_command_line() c_comment = ''' @@ -206,32 +225,12 @@ fdecl.write(mcgen(''' ''', prefix=prefix)) -exprs = QAPISchema(input_file).get_exprs() - event_enum_name = c_name(prefix + "QAPIEvent", protect=False) -event_names = [] -for expr in exprs: - if expr.has_key('event'): - event_name = expr['event'] - params = expr.get('data') - if params and len(params) == 0: - params = None - - api_name = _generate_event_api_name(event_name, params) - ret = generate_event_declaration(api_name) - fdecl.write(ret) - - # We need an enum value per event - ret = generate_event_implement(api_name, event_name, params) - fdef.write(ret) - - # Record it, and generate enum later - event_names.append(event_name) - -ret = generate_enum(event_enum_name, event_names) -fdecl.write(ret) -ret = generate_enum_lookup(event_enum_name, event_names) -fdef.write(ret) +schema = QAPISchema(input_file) +gen = QAPISchemaGenEventVisitor() +schema.visit(gen) +fdef.write(gen.defn) +fdecl.write(gen.decl) close_output(fdef, fdecl) diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index b182174..90b4740 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -126,9 +126,6 @@ { 'alternate': '__org.qemu_x-Alt', 'data': { '__org.qemu_x-branch': 'str', 'b': '__org.qemu_x-Base' } } { 'event': '__ORG.QEMU_X-EVENT', 'data': '__org.qemu_x-Struct' } -# FIXME generated qapi_event_send___org_qemu_x_event() has only a -# parameter for data's member __org_qemu_x_member2, none for its base -# __org.qemu_x-Base's member __org_qemu_x_member1 { 'command': '__org.qemu_x-command', 'data': { 'a': ['__org.qemu_x-Enum'], 'b': ['__org.qemu_x-Struct'], 'c': '__org.qemu_x-Union2', 'd': '__org.qemu_x-Alt' },
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' } } 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) { QDict *qmp; @@ -224,6 +225,10 @@ void qapi_event_send___org_qemu_x_event( goto clean; } + visit_type___org_qemu_x_Enum(v, &__org_qemu_x_member1, "__org.qemu_x-member1", &local_err); + if (local_err) { + goto clean; + } visit_type_str(v, (char **)&__org_qemu_x_member2, "__org.qemu_x-member2", &local_err); if (local_err) { goto clean; 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(-)