Re: [Qemu-devel] [PATCH RFC 2/3] qapi script: add support of event

[Luiz Capitulino]

On Thu, 28 Nov 2013 15:16:08 +0800
Wenchao Xia <address@hidden> wrote:

> 于 2013/11/28 8:48, Luiz Capitulino 写道:
> > On Wed, 13 Nov 2013 09:44:52 +0800
> > Wenchao Xia <address@hidden> 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 <address@hidden>
> >> ---
> >>   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 <address@hidden>
> >> +#
> >> +# 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   <address@hidden>
> >> + *
> >> + * 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  <address@hidden>
> >> + *
> >> + * 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()
> >
>