| Message ID | 1467514729-29366-12-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > When an unboxed event has accompanying data, we are exposing all > of its members alongside our local variables in the generated > qapi_event_send_FOO() function. So far, we haven't hit a > collision, but it may be a matter of time before someone wants > to name a QMP data element 'err' or similar. We can separate > the names by making the public function (qapi_event_send_FOO()) > a shell that boxes things up without having to worry about > collisions, then calls into a new worker function > (do_qapi_event_send_FOO()) that gets generated to look like what > we already output for boxed events. > > There is still a chance for collision with 'errp' (if that happens, > tweak c_name() to rename a QMP member 'errp' into the C member > 'q_errp'), and with 'param' (if that happens, tweak gen_event_send() > and gen_param_var() to name the temporary variable 'q_param'). But > with the division done here, the real worker function no longer has > to worry about collisions. > > Adding the new wrapper now means that we need .c_type() for an > anonymous type given as data to an event, because that type is used > in the signature of the new helper function, so we have to relax > an assertion in QAPISchemaObjectType. > > The generated file is unchanged for events without data, and for > boxed events; for unboxed events, the changes are as follows: > > |-void qapi_event_send_migration(MigrationStatus status, Error **errp) > |+static void do_qapi_event_send_migration(q_obj_MIGRATION_arg *arg, Error **errp) > | { > | QDict *qmp; > | Error *err = NULL; > | QMPEventFuncEmit emit; > | QObject *obj; > | Visitor *v; > |- q_obj_MIGRATION_arg param = { > |- status > |- }; > | > | emit = qmp_event_get_func_emit(); > | if (!emit) { > ... > | if (err) { > | goto out; > | } > |- visit_type_q_obj_MIGRATION_arg_members(v, ¶m, &err); > |+ visit_type_q_obj_MIGRATION_arg_members(v, arg, &err); > | if (!err) { > | visit_check_struct(v, &err); > ... > |+void qapi_event_send_migration(MigrationStatus status, Error **errp) > |+{ > |+ q_obj_MIGRATION_arg param = { > |+ status > |+ }; > |+ do_qapi_event_send_migration(¶m, errp); > |+} > |+ > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v8: don't open-code for boxed types, improve commit message, > s/intro/prefix/ > v7: new patch > --- > scripts/qapi.py | 1 - > scripts/qapi-event.py | 45 ++++++++++++++++++++++++++------------------- > 2 files changed, 26 insertions(+), 20 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 48263c4..e051892 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -1019,7 +1019,6 @@ class QAPISchemaObjectType(QAPISchemaType): def c_name(self): > return QAPISchemaType.c_name(self) Aside: this method became pointless in commit 7ce106a. > > def c_type(self): > - assert not self.is_implicit() > return c_name(self.name) + pointer_suffix Correct if we emit a C type with this name for *any* object type. I don't think we do for the empty object type. Any others? The assertion should be relaxed to reject exactly the object types for which we don't emit a C type. > > def c_unboxed_type(self): > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > index 2cab588..b2da0a9 100644 > --- a/scripts/qapi-event.py > +++ b/scripts/qapi-event.py > @@ -14,8 +14,13 @@ > from qapi import * > > > -def gen_event_send_proto(name, arg_type, box): > - return 'void qapi_event_send_%(c_name)s(%(param)s)' % { > +def gen_event_send_proto(name, arg_type, box, impl=False): The function has changed from one that does just one thing to one that can do many things depending on arg_type, box and impl. It's not even obvious which combinations are valid. Either split it into simple, self-documenting functions, or explain this complex one with a function comment. > + prefix='void ' > + if impl and arg_type and not box: > + box = True > + prefix='static void do_' > + return '%(prefix)sqapi_event_send_%(c_name)s(%(param)s)' % { > + 'prefix': prefix, > 'c_name': c_name(name.lower()), > 'param': gen_params(arg_type, box, 'Error **errp')} > > @@ -49,21 +54,12 @@ def gen_param_var(typ): > > }; > ''') > - if not typ.is_implicit(): > - ret += mcgen(''' > - %(c_name)s *arg = ¶m; > -''', > - c_name=typ.c_name()) > return ret > > > def gen_event_send(name, arg_type, box): > - # 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. > + if not arg_type or arg_type.is_empty(): > + assert not box > ret = mcgen(''' > > %(proto)s > @@ -72,17 +68,13 @@ def gen_event_send(name, arg_type, box): > Error *err = NULL; > QMPEventFuncEmit emit; > ''', > - proto=gen_event_send_proto(name, arg_type, box)) > + proto=gen_event_send_proto(name, arg_type, box, True)) > > if arg_type and not arg_type.is_empty(): > ret += mcgen(''' > QObject *obj; > Visitor *v; > ''') > - if not box: > - ret += gen_param_var(arg_type) > - else: > - assert not box > > ret += mcgen(''' > > @@ -112,7 +104,7 @@ def gen_event_send(name, arg_type, box): > if (err) { > goto out; > } > - visit_type_%(c_name)s_members(v, ¶m, &err); > + visit_type_%(c_name)s_members(v, arg, &err); > if (!err) { > visit_check_struct(v, &err); > } > @@ -144,6 +136,21 @@ out: > QDECREF(qmp); > } > ''') > + > + if arg_type and not box: > + ret += mcgen(''' > + > +%(proto)s > +{ > +''', > + proto=gen_event_send_proto(name, arg_type, box)) > + ret += gen_param_var(arg_type) > + ret += mcgen(''' > + do_qapi_event_send_%(c_name)s(¶m, errp); > +} > +''', > + c_name=c_name(name.lower())) > + > return ret This function spans 95 lines, and you have to read to the end to see that it first emits a function that can either be the qapi_event_send_FOO() function or a helper, and if it's the latter, then emits the former. Either split it up into parts that can be more easily understood, or add suitable comments to the complex function.
On 07/07/2016 05:37 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> When an unboxed event has accompanying data, we are exposing all >> of its members alongside our local variables in the generated >> qapi_event_send_FOO() function. So far, we haven't hit a >> collision, but it may be a matter of time before someone wants >> to name a QMP data element 'err' or similar. We can separate >> the names by making the public function (qapi_event_send_FOO()) >> a shell that boxes things up without having to worry about >> collisions, then calls into a new worker function >> (do_qapi_event_send_FOO()) that gets generated to look like what >> we already output for boxed events. >> >> +++ b/scripts/qapi.py >> @@ -1019,7 +1019,6 @@ class QAPISchemaObjectType(QAPISchemaType): > def c_name(self): >> return QAPISchemaType.c_name(self) > > Aside: this method became pointless in commit 7ce106a. > Sounds like an easy separate cleanup. >> >> def c_type(self): >> - assert not self.is_implicit() >> return c_name(self.name) + pointer_suffix > > Correct if we emit a C type with this name for *any* object type. I > don't think we do for the empty object type. Any others? > > The assertion should be relaxed to reject exactly the object types for > which we don't emit a C type. Will fix. > >> >> def c_unboxed_type(self): >> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py >> index 2cab588..b2da0a9 100644 >> --- a/scripts/qapi-event.py >> +++ b/scripts/qapi-event.py >> @@ -14,8 +14,13 @@ >> from qapi import * >> >> >> -def gen_event_send_proto(name, arg_type, box): >> - return 'void qapi_event_send_%(c_name)s(%(param)s)' % { >> +def gen_event_send_proto(name, arg_type, box, impl=False): > > The function has changed from one that does just one thing to one that > can do many things depending on arg_type, box and impl. It's not even > obvious which combinations are valid. Either split it into simple, > self-documenting functions, or explain this complex one with a function > comment. At this point, this cleanup is separate from the main work to get 'boxed' commands working, so I'll focus on getting the important first half of the series posted today, and leave this particular patch for when I have more time to get the refactoring done right. > > This function spans 95 lines, and you have to read to the end to see > that it first emits a function that can either be the > qapi_event_send_FOO() function or a helper, and if it's the latter, then > emits the former. Either split it up into parts that can be more easily > understood, or add suitable comments to the complex function. Indeed. The changes were all made piecemeal with minimal churn, but the end result is messier than if we step back and do it right.
diff --git a/scripts/qapi.py b/scripts/qapi.py index 48263c4..e051892 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1019,7 +1019,6 @@ class QAPISchemaObjectType(QAPISchemaType): return QAPISchemaType.c_name(self) def c_type(self): - assert not self.is_implicit() return c_name(self.name) + pointer_suffix def c_unboxed_type(self): diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py index 2cab588..b2da0a9 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -14,8 +14,13 @@ from qapi import * -def gen_event_send_proto(name, arg_type, box): - return 'void qapi_event_send_%(c_name)s(%(param)s)' % { +def gen_event_send_proto(name, arg_type, box, impl=False): + prefix='void ' + if impl and arg_type and not box: + box = True + prefix='static void do_' + return '%(prefix)sqapi_event_send_%(c_name)s(%(param)s)' % { + 'prefix': prefix, 'c_name': c_name(name.lower()), 'param': gen_params(arg_type, box, 'Error **errp')} @@ -49,21 +54,12 @@ def gen_param_var(typ): }; ''') - if not typ.is_implicit(): - ret += mcgen(''' - %(c_name)s *arg = ¶m; -''', - c_name=typ.c_name()) return ret def gen_event_send(name, arg_type, box): - # 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. + if not arg_type or arg_type.is_empty(): + assert not box ret = mcgen(''' %(proto)s @@ -72,17 +68,13 @@ def gen_event_send(name, arg_type, box): Error *err = NULL; QMPEventFuncEmit emit; ''', - proto=gen_event_send_proto(name, arg_type, box)) + proto=gen_event_send_proto(name, arg_type, box, True)) if arg_type and not arg_type.is_empty(): ret += mcgen(''' QObject *obj; Visitor *v; ''') - if not box: - ret += gen_param_var(arg_type) - else: - assert not box ret += mcgen(''' @@ -112,7 +104,7 @@ def gen_event_send(name, arg_type, box): if (err) { goto out; } - visit_type_%(c_name)s_members(v, ¶m, &err); + visit_type_%(c_name)s_members(v, arg, &err); if (!err) { visit_check_struct(v, &err); } @@ -144,6 +136,21 @@ out: QDECREF(qmp); } ''') + + if arg_type and not box: + ret += mcgen(''' + +%(proto)s +{ +''', + proto=gen_event_send_proto(name, arg_type, box)) + ret += gen_param_var(arg_type) + ret += mcgen(''' + do_qapi_event_send_%(c_name)s(¶m, errp); +} +''', + c_name=c_name(name.lower())) + return ret
When an unboxed event has accompanying data, we are exposing all of its members alongside our local variables in the generated qapi_event_send_FOO() function. So far, we haven't hit a collision, but it may be a matter of time before someone wants to name a QMP data element 'err' or similar. We can separate the names by making the public function (qapi_event_send_FOO()) a shell that boxes things up without having to worry about collisions, then calls into a new worker function (do_qapi_event_send_FOO()) that gets generated to look like what we already output for boxed events. There is still a chance for collision with 'errp' (if that happens, tweak c_name() to rename a QMP member 'errp' into the C member 'q_errp'), and with 'param' (if that happens, tweak gen_event_send() and gen_param_var() to name the temporary variable 'q_param'). But with the division done here, the real worker function no longer has to worry about collisions. Adding the new wrapper now means that we need .c_type() for an anonymous type given as data to an event, because that type is used in the signature of the new helper function, so we have to relax an assertion in QAPISchemaObjectType. The generated file is unchanged for events without data, and for boxed events; for unboxed events, the changes are as follows: |-void qapi_event_send_migration(MigrationStatus status, Error **errp) |+static void do_qapi_event_send_migration(q_obj_MIGRATION_arg *arg, Error **errp) | { | QDict *qmp; | Error *err = NULL; | QMPEventFuncEmit emit; | QObject *obj; | Visitor *v; |- q_obj_MIGRATION_arg param = { |- status |- }; | | emit = qmp_event_get_func_emit(); | if (!emit) { ... | if (err) { | goto out; | } |- visit_type_q_obj_MIGRATION_arg_members(v, ¶m, &err); |+ visit_type_q_obj_MIGRATION_arg_members(v, arg, &err); | if (!err) { | visit_check_struct(v, &err); ... |+void qapi_event_send_migration(MigrationStatus status, Error **errp) |+{ |+ q_obj_MIGRATION_arg param = { |+ status |+ }; |+ do_qapi_event_send_migration(¶m, errp); |+} |+ Signed-off-by: Eric Blake <eblake@redhat.com> --- v8: don't open-code for boxed types, improve commit message, s/intro/prefix/ v7: new patch --- scripts/qapi.py | 1 - scripts/qapi-event.py | 45 ++++++++++++++++++++++++++------------------- 2 files changed, 26 insertions(+), 20 deletions(-)