| Message ID | 1457571335-10938-8-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@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, 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}; 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 <eblake@redhat.com> > > --- > 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, ¶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);
On 03/10/2016 12:05 PM, Markus Armbruster wrote: > Eric Blake <eblake@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. >> >> Likewise, command marshalling generates call arguments from a >> stack-allocated struct, rather than a list of local variables: >> >> |- goto out; >> |- } >> |+ q_obj_add_fd_arg qapi = {0}; > > Let's calls this arg. Sure. > >> |+ >> |+ 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); and this line then gets a bit shorter. >> +++ 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)) >> @@ -50,6 +74,7 @@ def gen_event_send(name, arg_type): >> QmpOutputVisitor *qov; >> Visitor *v; >> ''') >> + ret += gen_param_var(arg_type) >> >> ret += mcgen(''' >> This is why I moved the blank line in 6/14. But I can rearrange things as you requested. I'm also wondering if this should be split into two patches (one for qapi-visit, one for qapi-commands); when I first started writing it, I thought there would be some code sharing between the two with edits to qapi.py (see the v4 posting); but now they are distinct enough that two commits is just as easy to do.
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 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);
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. This patch also paves the way for some followup simplifications in the generator, in subsequent patches. Signed-off-by: Eric Blake <eblake@redhat.com> --- 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(-)