qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v5 07/14] qapi: Utilize implicit struct visits


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v5 07/14] qapi: Utilize implicit struct visits
Date: Thu, 10 Mar 2016 20:05:54 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

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, through a new gen_param_var() helper, like:
>
> |@@ -338,6 +250,9 @@ void qapi_event_send_block_job_error(con
> |     QMPEventFuncEmit emit = qmp_event_get_func_emit();
> |     QmpOutputVisitor *qov;
> |     Visitor *v;
> |+    q_obj_BLOCK_JOB_ERROR_arg param = {
> |+        (char *)device, operation, action
> |+    };
> |
> |     if (!emit) {
> |         return;
> @@ -351,19 +266,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_q_obj_BLOCK_JOB_ERROR_arg_members(v, &param, &err);
> |     visit_end_struct(v, err ? NULL : &err);
>
> Notice that the initialization of 'param' has to cast away const
> (just as the old gen_visit_members() had to do): we can't change
> the signature of the user function (which uses 'const char *'), but
> have to assign it to a non-const QAPI object (which requires
> 'char *').
>
> Likewise, command marshalling generates call arguments from a
> stack-allocated struct, rather than a list of local variables:
>
> |@@ -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;
> |-        }
> |+    q_obj_add_fd_arg qapi = {0};

Let's calls this arg.

> |+
> |+    v = qmp_input_get_visitor(qiv);
> |+    visit_type_q_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_q_obj_add_fd_arg_members(v, &qapi, NULL);
> |     qapi_dealloc_visitor_cleanup(qdv);
> | }
>
> For the marshaller, it has the nice side effect of eliminating a
> chance of collision between argument QMP names and local variables.
>
> This patch also paves the way for some followup simplifications
> in the generator, in subsequent patches.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v5: move qapi.py:gen_struct_init() to qapi-event.py:gen_param_var(),
> improve commit message
> v4: new patch
> ---
>  scripts/qapi-commands.py | 28 ++++++++++++----------------
>  scripts/qapi-event.py    | 40 +++++++++++++++++++++++++++++++---------
>  2 files changed, 43 insertions(+), 25 deletions(-)
>
> 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:
> @@ -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('''
>
> @@ -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)
> +    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 02c9556..c125ecb 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -28,6 +28,30 @@ def gen_event_send_decl(name, arg_type):
>                   proto=gen_event_send_proto(name, arg_type))
>
>
> +# Declare and initialize an object 'qapi' using parameters from gen_params()
> +def gen_param_var(typ):
> +    assert not typ.variants
> +    ret = mcgen('''
> +    %(c_name)s param = {
> +''',
> +                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 *)'
> +        ret += c_name(memb.name)
> +    ret += mcgen('''
> +
> +    };
> +''')
> +    return ret
> +
> +
>  def gen_event_send(name, arg_type):
>      # FIXME: Our declaration of local variables (and of 'errp' in the
>      # parameter list) can collide with exploded members of the event's
> @@ -50,6 +74,7 @@ def gen_event_send(name, arg_type):
>      QmpOutputVisitor *qov;
>      Visitor *v;
>  ''')
> +        ret += gen_param_var(arg_type)
>
>      ret += mcgen('''
>
> @@ -62,25 +87,22 @@ def gen_event_send(name, arg_type):
>                   name=name)
>
>      if arg_type and arg_type.members:
> -        assert not arg_type.variants

Already asserted by new gen_param_var().  Thanks for paying attention to
details.

>          ret += mcgen('''
>      qov = qmp_output_visitor_new();
>      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, &param, &err);
>      visit_end_struct(v, err ? NULL : &err);
>      if (err) {
>          goto out;
>      }
>      qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov));
> -''')
> +''',
> +                     name=name, c_name=arg_type.c_name())
>
>      ret += mcgen('''
>      emit(%(c_enum)s, qmp, &err);



reply via email to

[Prev in Thread] Current Thread [Next in Thread]