Re: [Qemu-devel] [PATCH v3 2/3] qapi: change qapi to convert schema json

[Amos Kong]

On Wed, Jan 22, 2014 at 01:06:51PM -0500, Luiz Capitulino wrote:
> On Sun,  5 Jan 2014 20:02:30 +0800
> Amos Kong <address@hidden> wrote:
> 
> > QMP schema is defined in a json file, it will be parsed by
> > qapi scripts and generate C files.
> > 
> > We want to return the schema information to management,
> > this patch converts the json file to a string table in a
> > C head file, then we can use the json content in QEMU code.
> > 
> > eg: (qmp-schema.h)
> >   const char *const qmp_schema_table[] = {
> >     "{ 'type': 'NameInfo', 'data': {'*name': 'str'} }",
> >     "{ 'command': 'query-name', 'returns': 'NameInfo' }",
> >     ...
> >   }
> > 
> > Signed-off-by: Amos Kong <address@hidden>
> > ---
> >  Makefile                 |  5 ++++-
> >  scripts/qapi-commands.py |  2 +-
> >  scripts/qapi-types.py    | 48 
> > +++++++++++++++++++++++++++++++++++++++++++++---
> >  scripts/qapi-visit.py    |  2 +-
> >  scripts/qapi.py          | 20 +++++++++++++++-----
> >  5 files changed, 66 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index bdff4e4..2c29755 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -45,7 +45,7 @@ endif
> >  endif
> >  
> >  GENERATED_HEADERS = config-host.h qemu-options.def
> > -GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h
> > +GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qmp-schema.h
> >  GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c
> >  
> >  GENERATED_HEADERS += trace/generated-events.h
> > @@ -229,6 +229,9 @@ $(SRC_PATH)/qapi-schema.json 
> > $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> >  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   $@")
> > +qmp-schema.h:\
> > +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> > +   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
> > $(gen-out-type) -o "." -s "$@" < $<, "  GEN   $@")
> >  
> >  QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h 
> > qga-qapi-visit.h qga-qmp-commands.h)
> >  $(qga-obj-y) qemu-ga.o: $(QGALIB_GEN)
> > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> > index b12b696..5f4fb94 100644
> > --- a/scripts/qapi-commands.py
> > +++ b/scripts/qapi-commands.py
> > @@ -440,7 +440,7 @@ except os.error, e:
> >      if e.errno != errno.EEXIST:
> >          raise
> >  
> > -exprs = parse_schema(sys.stdin)
> > +exprs = parse_schema(sys.stdin)[0]
> >  commands = filter(lambda expr: expr.has_key('command'), exprs)
> >  commands = filter(lambda expr: not expr.has_key('gen'), commands)
> >  
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index 4a1652b..0f86b95 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -15,6 +15,7 @@ import sys
> >  import os
> >  import getopt
> >  import errno
> > +import re
> >  
> >  def generate_fwd_struct(name, members, builtin_type=False):
> >      if builtin_type:
> > @@ -282,9 +283,10 @@ void qapi_free_%(type)s(%(c_type)s obj)
> >  
> >  
> >  try:
> > -    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
> > +    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbs:p:o:",
> >                                     ["source", "header", "builtins",
> > -                                    "prefix=", "output-dir="])
> > +                                    "schema-dump-file=", "prefix=",
> > +                                    "output-dir="])
> >  except getopt.GetoptError, err:
> >      print str(err)
> >      sys.exit(1)
> > @@ -293,6 +295,7 @@ output_dir = ""
> >  prefix = ""
> >  c_file = 'qapi-types.c'
> >  h_file = 'qapi-types.h'
> > +schema_dump_file = ""
> >  
> >  do_c = False
> >  do_h = False
> > @@ -309,11 +312,17 @@ for o, a in opts:
> >          do_h = True
> >      elif o in ("-b", "--builtins"):
> >          do_builtins = True
> > +    elif o in ("-s", "--schema-dump-file"):
> > +        schema_dump_file = a
> 
> Instead of adding this to qapi-types.py, wouldn't it be clearer to add
> a qapi-dump.py file instead?

I used scripts/qapi-introspect.py to generate qapi-introspect.h.
Q: qapi-introspect.py or qapi-dump.py? which one is better?

It also helps to extend schema and generate a nested dictionaries with
metadata, it's very simple to use python to extend.

/* OrderedDict([('command', 'query-name'), ('returns', 'NameInfo')]) */
    "{'_obj_member': 'False', '_obj_type': 'command', '_obj_name': 
'query-name', '_obj_data': {'returns': {'_obj_type': 'type', '_obj_member': 
'False', '_obj_name': 'NameInfo', '_obj_data': {'data': {'_obj_type': 
'anonymous-struct', '_obj_member': 'True', '_obj_name': '', '_obj_data': 
{'*name': 'str'}, '_obj_recursion': 'false'}}, '_obj_recursion': 'true'}}, 
'_obj_recursion': 'false'}",

Then in qmp.c, we only need to visit the dictionaries and fill the data
to allocated structs, which will be returned to QMP monitor.
 
> Also, I think it would be interesting to split this patch into two. The first
> patch changes qapi.py (and related files), this will allow you to better
> describe your changes to that file. The second patch adds qapi-dump.py.
> 
> In general, this looks good to me but I haven't looked into the
> changes done in qapi.py in detail.
 
In v3, we just change qapi.py:parse_schema() to additionally return raw json 
string.
In my latest patches, we don't need to change qapi.py, we can directly use 
OrderedDicts.

I'm working in qmp.c part, maybe we can try to simple DataObject definitions in 
V4.

BTW, no need to review v3, please wait my refreshed V4 :-)

Thanks, Amos