[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 10/15] qapi-event: Reduce chance of collision
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v7 10/15] qapi-event: Reduce chance of collision with event data |
Date: |
Thu, 16 Jun 2016 14:25:12 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Markus Armbruster <address@hidden> writes:
> Eric Blake <address@hidden> 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 <address@hidden>
>>
>> ---
>> 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