From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58198) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vm2d7-0003aL-Bu for qemu-devel@nongnu.org; Thu, 28 Nov 2013 09:31:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vm2d2-0006wS-C8 for qemu-devel@nongnu.org; Thu, 28 Nov 2013 09:31:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:26225) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vm2d2-0006wM-1I for qemu-devel@nongnu.org; Thu, 28 Nov 2013 09:31:40 -0500 Date: Thu, 28 Nov 2013 09:31:35 -0500 From: Luiz Capitulino Message-ID: <20131128093135.2ef9e21f@redhat.com> In-Reply-To: <5296EDB8.9040904@linux.vnet.ibm.com> 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> <5296EDB8.9040904@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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: Wenchao Xia Cc: kwolf@redhat.com, armbru@redhat.com, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, pbonzini@redhat.com On Thu, 28 Nov 2013 15:16:08 +0800 Wenchao Xia wrote: > =E4=BA=8E 2013/11/28 8:48, Luiz Capitulino =E5=86=99=E9=81=93: > > On Wed, 13 Nov 2013 09:44:52 +0800 > > Wenchao Xia wrote: > > > >> Nested structure is not supported now, so following define is not vali= d: > >> { '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). >=20 > OK. >=20 > > > > 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. > > >=20 > 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. >=20 > > More below. > > >=20 > >> 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 =3D config-host.h qemu-options.def > >> -GENERATED_HEADERS +=3D qmp-commands.h qapi-types.h qapi-visit.h > >> -GENERATED_SOURCES +=3D qmp-marshal.c qapi-types.c qapi-visit.c > >> +GENERATED_HEADERS +=3D qmp-commands.h qapi-types.h qapi-visit.h qapi-= event.h > >> +GENERATED_SOURCES +=3D qmp-marshal.c qapi-types.c qapi-visit.c qapi-e= vent.c > >> > >> GENERATED_HEADERS +=3D trace/generated-events.h > >> GENERATED_SOURCES +=3D 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/q= api-types.py $(qapi-py) > >> qapi-visit.c qapi-visit.h :\ > >> $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qap= i-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 $(g= en-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 +=3D main-loop.o iohandler.o qemu-timer.o > >> block-obj-$(CONFIG_POSIX) +=3D aio-posix.o > >> block-obj-$(CONFIG_WIN32) +=3D aio-win32.o > >> block-obj-y +=3D block/ > >> -block-obj-y +=3D qapi-types.o qapi-visit.o > >> +block-obj-y +=3D qapi-types.o qapi-visit.o qapi-event.o > >> block-obj-y +=3D qemu-io-cmds.o > >> > >> block-obj-y +=3D qemu-coroutine.o qemu-coroutine-lock.o qemu-corouti= ne-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 +=3D qmp-output-visitor.o qmp-registry.o qm= p-dispatch.o > >> util-obj-y +=3D string-input-visitor.o string-output-visitor.o > >> > >> util-obj-y +=3D opts-visitor.o > >> +util-obj-y +=3D 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 `libqemu= util.a'. Stop. > > ~/work/src/upstream/qmp-unstable/build (tmp|AM)/ > > > A draft file I forgot to remove in Makefile, will fix. >=20 > >> --- /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 =3D "void qapi_event_gen_%s(" % c_fun(event_name); > > > > I'd prefer a name like qmp_notify_NAME() or qmp_send_event_NAME(). > > >=20 > 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 =3D 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 +=3D "bool has_%s,\n" % c_var(argname) > >> + api_name +=3D "".ljust(l) > >> + > >> + if argentry =3D=3D "str": > >> + api_name +=3D "const " > >> + api_name +=3D "%s %s,\n" % (c_type(argentry), c_var(argname)) > >> + api_name +=3D "".ljust(l) > >> + > >> + api_name +=3D "Error **errp)" > >> + return api_name; > >> + > >> +def _generate_event_implement_with_params(api_name, event_name, param= s): > >> + ret =3D mcgen(""" > >> + > >> +%(api_name)s > >> +{ > >> + QmpOutputVisitor *qov; > >> + Visitor *v; > >> + Error *err =3D NULL; > > > > We usually call it *local_err; > > >=20 > OK. >=20 > >> + QObject *obj; > >> + QDict *qmp =3D NULL; > > > > It's not needed to initialize qmp. > > >=20 > Will fix. >=20 > >> + > >> + if (!qapi_event_functions.emit) { > > > > Better to return an error here instead of silently failing. > > >=20 > The purpose is allowing emit=3DNULL 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=3DNULL 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)) { > ... > } >=20 > err is always set when emit=3DNULL, but we may allow emit=3DNULL, > for example, qemu-img will have emit=3DNULL. >=20 > >> + return; > >> + } > >> + > >> + qmp =3D 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)? > > >=20 > Seems good, will use it. >=20 > >> + > >> + qov =3D qmp_output_visitor_new(); > >> + g_assert(qov); > >> + > >> + v =3D 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 =3D api_name, > >> + event_name =3D 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 +=3D mcgen(""" > >> + if (has_%(var)s) { > >> +""", > >> + var =3D c_var(argname)) > >> + push_indent() > >> + > >> + if argentry =3D=3D "str": > >> + var_type =3D "(char **)" > >> + else: > >> + var_type =3D "" > >> + > >> + ret +=3D mcgen(""" > >> + visit_type_%(type)s(v, %(var_type)s&%(var)s, "%(name)s", &err); > >> + if (error_is_set(&err)) { > >> + goto clean; > >> + } > >> +""", > >> + var_type =3D var_type, > >> + var =3D c_var(argname), > >> + type =3D type_name(argentry), > >> + name =3D argname) > >> + > >> + if optional: > >> + pop_indent() > >> + ret +=3D mcgen(""" > >> + } > >> +""") > >> + > >> + ret +=3D mcgen(""" > >> + > >> + visit_end_struct(v, &err); > >> + if (error_is_set(&err)) { > >> + goto clean; > >> + } > >> + > >> + obj =3D qmp_output_get_qobject(qov); > >> + g_assert(obj !=3D 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 =3D NULL; > > > > It's not needed to initialize qmp. > > >=20 > OK. >=20 > >> + > >> + if (!qapi_event_functions.emit) { > >> + return; > >> + } > >> + > >> + qmp =3D 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 =3D api_name, > >> + event_name =3D event_name) > >> + > >> + > >> +def generate_event_declaration(event_name, params): > >> + if params and len(params) > 0: > >> + api_name =3D _generate_event_api_name_with_params(event_name,= params) > >> + else: > >> + api_name =3D _generate_event_api_name(event_name) > >> + > >> + return mcgen(''' > >> + > >> +%(api_name)s; > >> +''', > >> + api_name =3D api_name) > >> + > >> +def generate_event_implement(event_name, params): > >> + if params and len(params) > 0: > >> + api_name =3D _generate_event_api_name_with_params(event_name,= params) > >> + ret =3D _generate_event_implement_with_params(api_name, > >> + event_name, > >> + params) > >> + > >> + else: > >> + api_name =3D _generate_event_api_name(event_name) > >> + ret =3D _generate_event_implement(api_name, > >> + event_name) > >> + return ret > >> + > >> +try: > >> + opts, args =3D getopt.gnu_getopt(sys.argv[1:], "chbp:o:", > >> + ["source", "header", "builtins", "= prefix=3D", > >> + "output-dir=3D"]) > >> +except getopt.GetoptError, err: > >> + print str(err) > >> + sys.exit(1) > >> + > >> +output_dir =3D "" > >> +prefix =3D "" > >> +c_file =3D 'qapi-event.c' > >> +h_file =3D 'qapi-event.h' > >> + > >> +do_c =3D False > >> +do_h =3D False > >> +do_builtins =3D False > >> + > >> +for o, a in opts: > >> + if o in ("-p", "--prefix"): > >> + prefix =3D a > >> + elif o in ("-o", "--output-dir"): > >> + output_dir =3D a + "/" > >> + elif o in ("-c", "--source"): > >> + do_c =3D True > >> + elif o in ("-h", "--header"): > >> + do_h =3D True > >> + elif o in ("-b", "--builtins"): > >> + do_builtins =3D True > >> + > >> +if not do_c and not do_h: > >> + do_c =3D True > >> + do_h =3D True > >> + > >> +c_file =3D output_dir + prefix + c_file > >> +h_file =3D output_dir + prefix + h_file > >> + > >> +try: > >> + os.makedirs(output_dir) > >> +except os.error, e: > >> + if e.errno !=3D errno.EEXIST: > >> + raise > >> + > >> +def maybe_open(really, name, opt): > >> + if really: > >> + return open(name, opt) > >> + else: > >> + import StringIO > >> + return StringIO.StringIO() > >> + > >> +fdef =3D maybe_open(do_c, c_file, 'w') > >> +fdecl =3D 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 =3D emit; > >> +} > >> + > >> +static void timestamp_put(QDict *qdict) > >> +{ > >> + int err; > >> + QObject *obj; > >> + qemu_timeval tv; > >> + > >> + err =3D qemu_gettimeofday(&tv); > >> + if (err < 0) > >> + return; > >> + > >> + obj =3D 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? > > >=20 > 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. >=20 > >> + > >> +''', > >> + prefix=3Dprefix, header=3Dbasename(h_file), p=3D"%")) > >> + > >> +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=3Dprefix, guard=3Dguardname(h_file))) > >> + > >> +exprs =3D parse_schema(sys.stdin) > >> + > >> +for expr in exprs: > >> + if expr.has_key('event'): > >> + event_name =3D expr['event'] > >> + params =3D expr.get('data') > >> + > >> + ret =3D generate_event_declaration(event_name, params) > >> + fdecl.write(ret) > >> + > >> + ret =3D generate_event_implement(event_name, params) > >> + fdef.write(ret) > >> + > >> +fdecl.write(''' > >> +#endif > >> +''') > >> + > >> +fdecl.flush() > >> +fdecl.close() > >> + > >> +fdef.flush() > >> +fdef.close() > > >=20