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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 23 Jan 2014 11:05:06 +0800
Amos Kong <akong@redhat.com> wrote:

> On Wed, Jan 22, 2014 at 01:06:51PM -0500, Luiz Capitulino wrote:
> > On Sun,  5 Jan 2014 20:02:30 +0800
> > Amos Kong <akong@redhat.com> 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 <akong@redhat.com>
> > > ---
> > >  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?

qapi-introspect.py is good.

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