From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48754) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vlvq7-00075e-L3 for qemu-devel@nongnu.org; Thu, 28 Nov 2013 02:16:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vlvq0-0006Dn-Co for qemu-devel@nongnu.org; Thu, 28 Nov 2013 02:16:43 -0500 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:47851) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vlvpz-0006Da-9Q for qemu-devel@nongnu.org; Thu, 28 Nov 2013 02:16:36 -0500 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 28 Nov 2013 17:16:19 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id 6B4A92BB0055 for ; Thu, 28 Nov 2013 18:16:17 +1100 (EST) Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id rAS6w4mt10092902 for ; Thu, 28 Nov 2013 17:58:11 +1100 Received: from d23av02.au.ibm.com (localhost [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id rAS7G9Hl016061 for ; Thu, 28 Nov 2013 18:16:09 +1100 Message-ID: <5296EDB8.9040904@linux.vnet.ibm.com> Date: Thu, 28 Nov 2013 15:16:08 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1384307094-5836-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1384307094-5836-3-git-send-email-xiawenc@linux.vnet.ibm.com> <20131127194830.6715cc72@redhat.com> In-Reply-To: <20131127194830.6715cc72@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH RFC 2/3] qapi script: add support of event List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kwolf@redhat.com, armbru@redhat.com, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, pbonzini@redhat.com 于 2013/11/28 8:48, Luiz Capitulino 写道: > On Wed, 13 Nov 2013 09:44:52 +0800 > 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' } > > 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 >> --- >> 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 >> +# >> +# 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 >> + * >> + * 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 >> + * >> + * 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() >