Here's how I'd rebase this on the simplified PATCH 06 I just posted as a reply to yours:
>From e35449172828351f0acd85f236add45d3eb575a7 Mon Sep 17 00:00:00 2001 From: Eric Blake <ebl...@redhat.com> Date: Wed, 9 Mar 2016 17:55:28 -0700 Subject: [PATCH 07/14] qapi: Utilize implicit struct visits 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, ¶m, &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}; |+ |+ 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. A similar collision still exists in the qapi_event_send_FOO(). Document it with a FIXME comment. This patch also paves the way for some followup simplifications in the generator, in subsequent patches. Signed-off-by: Eric Blake <ebl...@redhat.com> Message-Id: <1457571335-10938-8-git-send-email-ebl...@redhat.com> --- scripts/qapi-commands.py | 28 ++++++++++++---------------- scripts/qapi-event.py | 48 ++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 26 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 27af206..9b5c5b5 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -28,7 +28,37 @@ 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 + # data type passed in as parameters. If this collision ever hits in + # practice, we can rename our local variables with a leading _ prefix, + # or split the code into a wrapper function that creates a boxed + # 'param' object then calls another to do the real work. ret = mcgen(''' %(proto)s @@ -43,10 +73,11 @@ def gen_event_send(name, arg_type): ret += mcgen(''' QmpOutputVisitor *qov; Visitor *v; - ''') + ret += gen_param_var(arg_type) ret += mcgen(''' + emit = qmp_event_get_func_emit(); if (!emit) { return; @@ -58,26 +89,23 @@ def gen_event_send(name, arg_type): name=name) if arg_type and arg_type.members: - assert not arg_type.variants 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, ¶m, &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); -- 2.4.3