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

[Marc-André Lureau]

Hi

On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <address@hidden> wrote:
> 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.  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, rearrange the declaration to
> make it easier for a future patch to introduce the notion of
> a boxed event visit, and pass NULL rather than "" as the 'kind'
> parameter (matching most other uses where obj is NULL).
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> 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 | 14 ++++++--------
>  scripts/qapi.py       |  5 +++--
>  2 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 720486f..e37c07a 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -41,9 +41,9 @@ def gen_event_send(name, arg_type):
>
>      if arg_type and arg_type.members:
>          ret += mcgen('''
> +    QObject *obj;
>      QmpOutputVisitor *qov;
>      Visitor *v;
> -    QObject *obj;

This looks like churning code.

>
>  ''')
>
> @@ -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);
> -
>      v = qmp_output_get_visitor(qov);
> -    g_assert(v);
>
> -    /* 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);

The comment seemed somewhat useful to me.

>  ''',
>                       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')
>          ret += mcgen('''
> +out_obj:
>      visit_end_struct(v, &err);
>      if (err) {
>          goto out;
>      }
>
>      obj = qmp_output_get_qobject(qov);
> -    g_assert(obj != NULL);
> +    g_assert(obj);
>
>      qdict_put_obj(qmp, "data", obj);
>  ''')
> 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'):
>      ret = ''
>      if skiperr:
>          errparg = 'NULL'
> @@ -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()
> --
> 2.4.3
>
>

Reviewed-by: Marc-André Lureau <address@hidden>

-- 
Marc-André Lureau