Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
>
> ---
> 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);
