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

[Luiz Capitulino <lcapitulino@redhat.com>]

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?

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.

>  
>  if not do_c and not do_h:
>      do_c = True
>      do_h = True
>  
> +if schema_dump_file:
> +    do_c = False
> +    do_h = False
> +
>  c_file = output_dir + prefix + c_file
>  h_file = output_dir + prefix + h_file
>  
> @@ -381,7 +390,40 @@ fdecl.write(mcgen('''
>  ''',
>                    guard=guardname(h_file)))
>  
> -exprs = parse_schema(sys.stdin)
> +exprs_all = parse_schema(sys.stdin)
> +
> +schema_table = """/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +
> +/*
> + * Schema json string table converted from qapi-schema.json
> + *
> + * Copyright (c) 2013 Red Hat, Inc.
> + *
> + * Authors:
> + *  Amos Kong <akong@redhat.com>
> + *
> + * 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.
> + *
> + */
> +
> +const char *const qmp_schema_table[] = {
> +"""
> +
> +if schema_dump_file:
> +    for line in exprs_all[1]:
> +        line = re.sub(r'#.*\n', ' ', line.strip())
> +        line = re.sub(r'\n', ' ', line.strip())
> +        line = re.sub(r' +', ' ', line)
> +        schema_table += '  "%s",\n' % (line)
> +
> +    schema_table += '  NULL };\n'
> +    f = open(schema_dump_file, "w")
> +    f.write(schema_table)
> +    f.flush()
> +    f.close()
> +
> +exprs = exprs_all[0]
>  exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
>  
>  fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 65f1a54..db68084 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -480,7 +480,7 @@ fdecl.write(mcgen('''
>  ''',
>                    prefix=prefix, guard=guardname(h_file)))
>  
> -exprs = parse_schema(sys.stdin)
> +exprs = parse_schema(sys.stdin)[0]
>  
>  # to avoid header dependency hell, we always generate declarations
>  # for built-in types in our header files and simply guard them
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4263bbd..43cc401 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -61,10 +61,13 @@ class QAPISchema:
>              self.src += '\n'
>          self.cursor = 0
>          self.exprs = []
> +        self.raw_exprs = []
>          self.accept()
>  
>          while self.tok != None:
> -            self.exprs.append(self.get_expr(False))
> +            self.cur_entry= ''
> +            self.exprs.append(self.get_expr(False, True))
> +            self.raw_exprs.append(self.cur_entry)
>  
>      def accept(self):
>          while True:
> @@ -103,9 +106,11 @@ class QAPISchema:
>              elif not self.tok.isspace():
>                  raise QAPISchemaError(self, 'Stray "%s"' % self.tok)
>  
> -    def get_members(self):
> +    def get_members(self, start=None):
>          expr = OrderedDict()
>          if self.tok == '}':
> +            if start != None:
> +                self.cur_entry = self.src[start:self.cursor]
>              self.accept()
>              return expr
>          if self.tok != "'":
> @@ -118,6 +123,8 @@ class QAPISchema:
>              self.accept()
>              expr[key] = self.get_expr(True)
>              if self.tok == '}':
> +                if start != None:
> +                    self.cur_entry = self.src[start:self.cursor]
>                  self.accept()
>                  return expr
>              if self.tok != ',':
> @@ -142,12 +149,15 @@ class QAPISchema:
>                  raise QAPISchemaError(self, 'Expected "," or "]"')
>              self.accept()
>  
> -    def get_expr(self, nested):
> +    def get_expr(self, nested, first=False):
>          if self.tok != '{' and not nested:
>              raise QAPISchemaError(self, 'Expected "{"')
>          if self.tok == '{':
> +            start = None
> +            if first:
> +                start = self.cursor - 1
>              self.accept()
> -            expr = self.get_members()
> +            expr = self.get_members(start)
>          elif self.tok == '[':
>              self.accept()
>              expr = self.get_values()
> @@ -174,7 +184,7 @@ def parse_schema(fp):
>          elif expr.has_key('type'):
>              add_struct(expr)
>  
> -    return schema.exprs
> +    return schema.exprs, schema.raw_exprs
>  
>  def parse_args(typeinfo):
>      if isinstance(typeinfo, basestring):