qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Wenchao Xia
Subject: Re: [Qemu-devel] [PATCH RFC 2/3] qapi script: add support of event
Date: Thu, 28 Nov 2013 15:16:08 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.1.1

于 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.

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()?

+    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   <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)

+
+''',
+                 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()





reply via email to

[Prev in Thread] Current Thread [Next in Thread]