[Qemu-devel] [PATCH v6 16/16] qapi: Share gen_visit_fields()

[Eric Blake]

Consolidate the code between visit, command marshalling, and
event generation that iterates over the members of a struct.
It reduces code duplication in the generator, so that a future
patch can reduce the size of generated code while touching only
one instead of three locations.

There are no changes to generated marshal code.

The visitor code is slightly more verbose, but semantically
equivalent, and actually easier to read as it follows a more
common idiom:

|     visit_optional(v, &(*obj)->has_device, "device", &err);
|-    if (!err && (*obj)->has_device) {
|-        visit_type_str(v, &(*obj)->device, "device", &err);
|-    }
|     if (err) {
|         goto out;
|     }
|+    if ((*obj)->has_device) {
|+        visit_type_str(v, &(*obj)->device, "device", &err);
|+        if (err) {
|+            goto out;
|+        }
|+    }

The event code is slightly more verbose, but arguably a bug
fix (although the visitors are not well documented, use of an
optional member should not be attempted unless guarded by a
prior call to visit_optional()) - we were lucky that the output
qmp visitor has a no-op visit_optional():

|+    visit_optional(v, &has_offset, "offset", &err);
|+    if (err) {
|+        goto out;
|+    }
|     if (has_offset) {
|         visit_type_int(v, &offset, "offset", &err);

Signed-off-by: Eric Blake <address@hidden>

---
v6: retitle 10/46, split gen_err_check() to its own patch,
improve commit message, use named parameters in gen_visit_fields()
to minimize effort of callers
---
 scripts/qapi-commands.py | 27 +--------------------------
 scripts/qapi-event.py    | 31 +------------------------------
 scripts/qapi-visit.py    | 22 +---------------------
 scripts/qapi.py          | 43 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 46 insertions(+), 77 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 713aa0b..b44ab1e 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -101,7 +101,6 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
         return ret

     if dealloc:
-        errparg = 'NULL'
         errarg = None
         ret += mcgen('''
     qmp_input_visitor_cleanup(qiv);
@@ -109,36 +108,12 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
     v = qapi_dealloc_get_visitor(qdv);
 ''')
     else:
-        errparg = '&err'
         errarg = 'err'
         ret += mcgen('''
     v = qmp_input_get_visitor(qiv);
 ''')

-    for memb in arg_type.members:
-        if memb.optional:
-            ret += mcgen('''
-    visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
-''',
-                         c_name=c_name(memb.name), name=memb.name,
-                         errp=errparg)
-            ret += gen_err_check(err=errarg)
-            ret += mcgen('''
-    if (has_%(c_name)s) {
-''',
-                         c_name=c_name(memb.name))
-            push_indent()
-        ret += mcgen('''
-    visit_type_%(c_type)s(v, &%(c_name)s, "%(name)s", %(errp)s);
-''',
-                     c_name=c_name(memb.name), name=memb.name,
-                     c_type=memb.type.c_name(), errp=errparg)
-        ret += gen_err_check(err=errarg)
-        if memb.optional:
-            pop_indent()
-            ret += mcgen('''
-    }
-''')
+    ret += gen_visit_fields(arg_type.members, errarg=errarg)

     if dealloc:
         ret += mcgen('''
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index bbc6169..7eecaeb 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -72,36 +72,7 @@ def gen_event_send(name, arg_type):
                      name=name)
         ret += gen_err_check()
         ret += '\n'
-
-        for memb in arg_type.members:
-            if memb.optional:
-                ret += mcgen('''
-    if (has_%(c_name)s) {
-''',
-                             c_name=c_name(memb.name))
-                push_indent()
-
-            # Ugly: need to cast away the const
-            if memb.type.name == "str":
-                cast = '(char **)'
-            else:
-                cast = ''
-
-            ret += mcgen('''
-    visit_type_%(c_type)s(v, %(cast)s&%(c_name)s, "%(name)s", &err);
-''',
-                         cast=cast,
-                         c_name=c_name(memb.name),
-                         c_type=memb.type.c_name(),
-                         name=memb.name)
-            ret += gen_err_check()
-
-            if memb.optional:
-                pop_indent()
-                ret += mcgen('''
-    }
-''')
-
+        ret += gen_visit_fields(arg_type.members, need_cast=True)
         ret += mcgen('''

     visit_end_struct(v, &err);
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index fec07ad..65b5e8d 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -85,27 +85,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
%(c_name)s **obj, Error **e
                      c_type=base.c_name(), c_name=c_name('base'))
         ret += gen_err_check()

-    for memb in members:
-        if memb.optional:
-            ret += mcgen('''
-    visit_optional(v, &(*obj)->has_%(c_name)s, "%(name)s", &err);
-    if (!err && (*obj)->has_%(c_name)s) {
-''',
-                         c_name=c_name(memb.name), name=memb.name)
-            push_indent()
-
-        ret += mcgen('''
-    visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
-''',
-                     c_type=memb.type.c_name(), c_name=c_name(memb.name),
-                     name=memb.name)
-
-        if memb.optional:
-            pop_indent()
-            ret += mcgen('''
-    }
-''')
-        ret += gen_err_check()
+    ret += gen_visit_fields(members, prefix='(*obj)->')

     if re.search('^ *goto out;', ret, re.MULTILINE):
         ret += mcgen('''
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 0593b71..d3fdf46 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1548,6 +1548,49 @@ def gen_err_check(err='err', label='out'):
                  err=err, label=label)


+def gen_visit_fields(members, prefix='', need_cast=False, errarg='err'):
+    ret = ''
+    if errarg:
+        errparg = '&' + errarg
+    else:
+        errparg = 'NULL'
+
+    for memb in members:
+        if memb.optional:
+            ret += mcgen('''
+    visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s", %(errp)s);
+''',
+                         prefix=prefix, c_name=c_name(memb.name),
+                         name=memb.name, errp=errparg)
+            ret += gen_err_check(err=errarg)
+            ret += mcgen('''
+    if (%(prefix)shas_%(c_name)s) {
+''',
+                         prefix=prefix, c_name=c_name(memb.name))
+            push_indent()
+
+        # Ugly: sometimes we need to cast away const
+        if need_cast and memb.type.name == 'str':
+            cast = '(char **)'
+        else:
+            cast = ''
+
+        ret += mcgen('''
+    visit_type_%(c_type)s(v, %(cast)s&%(prefix)s%(c_name)s, "%(name)s", 
%(errp)s);
+''',
+                     c_type=memb.type.c_name(), prefix=prefix, cast=cast,
+                     c_name=c_name(memb.name), name=memb.name,
+                     errp=errparg)
+        ret += gen_err_check(err=errarg)
+
+        if memb.optional:
+            pop_indent()
+            ret += mcgen('''
+    }
+''')
+    return ret
+
+
 #
 # Common command line parsing
 #
-- 
2.4.3