Re: [Qemu-devel] [PATCH v4 05/10] qapi: Utilize implicit struct visits

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Rather than generate inline per-member visits, take advantage
> of the 'visit_type_FOO_members()' function for both event and
> command marshalling.  This is possible now that implicit
> structs can be visited like any other.
>
> Generated code shrinks accordingly; events initialize a struct
> based on parameters, such as:
>
> |@@ -381,6 +293,9 @@ void qapi_event_send_block_job_error(con
> |     QmpOutputVisitor *qov;
> |     Visitor *v;
> |     QObject *obj;
> |+    _obj_BLOCK_JOB_ERROR_arg qapi = {
> |+        (char *)device, operation, action
> |+    };
> |
> |     emit = qmp_event_get_func_emit();
> |     if (!emit) {
> |@@ -396,19 +311,7 @@ void qapi_event_send_block_job_error(con
> |     if (err) {
> |         goto out;
> |     }
> |-    visit_type_str(v, "device", (char **)&device, &err);
> |-    if (err) {
> |-        goto out_obj;
> |-    }
> |-    visit_type_IoOperationType(v, "operation", &operation, &err);
> |-    if (err) {
> |-        goto out_obj;
> |-    }
> |-    visit_type_BlockErrorAction(v, "action", &action, &err);
> |-    if (err) {
> |-        goto out_obj;
> |-    }
> |-out_obj:
> |+    visit_type__obj_BLOCK_JOB_ERROR_arg_members(v, &qapi, &err);
> |     visit_end_struct(v, err ? NULL : &err);
>
> And command marshalling generates call arguments from a stack-allocated
> struct:

I see qmp-marshal.c shrink from ~5700 lines to ~4300.  Neat!

>
> |@@ -57,26 +57,15 @@ void qmp_marshal_add_fd(QDict *args, QOb
> |    QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
> |    QapiDeallocVisitor *qdv;
> |    Visitor *v;
> |-    bool has_fdset_id = false;
> |-    int64_t fdset_id = 0;
> |-    bool has_opaque = false;
> |-    char *opaque = NULL;
> |-
> |-    v = qmp_input_get_visitor(qiv);
> |-    if (visit_optional(v, "fdset-id", &has_fdset_id)) {
> |-        visit_type_int(v, "fdset-id", &fdset_id, &err);
> |-        if (err) {
> |-            goto out;
> |-        }
> |-    }
> |-    if (visit_optional(v, "opaque", &has_opaque)) {
> |-        visit_type_str(v, "opaque", &opaque, &err);
> |-        if (err) {
> |-            goto out;
> |-        }
> |+    _obj_add_fd_arg qapi = {0};
> |+
> |+    v = qmp_input_get_visitor(qiv);
> |+    visit_type__obj_add_fd_arg_members(v, &qapi, &err);
> |+    if (err) {
> |+        goto out;
> |     }
> |
> |-    retval = qmp_add_fd(has_fdset_id, fdset_id, has_opaque, opaque, &err);
> |+    retval = qmp_add_fd(qapi.has_fdset_id, qapi.fdset_id, qapi.has_opaque, 
> qapi.opaque, &err);
> |     if (err) {
> |         goto out;
> |     }
> |@@ -88,12 +77,7 @@ out:
> |     qmp_input_visitor_cleanup(qiv);
> |     qdv = qapi_dealloc_visitor_new();
> |     v = qapi_dealloc_get_visitor(qdv);
> |-    if (visit_optional(v, "fdset-id", &has_fdset_id)) {
> |-        visit_type_int(v, "fdset-id", &fdset_id, NULL);
> |-    }
> |-    if (visit_optional(v, "opaque", &has_opaque)) {
> |-        visit_type_str(v, "opaque", &opaque, NULL);
> |-    }
> |+    visit_type__obj_add_fd_arg_members(v, &qapi, NULL);
> |     qapi_dealloc_visitor_cleanup(qdv);
> | }
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v4: new patch
> ---
>  scripts/qapi.py          | 25 +++++++++++++++++++++++++
>  scripts/qapi-commands.py | 28 ++++++++++++----------------
>  scripts/qapi-event.py    | 16 +++++++---------
>  3 files changed, 44 insertions(+), 25 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 6cf0a75..797ac7a 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1640,6 +1640,7 @@ extern const char *const %(c_name)s_lookup[];
>      return ret
>
>
> +# Explode arg_type into a parameter list, along with extra parameters
>  def gen_params(arg_type, extra):
>      if not arg_type:
>          return extra
> @@ -1657,6 +1658,30 @@ def gen_params(arg_type, extra):
>      return ret
>
>
> +# Declare and initialize an object 'qapi' using parameters from gen_params()
> +def gen_struct_init(typ):

It's not just a "struct init", it's a variable declaration with an
initializer.  gen_param_var()?

Name the variable param rather than qapi?

> +    assert not typ.variants
> +    ret = mcgen('''
> +    %(c_name)s qapi = {
> +''',
> +                c_name=typ.c_name())
> +    sep = '        '
> +    for memb in typ.members:
> +        ret += sep
> +        sep = ', '
> +        if memb.optional:
> +            ret += 'has_' + c_name(memb.name) + sep
> +        if memb.type.name == 'str':
> +            # Cast away const added in gen_params()
> +            ret += '(char *)'

Why is that useful?

> +        ret += c_name(memb.name)
> +    ret += mcgen('''
> +
> +    };
> +''')
> +    return ret

Odd: you use this only in qapi-event.py, not in qapi-commands.py.
Should it live in qapi-event.py then?

> +
> +
>  def gen_err_check(label='out', skiperr=False):
>      if skiperr:
>          return ''
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 3784f33..5ffc381 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -33,8 +33,8 @@ def gen_call(name, arg_type, ret_type):
>          assert not arg_type.variants
>          for memb in arg_type.members:
>              if memb.optional:
> -                argstr += 'has_%s, ' % c_name(memb.name)
> -            argstr += '%s, ' % c_name(memb.name)
> +                argstr += 'qapi.has_%s, ' % c_name(memb.name)
> +            argstr += 'qapi.%s, ' % c_name(memb.name)
>
>      lhs = ''
>      if ret_type:

This is necessary, because...

> @@ -71,21 +71,10 @@ def gen_marshal_vars(arg_type, ret_type):
>      QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
>      QapiDeallocVisitor *qdv;
>      Visitor *v;
> -''')
> +    %(c_name)s qapi = {0};
>
> -        for memb in arg_type.members:
> -            if memb.optional:
> -                ret += mcgen('''
> -    bool has_%(c_name)s = false;
>  ''',
> -                             c_name=c_name(memb.name))
> -            ret += mcgen('''
> -    %(c_type)s %(c_name)s = %(c_null)s;
> -''',
> -                         c_name=c_name(memb.name),
> -                         c_type=memb.type.c_type(),
> -                         c_null=memb.type.c_null())
> -        ret += '\n'
> +                     c_name=arg_type.c_name())
>      else:
>          ret += mcgen('''

... you wrap the parameter variables in a struct here.  Okay.

>
> @@ -107,17 +96,24 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
>      qdv = qapi_dealloc_visitor_new();
>      v = qapi_dealloc_get_visitor(qdv);
>  ''')
> +        errp = 'NULL'
>      else:
>          ret += mcgen('''
>      v = qmp_input_get_visitor(qiv);
>  ''')
> +        errp = '&err'
>
> -    ret += gen_visit_members(arg_type.members, skiperr=dealloc)

Unless I'm mistaken, this is the only use of skiperr.  Follow up with a
patch to drop the parameter and simplify?

> +    ret += mcgen('''
> +    visit_type_%(c_name)s_members(v, &qapi, %(errp)s);
> +''',
> +                 c_name=arg_type.c_name(), errp=errp)
>
>      if dealloc:
>          ret += mcgen('''
>      qapi_dealloc_visitor_cleanup(qdv);
>  ''')
> +    else:
> +        ret += gen_err_check()
>      return ret
>
>
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index c03cb78..627e9a0 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -44,8 +44,8 @@ def gen_event_send(name, arg_type):
>      QmpOutputVisitor *qov;
>      Visitor *v;
>      QObject *obj;
> -
>  ''')
> +        ret += gen_struct_init(arg_type) + '\n'
>
>      ret += mcgen('''
>      emit = qmp_event_get_func_emit();
> @@ -65,13 +65,10 @@ def gen_event_send(name, arg_type):
>      v = qmp_output_get_visitor(qov);
>
>      visit_start_struct(v, "%(name)s", NULL, 0, &err);
> -''',
> -                     name=name)
> -        ret += gen_err_check()
> -        ret += gen_visit_members(arg_type.members, need_cast=True,
> -                                 label='out_obj')
> -        ret += mcgen('''
> -out_obj:
> +    if (err) {
> +        goto out;
> +    }
> +    visit_type_%(c_name)s_members(v, &qapi, &err);
>      visit_end_struct(v, err ? NULL : &err);
>      if (err) {
>          goto out;
> @@ -81,7 +78,8 @@ out_obj:
>      g_assert(obj);
>
>      qdict_put_obj(qmp, "data", obj);
> -''')
> +''',
> +                     name=name, c_name=arg_type.c_name())
>
>      ret += mcgen('''
>      emit(%(c_enum)s, qmp, &err);