Re: [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> qapi-commands had a nice helper gen_err_check(), but did not

In fact, it still has :)

> use it everywhere. In fact, using it in more places makes it
> easier to reduce the lines of code used in appending an error

Suggest "used for generating error checks"

> check in generated code (previously required a multi-line
> mcgen(), which didn't add any use of parameterization), which
> in turn makes it easier to write the next patch which will
> consolidate another common pattern among the generators.

I think we should burn some of the whiches ;)

Drop the paranthesis?

> The diffstat of this patch doesn't quite show as big a
> reduction in lines as I had hoped, but that is in part due to

Almost none...

> the duplication of some FIXME comments.
>
> Signed-off-by: Eric Blake <address@hidden>

Have you diffed the generated code before and after the patch?

> ---
> v6: new, split off of 10/46, add label parameter, and catch more
> instances that could use it
> ---
>  scripts/qapi-commands.py | 29 +++++++-------------
>  scripts/qapi-event.py    | 16 +++++------
>  scripts/qapi-visit.py    | 69 
> ++++++++++++++++++++++++------------------------
>  scripts/qapi.py          | 12 +++++++++
>  4 files changed, 62 insertions(+), 64 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 1d3c0d5..713aa0b 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)
> -
> -
>  def gen_call(name, arg_type, ret_type):
>      ret = ''
>
> @@ -56,7 +45,7 @@ def gen_call(name, arg_type, ret_type):
>  ''',
>                  c_name=c_name(name), args=argstr, lhs=lhs)
>      if ret_type:
> -        ret += gen_err_check('err')
> +        ret += gen_err_check()
>          ret += mcgen('''
>
>      qmp_marshal_output_%(c_name)s(retval, ret, &err);
> @@ -133,7 +122,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
>  ''',
>                           c_name=c_name(memb.name), name=memb.name,
>                           errp=errparg)
> -            ret += gen_err_check(errarg)
> +            ret += gen_err_check(err=errarg)
>              ret += mcgen('''
>      if (has_%(c_name)s) {
>  ''',
> @@ -144,7 +133,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
>  ''',
>                       c_name=c_name(memb.name), name=memb.name,
>                       c_type=memb.type.c_name(), errp=errparg)
> -        ret += gen_err_check(errarg)
> +        ret += gen_err_check(err=errarg)
>          if memb.optional:
>              pop_indent()
>              ret += mcgen('''
> @@ -159,7 +148,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
>
>
>  def gen_marshal_output(ret_type):
> -    return mcgen('''
> +    ret = mcgen('''
>
>  static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject 
> **ret_out, Error **errp)
>  {
> @@ -170,9 +159,10 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s 
> ret_in, QObject **ret_out,
>
>      v = qmp_output_get_visitor(qov);
>      visit_type_%(c_name)s(v, &ret_in, "unused", &err);
> -    if (err) {
> -        goto out;
> -    }
> +''',
> +                c_type=ret_type.c_type(), c_name=ret_type.c_name())
> +    ret += gen_err_check()
> +    ret += mcgen('''
>      *ret_out = qmp_output_get_qobject(qov);
>
>   out:

Here's a case that becomes more verbose, so it's not just comment
duplication.  Also becomes a bit harder to read, I think.

> @@ -184,7 +174,8 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s 
> ret_in, QObject **ret_out,
>      qapi_dealloc_visitor_cleanup(qdv);
>  }
>  ''',
> -                 c_type=ret_type.c_type(), c_name=ret_type.c_name())
> +                 c_name=ret_type.c_name())
> +    return ret
>
>
>  def gen_marshal_proto(name):
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index b43bbc2..bbc6169 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -68,12 +68,10 @@ def gen_event_send(name, arg_type):
>
>      /* Fake visit, as if all members are under a structure */
>      visit_start_struct(v, NULL, "", "%(name)s", 0, &err);
> -    if (err) {
> -        goto out;
> -    }
> -
>  ''',
>                       name=name)
> +        ret += gen_err_check()
> +        ret += '\n'
>
>          for memb in arg_type.members:
>              if memb.optional:

This one gets marginally shorter, but hardly simpler.

> @@ -91,14 +89,12 @@ def gen_event_send(name, arg_type):
>
>              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)
> +            ret += gen_err_check()
>
>              if memb.optional:
>                  pop_indent()
> @@ -109,9 +105,9 @@ def gen_event_send(name, arg_type):
>          ret += mcgen('''
>
>      visit_end_struct(v, &err);
> -    if (err) {
> -        goto out;
> -    }
> +''')
> +        ret += gen_err_check()
> +        ret += mcgen('''
>
>      obj = qmp_output_get_qobject(qov);
>      g_assert(obj != NULL);
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 7ce8616..fec07ad 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -81,11 +81,9 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
> %(c_name)s **obj, Error **e
>      if base:
>          ret += mcgen('''
>      visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
> -    if (err) {
> -        goto out;
> -    }
>  ''',
>                       c_type=base.c_name(), c_name=c_name('base'))
> +        ret += gen_err_check()
>
>      for memb in members:
>          if memb.optional:
> @@ -107,11 +105,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
> %(c_name)s **obj, Error **e
>              ret += mcgen('''
>      }
>  ''')
> -        ret += mcgen('''
> -    if (err) {
> -        goto out;
> -    }
> -''')
> +        ret += gen_err_check()
>
>      if re.search('^ *goto out;', ret, re.MULTILINE):
>          ret += mcgen('''
> @@ -154,7 +148,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
> const char *name, Error
>
>
>  def gen_visit_list(name, element_type):
> -    return mcgen('''
> +    ret = mcgen('''
>
>  void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, 
> Error **errp)
>  {
> @@ -162,9 +156,10 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
> const char *name, Error
>      GenericList *i, **prev;
>
>      visit_start_list(v, name, &err);
> -    if (err) {
> -        goto out;
> -    }
> +''',
> +                c_name=c_name(name), c_elt_type=element_type.c_name())
> +    ret += gen_err_check()
> +    ret += mcgen('''
>
>      for (prev = (GenericList **)obj;
>           !err && (i = visit_next_list(v, prev, &err)) != NULL;
> @@ -181,6 +176,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
> const char *name, Error
>  }
>  ''',
>                   c_name=c_name(name), c_elt_type=element_type.c_name())
> +    return ret
>
>
>  def gen_visit_enum(name):
> @@ -202,16 +198,17 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s 
> **obj, const char *name, Error
>      Error *err = NULL;
>
>      visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
> -    if (err) {
> -        goto out;
> -    }
> +''',
> +                c_name=c_name(name))
> +    ret += gen_err_check()
> +    ret += mcgen('''
>      visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, 
> &err);
> -    if (err) {
> -        goto out_obj;
> -    }
> +''',
> +                 c_name=c_name(name))
> +    ret += gen_err_check(label='out_obj')
> +    ret += mcgen('''
>      switch ((*obj)->kind) {
> -''',
> -                c_name=c_name(name))
> +''')
>
>      for var in variants.variants:
>          ret += mcgen('''
> @@ -259,23 +256,21 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s 
> **obj, const char *name, Error
>      Error *err = NULL;
>
>      visit_start_struct(v, (void **)obj, "%(name)s", name, 
> sizeof(%(c_name)s), &err);
> -    if (err) {
> -        goto out;
> -    }
> +''',
> +                 c_name=c_name(name), name=name)
> +    ret += gen_err_check()
> +    ret += mcgen('''
>      if (!*obj) {
>          goto out_obj;
>      }
> -''',
> -                 c_name=c_name(name), name=name)
> +''')
>
>      if base:
>          ret += mcgen('''
>      visit_type_%(c_name)s_fields(v, obj, &err);
> -    if (err) {
> -        goto out_obj;
> -    }
>  ''',
>                       c_name=c_name(name))
> +        ret += gen_err_check(label='out_obj')
>
>      tag_key = variants.tag_member.name
>      if not variants.tag_name:
> @@ -283,13 +278,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
> const char *name, Error
>          tag_key = 'type'
>      ret += mcgen('''
>      visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
> -    if (err) {
> -        goto out_obj;
> -    }
> -    if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
> -        goto out_obj;
> -    }
> -    switch ((*obj)->%(c_name)s) {
>  ''',
>                   c_type=variants.tag_member.type.c_name(),
>                   # TODO ugly special case for simple union
> @@ -297,6 +285,17 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
> const char *name, Error
>                   # it, then: c_name=c_name(variants.tag_member.name)
>                   c_name=c_name(variants.tag_name or 'kind'),
>                   name=tag_key)
> +    ret += gen_err_check(label='out_obj')
> +    ret += mcgen('''
> +    if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
> +        goto out_obj;
> +    }
> +    switch ((*obj)->%(c_name)s) {
> +''',
> +                 # TODO ugly special case for simple union
> +                 # Use same tag name in C as on the wire to get rid of
> +                 # it, then: c_name=c_name(variants.tag_member.name)
> +                 c_name=c_name(variants.tag_name or 'kind'))
>
>      for var in variants.variants:
>          # TODO ugly special case for simple union

Here's the duplicated comment.  It's a TODO, not a FIXME.

> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4825e62..0593b71 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1536,6 +1536,18 @@ def gen_params(arg_type, extra):
>          ret += sep + extra
>      return ret
>
> +
> +def gen_err_check(err='err', label='out'):
> +    if not err:
> +        return ''
> +    return mcgen('''
> +    if (%(err)s) {
> +        goto %(label)s;
> +    }
> +''',
> +                 err=err, label=label)
> +
> +
>  #
>  # Common command line parsing
>  #

I don't know.  Could be worthwhile if it really makes further work
easier.

To really cut the verbosity, I figure we'd have to do something more
radical, like having cgen() recognize a (short!) pattern and replace it
with a full-blown error check.  Not sure that's actually a good idea,
though :)