Re: [Qemu-devel] [PATCH v5 10/46] qapi: Merge generation of per-member visits

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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, with no change to
> generated marshal code, slightly more verbose visit code:
>
> |     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;
> |+        }
> |+    }

I think the more verbose code is easier to understand, because it checks
for errors exactly the same way as we do all the time, mimimizing
cognitive load.

> and slightly more verbose event code (recall that the qmp
> output visitor has a no-op visit_optional()):
>
> |+    visit_optional(v, &has_offset, "offset", &err);
> |+    if (err) {
> |+        goto out;
> |+    }

If we had a written contract, I suspect not calling visit_optional()
would be a bug.

> |     if (has_offset) {
> |         visit_type_int(v, &offset, "offset", &err);
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi-commands.py | 38 +---------------------------------
>  scripts/qapi-event.py    | 35 +++-----------------------------
>  scripts/qapi-visit.py    | 26 +-----------------------
>  scripts/qapi.py          | 53 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 58 insertions(+), 94 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 2151120..55287b1 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -25,17 +25,6 @@ def gen_command_decl(name, arg_type, ret_type):
>                   params=gen_params(arg_type, 'Error **errp'))
>
>
> -def gen_err_check(err):
> -    if not err:
> -        return ''
> -    return mcgen('''
> -if (%(err)s) {
> -    goto out;
> -}
> -''',
> -                 err=err)
> -
> -

Only code motion.

>  def gen_call(name, arg_type, ret_type):
>      ret = ''
>
> @@ -119,7 +108,6 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
>      push_indent()
>
>      if dealloc:
> -        errparg = 'NULL'
>          errarg = None
>          ret += mcgen('''
>  qmp_input_visitor_cleanup(mi);
> @@ -127,36 +115,12 @@ md = qapi_dealloc_visitor_new();
>  v = qapi_dealloc_get_visitor(md);
>  ''')
>      else:
> -        errparg = '&err'
>          errarg = 'err'
>          ret += mcgen('''
>  v = qmp_input_get_visitor(mi);
>  ''')
>
> -    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(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(errarg)
> -        if memb.optional:
> -            pop_indent()
> -            ret += mcgen('''
> -}
> -''')
> +    ret += gen_visit_fields(arg_type.members, '', False, errarg)

Perhaps a bit neater: make parameters prefix='', need_cast=False, and
say prefix=... and need_cast=True in the one call where you need it.

>
>      if dealloc:
>          ret += mcgen('''
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index b43bbc2..6c70a06 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -74,38 +74,9 @@ def gen_event_send(name, arg_type):
>
>  ''',
>                       name=name)
> -
> -        for memb in arg_type.members:
> -            if memb.optional:

Here's the missing visit_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);
> -    if (err) {
> -        goto out;
> -    }
> -''',
> -                         cast=cast,
> -                         c_name=c_name(memb.name),
> -                         c_type=memb.type.c_name(),
> -                         name=memb.name)
> -
> -            if memb.optional:
> -                pop_indent()
> -                ret += mcgen('''
> -    }
> -''')
> -
> +        push_indent()
> +        ret += gen_visit_fields(arg_type.members, '', True, 'err')
> +        pop_indent()
>          ret += mcgen('''
>
>      visit_end_struct(v, &err);
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 9c0328d..1f287ba 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -88,31 +88,7 @@ if (err) {
>  ''',
>                       c_type=base.c_name(), c_name=c_name('base'))
>
> -    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)

Here's the unconventional error checking.

> -            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 += mcgen('''
> -if (err) {
> -    goto out;
> -}
> -''')
> +    ret += gen_visit_fields(members, '(*obj)->', False, 'err')
>
>      pop_indent()
>      if re.search('^ *goto out;', ret, re.MULTILINE):
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 6f4e471..7275daa 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1531,6 +1531,59 @@ def gen_params(arg_type, extra):
>          ret += sep + extra
>      return ret
>
> +
> +def gen_err_check(err):
> +    if not err:
> +        return ''
> +    return mcgen('''
> +if (%(err)s) {
> +    goto out;
> +}
> +''',
> +                 err=err)
> +
> +
> +def gen_visit_fields(members, prefix, need_cast, errarg):
> +    ret = ''
> +    if errarg:
> +        errparg = '&' + errarg
> +    else:
> +        errparg = 'NULL'

Suggest a blank line here, just like in the code you replace.

> +    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(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(errarg)
> +
> +        if memb.optional:
> +            pop_indent()
> +            ret += mcgen('''
> +}
> +''')
> +    return ret
> +
>  #
>  # Common command line parsing
>  #