Re: [Qemu-devel] [PATCH v9 07/37] qapi: Improve generated event use of qapi visitor

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> All other successful clients of visit_start_struct() were paired
> with an unconditional visit_end_struct(); but the generated
> code for events was relying on qmp_output_visitor_cleanup() to
> work on an incomplete visit.

Disgusting, isn't it?  :)

>                               Alter the code to guarantee that
> the struct is completed, which will make a future patch to
> split visit_end_struct() easier to reason about.  While at it,
> drop some assertions and comments that are not present in other
> uses of the qmp output visitor, and pass NULL rather than "" as
> the 'kind' parameter (matching most other uses where obj is NULL).
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v9: save churn in declaration order for later series on boxed params,
> drop Marc-Andre's R-b
> v8: no change
> v7: place earlier in series, adjust handling of 'kind'
> v6: new patch
>
> If desired, I can defer the hunk re-ordering the declaration of
> obj to later in the series where it actually comes in handy.
> ---
>  scripts/qapi-event.py | 12 +++++-------
>  scripts/qapi.py       |  5 +++--
>  2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 720486f..761cda9 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -61,25 +61,23 @@ def gen_event_send(name, arg_type):
>      if arg_type and arg_type.members:
>          ret += mcgen('''
>      qov = qmp_output_visitor_new();
> -    g_assert(qov);
> -

Good riddance: qmp_output_visitor_new() can't fail.  If that ever
changes, qmp_output_get_visitor() will crash just fine.

>      v = qmp_output_get_visitor(qov);
> -    g_assert(v);

Good riddance^2: qmp_output_get_visitor() can't fail, either.

>
> -    /* Fake visit, as if all members are under a structure */
> -    visit_start_struct(v, NULL, "", "%(name)s", 0, &err);
> +    visit_start_struct(v, NULL, NULL, "%(name)s", 0, &err);
>  ''',
>                       name=name)
>          ret += gen_err_check()
> -        ret += gen_visit_fields(arg_type.members, need_cast=True)
> +        ret += gen_visit_fields(arg_type.members, need_cast=True,
> +                                label='out_obj')

On error, we now go to out_obj rather than out.  Some fields will be
unvisited then (possibly all), and err will be set.

Now I get to figure out what this change changes.

>          ret += mcgen('''
> +out_obj:
>      visit_end_struct(v, &err);
>      if (err) {
>          goto out;
>      }

Good: we actually call visit_end_struct() as we should.

Not so good: if we get here via the error exit of gen_visit_fields(),
err is set.  If visit_end_struct() tries to set another error...

I guess the idea is to go from gen_visit_fields() failure through
visit_end_struct() here to out.  Correct?

>
>      obj = qmp_output_get_qobject(qov);
> -    g_assert(obj != NULL);
> +    g_assert(obj);
>
>      qdict_put_obj(qmp, "data", obj);
>  ''')

       ret += mcgen('''
       emit(%(c_enum)s, qmp, &err);

   ''',
                    c_enum=c_enum_const(event_enum_name, name))

       if arg_type and arg_type.members:
           ret += mcgen('''
   out:
       qmp_output_visitor_cleanup(qov);
   ''')
       ret += mcgen('''
       error_propagate(errp, err);
       QDECREF(qmp);
   }
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 7dec611..497eaba 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1636,7 +1636,8 @@ def gen_err_check(label='out', skiperr=False):
>                   label=label)
>
>
> -def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
> +def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False,
> +                     label='out'):

Probably clearer than label=None, but duplicates gen_err_check()'s
default.  Fine with me.

>      ret = ''
>      if skiperr:
>          errparg = 'NULL'
       else:
           errparg = '&err'

       for memb in members:
           if memb.optional:
               ret += mcgen('''
       if (visit_optional(v, "%(name)s", &%(prefix)shas_%(c_name)s)) {
   ''',
                            prefix=prefix, c_name=c_name(memb.name),
                            name=memb.name, errp=errparg)
               push_indent()

errp=errparg unused here.  Not this patch's job to clean up.

> @@ -1664,7 +1665,7 @@ def gen_visit_fields(members, prefix='', 
> need_cast=False, skiperr=False):
>                       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(skiperr=skiperr)
> +        ret += gen_err_check(skiperr=skiperr, label=label)
>
>          if memb.optional:
>              pop_indent()