| Message ID | 1463784024-17242-11-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > When an event has data that is not boxed, we are exposing all of > its members alongside our local variables. 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 a shell that creates a > simple wrapper, then calls a worker that operates on only the > boxed version and thus has no user-supplied names to worry about > in naming its local variables. For boxed events, we don't need > the wrapper. > > 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. > > The generated file changes 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> > > --- > v7: new patch > --- > scripts/qapi.py | 1 - > scripts/qapi-event.py | 47 ++++++++++++++++++++++++++--------------------- > 2 files changed, 26 insertions(+), 22 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 6742e7a..7d568d9 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -1016,7 +1016,6 @@ class QAPISchemaObjectType(QAPISchemaType): > return QAPISchemaType.c_name(self) > > def c_type(self): > - assert not self.is_implicit() Huh? > 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 b8ca8c8..fe4e50d 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): > + intro='void ' > + if impl and arg_type and not box: > + box = True > + intro='static void do_' > + return '%(intro)sqapi_event_send_%(c_name)s(%(param)s)' % { > + 'intro': intro, > 'c_name': c_name(name.lower()), > 'param': gen_params(arg_type, box, 'Error **errp')} > I'd call it prefix rather than intro. > @@ -53,12 +58,8 @@ def gen_param_var(typ): > > > 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(): > + box = False > ret = mcgen(''' > > %(proto)s > @@ -67,15 +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) Because the not box case moves to the wrapper function (last hunk). > > ret += mcgen(''' > > @@ -93,20 +92,11 @@ def gen_event_send(name, arg_type, box): > ret += mcgen(''' > v = qmp_output_visitor_new(&obj); > > -''') > - > - if box: > - ret += mcgen(''' > - visit_type_%(c_name)s(v, NULL, &arg, &err); > -''', > - c_name=arg_type.c_name(), name=arg_type.name) > - else: > - ret += mcgen(''' > visit_start_struct(v, "%(name)s", NULL, 0, &err); > 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); > } Getting confused... why are we getting rid of the box case here? Too many conditionals... gen_event_send() has three cases: empty arg_type, non-empty arg_type and box, non-empty arg_type and not box. The commit message shows the change to generated code for the second case. It doesn't show visit_type_%(c_name)s(v, NULL, &arg, &err) going away. > @@ -136,6 +126,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
Markus Armbruster <armbru@redhat.com> writes: > Eric Blake <eblake@redhat.com> writes: > >> When an event has data that is not boxed, we are exposing all of >> its members alongside our local variables. 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 a shell that creates a >> simple wrapper, then calls a worker that operates on only the >> boxed version and thus has no user-supplied names to worry about >> in naming its local variables. For boxed events, we don't need >> the wrapper. >> >> 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. >> >> The generated file changes 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> >> >> --- >> v7: new patch >> --- >> scripts/qapi.py | 1 - >> scripts/qapi-event.py | 47 ++++++++++++++++++++++++++--------------------- >> 2 files changed, 26 insertions(+), 22 deletions(-) >> >> diff --git a/scripts/qapi.py b/scripts/qapi.py >> index 6742e7a..7d568d9 100644 >> --- a/scripts/qapi.py >> +++ b/scripts/qapi.py >> @@ -1016,7 +1016,6 @@ class QAPISchemaObjectType(QAPISchemaType): >> return QAPISchemaType.c_name(self) >> >> def c_type(self): >> - assert not self.is_implicit() > > Huh? > >> 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 b8ca8c8..fe4e50d 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): >> + intro='void ' >> + if impl and arg_type and not box: >> + box = True >> + intro='static void do_' >> + return '%(intro)sqapi_event_send_%(c_name)s(%(param)s)' % { >> + 'intro': intro, >> 'c_name': c_name(name.lower()), >> 'param': gen_params(arg_type, box, 'Error **errp')} >> > > I'd call it prefix rather than intro. > >> @@ -53,12 +58,8 @@ def gen_param_var(typ): >> >> >> 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(): >> + box = False >> ret = mcgen(''' >> >> %(proto)s >> @@ -67,15 +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) > > Because the not box case moves to the wrapper function (last hunk). > >> >> ret += mcgen(''' >> >> @@ -93,20 +92,11 @@ def gen_event_send(name, arg_type, box): >> ret += mcgen(''' >> v = qmp_output_visitor_new(&obj); >> >> -''') >> - >> - if box: >> - ret += mcgen(''' >> - visit_type_%(c_name)s(v, NULL, &arg, &err); >> -''', >> - c_name=arg_type.c_name(), name=arg_type.name) >> - else: >> - ret += mcgen(''' >> visit_start_struct(v, "%(name)s", NULL, 0, &err); >> 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); >> } > > Getting confused... why are we getting rid of the box case here? > > Too many conditionals... gen_event_send() has three cases: empty > arg_type, non-empty arg_type and box, non-empty arg_type and not box. > The commit message shows the change to generated code for the second > case. It doesn't show visit_type_%(c_name)s(v, NULL, &arg, &err) going > away. Case empty arg_type: no change Example: POWERDOWN Case non-empty arg_type and box: visit gets open-coded Example: EVENT_E void qapi_event_send_event_e(UserDefZero *arg, Error **errp) { QDict *qmp; Error *err = NULL; QMPEventFuncEmit emit; QObject *obj; Visitor *v; emit = qmp_event_get_func_emit(); if (!emit) { return; } qmp = qmp_event_build_dict("EVENT_E"); v = qmp_output_visitor_new(&obj); - visit_type_UserDefZero(v, NULL, &arg, &err); + visit_start_struct(v, "EVENT_E", NULL, 0, &err); + if (err) { + goto out; + } + visit_type_UserDefZero_members(v, arg, &err); + if (!err) { + visit_check_struct(v, &err); + } + visit_end_struct(v, NULL); if (err) { goto out; } visit_complete(v, &obj); qdict_put_obj(qmp, "data", obj); emit(TEST_QAPI_EVENT_EVENT_E, qmp, &err); out: visit_free(v); error_propagate(errp, err); QDECREF(qmp); } Compare: void visit_type_UserDefZero(Visitor *v, const char *name, UserDefZero **obj, Error **errp) { Error *err = NULL; visit_start_struct(v, name, (void **)obj, sizeof(UserDefZero), &err); if (err) { goto out; } ---> if (!*obj) { ---> goto out_obj; ---> } visit_type_UserDefZero_members(v, *obj, &err); ---> if (err) { ---> goto out_obj; ---> } ---> visit_check_struct(v, &err); out_obj: visit_end_struct(v, (void **)obj); ---> if (err && visit_is_input(v)) { ---> qapi_free_UserDefZero(*obj); ---> *obj = NULL; ---> } out: error_propagate(errp, err); } The open-coded visit drops the !*obj check (okay, @arg isn't going anywhere), skips the visit_check_struct() differently, and drops the qapi_free_FOO() (okay, condition is always false here). So this isn't wrong. But why open-code? Case non-empty arg_type and not box: Example: ACPI_DEVICE_OST -void qapi_event_send_acpi_device_ost(ACPIOSTInfo *info, Error **errp) +static void do_qapi_event_send_acpi_device_ost(q_obj_ACPI_DEVICE_OST_arg *arg, Error **errp) { QDict *qmp; Error *err = NULL; QMPEventFuncEmit emit; QObject *obj; Visitor *v; - q_obj_ACPI_DEVICE_OST_arg param = { - info - }; emit = qmp_event_get_func_emit(); if (!emit) { return; } qmp = qmp_event_build_dict("ACPI_DEVICE_OST"); v = qmp_output_visitor_new(&obj); visit_start_struct(v, "ACPI_DEVICE_OST", NULL, 0, &err); if (err) { goto out; } - visit_type_q_obj_ACPI_DEVICE_OST_arg_members(v, ¶m, &err); + visit_type_q_obj_ACPI_DEVICE_OST_arg_members(v, arg, &err); if (!err) { visit_check_struct(v, &err); } visit_end_struct(v, NULL); if (err) { goto out; } visit_complete(v, &obj); qdict_put_obj(qmp, "data", obj); emit(QAPI_EVENT_ACPI_DEVICE_OST, qmp, &err); out: visit_free(v); error_propagate(errp, err); QDECREF(qmp); } -void qapi_event_send_balloon_change(int64_t actual, Error **errp) +void qapi_event_send_acpi_device_ost(ACPIOSTInfo *info, Error **errp) +{ + q_obj_ACPI_DEVICE_OST_arg param = { + info + }; + do_qapi_event_send_acpi_device_ost(¶m, errp); +} + This is the case the commit message advertises. There is no visit_type_FOO() we could compare too, since FOO is an implicit type >> @@ -136,6 +126,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
On 06/16/2016 06:25 AM, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: > >> Eric Blake <eblake@redhat.com> writes: >> >>> When an event has data that is not boxed, we are exposing all of >>> its members alongside our local variables. 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 a shell that creates a >>> simple wrapper, then calls a worker that operates on only the >>> boxed version and thus has no user-supplied names to worry about >>> in naming its local variables. For boxed events, we don't need >>> the wrapper. >>> >>> 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. >>> >>> +++ b/scripts/qapi.py >>> @@ -1016,7 +1016,6 @@ class QAPISchemaObjectType(QAPISchemaType): >>> return QAPISchemaType.c_name(self) >>> >>> def c_type(self): >>> - assert not self.is_implicit() >> >> Huh? Required, because we now pass a pointer to an implicit type from qapi_event_send_FOO() to do_qapi_event_send_FOO(), so the c_type() of that implicit type is required for generating the C type for that parameter. Will document it better in the commit message. >>> @@ -93,20 +92,11 @@ def gen_event_send(name, arg_type, box): >>> ret += mcgen(''' >>> v = qmp_output_visitor_new(&obj); >>> >>> -''') >>> - >>> - if box: >>> - ret += mcgen(''' >>> - visit_type_%(c_name)s(v, NULL, &arg, &err); >>> -''', >>> - c_name=arg_type.c_name(), name=arg_type.name) >>> - else: >>> - ret += mcgen(''' >>> visit_start_struct(v, "%(name)s", NULL, 0, &err); >>> 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); >>> } >> >> Getting confused... why are we getting rid of the box case here? No good reason. The visit_type_FOO() is more compact than visit_type_FOO_members(), but only when we have a non-implicit type. So for v8, I'm switching the conditional from 'if box:' to 'if arg_type.is_implicit():', more or less. >> >> Too many conditionals... gen_event_send() has three cases: empty >> arg_type, non-empty arg_type and box, non-empty arg_type and not box. >> The commit message shows the change to generated code for the second >> case. It doesn't show visit_type_%(c_name)s(v, NULL, &arg, &err) going >> away. > > Case empty arg_type: no change > Example: POWERDOWN Good. > > Case non-empty arg_type and box: visit gets open-coded > Example: EVENT_E Fixed in v8 so that it no longer changes. > The open-coded visit drops the !*obj check (okay, @arg isn't going > anywhere), skips the visit_check_struct() differently, and drops the > qapi_free_FOO() (okay, condition is always false here). > > So this isn't wrong. But why open-code? No need to add new open-coding, but we already had existing open-coding for anonymous non-boxed 'data' (in part because commit 7ce106a9 intentionally chose not to create visit_type_FOO() for implicit types). > > Case non-empty arg_type and not box: > Example: ACPI_DEVICE_OST > > > This is the case the commit message advertises. > > There is no visit_type_FOO() we could compare too, since FOO is an > implicit type And in reviewing your message, I realize we have NO testsuite coverage of: { 'event': 'EVENT', 'data': 'NamedStruct' } Guess I get to add that first. Such a usage will then be improved by using visit_type_NamedStruct() rather than open-coding around visit_type_NamedStruct_members().
Eric Blake <eblake@redhat.com> writes: > On 06/16/2016 06:25 AM, Markus Armbruster wrote: >> Markus Armbruster <armbru@redhat.com> writes: >> >>> Eric Blake <eblake@redhat.com> writes: >>> >>>> When an event has data that is not boxed, we are exposing all of >>>> its members alongside our local variables. 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 a shell that creates a >>>> simple wrapper, then calls a worker that operates on only the >>>> boxed version and thus has no user-supplied names to worry about >>>> in naming its local variables. For boxed events, we don't need >>>> the wrapper. >>>> >>>> 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. >>>> > >>>> +++ b/scripts/qapi.py >>>> @@ -1016,7 +1016,6 @@ class QAPISchemaObjectType(QAPISchemaType): >>>> return QAPISchemaType.c_name(self) >>>> >>>> def c_type(self): >>>> - assert not self.is_implicit() >>> >>> Huh? > > Required, because we now pass a pointer to an implicit type from > qapi_event_send_FOO() to do_qapi_event_send_FOO(), so the c_type() of > that implicit type is required for generating the C type for that > parameter. Will document it better in the commit message. Hmm. The real assertion here is "we generate a C type for this QAPI object type." Can we express that in code? >>>> @@ -93,20 +92,11 @@ def gen_event_send(name, arg_type, box): >>>> ret += mcgen(''' >>>> v = qmp_output_visitor_new(&obj); >>>> >>>> -''') >>>> - >>>> - if box: >>>> - ret += mcgen(''' >>>> - visit_type_%(c_name)s(v, NULL, &arg, &err); >>>> -''', >>>> - c_name=arg_type.c_name(), name=arg_type.name) >>>> - else: >>>> - ret += mcgen(''' >>>> visit_start_struct(v, "%(name)s", NULL, 0, &err); >>>> 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); >>>> } >>> >>> Getting confused... why are we getting rid of the box case here? > > No good reason. The visit_type_FOO() is more compact than > visit_type_FOO_members(), but only when we have a non-implicit type. So > for v8, I'm switching the conditional from 'if box:' to 'if > arg_type.is_implicit():', more or less. > >>> >>> Too many conditionals... gen_event_send() has three cases: empty >>> arg_type, non-empty arg_type and box, non-empty arg_type and not box. >>> The commit message shows the change to generated code for the second >>> case. It doesn't show visit_type_%(c_name)s(v, NULL, &arg, &err) going >>> away. >> >> Case empty arg_type: no change >> Example: POWERDOWN > > Good. > >> >> Case non-empty arg_type and box: visit gets open-coded >> Example: EVENT_E > > Fixed in v8 so that it no longer changes. > >> The open-coded visit drops the !*obj check (okay, @arg isn't going >> anywhere), skips the visit_check_struct() differently, and drops the >> qapi_free_FOO() (okay, condition is always false here). >> >> So this isn't wrong. But why open-code? > > No need to add new open-coding, but we already had existing open-coding > for anonymous non-boxed 'data' (in part because commit 7ce106a9 > intentionally chose not to create visit_type_FOO() for implicit types). > >> >> Case non-empty arg_type and not box: >> Example: ACPI_DEVICE_OST >> > >> >> This is the case the commit message advertises. >> >> There is no visit_type_FOO() we could compare too, since FOO is an >> implicit type > > And in reviewing your message, I realize we have NO testsuite coverage of: > > { 'event': 'EVENT', 'data': 'NamedStruct' } > > Guess I get to add that first. Such a usage will then be improved by > using visit_type_NamedStruct() rather than open-coding around > visit_type_NamedStruct_members(). Makes sense.
diff --git a/scripts/qapi.py b/scripts/qapi.py index 6742e7a..7d568d9 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1016,7 +1016,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 b8ca8c8..fe4e50d 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): + intro='void ' + if impl and arg_type and not box: + box = True + intro='static void do_' + return '%(intro)sqapi_event_send_%(c_name)s(%(param)s)' % { + 'intro': intro, 'c_name': c_name(name.lower()), 'param': gen_params(arg_type, box, 'Error **errp')} @@ -53,12 +58,8 @@ def gen_param_var(typ): 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(): + box = False ret = mcgen(''' %(proto)s @@ -67,15 +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) ret += mcgen(''' @@ -93,20 +92,11 @@ def gen_event_send(name, arg_type, box): ret += mcgen(''' v = qmp_output_visitor_new(&obj); -''') - - if box: - ret += mcgen(''' - visit_type_%(c_name)s(v, NULL, &arg, &err); -''', - c_name=arg_type.c_name(), name=arg_type.name) - else: - ret += mcgen(''' visit_start_struct(v, "%(name)s", NULL, 0, &err); 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); } @@ -136,6 +126,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 event has data that is not boxed, we are exposing all of its members alongside our local variables. 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 a shell that creates a simple wrapper, then calls a worker that operates on only the boxed version and thus has no user-supplied names to worry about in naming its local variables. For boxed events, we don't need the wrapper. 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. The generated file changes 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> --- v7: new patch --- scripts/qapi.py | 1 - scripts/qapi-event.py | 47 ++++++++++++++++++++++++++--------------------- 2 files changed, 26 insertions(+), 22 deletions(-)