| Message ID | 1457194595-16189-6-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| 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, 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 <eblake@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);
On 03/08/2016 08:10 AM, 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. >> >> Generated code shrinks accordingly; events initialize a struct >> based on parameters, such as: >> >> >> And command marshalling generates call arguments from a stack-allocated >> struct: > > I see qmp-marshal.c shrink from ~5700 lines to ~4300. Neat! Yeah, it nicely balances out the .h getting so much larger, except that the .h gets parsed a lot more by the compiler. >> >> +# 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? Sure, I'm not tied to a specific name. I will point out that we have a potential collision point - any local variable we create here can collide with members of the QAPI struct passed to the event. We haven't hit the collision on any events we've created so far, and it's easy to rename our local variables at such time if we do run into the collision down the road, so I won't worry about it now. > >> + 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? The compiler complains if you try to initialize a 'char *' member of a QAPI C struct with a 'const char *' parameter. It's no different than the fact that the gen_visit_members() call we are getting rid of also has to cast away const. > >> + 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? Yeah, I guess so. I originally put it in qapi.py thinking that I could reuse it in qapi-commands.py, then realized that the two are opposite (event collects parameters into a struct, commands parses a QObject into parameters), so I couldn't share it after all. >> @@ -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? Oh, nice. I noticed some cleanups in patch 6/10, but missed this one as a reasonable improvement. In fact, gen_visit_members() is now only used in qapi-visits.py, so maybe I can move it back there (it used to live there before commit 82ca8e469 moved it for sharing with the two files simplified here).
Eric Blake <eblake@redhat.com> writes: > On 03/08/2016 08:10 AM, 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. >>> >>> Generated code shrinks accordingly; events initialize a struct >>> based on parameters, such as: >>> > >>> >>> And command marshalling generates call arguments from a stack-allocated >>> struct: >> >> I see qmp-marshal.c shrink from ~5700 lines to ~4300. Neat! > > Yeah, it nicely balances out the .h getting so much larger, except that > the .h gets parsed a lot more by the compiler. > > >>> >>> +# 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? > > Sure, I'm not tied to a specific name. I will point out that we have a > potential collision point - any local variable we create here can > collide with members of the QAPI struct passed to the event. We haven't > hit the collision on any events we've created so far, and it's easy to > rename our local variables at such time if we do run into the collision > down the road, so I won't worry about it now. This patch actually fixes a similar issue in the qmp_marshal_FOO() functions. To keep ignoring it in the qapi_event_send_BAR() functions is okay. It's fairly easy to fix now, though: split them into two, so that the outer half does nothing but parameter wrapping. For instance, void qapi_event_send_block_io_error(const char *device, IoOperationType operation, BlockErrorAction action, bool has_nospace, bool nospace, const char *reason, Error **errp) { QDict *qmp; Error *err = NULL; QMPEventFuncEmit emit; QmpOutputVisitor *qov; Visitor *v; QObject *obj; _obj_BLOCK_IO_ERROR_arg param = { (char *)device, operation, action, has_nospace, nospace, (char *)reason }; [do stuff...] } becomes static inline void do_event_send_block_io_error(_obj_BLOCK_IO_ERROR_arg param, Error **errp) { QDict *qmp; Error *err = NULL; QMPEventFuncEmit emit; QmpOutputVisitor *qov; Visitor *v; QObject *obj; [do stuff...] } void qapi_event_send_block_io_error(const char *device, IoOperationType operation, BlockErrorAction action, bool has_nospace, bool nospace, const char *reason, Error **errp) { do_event_send_block_io_error((_obj_BLOCK_IO_ERROR_arg){ (char *)device, operation, action, has_nospace, nospace, (char *)reason }, errp); }; } Feel free not to do that now, but mark the spot with a comment then. Since it's technically wrong, we could even mark it FIXME. >> >>> + 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? > > The compiler complains if you try to initialize a 'char *' member of a > QAPI C struct with a 'const char *' parameter. It's no different than > the fact that the gen_visit_members() call we are getting rid of also > has to cast away const. I see. Const never fails to annoy. [...]
On 03/08/2016 11:09 AM, Markus Armbruster wrote: > This patch actually fixes a similar issue in the qmp_marshal_FOO() > functions. Indeed, and I didn't even realize it. I'll add that to the commit message :) > > To keep ignoring it in the qapi_event_send_BAR() functions is okay. > It's fairly easy to fix now, though: split them into two, so that the > outer half does nothing but parameter wrapping. For instance, > > void qapi_event_send_block_io_error(const char *device, IoOperationType operation, BlockErrorAction action, bool has_nospace, bool nospace, const char *reason, Error **errp) > { > QDict *qmp; > Error *err = NULL; > QMPEventFuncEmit emit; > QmpOutputVisitor *qov; > Visitor *v; > QObject *obj; > _obj_BLOCK_IO_ERROR_arg param = { > (char *)device, operation, action, has_nospace, nospace, (char *)reason > }; > > [do stuff...] > } > > becomes > > static inline void do_event_send_block_io_error(_obj_BLOCK_IO_ERROR_arg param, Error **errp) > { > QDict *qmp; > Error *err = NULL; > QMPEventFuncEmit emit; > QmpOutputVisitor *qov; > Visitor *v; > QObject *obj; > > [do stuff...] > } > > void qapi_event_send_block_io_error(const char *device, IoOperationType operation, BlockErrorAction action, bool has_nospace, bool nospace, const char *reason, Error **errp) Still means we can't have 'errp' as a QMP member of the error, without some sort of renaming. Again, not worth worrying about until we actually want to avoid the collision. > { > do_event_send_block_io_error((_obj_BLOCK_IO_ERROR_arg){ > (char *)device, operation, action, has_nospace, nospace, (char *)reason > }, errp); > }; > } > > Feel free not to do that now, but mark the spot with a comment then. > Since it's technically wrong, we could even mark it FIXME. In fact, I have a patch in a later series [1] that WANTS to let the user supply a boxed parameter - at which point, the difference between two vs. one function would be whether the user requested boxing. Sounds like I add the FIXME here, and then that series can take care of the possible split. [1] https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg04394.html
Eric Blake <eblake@redhat.com> writes: > On 03/08/2016 11:09 AM, Markus Armbruster wrote: > >> This patch actually fixes a similar issue in the qmp_marshal_FOO() >> functions. > > Indeed, and I didn't even realize it. I'll add that to the commit message :) > >> >> To keep ignoring it in the qapi_event_send_BAR() functions is okay. >> It's fairly easy to fix now, though: split them into two, so that the >> outer half does nothing but parameter wrapping. For instance, >> >> void qapi_event_send_block_io_error(const char *device, IoOperationType operation, BlockErrorAction action, bool has_nospace, bool nospace, const char *reason, Error **errp) >> { >> QDict *qmp; >> Error *err = NULL; >> QMPEventFuncEmit emit; >> QmpOutputVisitor *qov; >> Visitor *v; >> QObject *obj; >> _obj_BLOCK_IO_ERROR_arg param = { >> (char *)device, operation, action, has_nospace, nospace, (char *)reason >> }; >> >> [do stuff...] >> } >> >> becomes >> >> static inline void do_event_send_block_io_error(_obj_BLOCK_IO_ERROR_arg param, Error **errp) >> { >> QDict *qmp; >> Error *err = NULL; >> QMPEventFuncEmit emit; >> QmpOutputVisitor *qov; >> Visitor *v; >> QObject *obj; >> >> [do stuff...] >> } >> >> void qapi_event_send_block_io_error(const char *device, IoOperationType operation, BlockErrorAction action, bool has_nospace, bool nospace, const char *reason, Error **errp) > > Still means we can't have 'errp' as a QMP member of the error, without > some sort of renaming. Again, not worth worrying about until we > actually want to avoid the collision. Rename it to q_errp. >> { >> do_event_send_block_io_error((_obj_BLOCK_IO_ERROR_arg){ >> (char *)device, operation, action, has_nospace, nospace, (char *)reason >> }, errp); >> }; >> } >> >> Feel free not to do that now, but mark the spot with a comment then. >> Since it's technically wrong, we could even mark it FIXME. > > In fact, I have a patch in a later series [1] that WANTS to let the user > supply a boxed parameter - at which point, the difference between two > vs. one function would be whether the user requested boxing. Sounds > like I add the FIXME here, and then that series can take care of the > possible split. > > [1] https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg04394.html Works for me.
On 03/08/2016 09:11 AM, Eric Blake wrote: >>> >>> - 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? > > Oh, nice. I noticed some cleanups in patch 6/10, but missed this one as > a reasonable improvement. > > In fact, gen_visit_members() is now only used in qapi-visits.py, so > maybe I can move it back there (it used to live there before commit > 82ca8e469 moved it for sharing with the two files simplified here). In fact, this also nukes the only use of c_null(). I'll have a couple of cleanup patches in v5.
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): + 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 *)' + ret += c_name(memb.name) + ret += mcgen(''' + + }; +''') + return ret + + 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: @@ -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 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);
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: |@@ -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 <eblake@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(-)