[Qemu-devel] [PATCH v2 0/4] Fix C identifiers generated for names containing '.'

[Qemu-devel] [PATCH v2 0/4] Fix C identifiers generated for names containing '.'

[Eric Blake]

This series of 4 replaces the RFC v1 of 02/19 of Markus' series:
https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00361.html

and requires as prereq: v6 of my nested qapi series (currently at 38/36):
https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00486.html

as well as prereq: v2 qapi: Drop dead genlist parameter:
https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01272.html

I took Markus' work and further extended it until the testsuite uses
downstream extension names everywhere in the schema (actually, I
wrote the test first, then hammered on the generator until it passed,
and rearranged the patches).

Eric Blake (3):
  qapi: Drop duplicate c_fun() in favor of c_var()
  qapi: Correctly handle downstream extensions in more locations
  qapi: Test name mangling of downstream extensions

Markus Armbruster (1):
  qapi: Fix C identifiers generated for names containing '.'

 scripts/qapi-commands.py                | 19 ++++++-------
 scripts/qapi-event.py                   |  2 +-
 scripts/qapi-types.py                   | 29 ++++++++++----------
 scripts/qapi-visit.py                   | 48 ++++++++++++++++++---------------
 scripts/qapi.py                         | 20 +++++++-------
 tests/qapi-schema/qapi-schema-test.json | 19 +++++++++++++
 tests/qapi-schema/qapi-schema-test.out  | 21 ++++++++++++---
 tests/test-qmp-commands.c               |  8 ++++++
 8 files changed, 108 insertions(+), 58 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v2 1/4] qapi: Fix C identifiers generated for names containing '.'

[Eric Blake]

From: Markus Armbruster <armbru@redhat.com>

c_fun() maps '.' to '_', c_var() doesn't.  Nothing prevents '.' in
QAPI names that get passed to c_var().

Which QAPI names get passed to c_fun(), to c_var(), or to both is not
obvious.  Names of command parameters and struct type members get
passed to c_var().

c_var() strips a leading '*', but this cannot happen.  c_fun()
doesn't.

Fix c_var() to work exactly like c_fun().

Perhaps they should be replaced by a single mapping function.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
[add 'import string']
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi.py | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index fff4462..82bf10d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -15,6 +15,7 @@ import re
 from ordereddict import OrderedDict
 import os
 import sys
+import string

 builtin_types = {
     'str':      'QTYPE_QSTRING',
@@ -747,6 +748,8 @@ def camel_case(name):
             new_name += ch.lower()
     return new_name

+c_var_trans = string.maketrans('.-', '__')
+
 def c_var(name, protect=True):
     # ANSI X3J11/88-090, 3.1.1
     c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
@@ -776,10 +779,10 @@ def c_var(name, protect=True):
     polluted_words = set(['unix', 'errno'])
     if protect and (name in c89_words | c99_words | c11_words | gcc_words | cpp_words | polluted_words):
         return "q_" + name
-    return name.replace('-', '_').lstrip("*")
+    return name.translate(c_var_trans)

 def c_fun(name, protect=True):
-    return c_var(name, protect).replace('.', '_')
+    return c_var(name, protect)

 def c_list_type(name):
     return '%sList' % name
-- 
2.1.0

[Qemu-devel] [PATCH v2 2/4] qapi: Drop duplicate c_fun() in favor of c_var()

[Eric Blake]

Now that the two functions are identical, we only need one of them.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-commands.py | 15 ++++++++-------
 scripts/qapi-event.py    |  2 +-
 scripts/qapi-types.py    |  4 ++--
 scripts/qapi-visit.py    |  4 ++--
 scripts/qapi.py          |  5 +----
 5 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index db81044..af1e1a1 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -36,7 +36,8 @@ def generate_command_decl(name, args, ret_type):
     return mcgen('''
 %(ret_type)s qmp_%(name)s(%(args)sError **errp);
 ''',
-                 ret_type=c_type(ret_type), name=c_fun(name), args=arglist).strip()
+                 ret_type=c_type(ret_type), name=c_var(name),
+                 args=arglist).strip()

 def gen_err_check(errvar):
     if errvar:
@@ -62,7 +63,7 @@ def gen_sync_call(name, args, ret_type, indent=0):
 %(retval)sqmp_%(name)s(%(args)s&local_err);

 ''',
-                name=c_fun(name), args=arglist, retval=retval).rstrip()
+                name=c_var(name), args=arglist, retval=retval).rstrip()
     if ret_type:
         ret += "\n" + gen_err_check('local_err')
         ret += "\n" + mcgen(''''
@@ -76,7 +77,7 @@ def gen_sync_call(name, args, ret_type, indent=0):
 def gen_marshal_output_call(name, ret_type):
     if not ret_type:
         return ""
-    return "qmp_marshal_output_%s(retval, ret, &local_err);" % c_fun(name)
+    return "qmp_marshal_output_%s(retval, ret, &local_err);" % c_var(name)

 def gen_visitor_input_containers_decl(args, obj):
     ret = ""
@@ -198,16 +199,16 @@ out:
     qapi_dealloc_visitor_cleanup(md);
 }
 ''',
-                c_ret_type=c_type(ret_type), c_name=c_fun(name),
+                c_ret_type=c_type(ret_type), c_name=c_var(name),
                 visitor=type_visitor(ret_type))

     return ret

 def gen_marshal_input_decl(name, args, ret_type, middle_mode):
     if middle_mode:
-        return 'int qmp_marshal_input_%s(Monitor *mon, const QDict *qdict, QObject **ret)' % c_fun(name)
+        return 'int qmp_marshal_input_%s(Monitor *mon, const QDict *qdict, QObject **ret)' % c_var(name)
     else:
-        return 'static void qmp_marshal_input_%s(QDict *args, QObject **ret, Error **errp)' % c_fun(name)
+        return 'static void qmp_marshal_input_%s(QDict *args, QObject **ret, Error **errp)' % c_var(name)



@@ -309,7 +310,7 @@ def gen_registry(commands):
         registry += mcgen('''
 qmp_register_command("%(name)s", qmp_marshal_input_%(c_name)s, %(opts)s);
 ''',
-                     name=cmd['command'], c_name=c_fun(cmd['command']),
+                     name=cmd['command'], c_name=c_var(cmd['command']),
                      opts=options)
     pop_indent()
     ret = mcgen('''
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 47dc041..bed809b 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -17,7 +17,7 @@ import getopt
 import errno

 def _generate_event_api_name(event_name, params):
-    api_name = "void qapi_event_send_%s(" % c_fun(event_name).lower();
+    api_name = "void qapi_event_send_%s(" % c_var(event_name).lower();
     l = len(api_name)

     if params:
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2bf8145..8cf6349 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -184,7 +184,7 @@ const int %(name)s_qtypes[QTYPE_MAX] = {
 ''',
         qtype = qtype,
         abbrev = de_camel_case(name).upper(),
-        enum = c_fun(de_camel_case(key),False).upper())
+        enum = c_var(de_camel_case(key),False).upper())

     ret += mcgen('''
 };
@@ -221,7 +221,7 @@ struct %(name)s
         %(c_type)s %(c_name)s;
 ''',
                      c_type=c_type(typeinfo[key]),
-                     c_name=c_fun(key))
+                     c_name=c_var(key))

     ret += mcgen('''
     };
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 6156162..f24dcfa 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -247,7 +247,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
 ''',
                 enum_full_value = enum_full_value,
                 c_type = type_name(members[key]),
-                c_name = c_fun(key))
+                c_name = c_var(key))

     ret += mcgen('''
     default:
@@ -348,7 +348,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
 ''',
                 enum_full_value = enum_full_value,
                 c_type=type_name(members[key]),
-                c_name=c_fun(key))
+                c_name=c_var(key))

     ret += mcgen('''
         default:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 82bf10d..1d64c62 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -781,9 +781,6 @@ def c_var(name, protect=True):
         return "q_" + name
     return name.translate(c_var_trans)

-def c_fun(name, protect=True):
-    return c_var(name, protect)
-
 def c_list_type(name):
     return '%sList' % name

@@ -940,7 +937,7 @@ def guardend(name):
 # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
 # ENUM24_Name -> ENUM24_NAME
 def _generate_enum_string(value):
-    c_fun_str = c_fun(value, False)
+    c_fun_str = c_var(value, False)
     if value.isupper():
         return c_fun_str

-- 
2.1.0

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

[Eric Blake]

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 <eblake@redhat.com>
---
 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('''
 };
@@ -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 == ""
     substructs = []
     ret = ''
     if not fn_prefix:
-        full_name = name
+        full_name = c_var(name)
     else:
-        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)
     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
-- 
2.1.0

[Qemu-devel] [PATCH v2 4/4] qapi: Test name mangling of downstream extensions

[Eric Blake]

So that we don't regress in supporting downstream extensions, make
sure that our testsuite covers downstream naming choices in as many
places as possible.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 19 +++++++++++++++++++
 tests/qapi-schema/qapi-schema-test.out  | 21 ++++++++++++++++++---
 tests/test-qmp-commands.c               |  8 ++++++++
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 8193dc1..a833b63 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -107,3 +107,22 @@
   'data': { '*a': 'int', '*b': 'UserDefOne', 'c': 'str' } }
 { 'event': 'EVENT_D',
   'data': { 'a' : 'EventStructOne', 'b' : 'str', '*c': 'str', '*enum3': 'EnumOne' } }
+
+# test that we correctly compile downstream extensions
+{ 'enum': '__org.qemu_x-Enum', 'data': [ '__org.qemu_x-value' ] }
+{ 'struct': '__org.qemu_x-Base',
+  'data': { '__org.qemu_x-member1': '__org.qemu_x-Enum' } }
+{ 'struct': '__org.qemu_x-Struct', 'base': '__org.qemu_x-Base',
+  'data': { '__org.qemu_x-member2': 'str' } }
+{ 'struct': '__org.qemu_x-Struct2',
+  'data': { 'array': ['__org.qemu_x-Union1'] } }
+{ 'union': '__org.qemu_x-Union1', 'data': { '__org.qemu_x-branch': 'str' } }
+{ 'union': '__org.qemu_x-Union2', 'base': '__org.qemu_x-Base',
+  'discriminator': '__org.qemu_x-member1',
+  'data': { '__org.qemu_x-value': '__org.qemu_x-Struct2' } }
+{ 'alternate': '__org.qemu_x-Alt',
+  'data': { '__org.qemu_x-branch': 'str', 'b': '__org.qemu_x-Base' } }
+{ 'event': '__ORG.QEMU_X-EVENT', 'data': '__org.qemu_x-Struct' }
+{ 'command': '__org.qemu_x-command',
+  'data': { 'a': ['__org.qemu_x-Enum'], 'b': ['__org.qemu_x-Struct'],
+            'c': '__org.qemu_x-Union2', 'd': '__org.qemu_x-Alt' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 93c4963..0e382d9 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -22,10 +22,22 @@
  OrderedDict([('event', 'EVENT_A')]),
  OrderedDict([('event', 'EVENT_B'), ('data', OrderedDict())]),
  OrderedDict([('event', 'EVENT_C'), ('data', OrderedDict([('*a', 'int'), ('*b', 'UserDefOne'), ('c', 'str')]))]),
- OrderedDict([('event', 'EVENT_D'), ('data', OrderedDict([('a', 'EventStructOne'), ('b', 'str'), ('*c', 'str'), ('*enum3', 'EnumOne')]))])]
+ OrderedDict([('event', 'EVENT_D'), ('data', OrderedDict([('a', 'EventStructOne'), ('b', 'str'), ('*c', 'str'), ('*enum3', 'EnumOne')]))]),
+ OrderedDict([('enum', '__org.qemu_x-Enum'), ('data', ['__org.qemu_x-value'])]),
+ OrderedDict([('struct', '__org.qemu_x-Base'), ('data', OrderedDict([('__org.qemu_x-member1', '__org.qemu_x-Enum')]))]),
+ OrderedDict([('struct', '__org.qemu_x-Struct'), ('base', '__org.qemu_x-Base'), ('data', OrderedDict([('__org.qemu_x-member2', 'str')]))]),
+ OrderedDict([('struct', '__org.qemu_x-Struct2'), ('data', OrderedDict([('array', ['__org.qemu_x-Union1'])]))]),
+ OrderedDict([('union', '__org.qemu_x-Union1'), ('data', OrderedDict([('__org.qemu_x-branch', 'str')]))]),
+ OrderedDict([('union', '__org.qemu_x-Union2'), ('base', '__org.qemu_x-Base'), ('discriminator', '__org.qemu_x-member1'), ('data', OrderedDict([('__org.qemu_x-value', '__org.qemu_x-Struct2')]))]),
+ OrderedDict([('alternate', '__org.qemu_x-Alt'), ('data', OrderedDict([('__org.qemu_x-branch', 'str'), ('b', '__org.qemu_x-Base')]))]),
+ OrderedDict([('event', '__ORG.QEMU_X-EVENT'), ('data', '__org.qemu_x-Struct')]),
+ OrderedDict([('command', '__org.qemu_x-command'), ('data', OrderedDict([('a', ['__org.qemu_x-Enum']), ('b', ['__org.qemu_x-Struct']), ('c', '__org.qemu_x-Union2'), ('d', '__org.qemu_x-Alt')]))])]
 [{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
+ {'enum_name': '__org.qemu_x-Enum', 'enum_values': ['__org.qemu_x-value']},
  {'enum_name': 'UserDefAlternateKind', 'enum_values': None},
- {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
+ {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None},
+ {'enum_name': '__org.qemu_x-Union1Kind', 'enum_values': None},
+ {'enum_name': '__org.qemu_x-AltKind', 'enum_values': None}]
 [OrderedDict([('struct', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
  OrderedDict([('struct', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('struct', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
@@ -37,4 +49,7 @@
  OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
  OrderedDict([('struct', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('struct', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))]),
- OrderedDict([('struct', 'EventStructOne'), ('data', OrderedDict([('struct1', 'UserDefOne'), ('string', 'str'), ('*enum2', 'EnumOne')]))])]
+ OrderedDict([('struct', 'EventStructOne'), ('data', OrderedDict([('struct1', 'UserDefOne'), ('string', 'str'), ('*enum2', 'EnumOne')]))]),
+ OrderedDict([('struct', '__org.qemu_x-Base'), ('data', OrderedDict([('__org.qemu_x-member1', '__org.qemu_x-Enum')]))]),
+ OrderedDict([('struct', '__org.qemu_x-Struct'), ('base', '__org.qemu_x-Base'), ('data', OrderedDict([('__org.qemu_x-member2', 'str')]))]),
+ OrderedDict([('struct', '__org.qemu_x-Struct2'), ('data', OrderedDict([('array', ['__org.qemu_x-Union1'])]))])]
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index ad2e403..a8036f2 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -51,6 +51,14 @@ int64_t qmp_user_def_cmd3(int64_t a, bool has_b, int64_t b, Error **errp)
     return a + (has_b ? b : 0);
 }

+void qmp___org_qemu_x_command(__org_qemu_x_EnumList *a,
+                              __org_qemu_x_StructList *b,
+                              __org_qemu_x_Union2 *c, __org_qemu_x_Alt *d,
+                              Error **errp)
+{
+}
+
+
 /* test commands with no input and no return value */
 static void test_dispatch_cmd(void)
 {
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 1/4] qapi: Fix C identifiers generated for names containing '.'

[Alberto Garcia]

On Sáb 11 Abr 2015 20:09:40 CEST, Eric Blake <eblake@redhat.com> wrote:
> From: Markus Armbruster <armbru@redhat.com>
>
> c_fun() maps '.' to '_', c_var() doesn't.  Nothing prevents '.' in
> QAPI names that get passed to c_var().
>
> Which QAPI names get passed to c_fun(), to c_var(), or to both is not
> obvious.  Names of command parameters and struct type members get
> passed to c_var().
>
> c_var() strips a leading '*', but this cannot happen.  c_fun()
> doesn't.
>
> Fix c_var() to work exactly like c_fun().
>
> Perhaps they should be replaced by a single mapping function.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> [add 'import string']
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v2 2/4] qapi: Drop duplicate c_fun() in favor of c_var()

[Alberto Garcia]

On Sáb 11 Abr 2015 20:09:41 CEST, Eric Blake wrote:
> Now that the two functions are identical, we only need one of them.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  scripts/qapi-commands.py | 15 ++++++++-------
>  scripts/qapi-event.py    |  2 +-
>  scripts/qapi-types.py    |  4 ++--
>  scripts/qapi-visit.py    |  4 ++--
>  scripts/qapi.py          |  5 +----
>  5 files changed, 14 insertions(+), 16 deletions(-)

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH v2 2/4] qapi: Drop duplicate c_fun() in favor of c_var()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Now that the two functions are identical, we only need one of them.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

c_var() isn't the possible best name, but it'll do.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
> ---
>  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.

Re: [Qemu-devel] [PATCH v2 4/4] qapi: Test name mangling of downstream extensions

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> So that we don't regress in supporting downstream extensions, make
> sure that our testsuite covers downstream naming choices in as many
> places as possible.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json | 19 +++++++++++++++++++
>  tests/qapi-schema/qapi-schema-test.out  | 21 ++++++++++++++++++---
>  tests/test-qmp-commands.c               |  8 ++++++++
>  3 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 8193dc1..a833b63 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -107,3 +107,22 @@
>    'data': { '*a': 'int', '*b': 'UserDefOne', 'c': 'str' } }
>  { 'event': 'EVENT_D',
>    'data': { 'a' : 'EventStructOne', 'b' : 'str', '*c': 'str', '*enum3': 'EnumOne' } }
> +
> +# test that we correctly compile downstream extensions
> +{ 'enum': '__org.qemu_x-Enum', 'data': [ '__org.qemu_x-value' ] }
> +{ 'struct': '__org.qemu_x-Base',
> +  'data': { '__org.qemu_x-member1': '__org.qemu_x-Enum' } }
> +{ 'struct': '__org.qemu_x-Struct', 'base': '__org.qemu_x-Base',
> +  'data': { '__org.qemu_x-member2': 'str' } }
> +{ 'struct': '__org.qemu_x-Struct2',
> +  'data': { 'array': ['__org.qemu_x-Union1'] } }
> +{ 'union': '__org.qemu_x-Union1', 'data': { '__org.qemu_x-branch': 'str' } }
> +{ 'union': '__org.qemu_x-Union2', 'base': '__org.qemu_x-Base',
> +  'discriminator': '__org.qemu_x-member1',
> +  'data': { '__org.qemu_x-value': '__org.qemu_x-Struct2' } }
> +{ 'alternate': '__org.qemu_x-Alt',
> +  'data': { '__org.qemu_x-branch': 'str', 'b': '__org.qemu_x-Base' } }
> +{ 'event': '__ORG.QEMU_X-EVENT', 'data': '__org.qemu_x-Struct' }
> +{ 'command': '__org.qemu_x-command',
> +  'data': { 'a': ['__org.qemu_x-Enum'], 'b': ['__org.qemu_x-Struct'],
> +            'c': '__org.qemu_x-Union2', 'd': '__org.qemu_x-Alt' } }

Can't say offhand whether 'returns' takes a separate path.  If it does,
we better test it here.

Since it can be fixed on top if it's necessary,
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3200 bytes --]

On 04/29/2015 05:29 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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.
>>

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

Sure, I can pull in more of your patches in my v3.


>> +++ 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 ""...

Hmm - looks like I was debugging whether the code was dead (by whether I
could trigger the assertion) and forgot to clean it up.  I'll split that
into a separate patch (I've already done another related cleanup that I
found while reviewing this code, in commit 6540e9f).


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

Probably because I was just hacking until tests passed, rather than
trying to go for optimal code (after all, that's what the review process
is for, to find better ways to do things).  Sounds like a good cleanup
for v3.

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

I'll try and split it some for the next round.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 4/4] qapi: Test name mangling of downstream extensions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

On 04/29/2015 05:32 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> So that we don't regress in supporting downstream extensions, make
>> sure that our testsuite covers downstream naming choices in as many
>> places as possible.
>>

>> +{ 'command': '__org.qemu_x-command',
>> +  'data': { 'a': ['__org.qemu_x-Enum'], 'b': ['__org.qemu_x-Struct'],
>> +            'c': '__org.qemu_x-Union2', 'd': '__org.qemu_x-Alt' } }
> 
> Can't say offhand whether 'returns' takes a separate path.  If it does,
> we better test it here.

Good catch.  I got lucky; no further tweaks needed to make it work.

> 
> Since it can be fixed on top if it's necessary,

Since I'm spinning v3 anyways, I'll fold it in.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

On 04/29/2015 09:30 AM, Eric Blake wrote:
> On 04/29/2015 05:29 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> 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.
>>>

>>>
>>>  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 ""...
> 
> Hmm - looks like I was debugging whether the code was dead (by whether I
> could trigger the assertion) and forgot to clean it up.  I'll split that
> into a separate patch (I've already done another related cleanup that I
> found while reviewing this code, in commit 6540e9f).

In fact, the code was NOT dead until I killed inline structs ("qapi:
Drop support for inline nested types").  Which means my cleanup wasn't
complete - but as that series is already at v7, I'll do the additional
cleanup as its own patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/4] qapi: Drop duplicate c_fun() in favor of c_var()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]

On 04/29/2015 05:04 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Now that the two functions are identical, we only need one of them.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> c_var() isn't the possible best name, but it'll do.

I'm looking at using c_name() in my v3.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

On 04/29/2015 05:29 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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.
>>

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

Oops, I think I missed this comment in my v3 posting, amidst all my
patch splitting.


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

v3 splits it up.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]