On Thu, 28 Nov 2013 15:16:08 +0800 Wenchao Xia <xiaw...@linux.vnet.ibm.com> wrote:
> δΊ 2013/11/28 8:48, Luiz Capitulino ει: > > On Wed, 13 Nov 2013 09:44:52 +0800 > > Wenchao Xia <xiaw...@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 <xiaw...@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 <xiaw...@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 <xiaw...@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 <xiaw...@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() > > >