| Message ID | 1384307094-5836-3-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 13 Nov 2013 09:44:52 +0800 Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > Nested structure is not supported now, so following define is not valid: > { 'event': 'EVENT_C', > 'data': { 'a': { 'a_a', 'str', 'a_b', 'str' }, 'b': 'int' } I think your general approach is reasonable, but there are a number of details to fix. The first one is documentation. This patch's changelog is quite bad, you don't say anything about what the does. You just mention a corner case which doesn't happen to work. Please, add full changelog explaining what this patch is about and please add examples of how an event entry would look like in the schema and maybe you could also add the important parts of a generated event function. Also, please add a "event" section to docs/writing-qmp-commands.txt (in a different patch). Secondly, for changes like this it's a very good idea to provide conversion examples (as patches, not as changelog doc) at least one or two so that we can see how it will look like. More below. > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > Makefile | 9 +- > Makefile.objs | 2 +- > qapi/Makefile.objs | 1 + > scripts/qapi-event.py | 355 +++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 363 insertions(+), 4 deletions(-) > create mode 100644 scripts/qapi-event.py > > diff --git a/Makefile b/Makefile > index b15003f..a3e465f 100644 > --- a/Makefile > +++ b/Makefile > @@ -38,8 +38,8 @@ endif > endif > > GENERATED_HEADERS = config-host.h qemu-options.def > -GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h > -GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c > +GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-event.h > +GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-event.c > > GENERATED_HEADERS += trace/generated-events.h > GENERATED_SOURCES += trace/generated-events.c > @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y) > # Build libraries > > libqemustub.a: $(stub-obj-y) > -libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o > +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o qapi-event.o > > ###################################################################### > > @@ -219,6 +219,9 @@ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py) > qapi-visit.c qapi-visit.h :\ > $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py) > $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o "." -b < $<, " GEN $@") > +qapi-event.c qapi-event.h :\ > +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-event.py $(qapi-py) > + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-event.py $(gen-out-type) -o "." -b < $<, " GEN $@") > qmp-commands.h qmp-marshal.c :\ > $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py) > $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, " GEN $@") > diff --git a/Makefile.objs b/Makefile.objs > index 2b6c1fe..33f5950 100644 > --- a/Makefile.objs > +++ b/Makefile.objs > @@ -12,7 +12,7 @@ block-obj-y += main-loop.o iohandler.o qemu-timer.o > block-obj-$(CONFIG_POSIX) += aio-posix.o > block-obj-$(CONFIG_WIN32) += aio-win32.o > block-obj-y += block/ > -block-obj-y += qapi-types.o qapi-visit.o > +block-obj-y += qapi-types.o qapi-visit.o qapi-event.o > block-obj-y += qemu-io-cmds.o > > block-obj-y += qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o > diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs > index 1f9c973..d14b769 100644 > --- a/qapi/Makefile.objs > +++ b/qapi/Makefile.objs > @@ -3,3 +3,4 @@ util-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o > util-obj-y += string-input-visitor.o string-output-visitor.o > > util-obj-y += opts-visitor.o > +util-obj-y += qmp-event.o > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > new file mode 100644 > index 0000000..4c6a0fe I didn't review this hunk, but this series doesn't build for me: CC qapi/string-output-visitor.o CC qapi/opts-visitor.o make: *** No rule to make target `qapi/qmp-event.o', needed by `libqemuutil.a'. Stop. ~/work/src/upstream/qmp-unstable/build (tmp|AM)/ > --- /dev/null > +++ b/scripts/qapi-event.py > @@ -0,0 +1,355 @@ > +# > +# QAPI event generator > +# > +# Copyright IBM, Corp. 2013 > +# > +# Authors: > +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> > +# > +# This work is licensed under the terms of the GNU GPLv2. > +# See the COPYING.LIB file in the top-level directory. > + > +from ordereddict import OrderedDict > +from qapi import * > +import sys > +import os > +import getopt > +import errno > + > +def _generate_event_api_name_with_params(event_name, params): > + api_name = "void qapi_event_gen_%s(" % c_fun(event_name); I'd prefer a name like qmp_notify_NAME() or qmp_send_event_NAME(). > + l = len(api_name) > + > + for argname, argentry, optional, structured in parse_args(params): > + if structured: > + sys.stderr.write("Nested structure define in event is not " > + "supported now, event '%s', argname '%s'\n" % > + (event_name, argname)) > + sys.exit(1) > + continue > + > + if optional: > + api_name += "bool has_%s,\n" % c_var(argname) > + api_name += "".ljust(l) > + > + if argentry == "str": > + api_name += "const " > + api_name += "%s %s,\n" % (c_type(argentry), c_var(argname)) > + api_name += "".ljust(l) > + > + api_name += "Error **errp)" > + return api_name; > + > +def _generate_event_implement_with_params(api_name, event_name, params): > + ret = mcgen(""" > + > +%(api_name)s > +{ > + QmpOutputVisitor *qov; > + Visitor *v; > + Error *err = NULL; We usually call it *local_err; > + QObject *obj; > + QDict *qmp = NULL; It's not needed to initialize qmp. > + > + if (!qapi_event_functions.emit) { Better to return an error here instead of silently failing. > + return; > + } > + > + qmp = qdict_new(); > + qdict_put(qmp, "event", qstring_from_str("%(event_name)s")); > + timestamp_put(qmp); Maybe it's a good idea to move this to a function? Say Qdict *qmp_build_event_dict(const char *event_name)? > + > + qov = qmp_output_visitor_new(); > + g_assert(qov); > + > + v = qmp_output_get_visitor(qov); > + g_assert(v); > + > + /* Fake visit, as if all member are under a structure */ > + visit_start_struct(v, NULL, "", "%(event_name)s", 0, &err); > + if (error_is_set(&err)) { > + goto clean; > + } > + > +""", > + api_name = api_name, > + event_name = event_name) > + > + for argname, argentry, optional, structured in parse_args(params): > + if structured: > + sys.stderr.write("Nested structure define in event is not " > + "supported now, event '%s', argname '%s'\n" % > + (event_name, argname)) > + sys.exit(1) > + continue > + > + if optional: > + ret += mcgen(""" > + if (has_%(var)s) { > +""", > + var = c_var(argname)) > + push_indent() > + > + if argentry == "str": > + var_type = "(char **)" > + else: > + var_type = "" > + > + ret += mcgen(""" > + visit_type_%(type)s(v, %(var_type)s&%(var)s, "%(name)s", &err); > + if (error_is_set(&err)) { > + goto clean; > + } > +""", > + var_type = var_type, > + var = c_var(argname), > + type = type_name(argentry), > + name = argname) > + > + if optional: > + pop_indent() > + ret += mcgen(""" > + } > +""") > + > + ret += mcgen(""" > + > + visit_end_struct(v, &err); > + if (error_is_set(&err)) { > + goto clean; > + } > + > + obj = qmp_output_get_qobject(qov); > + g_assert(obj != NULL); > + > + qdict_put_obj(qmp, "data", obj); > + > + qapi_event_functions.emit(qmp, &err); > + > + clean: > + error_propagate(errp, err); > + qmp_output_visitor_cleanup(qov); > + QDECREF(qmp); > +} > +""") > + > + return ret > + > +def _generate_event_api_name(event_name): > + return "void qapi_event_gen_%s(Error **errp)" % c_fun(event_name); > + > +def _generate_event_implement(api_name, event_name): > + return mcgen(""" > + > +%(api_name)s > +{ > + QDict *qmp = NULL; It's not needed to initialize qmp. > + > + if (!qapi_event_functions.emit) { > + return; > + } > + > + qmp = qdict_new(); > + qdict_put(qmp, "event", qstring_from_str("%(event_name)s")); > + timestamp_put(qmp); > + > + qapi_event_functions.emit(qmp, errp); > + > + QDECREF(qmp); > +} > +""", > + api_name = api_name, > + event_name = event_name) > + > + > +def generate_event_declaration(event_name, params): > + if params and len(params) > 0: > + api_name = _generate_event_api_name_with_params(event_name, params) > + else: > + api_name = _generate_event_api_name(event_name) > + > + return mcgen(''' > + > +%(api_name)s; > +''', > + api_name = api_name) > + > +def generate_event_implement(event_name, params): > + if params and len(params) > 0: > + api_name = _generate_event_api_name_with_params(event_name, params) > + ret = _generate_event_implement_with_params(api_name, > + event_name, > + params) > + > + else: > + api_name = _generate_event_api_name(event_name) > + ret = _generate_event_implement(api_name, > + event_name) > + return ret > + > +try: > + opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:", > + ["source", "header", "builtins", "prefix=", > + "output-dir="]) > +except getopt.GetoptError, err: > + print str(err) > + sys.exit(1) > + > +output_dir = "" > +prefix = "" > +c_file = 'qapi-event.c' > +h_file = 'qapi-event.h' > + > +do_c = False > +do_h = False > +do_builtins = False > + > +for o, a in opts: > + if o in ("-p", "--prefix"): > + prefix = a > + elif o in ("-o", "--output-dir"): > + output_dir = a + "/" > + elif o in ("-c", "--source"): > + do_c = True > + elif o in ("-h", "--header"): > + do_h = True > + elif o in ("-b", "--builtins"): > + do_builtins = True > + > +if not do_c and not do_h: > + do_c = True > + do_h = True > + > +c_file = output_dir + prefix + c_file > +h_file = output_dir + prefix + h_file > + > +try: > + os.makedirs(output_dir) > +except os.error, e: > + if e.errno != errno.EEXIST: > + raise > + > +def maybe_open(really, name, opt): > + if really: > + return open(name, opt) > + else: > + import StringIO > + return StringIO.StringIO() > + > +fdef = maybe_open(do_c, c_file, 'w') > +fdecl = maybe_open(do_h, h_file, 'w') > + > +fdef.write(mcgen(''' > +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */ > + > +/* > + * schema-defined QAPI event functions > + * > + * Copyright IBM, Corp. 2013 > + * > + * Authors: > + * Wenchao Xia <xiawenc@linux.vnet.ibm.com> > + * > + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. > + * See the COPYING.LIB file in the top-level directory. > + * > + */ > + > +#include "qemu-common.h" > +#include "%(header)s" > +#include "%(prefix)sqapi-visit.h" > +#include "qapi/qmp-output-visitor.h" > +#include "qapi/qmp/qstring.h" > +#include "qapi/qmp/qjson.h" > + > +#ifdef _WIN32 > +#include "sysemu/os-win32.h" > +#endif > + > +#ifdef CONFIG_POSIX > +#include "sysemu/os-posix.h" > +#endif > + > +typedef struct QAPIEventFunctions { > + void (*emit)(QDict *dict, Error **errp); > +} QAPIEventFunctions; > + > +QAPIEventFunctions qapi_event_functions; > + > +void qapi_event_set_func_emit(qapi_event_emit emit) > +{ > + qapi_event_functions.emit = emit; > +} > + > +static void timestamp_put(QDict *qdict) > +{ > + int err; > + QObject *obj; > + qemu_timeval tv; > + > + err = qemu_gettimeofday(&tv); > + if (err < 0) > + return; > + > + obj = qobject_from_jsonf("{ 'seconds': %(p)s" PRId64 ", " > + "'microseconds': %(p)s" PRId64 " }", > + (int64_t) tv.tv_sec, (int64_t) tv.tv_usec); > + qdict_put_obj(qdict, "timestamp", obj); > +} Any special reason to generate these functions? Maybe they could be put in qmp.c instead? > + > +''', > + prefix=prefix, header=basename(h_file), p="%")) > + > +fdecl.write(mcgen(''' > +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */ > + > +/* > + * schema-defined QAPI event function > + * > + * Copyright IBM, Corp. 2013 > + * > + * Authors: > + * Wenchao Xia <xiawenc@linux.vnet.ibm.com> > + * > + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. > + * See the COPYING.LIB file in the top-level directory. > + * > + */ > + > +#ifndef %(guard)s > +#define %(guard)s > + > +#include "qapi/qmp/qdict.h" > +#include "qapi/error.h" > +#include "qapi/visitor.h" > +#include "%(prefix)sqapi-types.h" > + > +typedef void (*qapi_event_emit)(QDict *d, Error **errp); > + > +void qapi_event_set_func_emit(qapi_event_emit emit); > + > +''', > + prefix=prefix, guard=guardname(h_file))) > + > +exprs = parse_schema(sys.stdin) > + > +for expr in exprs: > + if expr.has_key('event'): > + event_name = expr['event'] > + params = expr.get('data') > + > + ret = generate_event_declaration(event_name, params) > + fdecl.write(ret) > + > + ret = generate_event_implement(event_name, params) > + fdef.write(ret) > + > +fdecl.write(''' > +#endif > +''') > + > +fdecl.flush() > +fdecl.close() > + > +fdef.flush() > +fdef.close()
于 2013/11/28 8:48, Luiz Capitulino 写道: > On Wed, 13 Nov 2013 09:44:52 +0800 > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > >> Nested structure is not supported now, so following define is not valid: >> { 'event': 'EVENT_C', >> 'data': { 'a': { 'a_a', 'str', 'a_b', 'str' }, 'b': 'int' } > > I think your general approach is reasonable, but there are a number of > details to fix. > > The first one is documentation. This patch's changelog is quite bad, > you don't say anything about what the does. You just mention a corner > case which doesn't happen to work. Please, add full changelog explaining > what this patch is about and please add examples of how an event > entry would look like in the schema and maybe you could also add the > important parts of a generated event function. Also, please add a > "event" section to docs/writing-qmp-commands.txt (in a different patch). OK. > > Secondly, for changes like this it's a very good idea to provide > conversion examples (as patches, not as changelog doc) at least > one or two so that we can see how it will look like. > Will add some in the intro part. By the way I think test cases in patch 3 shows a bit. > More below. > >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> --- >> Makefile | 9 +- >> Makefile.objs | 2 +- >> qapi/Makefile.objs | 1 + >> scripts/qapi-event.py | 355 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 363 insertions(+), 4 deletions(-) >> create mode 100644 scripts/qapi-event.py >> >> diff --git a/Makefile b/Makefile >> index b15003f..a3e465f 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -38,8 +38,8 @@ endif >> endif >> >> GENERATED_HEADERS = config-host.h qemu-options.def >> -GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h >> -GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c >> +GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-event.h >> +GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-event.c >> >> GENERATED_HEADERS += trace/generated-events.h >> GENERATED_SOURCES += trace/generated-events.c >> @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y) >> # Build libraries >> >> libqemustub.a: $(stub-obj-y) >> -libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o >> +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o qapi-event.o >> >> ###################################################################### >> >> @@ -219,6 +219,9 @@ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py) >> qapi-visit.c qapi-visit.h :\ >> $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py) >> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o "." -b < $<, " GEN $@") >> +qapi-event.c qapi-event.h :\ >> +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-event.py $(qapi-py) >> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-event.py $(gen-out-type) -o "." -b < $<, " GEN $@") >> qmp-commands.h qmp-marshal.c :\ >> $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py) >> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, " GEN $@") >> diff --git a/Makefile.objs b/Makefile.objs >> index 2b6c1fe..33f5950 100644 >> --- a/Makefile.objs >> +++ b/Makefile.objs >> @@ -12,7 +12,7 @@ block-obj-y += main-loop.o iohandler.o qemu-timer.o >> block-obj-$(CONFIG_POSIX) += aio-posix.o >> block-obj-$(CONFIG_WIN32) += aio-win32.o >> block-obj-y += block/ >> -block-obj-y += qapi-types.o qapi-visit.o >> +block-obj-y += qapi-types.o qapi-visit.o qapi-event.o >> block-obj-y += qemu-io-cmds.o >> >> block-obj-y += qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o >> diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs >> index 1f9c973..d14b769 100644 >> --- a/qapi/Makefile.objs >> +++ b/qapi/Makefile.objs >> @@ -3,3 +3,4 @@ util-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o >> util-obj-y += string-input-visitor.o string-output-visitor.o >> >> util-obj-y += opts-visitor.o >> +util-obj-y += qmp-event.o >> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py >> new file mode 100644 >> index 0000000..4c6a0fe > > I didn't review this hunk, but this series doesn't build for me: > > CC qapi/string-output-visitor.o > CC qapi/opts-visitor.o > make: *** No rule to make target `qapi/qmp-event.o', needed by `libqemuutil.a'. Stop. > ~/work/src/upstream/qmp-unstable/build (tmp|AM)/ > A draft file I forgot to remove in Makefile, will fix. >> --- /dev/null >> +++ b/scripts/qapi-event.py >> @@ -0,0 +1,355 @@ >> +# >> +# QAPI event generator >> +# >> +# Copyright IBM, Corp. 2013 >> +# >> +# Authors: >> +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> +# >> +# This work is licensed under the terms of the GNU GPLv2. >> +# See the COPYING.LIB file in the top-level directory. >> + >> +from ordereddict import OrderedDict >> +from qapi import * >> +import sys >> +import os >> +import getopt >> +import errno >> + >> +def _generate_event_api_name_with_params(event_name, params): >> + api_name = "void qapi_event_gen_%s(" % c_fun(event_name); > > I'd prefer a name like qmp_notify_NAME() or qmp_send_event_NAME(). > do you think NAME should be capitalized as: qmp_notify_SHUTDOWN()? >> + l = len(api_name) >> + >> + for argname, argentry, optional, structured in parse_args(params): >> + if structured: >> + sys.stderr.write("Nested structure define in event is not " >> + "supported now, event '%s', argname '%s'\n" % >> + (event_name, argname)) >> + sys.exit(1) >> + continue >> + >> + if optional: >> + api_name += "bool has_%s,\n" % c_var(argname) >> + api_name += "".ljust(l) >> + >> + if argentry == "str": >> + api_name += "const " >> + api_name += "%s %s,\n" % (c_type(argentry), c_var(argname)) >> + api_name += "".ljust(l) >> + >> + api_name += "Error **errp)" >> + return api_name; >> + >> +def _generate_event_implement_with_params(api_name, event_name, params): >> + ret = mcgen(""" >> + >> +%(api_name)s >> +{ >> + QmpOutputVisitor *qov; >> + Visitor *v; >> + Error *err = NULL; > > We usually call it *local_err; > OK. >> + QObject *obj; >> + QDict *qmp = NULL; > > It's not needed to initialize qmp. > Will fix. >> + >> + if (!qapi_event_functions.emit) { > > Better to return an error here instead of silently failing. > The purpose is allowing emit=NULL and skip event code in that case. If err is set, caller can't distinguish it from real error case. caller: qmp_event_notify_SHUTDOWN(&err); if (error_is_set(&err)) { ... } err is always set when emit=NULL, but we may allow emit=NULL, for example, qemu-img will have emit=NULL. >> + return; >> + } >> + >> + qmp = qdict_new(); >> + qdict_put(qmp, "event", qstring_from_str("%(event_name)s")); >> + timestamp_put(qmp); > > Maybe it's a good idea to move this to a function? Say > Qdict *qmp_build_event_dict(const char *event_name)? > Seems good, will use it. >> + >> + qov = qmp_output_visitor_new(); >> + g_assert(qov); >> + >> + v = qmp_output_get_visitor(qov); >> + g_assert(v); >> + >> + /* Fake visit, as if all member are under a structure */ >> + visit_start_struct(v, NULL, "", "%(event_name)s", 0, &err); >> + if (error_is_set(&err)) { >> + goto clean; >> + } >> + >> +""", >> + api_name = api_name, >> + event_name = event_name) >> + >> + for argname, argentry, optional, structured in parse_args(params): >> + if structured: >> + sys.stderr.write("Nested structure define in event is not " >> + "supported now, event '%s', argname '%s'\n" % >> + (event_name, argname)) >> + sys.exit(1) >> + continue >> + >> + if optional: >> + ret += mcgen(""" >> + if (has_%(var)s) { >> +""", >> + var = c_var(argname)) >> + push_indent() >> + >> + if argentry == "str": >> + var_type = "(char **)" >> + else: >> + var_type = "" >> + >> + ret += mcgen(""" >> + visit_type_%(type)s(v, %(var_type)s&%(var)s, "%(name)s", &err); >> + if (error_is_set(&err)) { >> + goto clean; >> + } >> +""", >> + var_type = var_type, >> + var = c_var(argname), >> + type = type_name(argentry), >> + name = argname) >> + >> + if optional: >> + pop_indent() >> + ret += mcgen(""" >> + } >> +""") >> + >> + ret += mcgen(""" >> + >> + visit_end_struct(v, &err); >> + if (error_is_set(&err)) { >> + goto clean; >> + } >> + >> + obj = qmp_output_get_qobject(qov); >> + g_assert(obj != NULL); >> + >> + qdict_put_obj(qmp, "data", obj); >> + >> + qapi_event_functions.emit(qmp, &err); >> + >> + clean: >> + error_propagate(errp, err); >> + qmp_output_visitor_cleanup(qov); >> + QDECREF(qmp); >> +} >> +""") >> + >> + return ret >> + >> +def _generate_event_api_name(event_name): >> + return "void qapi_event_gen_%s(Error **errp)" % c_fun(event_name); >> + >> +def _generate_event_implement(api_name, event_name): >> + return mcgen(""" >> + >> +%(api_name)s >> +{ >> + QDict *qmp = NULL; > > It's not needed to initialize qmp. > OK. >> + >> + if (!qapi_event_functions.emit) { >> + return; >> + } >> + >> + qmp = qdict_new(); >> + qdict_put(qmp, "event", qstring_from_str("%(event_name)s")); >> + timestamp_put(qmp); >> + >> + qapi_event_functions.emit(qmp, errp); >> + >> + QDECREF(qmp); >> +} >> +""", >> + api_name = api_name, >> + event_name = event_name) >> + >> + >> +def generate_event_declaration(event_name, params): >> + if params and len(params) > 0: >> + api_name = _generate_event_api_name_with_params(event_name, params) >> + else: >> + api_name = _generate_event_api_name(event_name) >> + >> + return mcgen(''' >> + >> +%(api_name)s; >> +''', >> + api_name = api_name) >> + >> +def generate_event_implement(event_name, params): >> + if params and len(params) > 0: >> + api_name = _generate_event_api_name_with_params(event_name, params) >> + ret = _generate_event_implement_with_params(api_name, >> + event_name, >> + params) >> + >> + else: >> + api_name = _generate_event_api_name(event_name) >> + ret = _generate_event_implement(api_name, >> + event_name) >> + return ret >> + >> +try: >> + opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:", >> + ["source", "header", "builtins", "prefix=", >> + "output-dir="]) >> +except getopt.GetoptError, err: >> + print str(err) >> + sys.exit(1) >> + >> +output_dir = "" >> +prefix = "" >> +c_file = 'qapi-event.c' >> +h_file = 'qapi-event.h' >> + >> +do_c = False >> +do_h = False >> +do_builtins = False >> + >> +for o, a in opts: >> + if o in ("-p", "--prefix"): >> + prefix = a >> + elif o in ("-o", "--output-dir"): >> + output_dir = a + "/" >> + elif o in ("-c", "--source"): >> + do_c = True >> + elif o in ("-h", "--header"): >> + do_h = True >> + elif o in ("-b", "--builtins"): >> + do_builtins = True >> + >> +if not do_c and not do_h: >> + do_c = True >> + do_h = True >> + >> +c_file = output_dir + prefix + c_file >> +h_file = output_dir + prefix + h_file >> + >> +try: >> + os.makedirs(output_dir) >> +except os.error, e: >> + if e.errno != errno.EEXIST: >> + raise >> + >> +def maybe_open(really, name, opt): >> + if really: >> + return open(name, opt) >> + else: >> + import StringIO >> + return StringIO.StringIO() >> + >> +fdef = maybe_open(do_c, c_file, 'w') >> +fdecl = maybe_open(do_h, h_file, 'w') >> + >> +fdef.write(mcgen(''' >> +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */ >> + >> +/* >> + * schema-defined QAPI event functions >> + * >> + * Copyright IBM, Corp. 2013 >> + * >> + * Authors: >> + * Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> + * >> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. >> + * See the COPYING.LIB file in the top-level directory. >> + * >> + */ >> + >> +#include "qemu-common.h" >> +#include "%(header)s" >> +#include "%(prefix)sqapi-visit.h" >> +#include "qapi/qmp-output-visitor.h" >> +#include "qapi/qmp/qstring.h" >> +#include "qapi/qmp/qjson.h" >> + >> +#ifdef _WIN32 >> +#include "sysemu/os-win32.h" >> +#endif >> + >> +#ifdef CONFIG_POSIX >> +#include "sysemu/os-posix.h" >> +#endif >> + >> +typedef struct QAPIEventFunctions { >> + void (*emit)(QDict *dict, Error **errp); >> +} QAPIEventFunctions; >> + >> +QAPIEventFunctions qapi_event_functions; >> + >> +void qapi_event_set_func_emit(qapi_event_emit emit) >> +{ >> + qapi_event_functions.emit = emit; >> +} >> + >> +static void timestamp_put(QDict *qdict) >> +{ >> + int err; >> + QObject *obj; >> + qemu_timeval tv; >> + >> + err = qemu_gettimeofday(&tv); >> + if (err < 0) >> + return; >> + >> + obj = qobject_from_jsonf("{ 'seconds': %(p)s" PRId64 ", " >> + "'microseconds': %(p)s" PRId64 " }", >> + (int64_t) tv.tv_sec, (int64_t) tv.tv_usec); >> + qdict_put_obj(qdict, "timestamp", obj); >> +} > > Any special reason to generate these functions? Maybe they could be > put in qmp.c instead? > No, I just found no better place to go. qmp.c seems irrelevent to event, how about new file qapi/qmp-event.c?(which I used before and triggered the build error you met) >> + >> +''', >> + prefix=prefix, header=basename(h_file), p="%")) >> + >> +fdecl.write(mcgen(''' >> +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */ >> + >> +/* >> + * schema-defined QAPI event function >> + * >> + * Copyright IBM, Corp. 2013 >> + * >> + * Authors: >> + * Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> + * >> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. >> + * See the COPYING.LIB file in the top-level directory. >> + * >> + */ >> + >> +#ifndef %(guard)s >> +#define %(guard)s >> + >> +#include "qapi/qmp/qdict.h" >> +#include "qapi/error.h" >> +#include "qapi/visitor.h" >> +#include "%(prefix)sqapi-types.h" >> + >> +typedef void (*qapi_event_emit)(QDict *d, Error **errp); >> + >> +void qapi_event_set_func_emit(qapi_event_emit emit); >> + >> +''', >> + prefix=prefix, guard=guardname(h_file))) >> + >> +exprs = parse_schema(sys.stdin) >> + >> +for expr in exprs: >> + if expr.has_key('event'): >> + event_name = expr['event'] >> + params = expr.get('data') >> + >> + ret = generate_event_declaration(event_name, params) >> + fdecl.write(ret) >> + >> + ret = generate_event_implement(event_name, params) >> + fdef.write(ret) >> + >> +fdecl.write(''' >> +#endif >> +''') >> + >> +fdecl.flush() >> +fdecl.close() >> + >> +fdef.flush() >> +fdef.close() >
On Thu, 28 Nov 2013 15:16:08 +0800 Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > 于 2013/11/28 8:48, Luiz Capitulino 写道: > > On Wed, 13 Nov 2013 09:44:52 +0800 > > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > > > >> Nested structure is not supported now, so following define is not valid: > >> { 'event': 'EVENT_C', > >> 'data': { 'a': { 'a_a', 'str', 'a_b', 'str' }, 'b': 'int' } > > > > I think your general approach is reasonable, but there are a number of > > details to fix. > > > > The first one is documentation. This patch's changelog is quite bad, > > you don't say anything about what the does. You just mention a corner > > case which doesn't happen to work. Please, add full changelog explaining > > what this patch is about and please add examples of how an event > > entry would look like in the schema and maybe you could also add the > > important parts of a generated event function. Also, please add a > > "event" section to docs/writing-qmp-commands.txt (in a different patch). > > OK. > > > > > Secondly, for changes like this it's a very good idea to provide > > conversion examples (as patches, not as changelog doc) at least > > one or two so that we can see how it will look like. > > > > Will add some in the intro part. By the way I think test > cases in patch 3 shows a bit. I didn't look at the test to be honest (it didn't apply and I concentrated on the second patch). Having a test is awesome, but you still have to provide at least one conversion so that we can see how it will look like. > > > More below. > > > > >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > >> --- > >> Makefile | 9 +- > >> Makefile.objs | 2 +- > >> qapi/Makefile.objs | 1 + > >> scripts/qapi-event.py | 355 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> 4 files changed, 363 insertions(+), 4 deletions(-) > >> create mode 100644 scripts/qapi-event.py > >> > >> diff --git a/Makefile b/Makefile > >> index b15003f..a3e465f 100644 > >> --- a/Makefile > >> +++ b/Makefile > >> @@ -38,8 +38,8 @@ endif > >> endif > >> > >> GENERATED_HEADERS = config-host.h qemu-options.def > >> -GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h > >> -GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c > >> +GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-event.h > >> +GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-event.c > >> > >> GENERATED_HEADERS += trace/generated-events.h > >> GENERATED_SOURCES += trace/generated-events.c > >> @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y) > >> # Build libraries > >> > >> libqemustub.a: $(stub-obj-y) > >> -libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o > >> +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o qapi-event.o > >> > >> ###################################################################### > >> > >> @@ -219,6 +219,9 @@ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py) > >> qapi-visit.c qapi-visit.h :\ > >> $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py) > >> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o "." -b < $<, " GEN $@") > >> +qapi-event.c qapi-event.h :\ > >> +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-event.py $(qapi-py) > >> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-event.py $(gen-out-type) -o "." -b < $<, " GEN $@") > >> qmp-commands.h qmp-marshal.c :\ > >> $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py) > >> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, " GEN $@") > >> diff --git a/Makefile.objs b/Makefile.objs > >> index 2b6c1fe..33f5950 100644 > >> --- a/Makefile.objs > >> +++ b/Makefile.objs > >> @@ -12,7 +12,7 @@ block-obj-y += main-loop.o iohandler.o qemu-timer.o > >> block-obj-$(CONFIG_POSIX) += aio-posix.o > >> block-obj-$(CONFIG_WIN32) += aio-win32.o > >> block-obj-y += block/ > >> -block-obj-y += qapi-types.o qapi-visit.o > >> +block-obj-y += qapi-types.o qapi-visit.o qapi-event.o > >> block-obj-y += qemu-io-cmds.o > >> > >> block-obj-y += qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o > >> diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs > >> index 1f9c973..d14b769 100644 > >> --- a/qapi/Makefile.objs > >> +++ b/qapi/Makefile.objs > >> @@ -3,3 +3,4 @@ util-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o > >> util-obj-y += string-input-visitor.o string-output-visitor.o > >> > >> util-obj-y += opts-visitor.o > >> +util-obj-y += qmp-event.o > >> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > >> new file mode 100644 > >> index 0000000..4c6a0fe > > > > I didn't review this hunk, but this series doesn't build for me: > > > > CC qapi/string-output-visitor.o > > CC qapi/opts-visitor.o > > make: *** No rule to make target `qapi/qmp-event.o', needed by `libqemuutil.a'. Stop. > > ~/work/src/upstream/qmp-unstable/build (tmp|AM)/ > > > A draft file I forgot to remove in Makefile, will fix. > > >> --- /dev/null > >> +++ b/scripts/qapi-event.py > >> @@ -0,0 +1,355 @@ > >> +# > >> +# QAPI event generator > >> +# > >> +# Copyright IBM, Corp. 2013 > >> +# > >> +# Authors: > >> +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> > >> +# > >> +# This work is licensed under the terms of the GNU GPLv2. > >> +# See the COPYING.LIB file in the top-level directory. > >> + > >> +from ordereddict import OrderedDict > >> +from qapi import * > >> +import sys > >> +import os > >> +import getopt > >> +import errno > >> + > >> +def _generate_event_api_name_with_params(event_name, params): > >> + api_name = "void qapi_event_gen_%s(" % c_fun(event_name); > > > > I'd prefer a name like qmp_notify_NAME() or qmp_send_event_NAME(). > > > > do you think NAME should be capitalized as: > qmp_notify_SHUTDOWN()? No way :) This kind of detail of the wire format shouldn't be part of the C interface (and this is an unfortunate detail, btw). > >> + l = len(api_name) > >> + > >> + for argname, argentry, optional, structured in parse_args(params): > >> + if structured: > >> + sys.stderr.write("Nested structure define in event is not " > >> + "supported now, event '%s', argname '%s'\n" % > >> + (event_name, argname)) > >> + sys.exit(1) > >> + continue > >> + > >> + if optional: > >> + api_name += "bool has_%s,\n" % c_var(argname) > >> + api_name += "".ljust(l) > >> + > >> + if argentry == "str": > >> + api_name += "const " > >> + api_name += "%s %s,\n" % (c_type(argentry), c_var(argname)) > >> + api_name += "".ljust(l) > >> + > >> + api_name += "Error **errp)" > >> + return api_name; > >> + > >> +def _generate_event_implement_with_params(api_name, event_name, params): > >> + ret = mcgen(""" > >> + > >> +%(api_name)s > >> +{ > >> + QmpOutputVisitor *qov; > >> + Visitor *v; > >> + Error *err = NULL; > > > > We usually call it *local_err; > > > > OK. > > >> + QObject *obj; > >> + QDict *qmp = NULL; > > > > It's not needed to initialize qmp. > > > > Will fix. > > >> + > >> + if (!qapi_event_functions.emit) { > > > > Better to return an error here instead of silently failing. > > > > The purpose is allowing emit=NULL and skip event code in that case. But the code will do nothing and the caller won't know that. Actually, I wonder if the code should even abort() in such a case, as emit=NULL would be a programming today. > If err is set, caller can't distinguish it from real error case. > caller: > qmp_event_notify_SHUTDOWN(&err); > if (error_is_set(&err)) { > ... > } > > err is always set when emit=NULL, but we may allow emit=NULL, > for example, qemu-img will have emit=NULL. > > >> + return; > >> + } > >> + > >> + qmp = qdict_new(); > >> + qdict_put(qmp, "event", qstring_from_str("%(event_name)s")); > >> + timestamp_put(qmp); > > > > Maybe it's a good idea to move this to a function? Say > > Qdict *qmp_build_event_dict(const char *event_name)? > > > > Seems good, will use it. > > >> + > >> + qov = qmp_output_visitor_new(); > >> + g_assert(qov); > >> + > >> + v = qmp_output_get_visitor(qov); > >> + g_assert(v); > >> + > >> + /* Fake visit, as if all member are under a structure */ > >> + visit_start_struct(v, NULL, "", "%(event_name)s", 0, &err); > >> + if (error_is_set(&err)) { > >> + goto clean; > >> + } > >> + > >> +""", > >> + api_name = api_name, > >> + event_name = event_name) > >> + > >> + for argname, argentry, optional, structured in parse_args(params): > >> + if structured: > >> + sys.stderr.write("Nested structure define in event is not " > >> + "supported now, event '%s', argname '%s'\n" % > >> + (event_name, argname)) > >> + sys.exit(1) > >> + continue > >> + > >> + if optional: > >> + ret += mcgen(""" > >> + if (has_%(var)s) { > >> +""", > >> + var = c_var(argname)) > >> + push_indent() > >> + > >> + if argentry == "str": > >> + var_type = "(char **)" > >> + else: > >> + var_type = "" > >> + > >> + ret += mcgen(""" > >> + visit_type_%(type)s(v, %(var_type)s&%(var)s, "%(name)s", &err); > >> + if (error_is_set(&err)) { > >> + goto clean; > >> + } > >> +""", > >> + var_type = var_type, > >> + var = c_var(argname), > >> + type = type_name(argentry), > >> + name = argname) > >> + > >> + if optional: > >> + pop_indent() > >> + ret += mcgen(""" > >> + } > >> +""") > >> + > >> + ret += mcgen(""" > >> + > >> + visit_end_struct(v, &err); > >> + if (error_is_set(&err)) { > >> + goto clean; > >> + } > >> + > >> + obj = qmp_output_get_qobject(qov); > >> + g_assert(obj != NULL); > >> + > >> + qdict_put_obj(qmp, "data", obj); > >> + > >> + qapi_event_functions.emit(qmp, &err); > >> + > >> + clean: > >> + error_propagate(errp, err); > >> + qmp_output_visitor_cleanup(qov); > >> + QDECREF(qmp); > >> +} > >> +""") > >> + > >> + return ret > >> + > >> +def _generate_event_api_name(event_name): > >> + return "void qapi_event_gen_%s(Error **errp)" % c_fun(event_name); > >> + > >> +def _generate_event_implement(api_name, event_name): > >> + return mcgen(""" > >> + > >> +%(api_name)s > >> +{ > >> + QDict *qmp = NULL; > > > > It's not needed to initialize qmp. > > > > OK. > > >> + > >> + if (!qapi_event_functions.emit) { > >> + return; > >> + } > >> + > >> + qmp = qdict_new(); > >> + qdict_put(qmp, "event", qstring_from_str("%(event_name)s")); > >> + timestamp_put(qmp); > >> + > >> + qapi_event_functions.emit(qmp, errp); > >> + > >> + QDECREF(qmp); > >> +} > >> +""", > >> + api_name = api_name, > >> + event_name = event_name) > >> + > >> + > >> +def generate_event_declaration(event_name, params): > >> + if params and len(params) > 0: > >> + api_name = _generate_event_api_name_with_params(event_name, params) > >> + else: > >> + api_name = _generate_event_api_name(event_name) > >> + > >> + return mcgen(''' > >> + > >> +%(api_name)s; > >> +''', > >> + api_name = api_name) > >> + > >> +def generate_event_implement(event_name, params): > >> + if params and len(params) > 0: > >> + api_name = _generate_event_api_name_with_params(event_name, params) > >> + ret = _generate_event_implement_with_params(api_name, > >> + event_name, > >> + params) > >> + > >> + else: > >> + api_name = _generate_event_api_name(event_name) > >> + ret = _generate_event_implement(api_name, > >> + event_name) > >> + return ret > >> + > >> +try: > >> + opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:", > >> + ["source", "header", "builtins", "prefix=", > >> + "output-dir="]) > >> +except getopt.GetoptError, err: > >> + print str(err) > >> + sys.exit(1) > >> + > >> +output_dir = "" > >> +prefix = "" > >> +c_file = 'qapi-event.c' > >> +h_file = 'qapi-event.h' > >> + > >> +do_c = False > >> +do_h = False > >> +do_builtins = False > >> + > >> +for o, a in opts: > >> + if o in ("-p", "--prefix"): > >> + prefix = a > >> + elif o in ("-o", "--output-dir"): > >> + output_dir = a + "/" > >> + elif o in ("-c", "--source"): > >> + do_c = True > >> + elif o in ("-h", "--header"): > >> + do_h = True > >> + elif o in ("-b", "--builtins"): > >> + do_builtins = True > >> + > >> +if not do_c and not do_h: > >> + do_c = True > >> + do_h = True > >> + > >> +c_file = output_dir + prefix + c_file > >> +h_file = output_dir + prefix + h_file > >> + > >> +try: > >> + os.makedirs(output_dir) > >> +except os.error, e: > >> + if e.errno != errno.EEXIST: > >> + raise > >> + > >> +def maybe_open(really, name, opt): > >> + if really: > >> + return open(name, opt) > >> + else: > >> + import StringIO > >> + return StringIO.StringIO() > >> + > >> +fdef = maybe_open(do_c, c_file, 'w') > >> +fdecl = maybe_open(do_h, h_file, 'w') > >> + > >> +fdef.write(mcgen(''' > >> +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */ > >> + > >> +/* > >> + * schema-defined QAPI event functions > >> + * > >> + * Copyright IBM, Corp. 2013 > >> + * > >> + * Authors: > >> + * Wenchao Xia <xiawenc@linux.vnet.ibm.com> > >> + * > >> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. > >> + * See the COPYING.LIB file in the top-level directory. > >> + * > >> + */ > >> + > >> +#include "qemu-common.h" > >> +#include "%(header)s" > >> +#include "%(prefix)sqapi-visit.h" > >> +#include "qapi/qmp-output-visitor.h" > >> +#include "qapi/qmp/qstring.h" > >> +#include "qapi/qmp/qjson.h" > >> + > >> +#ifdef _WIN32 > >> +#include "sysemu/os-win32.h" > >> +#endif > >> + > >> +#ifdef CONFIG_POSIX > >> +#include "sysemu/os-posix.h" > >> +#endif > >> + > >> +typedef struct QAPIEventFunctions { > >> + void (*emit)(QDict *dict, Error **errp); > >> +} QAPIEventFunctions; > >> + > >> +QAPIEventFunctions qapi_event_functions; > >> + > >> +void qapi_event_set_func_emit(qapi_event_emit emit) > >> +{ > >> + qapi_event_functions.emit = emit; > >> +} > >> + > >> +static void timestamp_put(QDict *qdict) > >> +{ > >> + int err; > >> + QObject *obj; > >> + qemu_timeval tv; > >> + > >> + err = qemu_gettimeofday(&tv); > >> + if (err < 0) > >> + return; > >> + > >> + obj = qobject_from_jsonf("{ 'seconds': %(p)s" PRId64 ", " > >> + "'microseconds': %(p)s" PRId64 " }", > >> + (int64_t) tv.tv_sec, (int64_t) tv.tv_usec); > >> + qdict_put_obj(qdict, "timestamp", obj); > >> +} > > > > Any special reason to generate these functions? Maybe they could be > > put in qmp.c instead? > > > > No, I just found no better place to go. qmp.c seems irrelevent to > event, how about new file qapi/qmp-event.c?(which I used before and > triggered the build error you met) That works for me. > > >> + > >> +''', > >> + prefix=prefix, header=basename(h_file), p="%")) > >> + > >> +fdecl.write(mcgen(''' > >> +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */ > >> + > >> +/* > >> + * schema-defined QAPI event function > >> + * > >> + * Copyright IBM, Corp. 2013 > >> + * > >> + * Authors: > >> + * Wenchao Xia <xiawenc@linux.vnet.ibm.com> > >> + * > >> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. > >> + * See the COPYING.LIB file in the top-level directory. > >> + * > >> + */ > >> + > >> +#ifndef %(guard)s > >> +#define %(guard)s > >> + > >> +#include "qapi/qmp/qdict.h" > >> +#include "qapi/error.h" > >> +#include "qapi/visitor.h" > >> +#include "%(prefix)sqapi-types.h" > >> + > >> +typedef void (*qapi_event_emit)(QDict *d, Error **errp); > >> + > >> +void qapi_event_set_func_emit(qapi_event_emit emit); > >> + > >> +''', > >> + prefix=prefix, guard=guardname(h_file))) > >> + > >> +exprs = parse_schema(sys.stdin) > >> + > >> +for expr in exprs: > >> + if expr.has_key('event'): > >> + event_name = expr['event'] > >> + params = expr.get('data') > >> + > >> + ret = generate_event_declaration(event_name, params) > >> + fdecl.write(ret) > >> + > >> + ret = generate_event_implement(event_name, params) > >> + fdef.write(ret) > >> + > >> +fdecl.write(''' > >> +#endif > >> +''') > >> + > >> +fdecl.flush() > >> +fdecl.close() > >> + > >> +fdef.flush() > >> +fdef.close() > > >
>> >>>> + >>>> + if (!qapi_event_functions.emit) { >>> >>> Better to return an error here instead of silently failing. >>> >> >> The purpose is allowing emit=NULL and skip event code in that case. > > But the code will do nothing and the caller won't know that. > Now the caller also won't know that useless code will be executed, when qemu-img link with stub of monitor_event functions. :) > Actually, I wonder if the code should even abort() in such a case, > as emit=NULL would be a programming today. > I am not sure why the code should always do something. The code may actually take CPU resource to do nothing meanful, such as build up a qdict and release it later, when emit is not a valid function. So I did this as an improvement: check emit function ahead to escape useless work.
于 2013/12/2 14:48, Wenchao Xia 写道: > >>> >>>>> + >>>>> + if (!qapi_event_functions.emit) { >>>> >>>> Better to return an error here instead of silently failing. >>>> >>> >>> The purpose is allowing emit=NULL and skip event code in that case. >> >> But the code will do nothing and the caller won't know that. >> > Now the caller also won't know that useless code will be executed, > when qemu-img link with stub of monitor_event functions. :) > >> Actually, I wonder if the code should even abort() in such a case, >> as emit=NULL would be a programming today. >> > > I am not sure why the code should always do something. The code may > actually take CPU resource to do nothing meanful, such as build up a > qdict and release it later, when emit is not a valid function. So I > did this as an improvement: check emit function ahead to escape useless > work. > > Luiz, do you agree with me?
On 11/12/2013 06:44 PM, Wenchao Xia wrote: > Nested structure is not supported now, so following define is not valid: > { 'event': 'EVENT_C', > 'data': { 'a': { 'a_a', 'str', 'a_b', 'str' }, 'b': 'int' } But what IS valid? You need to document this in docs/qapi-code-gen.txt at a bare minimum. This patch series is hard to review, and still has the RFC subject line. At this point, I think it's worth rebasing and resending what you have; even if it needs more review, it will at least get it to a state that is easier to review. > +++ b/scripts/qapi-event.py > @@ -0,0 +1,355 @@ > +# > +# QAPI event generator > +# > +# Copyright IBM, Corp. 2013 > +# > +# Authors: > +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> > +# > +# This work is licensed under the terms of the GNU GPLv2. Can you please use GPLv2+ (that is, add the "or later" clause)? We already have GPLv2-only code, but I don't want to increase the size of that unfortunate license choice.
Am 13.12.2013 um 14:31 hat Eric Blake geschrieben: > On 11/12/2013 06:44 PM, Wenchao Xia wrote: > > +++ b/scripts/qapi-event.py > > @@ -0,0 +1,355 @@ > > +# > > +# QAPI event generator > > +# > > +# Copyright IBM, Corp. 2013 > > +# > > +# Authors: > > +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> > > +# > > +# This work is licensed under the terms of the GNU GPLv2. > > Can you please use GPLv2+ (that is, add the "or later" clause)? We > already have GPLv2-only code, but I don't want to increase the size of > that unfortunate license choice. In fact, it's even worse: +# This work is licensed under the terms of the GNU GPLv2. +# See the COPYING.LIB file in the top-level directory. These two lines contradict each other, COPYING.LIB contains the LGPL 2.1. The same bad license header is in the other QAPI generator scripts, so it's only copy&paste here. This doesn't make things easier, because if things are copied, the license of the source must be respected. And it seems rather dubious to me what this license actually is. If it's GPLv2-only, we can't just change it in the new copy. Kevin
于 2013/12/13 21:43, Kevin Wolf 写道: > Am 13.12.2013 um 14:31 hat Eric Blake geschrieben: >> On 11/12/2013 06:44 PM, Wenchao Xia wrote: >>> +++ b/scripts/qapi-event.py >>> @@ -0,0 +1,355 @@ >>> +# >>> +# QAPI event generator >>> +# >>> +# Copyright IBM, Corp. 2013 >>> +# >>> +# Authors: >>> +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> >>> +# >>> +# This work is licensed under the terms of the GNU GPLv2. >> >> Can you please use GPLv2+ (that is, add the "or later" clause)? We >> already have GPLv2-only code, but I don't want to increase the size of >> that unfortunate license choice. > > In fact, it's even worse: > > +# This work is licensed under the terms of the GNU GPLv2. > +# See the COPYING.LIB file in the top-level directory. > > These two lines contradict each other, COPYING.LIB contains the > LGPL 2.1. The same bad license header is in the other QAPI generator > scripts, so it's only copy&paste here. > > This doesn't make things easier, because if things are copied, the > license of the source must be respected. And it seems rather dubious to > me what this license actually is. If it's GPLv2-only, we can't just > change it in the new copy. > > Kevin > ah..I am bad in license problem, will use the doc as LGPL from other file.
于 2013/12/13 21:31, Eric Blake 写道: > On 11/12/2013 06:44 PM, Wenchao Xia wrote: >> Nested structure is not supported now, so following define is not valid: >> { 'event': 'EVENT_C', >> 'data': { 'a': { 'a_a', 'str', 'a_b', 'str' }, 'b': 'int' } > > But what IS valid? You need to document this in docs/qapi-code-gen.txt > at a bare minimum. > > This patch series is hard to review, and still has the RFC subject line. > At this point, I think it's worth rebasing and resending what you have; > even if it needs more review, it will at least get it to a state that is > easier to review. > Sure, I will wait some time and respin. >> +++ b/scripts/qapi-event.py >> @@ -0,0 +1,355 @@ >> +# >> +# QAPI event generator >> +# >> +# Copyright IBM, Corp. 2013 >> +# >> +# Authors: >> +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> +# >> +# This work is licensed under the terms of the GNU GPLv2. > > Can you please use GPLv2+ (that is, add the "or later" clause)? We > already have GPLv2-only code, but I don't want to increase the size of > that unfortunate license choice. >
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes: > 于 2013/12/13 21:43, Kevin Wolf 写道: >> Am 13.12.2013 um 14:31 hat Eric Blake geschrieben: >>> On 11/12/2013 06:44 PM, Wenchao Xia wrote: >>>> +++ b/scripts/qapi-event.py >>>> @@ -0,0 +1,355 @@ >>>> +# >>>> +# QAPI event generator >>>> +# >>>> +# Copyright IBM, Corp. 2013 >>>> +# >>>> +# Authors: >>>> +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> >>>> +# >>>> +# This work is licensed under the terms of the GNU GPLv2. >>> >>> Can you please use GPLv2+ (that is, add the "or later" clause)? We >>> already have GPLv2-only code, but I don't want to increase the size of >>> that unfortunate license choice. >> >> In fact, it's even worse: >> >> +# This work is licensed under the terms of the GNU GPLv2. >> +# See the COPYING.LIB file in the top-level directory. >> >> These two lines contradict each other, COPYING.LIB contains the >> LGPL 2.1. The same bad license header is in the other QAPI generator >> scripts, so it's only copy&paste here. >> >> This doesn't make things easier, because if things are copied, the >> license of the source must be respected. And it seems rather dubious to >> me what this license actually is. If it's GPLv2-only, we can't just >> change it in the new copy. >> >> Kevin >> > ah..I am bad in license problem, will use the doc as LGPL from other > file. Please use GPLv2+ unless you have a specific reason for another license.
[Licensing problem, cc: Anthony] Kevin Wolf <kwolf@redhat.com> writes: > Am 13.12.2013 um 14:31 hat Eric Blake geschrieben: >> On 11/12/2013 06:44 PM, Wenchao Xia wrote: >> > +++ b/scripts/qapi-event.py >> > @@ -0,0 +1,355 @@ >> > +# >> > +# QAPI event generator >> > +# >> > +# Copyright IBM, Corp. 2013 >> > +# >> > +# Authors: >> > +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> > +# >> > +# This work is licensed under the terms of the GNU GPLv2. >> >> Can you please use GPLv2+ (that is, add the "or later" clause)? We >> already have GPLv2-only code, but I don't want to increase the size of >> that unfortunate license choice. > > In fact, it's even worse: > > +# This work is licensed under the terms of the GNU GPLv2. > +# See the COPYING.LIB file in the top-level directory. > > These two lines contradict each other, COPYING.LIB contains the > LGPL 2.1. The same bad license header is in the other QAPI generator > scripts, so it's only copy&paste here. Specifically: File Commit scripts/qapi-commands.py c17d9908 scripts/qapi-visit.py fb3182ce scripts/qapi-types.py 06d64c62 scripts/qapi.py 0f923be2 All four from Michael Roth via Luiz. > This doesn't make things easier, because if things are copied, the > license of the source must be respected. And it seems rather dubious to > me what this license actually is. If it's GPLv2-only, we can't just > change it in the new copy. IANAL, and I wouldn't dare to judge which of the two conflicting license claims takes precedence. Possibly neither, and then the files might technically not be distributable. Anyway, this mess needs to be addressed. Michael, what was your *intended* license? If it wasn't GPLv2+, then why? Do we need formal ACKs from all contributors to fix the licensing comment in these four files?
Ping? Markus Armbruster <armbru@redhat.com> writes: > [Licensing problem, cc: Anthony] > > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 13.12.2013 um 14:31 hat Eric Blake geschrieben: >>> On 11/12/2013 06:44 PM, Wenchao Xia wrote: >>> > +++ b/scripts/qapi-event.py >>> > @@ -0,0 +1,355 @@ >>> > +# >>> > +# QAPI event generator >>> > +# >>> > +# Copyright IBM, Corp. 2013 >>> > +# >>> > +# Authors: >>> > +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> >>> > +# >>> > +# This work is licensed under the terms of the GNU GPLv2. >>> >>> Can you please use GPLv2+ (that is, add the "or later" clause)? We >>> already have GPLv2-only code, but I don't want to increase the size of >>> that unfortunate license choice. >> >> In fact, it's even worse: >> >> +# This work is licensed under the terms of the GNU GPLv2. >> +# See the COPYING.LIB file in the top-level directory. >> >> These two lines contradict each other, COPYING.LIB contains the >> LGPL 2.1. The same bad license header is in the other QAPI generator >> scripts, so it's only copy&paste here. > > Specifically: > > File Commit > scripts/qapi-commands.py c17d9908 > scripts/qapi-visit.py fb3182ce > scripts/qapi-types.py 06d64c62 > scripts/qapi.py 0f923be2 > > All four from Michael Roth via Luiz. > >> This doesn't make things easier, because if things are copied, the >> license of the source must be respected. And it seems rather dubious to >> me what this license actually is. If it's GPLv2-only, we can't just >> change it in the new copy. > > IANAL, and I wouldn't dare to judge which of the two conflicting license > claims takes precedence. Possibly neither, and then the files might > technically not be distributable. > > Anyway, this mess needs to be addressed. Michael, what was your > *intended* license? > > If it wasn't GPLv2+, then why? > > Do we need formal ACKs from all contributors to fix the licensing > comment in these four files?
Ping^2! Markus Armbruster <armbru@redhat.com> writes: > Ping? > > Markus Armbruster <armbru@redhat.com> writes: > >> [Licensing problem, cc: Anthony] >> >> Kevin Wolf <kwolf@redhat.com> writes: >> >>> Am 13.12.2013 um 14:31 hat Eric Blake geschrieben: >>>> On 11/12/2013 06:44 PM, Wenchao Xia wrote: >>>> > +++ b/scripts/qapi-event.py >>>> > @@ -0,0 +1,355 @@ >>>> > +# >>>> > +# QAPI event generator >>>> > +# >>>> > +# Copyright IBM, Corp. 2013 >>>> > +# >>>> > +# Authors: >>>> > +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> >>>> > +# >>>> > +# This work is licensed under the terms of the GNU GPLv2. >>>> >>>> Can you please use GPLv2+ (that is, add the "or later" clause)? We >>>> already have GPLv2-only code, but I don't want to increase the size of >>>> that unfortunate license choice. >>> >>> In fact, it's even worse: >>> >>> +# This work is licensed under the terms of the GNU GPLv2. >>> +# See the COPYING.LIB file in the top-level directory. >>> >>> These two lines contradict each other, COPYING.LIB contains the >>> LGPL 2.1. The same bad license header is in the other QAPI generator >>> scripts, so it's only copy&paste here. >> >> Specifically: >> >> File Commit >> scripts/qapi-commands.py c17d9908 >> scripts/qapi-visit.py fb3182ce >> scripts/qapi-types.py 06d64c62 >> scripts/qapi.py 0f923be2 >> >> All four from Michael Roth via Luiz. >> >>> This doesn't make things easier, because if things are copied, the >>> license of the source must be respected. And it seems rather dubious to >>> me what this license actually is. If it's GPLv2-only, we can't just >>> change it in the new copy. >> >> IANAL, and I wouldn't dare to judge which of the two conflicting license >> claims takes precedence. Possibly neither, and then the files might >> technically not be distributable. >> >> Anyway, this mess needs to be addressed. Michael, what was your >> *intended* license? >> >> If it wasn't GPLv2+, then why? >> >> Do we need formal ACKs from all contributors to fix the licensing >> comment in these four files?
于 2014/1/13 18:08, Markus Armbruster 写道: > Ping^2! > > Markus Armbruster <armbru@redhat.com> writes: > >> Ping? >> >> Markus Armbruster <armbru@redhat.com> writes: >> >>> [Licensing problem, cc: Anthony] >>> >>> Kevin Wolf <kwolf@redhat.com> writes: >>> >>>> Am 13.12.2013 um 14:31 hat Eric Blake geschrieben: >>>>> On 11/12/2013 06:44 PM, Wenchao Xia wrote: >>>>>> +++ b/scripts/qapi-event.py >>>>>> @@ -0,0 +1,355 @@ >>>>>> +# >>>>>> +# QAPI event generator >>>>>> +# >>>>>> +# Copyright IBM, Corp. 2013 >>>>>> +# >>>>>> +# Authors: >>>>>> +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> >>>>>> +# >>>>>> +# This work is licensed under the terms of the GNU GPLv2. >>>>> >>>>> Can you please use GPLv2+ (that is, add the "or later" clause)? We >>>>> already have GPLv2-only code, but I don't want to increase the size of >>>>> that unfortunate license choice. >>>> >>>> In fact, it's even worse: >>>> >>>> +# This work is licensed under the terms of the GNU GPLv2. >>>> +# See the COPYING.LIB file in the top-level directory. >>>> >>>> These two lines contradict each other, COPYING.LIB contains the >>>> LGPL 2.1. The same bad license header is in the other QAPI generator >>>> scripts, so it's only copy&paste here. >>> >>> Specifically: >>> >>> File Commit >>> scripts/qapi-commands.py c17d9908 >>> scripts/qapi-visit.py fb3182ce >>> scripts/qapi-types.py 06d64c62 >>> scripts/qapi.py 0f923be2 >>> >>> All four from Michael Roth via Luiz. >>> >>>> This doesn't make things easier, because if things are copied, the >>>> license of the source must be respected. And it seems rather dubious to >>>> me what this license actually is. If it's GPLv2-only, we can't just >>>> change it in the new copy. >>> >>> IANAL, and I wouldn't dare to judge which of the two conflicting license >>> claims takes precedence. Possibly neither, and then the files might >>> technically not be distributable. >>> >>> Anyway, this mess needs to be addressed. Michael, what was your >>> *intended* license? >>> >>> If it wasn't GPLv2+, then why? >>> >>> Do we need formal ACKs from all contributors to fix the licensing >>> comment in these four files? > I used GPLv2+ in my new files of v2, but not sure about other files. Michael, do you think other scripts should be changed either?
Quoting Markus Armbruster (2013-12-16 03:13:08) > [Licensing problem, cc: Anthony] > > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 13.12.2013 um 14:31 hat Eric Blake geschrieben: > >> On 11/12/2013 06:44 PM, Wenchao Xia wrote: > >> > +++ b/scripts/qapi-event.py > >> > @@ -0,0 +1,355 @@ > >> > +# > >> > +# QAPI event generator > >> > +# > >> > +# Copyright IBM, Corp. 2013 > >> > +# > >> > +# Authors: > >> > +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> > >> > +# > >> > +# This work is licensed under the terms of the GNU GPLv2. > >> > >> Can you please use GPLv2+ (that is, add the "or later" clause)? We > >> already have GPLv2-only code, but I don't want to increase the size of > >> that unfortunate license choice. > > > > In fact, it's even worse: > > > > +# This work is licensed under the terms of the GNU GPLv2. > > +# See the COPYING.LIB file in the top-level directory. > > > > These two lines contradict each other, COPYING.LIB contains the > > LGPL 2.1. The same bad license header is in the other QAPI generator > > scripts, so it's only copy&paste here. > > Specifically: > > File Commit > scripts/qapi-commands.py c17d9908 > scripts/qapi-visit.py fb3182ce > scripts/qapi-types.py 06d64c62 > scripts/qapi.py 0f923be2 > > All four from Michael Roth via Luiz. > > > This doesn't make things easier, because if things are copied, the > > license of the source must be respected. And it seems rather dubious to > > me what this license actually is. If it's GPLv2-only, we can't just > > change it in the new copy. > > IANAL, and I wouldn't dare to judge which of the two conflicting license > claims takes precedence. Possibly neither, and then the files might > technically not be distributable. IAAlsoNAL, but GPLv2 is explicit, whereas the "COPYING.LIB" simply references a document with no information relevant to GPLv2, so I think a strong case can be made that the intended license was GPLv2 and the "clarification" is effectively a no-op. > Anyway, this mess needs to be addressed. Michael, what was your > *intended* license? GPLv2 was my intention at least (I meant to reference COPYING). But I think we need Anthony's ack to be certain, since he was the original author, and I added the screwed up license header after-the-fact under the assumption that the code was to be GPLv2. Here's the original: http://repo.or.cz/w/qemu/aliguori.git/blob_plain/glib:/scripts/qapi-types.py > > If it wasn't GPLv2+, then why? This was committed prior to the push to switch to GPLv2+, but I'm fine with relicensing my contributions as GPLv2+ should we opt to do so, but I think that's a separate issue. > > Do we need formal ACKs from all contributors to fix the licensing > comment in these four files? If we were actually re-licensing then yes (at least, that's what we've done in the past). To clarify the existing license maybe not, but we should probably err on the side of caution. Current list seems to be: mdroth@loki:~/w/qemu.git$ git log --format="%an: %ae" scripts/qapi* | sort | uniq Amos Kong: akong@redhat.com Anthony Liguori: aliguori@us.ibm.com Anthony Liguori: anthony@codemonkey.ws Avi Kivity: avi@redhat.com Blue Swirl: blauwirbel@gmail.com Cole Robinson: crobinso@redhat.com Federico Simoncelli: fsimonce@redhat.com Igor Mammedov: imammedo@redhat.com Kevin Wolf: kwolf@redhat.com Laszlo Ersek: lersek@redhat.com Luiz Capitulino: lcapitulino@redhat.com Markus Armbruster: armbru@redhat.com Michael Roth: mdroth@linux.vnet.ibm.com Paolo Bonzini: pbonzini@redhat.com Peter Maydell: peter.maydell@linaro.org Richard Henderson: rth@twiddle.net Stefan Weil: sw@weilnetz.de Tomoki Sekiyama: tomoki.sekiyama@hds.com If we go to that effort, it may make sense to try to re-license to GPLv2+ while we're at it, but either way I think this should be done as a separate patchset, and shouldn't hold up Wenchao's series. I can send that out, since it's my screw-up.
Il 16/01/2014 03:50, Michael Roth ha scritto: > If we go to that effort, it may make sense to try to re-license to GPLv2+ > while we're at it, but either way I think this should be done as a separate > patchset, and shouldn't hold up Wenchao's series. I can send that out, since > it's my screw-up. > Removing Red Hat, Blue Swirl and Stefan Weil, who have agreed on a blanket relicensing of GPLv2-only code to GPLv2+ leaves only half a dozen people: Anthony Liguori: anthony@codemonkey.ws (IBM?) Michael Roth: mdroth@linux.vnet.ibm.com (IBM?) Peter Maydell: peter.maydell@linaro.org Richard Henderson: rth@twiddle.net Tomoki Sekiyama: tomoki.sekiyama@hds.com Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 16/01/2014 03:50, Michael Roth ha scritto: >> If we go to that effort, it may make sense to try to re-license to GPLv2+ >> while we're at it, but either way I think this should be done as a separate >> patchset, and shouldn't hold up Wenchao's series. I can send that out, since >> it's my screw-up. >> > > Removing Red Hat, Blue Swirl and Stefan Weil, who have agreed on a > blanket relicensing of GPLv2-only code to GPLv2+ leaves only half a > dozen people: > > Anthony Liguori: anthony@codemonkey.ws (IBM?) > Michael Roth: mdroth@linux.vnet.ibm.com (IBM?) > Peter Maydell: peter.maydell@linaro.org > Richard Henderson: rth@twiddle.net > Tomoki Sekiyama: tomoki.sekiyama@hds.com I sent a patch. Now we hunt for the required ACKs.
diff --git a/Makefile b/Makefile index b15003f..a3e465f 100644 --- a/Makefile +++ b/Makefile @@ -38,8 +38,8 @@ endif endif GENERATED_HEADERS = config-host.h qemu-options.def -GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h -GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c +GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-event.h +GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-event.c GENERATED_HEADERS += trace/generated-events.h GENERATED_SOURCES += trace/generated-events.c @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y) # Build libraries libqemustub.a: $(stub-obj-y) -libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o qapi-event.o ###################################################################### @@ -219,6 +219,9 @@ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py) qapi-visit.c qapi-visit.h :\ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py) $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o "." -b < $<, " GEN $@") +qapi-event.c qapi-event.h :\ +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-event.py $(qapi-py) + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-event.py $(gen-out-type) -o "." -b < $<, " GEN $@") qmp-commands.h qmp-marshal.c :\ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py) $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, " GEN $@") diff --git a/Makefile.objs b/Makefile.objs index 2b6c1fe..33f5950 100644 --- a/Makefile.objs +++ b/Makefile.objs @@ -12,7 +12,7 @@ block-obj-y += main-loop.o iohandler.o qemu-timer.o block-obj-$(CONFIG_POSIX) += aio-posix.o block-obj-$(CONFIG_WIN32) += aio-win32.o block-obj-y += block/ -block-obj-y += qapi-types.o qapi-visit.o +block-obj-y += qapi-types.o qapi-visit.o qapi-event.o block-obj-y += qemu-io-cmds.o block-obj-y += qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs index 1f9c973..d14b769 100644 --- a/qapi/Makefile.objs +++ b/qapi/Makefile.objs @@ -3,3 +3,4 @@ util-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o util-obj-y += string-input-visitor.o string-output-visitor.o util-obj-y += opts-visitor.o +util-obj-y += qmp-event.o diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py new file mode 100644 index 0000000..4c6a0fe --- /dev/null +++ b/scripts/qapi-event.py @@ -0,0 +1,355 @@ +# +# QAPI event generator +# +# Copyright IBM, Corp. 2013 +# +# Authors: +# Wenchao Xia <xiawenc@linux.vnet.ibm.com> +# +# This work is licensed under the terms of the GNU GPLv2. +# See the COPYING.LIB file in the top-level directory. + +from ordereddict import OrderedDict +from qapi import * +import sys +import os +import getopt +import errno + +def _generate_event_api_name_with_params(event_name, params): + api_name = "void qapi_event_gen_%s(" % c_fun(event_name); + l = len(api_name) + + for argname, argentry, optional, structured in parse_args(params): + if structured: + sys.stderr.write("Nested structure define in event is not " + "supported now, event '%s', argname '%s'\n" % + (event_name, argname)) + sys.exit(1) + continue + + if optional: + api_name += "bool has_%s,\n" % c_var(argname) + api_name += "".ljust(l) + + if argentry == "str": + api_name += "const " + api_name += "%s %s,\n" % (c_type(argentry), c_var(argname)) + api_name += "".ljust(l) + + api_name += "Error **errp)" + return api_name; + +def _generate_event_implement_with_params(api_name, event_name, params): + ret = mcgen(""" + +%(api_name)s +{ + QmpOutputVisitor *qov; + Visitor *v; + Error *err = NULL; + QObject *obj; + QDict *qmp = NULL; + + if (!qapi_event_functions.emit) { + return; + } + + qmp = qdict_new(); + qdict_put(qmp, "event", qstring_from_str("%(event_name)s")); + timestamp_put(qmp); + + qov = qmp_output_visitor_new(); + g_assert(qov); + + v = qmp_output_get_visitor(qov); + g_assert(v); + + /* Fake visit, as if all member are under a structure */ + visit_start_struct(v, NULL, "", "%(event_name)s", 0, &err); + if (error_is_set(&err)) { + goto clean; + } + +""", + api_name = api_name, + event_name = event_name) + + for argname, argentry, optional, structured in parse_args(params): + if structured: + sys.stderr.write("Nested structure define in event is not " + "supported now, event '%s', argname '%s'\n" % + (event_name, argname)) + sys.exit(1) + continue + + if optional: + ret += mcgen(""" + if (has_%(var)s) { +""", + var = c_var(argname)) + push_indent() + + if argentry == "str": + var_type = "(char **)" + else: + var_type = "" + + ret += mcgen(""" + visit_type_%(type)s(v, %(var_type)s&%(var)s, "%(name)s", &err); + if (error_is_set(&err)) { + goto clean; + } +""", + var_type = var_type, + var = c_var(argname), + type = type_name(argentry), + name = argname) + + if optional: + pop_indent() + ret += mcgen(""" + } +""") + + ret += mcgen(""" + + visit_end_struct(v, &err); + if (error_is_set(&err)) { + goto clean; + } + + obj = qmp_output_get_qobject(qov); + g_assert(obj != NULL); + + qdict_put_obj(qmp, "data", obj); + + qapi_event_functions.emit(qmp, &err); + + clean: + error_propagate(errp, err); + qmp_output_visitor_cleanup(qov); + QDECREF(qmp); +} +""") + + return ret + +def _generate_event_api_name(event_name): + return "void qapi_event_gen_%s(Error **errp)" % c_fun(event_name); + +def _generate_event_implement(api_name, event_name): + return mcgen(""" + +%(api_name)s +{ + QDict *qmp = NULL; + + if (!qapi_event_functions.emit) { + return; + } + + qmp = qdict_new(); + qdict_put(qmp, "event", qstring_from_str("%(event_name)s")); + timestamp_put(qmp); + + qapi_event_functions.emit(qmp, errp); + + QDECREF(qmp); +} +""", + api_name = api_name, + event_name = event_name) + + +def generate_event_declaration(event_name, params): + if params and len(params) > 0: + api_name = _generate_event_api_name_with_params(event_name, params) + else: + api_name = _generate_event_api_name(event_name) + + return mcgen(''' + +%(api_name)s; +''', + api_name = api_name) + +def generate_event_implement(event_name, params): + if params and len(params) > 0: + api_name = _generate_event_api_name_with_params(event_name, params) + ret = _generate_event_implement_with_params(api_name, + event_name, + params) + + else: + api_name = _generate_event_api_name(event_name) + ret = _generate_event_implement(api_name, + event_name) + return ret + +try: + opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:", + ["source", "header", "builtins", "prefix=", + "output-dir="]) +except getopt.GetoptError, err: + print str(err) + sys.exit(1) + +output_dir = "" +prefix = "" +c_file = 'qapi-event.c' +h_file = 'qapi-event.h' + +do_c = False +do_h = False +do_builtins = False + +for o, a in opts: + if o in ("-p", "--prefix"): + prefix = a + elif o in ("-o", "--output-dir"): + output_dir = a + "/" + elif o in ("-c", "--source"): + do_c = True + elif o in ("-h", "--header"): + do_h = True + elif o in ("-b", "--builtins"): + do_builtins = True + +if not do_c and not do_h: + do_c = True + do_h = True + +c_file = output_dir + prefix + c_file +h_file = output_dir + prefix + h_file + +try: + os.makedirs(output_dir) +except os.error, e: + if e.errno != errno.EEXIST: + raise + +def maybe_open(really, name, opt): + if really: + return open(name, opt) + else: + import StringIO + return StringIO.StringIO() + +fdef = maybe_open(do_c, c_file, 'w') +fdecl = maybe_open(do_h, h_file, 'w') + +fdef.write(mcgen(''' +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */ + +/* + * schema-defined QAPI event functions + * + * Copyright IBM, Corp. 2013 + * + * Authors: + * Wenchao Xia <xiawenc@linux.vnet.ibm.com> + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + * + */ + +#include "qemu-common.h" +#include "%(header)s" +#include "%(prefix)sqapi-visit.h" +#include "qapi/qmp-output-visitor.h" +#include "qapi/qmp/qstring.h" +#include "qapi/qmp/qjson.h" + +#ifdef _WIN32 +#include "sysemu/os-win32.h" +#endif + +#ifdef CONFIG_POSIX +#include "sysemu/os-posix.h" +#endif + +typedef struct QAPIEventFunctions { + void (*emit)(QDict *dict, Error **errp); +} QAPIEventFunctions; + +QAPIEventFunctions qapi_event_functions; + +void qapi_event_set_func_emit(qapi_event_emit emit) +{ + qapi_event_functions.emit = emit; +} + +static void timestamp_put(QDict *qdict) +{ + int err; + QObject *obj; + qemu_timeval tv; + + err = qemu_gettimeofday(&tv); + if (err < 0) + return; + + obj = qobject_from_jsonf("{ 'seconds': %(p)s" PRId64 ", " + "'microseconds': %(p)s" PRId64 " }", + (int64_t) tv.tv_sec, (int64_t) tv.tv_usec); + qdict_put_obj(qdict, "timestamp", obj); +} + +''', + prefix=prefix, header=basename(h_file), p="%")) + +fdecl.write(mcgen(''' +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */ + +/* + * schema-defined QAPI event function + * + * Copyright IBM, Corp. 2013 + * + * Authors: + * Wenchao Xia <xiawenc@linux.vnet.ibm.com> + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + * + */ + +#ifndef %(guard)s +#define %(guard)s + +#include "qapi/qmp/qdict.h" +#include "qapi/error.h" +#include "qapi/visitor.h" +#include "%(prefix)sqapi-types.h" + +typedef void (*qapi_event_emit)(QDict *d, Error **errp); + +void qapi_event_set_func_emit(qapi_event_emit emit); + +''', + prefix=prefix, guard=guardname(h_file))) + +exprs = parse_schema(sys.stdin) + +for expr in exprs: + if expr.has_key('event'): + event_name = expr['event'] + params = expr.get('data') + + ret = generate_event_declaration(event_name, params) + fdecl.write(ret) + + ret = generate_event_implement(event_name, params) + fdef.write(ret) + +fdecl.write(''' +#endif +''') + +fdecl.flush() +fdecl.close() + +fdef.flush() +fdef.close()
Nested structure is not supported now, so following define is not valid: { 'event': 'EVENT_C', 'data': { 'a': { 'a_a', 'str', 'a_b', 'str' }, 'b': 'int' } Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- Makefile | 9 +- Makefile.objs | 2 +- qapi/Makefile.objs | 1 + scripts/qapi-event.py | 355 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 363 insertions(+), 4 deletions(-) create mode 100644 scripts/qapi-event.py