Re: [Qemu-devel] [PATCH v2 3/4] qapi: Correctly handle downstream extensions in more locations

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Now that c_var() handles '.' in downstream extension names, fix
> the generator to support such names as additional types, enums,
> members within an enum, branches of a union or alternate, and
> in arrays.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi-commands.py |  4 ++--
>  scripts/qapi-types.py    | 27 ++++++++++++++-------------
>  scripts/qapi-visit.py    | 44 ++++++++++++++++++++++++--------------------
>  scripts/qapi.py          | 10 ++++++----
>  4 files changed, 46 insertions(+), 39 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index af1e1a1..84b66bc 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -22,9 +22,9 @@ import errno
>
>  def type_visitor(name):
>      if type(name) == list:
> -        return 'visit_type_%sList' % name[0]
> +        return 'visit_type_%sList' % type_name(name[0])
>      else:
> -        return 'visit_type_%s' % name
> +        return 'visit_type_%s' % type_name(name)
>
>  def generate_command_decl(name, args, ret_type):
>      arglist=""
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 8cf6349..b33b8fd 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -45,7 +45,7 @@ typedef struct %(name)sList
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
> -                 name=name)
> +                 name=c_var(name))
>
>  def generate_fwd_enum_struct(name, members):
>      return mcgen('''
> @@ -58,7 +58,7 @@ typedef struct %(name)sList
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
> -                 name=name)
> +                 name=c_var(name))
>
>  def generate_struct_fields(members):
>      ret = ''
> @@ -87,7 +87,7 @@ def generate_struct(expr):
>  struct %(name)s
>  {
>  ''',
> -          name=structname)
> +          name=c_var(structname))
>
>      if base:
>          ret += generate_struct_fields({'base': base})
> @@ -115,7 +115,7 @@ def generate_enum_lookup(name, values):
>      ret = mcgen('''
>  const char *%(name)s_lookup[] = {
>  ''',
> -                         name=name)
> +                name=c_var(name))
>      i = 0
>      for value in values:
>          index = generate_enum_full_value(name, value)
> @@ -134,6 +134,7 @@ const char *%(name)s_lookup[] = {
>      return ret
>
>  def generate_enum(name, values):
> +    name = c_var(name)
>      lookup_decl = mcgen('''
>  extern const char *%(name)s_lookup[];
>  ''',
> @@ -173,18 +174,18 @@ def generate_alternate_qtypes(expr):
>      ret = mcgen('''
>  const int %(name)s_qtypes[QTYPE_MAX] = {
>  ''',
> -    name=name)
> +                name=c_var(name))
>
>      for key in members:
>          qtype = find_alternate_member_qtype(members[key])
>          assert qtype, "Invalid alternate member"
>
>          ret += mcgen('''
> -    [ %(qtype)s ] = %(abbrev)s_KIND_%(enum)s,
> +    [%(qtype)s] = %(value)s,
>  ''',
> -        qtype = qtype,
> -        abbrev = de_camel_case(name).upper(),
> -        enum = c_var(de_camel_case(key),False).upper())
> +                     qtype = qtype,
> +                     value = generate_enum_full_value("%sKind" %c_var(name),
> +                                                      key))
>
>      ret += mcgen('''
>  };

I fixed this one in my "[PATCH RFC 06/19] qapi: Use c_enum_const() in
generate_alternate_qtypes()".

You picked just my "[PATCH RFC 02/19] qapi: Fix C identifiers generated
for names containing '.'" into your series.  Would you mind picking all
the related patches, i.e PATCH 02-07?

  qapi: Fix C identifiers generated for names containing '.'
  qapi: Rename _generate_enum_string() to camel_to_upper()
  qapi: Rename generate_enum_full_value() to c_enum_const()
  qapi: Simplify c_enum_const()
  qapi: Use c_enum_const() in generate_alternate_qtypes()
  qapi: Move camel_to_upper(), c_enum_const() to closely related code

> @@ -194,7 +195,7 @@ const int %(name)s_qtypes[QTYPE_MAX] = {
>
>  def generate_union(expr, meta):
>
> -    name = expr[meta]
> +    name = c_var(expr[meta])
>      typeinfo = expr['data']
>
>      base = expr.get('base')
> @@ -214,7 +215,7 @@ struct %(name)s
>          void *data;
>  ''',
>                  name=name,
> -                discriminator_type_name=discriminator_type_name)
> +                discriminator_type_name=c_var(discriminator_type_name))
>
>      for key in typeinfo:
>          ret += mcgen('''
> @@ -251,7 +252,7 @@ def generate_type_cleanup_decl(name):
>      ret = mcgen('''
>  void qapi_free_%(type)s(%(c_type)s obj);
>  ''',
> -                c_type=c_type(name),type=name)
> +                c_type=c_type(name),type=c_var(name))
>      return ret
>
>  def generate_type_cleanup(name):
> @@ -272,7 +273,7 @@ void qapi_free_%(type)s(%(c_type)s obj)
>      qapi_dealloc_visitor_cleanup(md);
>  }
>  ''',
> -                c_type=c_type(name),type=name)
> +                c_type=c_type(name),type=c_var(name))
>      return ret
>
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index f24dcfa..d1d3fe6 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -44,12 +44,13 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, 
> %(c_type)s **obj, Error *
>                   c_type=type_name(type))
>
>  def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, 
> base = None):
> +    assert field_prefix == ""

Makes me wonder why we have a field_prefix parameter.

fn_prefix is also always ""...

>      substructs = []
>      ret = ''
>      if not fn_prefix:
> -        full_name = name
> +        full_name = c_var(name)
>      else:

... and therefore this this code is unreachable.

Just observing, not asking you to do anything here.

> -        full_name = "%s_%s" % (name, fn_prefix)
> +        full_name = "%s_%s" % (c_var(name), fn_prefix)
>
>      if base:
>          ret += generate_visit_implicit_struct(base)
> @@ -60,7 +61,7 @@ static void visit_type_%(full_name)s_fields(Visitor *m, 
> %(name)s **obj, Error **
>  {
>      Error *err = NULL;
>  ''',
> -        name=name, full_name=full_name)
> +        name=c_var(name), full_name=full_name)
>      push_indent()
>
>      if base:
> @@ -121,9 +122,9 @@ def generate_visit_struct_body(field_prefix, name, 
> members):
>  ''')
>
>      if not field_prefix:
> -        full_name = name
> +        full_name = c_var(name)
>      else:
> -        full_name = "%s_%s" % (field_prefix, name)
> +        full_name = "%s_%s" % (field_prefix, c_var(name))
>
>      if len(field_prefix):
>          ret += mcgen('''
> @@ -132,9 +133,9 @@ def generate_visit_struct_body(field_prefix, name, 
> members):
>                  name=name)
>      else:
>          ret += mcgen('''
> -    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), 
> &err);
> +    visit_start_struct(m, (void **)obj, "%(name)s", name, 
> sizeof(%(c_name)s), &err);
>  ''',
> -                name=name)
> +                     name=name, c_name=c_var(name))
>
>      ret += mcgen('''
>      if (!err) {
> @@ -162,7 +163,7 @@ def generate_visit_struct(expr):
>  void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error 
> **errp)
>  {
>  ''',
> -                name=name)
> +                 name=c_var(name))
>
>      ret += generate_visit_struct_body("", name, members)
>
> @@ -171,7 +172,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, 
> const char *name, Error **e
>  ''')
>      return ret
>
> -def generate_visit_list(name, members):
> +def generate_visit_list(name, members, builtin=False):
> +    if not builtin:
> +        name = c_var(name)

Fun.

c_var() does two things:

(a) it protects certain words if protect=True

(b) it maps funny characters to '_'.

When builtin, (a) is unwanted, and (b) does nothing.  That's why we need
the conditional.

A possible alternative could be c_var(name, not builtin).  Matter of
taste.

Hmm, just saw what type_name() does.  Why not just

    name = type_name(name)

?

>      return mcgen('''
>
>  void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char 
> *name, Error **errp)
> @@ -208,7 +211,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s *obj, const 
> char *name, Error **er
>      visit_type_enum(m, (int *)obj, %(name)s_lookup, "%(name)s", name, errp);
>  }
>  ''',
> -                 name=name)
> +                 name=c_var(name))
>
>  def generate_visit_alternate(name, members):
>      ret = mcgen('''
> @@ -227,11 +230,11 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, 
> const char *name, Error **e
>      }
>      switch ((*obj)->kind) {
>  ''',
> -    name=name)
> +                name=c_var(name))
>
>      # For alternate, always use the default enum type automatically generated
>      # as "'%sKind' % (name)"
> -    disc_type = '%sKind' % (name)
> +    disc_type = '%sKind' % c_var(name)
>
>      for key in members:
>          assert (members[key] in builtin_types.keys()
> @@ -277,12 +280,12 @@ def generate_visit_union(expr):
>      if enum_define:
>          # Use the enum type as discriminator
>          ret = ""
> -        disc_type = enum_define['enum_name']
> +        disc_type = c_var(enum_define['enum_name'])
>      else:
>          # There will always be a discriminator in the C switch code, by 
> default
>          # it is an enum type generated silently as "'%sKind' % (name)"
>          ret = generate_visit_enum('%sKind' % name, members.keys())
> -        disc_type = '%sKind' % (name)
> +        disc_type = '%sKind' % c_var(name)
>
>      if base:
>          assert discriminator
> @@ -306,7 +309,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, 
> const char *name, Error **e
>      }
>      if (*obj) {
>  ''',
> -                 name=name)
> +                 name=c_var(name))
>
>      if base:
>          ret += mcgen('''
> @@ -315,7 +318,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, 
> const char *name, Error **e
>              goto out_obj;
>          }
>  ''',
> -            name=name)
> +                     name=c_var(name))
>
>      if not discriminator:
>          disc_key = "type"
> @@ -372,6 +375,7 @@ out:
>  def generate_declaration(name, members, builtin_type=False):
>      ret = ""
>      if not builtin_type:
> +        name = c_var(name)
>          ret += mcgen('''
>
>  void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error 
> **errp);
> @@ -381,7 +385,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, 
> const char *name, Error **e
>      ret += mcgen('''
>  void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char 
> *name, Error **errp);
>  ''',
> -                 name=name)
> +                     name=name)
>
>      return ret
>
> @@ -389,7 +393,7 @@ def generate_enum_declaration(name, members):
>      ret = mcgen('''
>  void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char 
> *name, Error **errp);
>  ''',
> -                name=name)
> +                name=c_var(name))
>
>      return ret
>
> @@ -398,7 +402,7 @@ def generate_decl_enum(name, members):
>
>  void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error 
> **errp);
>  ''',
> -                name=name)
> +                 name=c_var(name))
>
>  try:
>      opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:i:o:",
> @@ -515,7 +519,7 @@ fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL"))
>  # over these cases
>  if do_builtins:
>      for typename in builtin_types.keys():
> -        fdef.write(generate_visit_list(typename, None))
> +        fdef.write(generate_visit_list(typename, None, builtin=True))
>
>  for expr in exprs:
>      if expr.has_key('struct'):
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 1d64c62..e706712 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -782,12 +782,14 @@ def c_var(name, protect=True):
>      return name.translate(c_var_trans)
>
>  def c_list_type(name):
> -    return '%sList' % name
> +    return '%sList' % type_name(name)
>
>  def type_name(name):
>      if type(name) == list:
>          return c_list_type(name[0])
> -    return name
> +    if name in builtin_types.keys():
> +        return name
> +    return c_var(name)
>
>  def add_name(name, info, meta, implicit = False):
>      global all_names
> @@ -869,13 +871,13 @@ def c_type(name, is_param=False):
>      elif type(name) == list:
>          return '%s *%s' % (c_list_type(name[0]), eatspace)
>      elif is_enum(name):
> -        return name
> +        return c_var(name)
>      elif name == None or len(name) == 0:
>          return 'void'
>      elif name in events:
>          return '%sEvent *%s' % (camel_case(name), eatspace)
>      else:
> -        return '%s *%s' % (name, eatspace)
> +        return '%s *%s' % (c_var(name), eatspace)
>
>  def is_c_ptr(name):
>      suffix = "*" + eatspace

If it was my patch, I'd be tempted to split it up some.  Matter of
taste, feel free to keep it a single patch.