Re: [Qemu-devel] [RFC PATCH V2 3/5] qapi script: add event support by qapi-event.py

[Luiz Capitulino <lcapitulino@redhat.com>]

[Pressed enter too soon, forgot two things]

On Mon, 6 Jan 2014 18:10:04 -0500
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Fri,  3 Jan 2014 07:10:32 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 
> > qapi-event.py will parse the schema and generate qapi-event.c, then
> > the API in qapi-event.c can be used to handle event in qemu code.
> > All API have prefix "qapi_event", all types have prefix "QAPIEvent".
> > Examples can be found in following patches.
> > 
> > The script mainly include three parts: generate API for each event
> > define, generate an enum type for all defined event, generate behavior
> > control functions.
> > 
> > Since in some case the real emit behavior may change, for example,
> > qemu-img would not send a event, a callback layer is added to
> > control the behavior. As a result, the stubs at compile time
> > can be saved, the binding of block layer code and monitor code
> > will become looser.
> > 
> > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> > ---
> >  Makefile              |    9 +-
> >  Makefile.objs         |    2 +-
> >  scripts/qapi-event.py |  432 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 439 insertions(+), 4 deletions(-)
> >  create mode 100644 scripts/qapi-event.py
> > 
> > diff --git a/Makefile b/Makefile
> > index bdff4e4..fa59765 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -45,8 +45,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
> > @@ -185,7 +185,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
> >  
> >  ######################################################################
> >  
> > @@ -226,6 +226,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/scripts/qapi-event.py b/scripts/qapi-event.py
> > new file mode 100644
> > index 0000000..7526366
> > --- /dev/null
> > +++ b/scripts/qapi-event.py
> > @@ -0,0 +1,432 @@
> > +#
> > +# QAPI event generator
> > +#
> > +# Copyright IBM, Corp. 2014
> > +#
> > +# Authors:
> > +#  Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> > +#
> > +# This work is licensed under the terms of the GNU GPLv2+ or later.
> > +# 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(event_name, params):
> 
> Why the underline? And, what you generate is a function declaration...
> 
> > +    api_name = "void qapi_event_send_%s(" % c_fun(event_name).lower();
> > +    l = len(api_name)
> > +
> > +    if params:
> > +        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;
> > +
> > +
> > +# Following are the core functions that transate user input into a qdict going
> 
> s/transate/translate
> 
> Although the comment doesn't make much sense to me.
> 
> > +# to be emitted in the wire.
> > +
> > +def generate_event_declaration(api_name):
> > +    return mcgen('''
> > +
> > +%(api_name)s;
> > +''',
> > +                 api_name = api_name)
> > +
> > +def generate_event_implement(api_name, event_name, params):
> 
> I wonder if it would be clearer to to generate the declaration here.
> 
> > +    # step 1: declare and variables
> > +    ret = mcgen("""
> > +
> > +%(api_name)s
> > +{
> > +    QDict *qmp;
> > +    Error *local_err = NULL;
> > +    QAPIEventFuncEmit emit;
> > +""",
> > +                api_name = api_name)
> > +
> > +    if params:
> > +        ret += mcgen("""
> > +    QmpOutputVisitor *qov;
> > +    Visitor *v;
> > +    QObject *obj;
> > +
> > +""")
> > +
> > +    # step 2: check emit function, create a dict
> > +    ret += mcgen("""
> > +    emit = qapi_event_get_func_emit();
> > +    if (!emit) {
> > +        return;
> > +    }
> > +
> > +    qmp = qmp_event_build_dict("%(event_name)s");
> > +
> > +""",
> > +                 event_name = event_name)
> > +
> > +    # step 3: visit the params if params != None
> > +    if params:
> > +        ret += mcgen("""
> > +    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, &local_err);
> > +    if (error_is_set(&local_err)) {
> > +        goto clean;
> > +    }
> > +
> > +""",
> > +                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)
> > +
> > +            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", &local_err);
> > +    if (error_is_set(&local_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, &local_err);
> > +    if (error_is_set(&local_err)) {
> > +        goto clean;
> > +    }
> > +
> > +    obj = qmp_output_get_qobject(qov);
> > +    g_assert(obj != NULL);
> > +
> > +    qdict_put_obj(qmp, "data", obj);
> > +""")
> > +
> > +    # step 4: call qmp event api
> > +    ret += mcgen("""
> > +    emit(%(event_enum_value)s, qmp, &local_err);
> > +
> > +""",
> > +                 event_enum_value = event_enum_value)
> > +
> > +    # step 5: clean up
> > +    if params:
> > +        ret += mcgen("""
> > + clean:
> > +    qmp_output_visitor_cleanup(qov);
> > +""")
> > +    ret += mcgen("""
> > +    error_propagate(errp, local_err);
> > +    QDECREF(qmp);
> > +}
> > +""")
> > +
> > +    return ret
> > +
> > +
> > +# Following are the functions that generate an enum type for all defined
> > +# events, similar with qapi-types.py. Here we already have enum name and
> > +# values which is generated before and recorded in event_enum_*. It also
> > +# walk around the issue that "import qapi-types" can't work.
> > +
> > +def generate_event_enum_decl(event_enum_name, event_enum_values):
> > +    lookup_decl = mcgen('''
> > +
> > +extern const char *%(event_enum_name)s_lookup[];
> > +''',
> > +                        event_enum_name = event_enum_name)
> > +
> > +    enum_decl = mcgen('''
> > +typedef enum %(event_enum_name)s
> > +{
> > +''',
> > +                      event_enum_name = event_enum_name)
> > +
> > +    # append automatically generated _MAX value
> > +    enum_max_value = generate_enum_full_value_string(event_enum_name, "MAX")
> > +    enum_values = event_enum_values + [ enum_max_value ]
> > +
> > +    i = 0
> > +    for value in enum_values:
> > +        enum_decl += mcgen('''
> > +    %(value)s = %(i)d,
> > +''',
> > +                     value = value,
> > +                     i = i)
> > +        i += 1
> > +
> > +    enum_decl += mcgen('''
> > +} %(event_enum_name)s;
> > +''',
> > +                       event_enum_name = event_enum_name)
> > +
> > +    return lookup_decl + enum_decl
> > +
> > +def generate_event_enum_lookup(event_enum_name, event_enum_strings):
> > +    ret = mcgen('''
> > +
> > +const char *%(event_enum_name)s_lookup[] = {
> > +''',
> > +                event_enum_name = event_enum_name)
> > +
> > +    i = 0
> > +    for string in event_enum_strings:
> > +        ret += mcgen('''
> > +    "%(string)s",
> > +''',
> > +                     string = string)
> > +
> > +    ret += mcgen('''
> > +    NULL,
> > +};
> > +''')
> > +    return ret
> > +
> > +
> > +# Following are the functions that generate event behavior control functions.
> > +# Those functions are put here in the qapi-event.c, since it need to include
> > +# qapi-event.h for the event enum type declaration, put them in other file
> > +# requiring other file include qapi-event.h, causing a cross including. For
> > +# example: if we have qmp-event.c and qmp-event.h, then qmp-event.c
> > +# ->qmp-event.h->qapi-event.h, qapi-event.c->qmp-event.h. Another problem
> > +# follow: test-qapi-event.c will meet event enum double declaration since it
> > +# include both test-qapi-event.h and qmp-event.h. One solution is putting event
> > +# enum declaration in a separate header file, but then qmp-event.h need to
> > +# include test-qapi-event.h or qapi-event.h on compile time condition. So the
> > +# easist way is, just generate them here.
> > +
> > +def generate_event_behavior_control_decl(event_enum_name):
> > +    ret = mcgen('''
> > +
> > +typedef void (*QAPIEventFuncEmit)(%(event_enum_name)s ev,
> > +                                  QDict *dict,
> > +                                  Error **errp);

Why does the emit function need 'ev'? Doesn't 'dict' contain all the
info it needs? Also, it's better to rename it to 'event' or 'qmp_event'.

> > +
> > +void qapi_event_set_func_emit(QAPIEventFuncEmit emit);
> > +
> > +QAPIEventFuncEmit qapi_event_get_func_emit(void);
> > +''',
> > +                event_enum_name = event_enum_name)
> > +    return ret;
> > +
> > +def generate_event_behavior_control_implement():
> > +    ret = mcgen('''
> > +
> > +typedef struct QAPIEventFunctions {
> > +    QAPIEventFuncEmit emit;
> > +} QAPIEventFunctions;
> > +
> > +QAPIEventFunctions qapi_event_functions;
> > +
> > +void qapi_event_set_func_emit(QAPIEventFuncEmit emit)
> > +{
> > +    qapi_event_functions.emit = emit;
> > +}

If this function and the typedefs don't change, I think they shouldn't
be generated.

> > +
> > +QAPIEventFuncEmit qapi_event_get_func_emit(void)
> > +{
> > +    return qapi_event_functions.emit;
> > +}
> > +''')
> > +    return ret
> > +
> > +
> > +# Start the real job
> > +
> > +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. 2014
> > + *
> > + * Authors:
> > + *  Wenchao Xia   <xiawenc@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPLv2+ 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-event.h"
> > +
> > +''',
> > +                 prefix=prefix, header=basename(h_file)))
> > +
> > +fdecl.write(mcgen('''
> > +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
> > +
> > +/*
> > + * schema-defined QAPI event function
> > + *
> > + * Copyright IBM, Corp. 2014
> > + *
> > + * Authors:
> > + *  Wenchao Xia  <xiawenc@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPLv2+ or later.
> > + * See the COPYING.LIB file in the top-level directory.
> > + *
> > + */
> > +
> > +#ifndef %(guard)s
> > +#define %(guard)s
> > +
> > +#include "qapi/error.h"
> > +#include "qapi/qmp/qdict.h"
> > +#include "%(prefix)sqapi-types.h"
> > +
> > +''',
> > +                  prefix=prefix, guard=guardname(h_file)))
> > +
> > +exprs = parse_schema(sys.stdin)
> > +
> > +event_enum_name = "QAPIEvent"
> > +event_enum_values = []
> > +event_enum_strings = []
> > +
> > +for expr in exprs:
> > +    if expr.has_key('event'):
> > +        event_name = expr['event']
> > +        params = expr.get('data')
> > +        if params and len(params) == 0:
> > +            params = None
> > +
> > +        api_name = _generate_event_api_name(event_name, params)
> > +        ret = generate_event_declaration(api_name)
> > +        fdecl.write(ret)
> > +
> > +        # We need an enum value per event
> > +        event_enum_value = generate_enum_full_value_string(event_enum_name,
> > +                                                           event_name)
> > +        ret = generate_event_implement(api_name, event_name, params)
> > +        fdef.write(ret)
> > +
> > +        # Record it, and generate enum later
> > +        event_enum_values.append(event_enum_value)
> > +        event_enum_strings.append(event_name)
> > +
> > +ret = generate_event_enum_decl(event_enum_name, event_enum_values)
> > +fdecl.write(ret)
> > +ret = generate_event_enum_lookup(event_enum_name, event_enum_strings)
> > +fdef.write(ret)
> > +ret = generate_event_behavior_control_decl(event_enum_name)
> > +fdecl.write(ret)
> > +ret = generate_event_behavior_control_implement()
> > +fdef.write(ret)
> > +
> > +fdecl.write('''
> > +#endif
> > +''')
> > +
> > +fdecl.flush()
> > +fdecl.close()
> > +
> > +fdef.flush()
> > +fdef.close()
>