Re: [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support

[mdroth <mdroth@linux.vnet.ibm.com>]

On Fri, May 10, 2013 at 10:07:45AM -0400, Luiz Capitulino wrote:
> On Thu,  9 May 2013 21:20:53 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > Teach type generators about native types so they can generate the
> > appropriate linked list types.
> > 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >  scripts/qapi-types.py |   43 ++++++++++++++++++++++++++++++++++++++++---
> >  scripts/qapi.py       |   21 +++++++++++++++++++++
> >  2 files changed, 61 insertions(+), 3 deletions(-)
> > 
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index 9e19920..96cb26d 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -16,7 +16,18 @@ import os
> >  import getopt
> >  import errno
> >  
> > -def generate_fwd_struct(name, members):
> > +def generate_fwd_struct(name, members, builtin_type=False):
> > +    if builtin_type:
> > +        return mcgen('''
> > +typedef struct %(name)sList
> > +{
> > +    %(type)s value;
> > +    struct %(name)sList *next;
> > +} %(name)sList;
> > +''',
> 
> Sorry for the utterly minor comment, but as you're going to respin please
> add a newline before ''' so that we get the declarations properly separated
> when generated.
> 
> > +                     type=c_type(name),
> > +                     name=name)
> > +
> >      return mcgen('''
> >  typedef struct %(name)s %(name)s;
> >  
> > @@ -164,6 +175,7 @@ void qapi_free_%(type)s(%(c_type)s obj);
> >  
> >  def generate_type_cleanup(name):
> >      ret = mcgen('''
> > +
> >  void qapi_free_%(type)s(%(c_type)s obj)
> >  {
> >      QapiDeallocVisitor *md;
> > @@ -184,8 +196,9 @@ void qapi_free_%(type)s(%(c_type)s obj)
> >  
> >  
> >  try:
> > -    opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
> > -                                   ["source", "header", "prefix=", "output-dir="])
> > +    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)
> > @@ -197,6 +210,7 @@ h_file = 'qapi-types.h'
> >  
> >  do_c = False
> >  do_h = False
> > +do_builtins = False
> >  
> >  for o, a in opts:
> >      if o in ("-p", "--prefix"):
> > @@ -207,6 +221,8 @@ for o, a in opts:
> >          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
> > @@ -282,6 +298,11 @@ fdecl.write(mcgen('''
> >  exprs = parse_schema(sys.stdin)
> >  exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
> >  
> > +fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> > +for typename in builtin_types:
> > +    fdecl.write(generate_fwd_struct(typename, None, builtin_type=True))
> > +fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> > +
> >  for expr in exprs:
> >      ret = "\n"
> >      if expr.has_key('type'):
> > @@ -298,6 +319,22 @@ for expr in exprs:
> >          continue
> >      fdecl.write(ret)
> >  
> > +# to avoid header dependency hell, we always generate declarations
> > +# for built-in types in our header files and simply guard them
> > +fdecl.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
> > +for typename in builtin_types:
> > +    fdecl.write(generate_type_cleanup_decl(typename + "List"))
> > +fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
> 
> I'm not sure I got why you're doing this. Is it because you're going to
> generate them in more .h files? This is a bit ugly :(
> 

The issue is that things like the types generated from
qapi-schema-test.json or qga/qapi-schema.json may end up referencing
intList/strList/visit_type_intList/etc, which we'll also have declared for
use in the main qapi-schema.json. qapi-schema.json of course can't
depend on tests or qga, so we generate the definitions for the builtin types
when running the code generators on qapi-schema.json.

For everyone else, tests/qga/etc, to be able to use/link against those
definitions we need to include the declarations by either #include'ing
qapi-types.h/qapi-visit.h etc, or by always declaring them and simply
adding a guard.

In this case I've taken the latter approach since hard-coding a
reference in the code generators to header files created by separate calls
to the code generators seemed less modular. qapi-schema-test.json should
be capable of generating self-contained code if need be (and the only
reason we don't make it self-contained is that test-cases rely on
libqemuutil to build, which actually does have a dependency on
qapi-schema.json due to qemu-sockets.

> > +
> > +# ...this doesn't work for cases where we link in multiple objects that
> > +# have the functions defined, so we use -b option to provide control
> > +# over these cases
> > +if do_builtins:
> > +    fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
> > +    for typename in builtin_types:
> > +        fdef.write(generate_type_cleanup(typename + "List"))
> > +    fdef.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
> > +
> >  for expr in exprs:
> >      ret = "\n"
> >      if expr.has_key('type'):
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index afc5f32..0ac8c2b 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -11,6 +11,10 @@
> >  
> >  from ordereddict import OrderedDict
> >  
> > +builtin_types = [
> > +    'str', 'int', 'number', 'bool'
> > +]
> > +
> >  def tokenize(data):
> >      while len(data):
> >          ch = data[0]
> > @@ -242,3 +246,20 @@ def guardname(filename):
> >      for substr in [".", " ", "-"]:
> >          guard = guard.replace(substr, "_")
> >      return guard.upper() + '_H'
> > +
> > +def guardstart(name):
> > +    return mcgen('''
> > +
> > +#ifndef %(name)s
> > +#define %(name)s
> > +
> > +''',
> > +                 name=guardname(name))
> > +
> > +def guardend(name):
> > +    return mcgen('''
> > +
> > +#endif /* %(name)s */
> > +
> > +''',
> > +                 name=guardname(name))
>