| Message ID | 20200317115459.31821-30-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Configurable policy for handling deprecated interfaces | expand |
On 3/17/20 6:54 AM, Markus Armbruster wrote: > This policy suppresses deprecated bits in output, and thus permits > "testing the future". Implement it for QMP events: suppress > deprecated ones. > > No QMP event is deprecated right now. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > @@ -140,6 +141,23 @@ static void test_event_d(TestEventData *data, > qobject_unref(data->expect); > } > > +static void test_event_deprecated(TestEventData *data, const void *unused) > +{ > + data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES1' }"); > + > + memset(&compat_policy, 0, sizeof(compat_policy)); > + > + qapi_event_send_test_event_features1(); > + g_assert(data->emitted); > + > + compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE; Umm, did you forget to set compat_policy.has_deprecated_output = true? (proof that we really want QAPI defaults to be working, to get rid of pointless has_* members...) > + data->emitted = false; > + qapi_event_send_test_event_features1(); > + g_assert(!data->emitted); > + > + qobject_unref(data->expect); > +} > + > +++ b/scripts/qapi/events.py > @@ -61,7 +61,8 @@ def gen_param_var(typ): > return ret > > > -def gen_event_send(name, arg_type, boxed, event_enum_name, event_emit): > +def gen_event_send(name, arg_type, features, boxed, > + event_enum_name, event_emit): > # 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 > @@ -86,6 +87,14 @@ def gen_event_send(name, arg_type, boxed, event_enum_name, event_emit): > if not boxed: > ret += gen_param_var(arg_type) > > + if 'deprecated' in [f.name for f in features]: > + ret += mcgen(''' > + > + if (compat_policy.deprecated_output == COMPAT_POLICY_OUTPUT_HIDE) { > + return; Here, you took the shortcut that if we always 0-initialize, deprecated_output will never be true except when has_deprecated_output is also true. But the test above violated that rule of thumb. Otherwise, this patch makes sense.
Eric Blake <eblake@redhat.com> writes: > On 3/17/20 6:54 AM, Markus Armbruster wrote: >> This policy suppresses deprecated bits in output, and thus permits >> "testing the future". Implement it for QMP events: suppress >> deprecated ones. >> >> No QMP event is deprecated right now. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- > >> @@ -140,6 +141,23 @@ static void test_event_d(TestEventData *data, >> qobject_unref(data->expect); >> } >> +static void test_event_deprecated(TestEventData *data, const void >> *unused) >> +{ >> + data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES1' }"); >> + >> + memset(&compat_policy, 0, sizeof(compat_policy)); >> + >> + qapi_event_send_test_event_features1(); >> + g_assert(data->emitted); >> + >> + compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE; > > Umm, did you forget to set compat_policy.has_deprecated_output = true? I did. Works anyway, because I don't bother checking it. I'll clean it up. > (proof that we really want QAPI defaults to be working, to get rid of > pointless has_* members...) > >> + data->emitted = false; >> + qapi_event_send_test_event_features1(); >> + g_assert(!data->emitted); >> + >> + qobject_unref(data->expect); >> +} >> + > >> +++ b/scripts/qapi/events.py >> @@ -61,7 +61,8 @@ def gen_param_var(typ): >> return ret >> -def gen_event_send(name, arg_type, boxed, event_enum_name, >> event_emit): >> +def gen_event_send(name, arg_type, features, boxed, >> + event_enum_name, event_emit): >> # 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 >> @@ -86,6 +87,14 @@ def gen_event_send(name, arg_type, boxed, event_enum_name, event_emit): >> if not boxed: >> ret += gen_param_var(arg_type) >> + if 'deprecated' in [f.name for f in features]: >> + ret += mcgen(''' >> + >> + if (compat_policy.deprecated_output == COMPAT_POLICY_OUTPUT_HIDE) { >> + return; > > Here, you took the shortcut that if we always 0-initialize, > deprecated_output will never be true except when has_deprecated_output > is also true. But the test above violated that rule of thumb. Generated code ensures FOO is zero when !has_FOO. Manual code should do the same. The pedantically correct check has_FOO && FOO == some_non_zero_value can then safely be abbreviated to just FOO == some_non_zero_value which I find less turing and more more legible. > Otherwise, this patch makes sense. Thanks!
diff --git a/tests/test-qmp-event.c b/tests/test-qmp-event.c index 7dd0053190..ae4913ceb3 100644 --- a/tests/test-qmp-event.c +++ b/tests/test-qmp-event.c @@ -14,6 +14,7 @@ #include "qemu/osdep.h" #include "qemu-common.h" +#include "qapi/compat-policy.h" #include "qapi/error.h" #include "qapi/qmp/qbool.h" #include "qapi/qmp/qdict.h" @@ -140,6 +141,23 @@ static void test_event_d(TestEventData *data, qobject_unref(data->expect); } +static void test_event_deprecated(TestEventData *data, const void *unused) +{ + data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES1' }"); + + memset(&compat_policy, 0, sizeof(compat_policy)); + + qapi_event_send_test_event_features1(); + g_assert(data->emitted); + + compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE; + data->emitted = false; + qapi_event_send_test_event_features1(); + g_assert(!data->emitted); + + qobject_unref(data->expect); +} + int main(int argc, char **argv) { g_test_init(&argc, &argv, NULL); @@ -148,6 +166,7 @@ int main(int argc, char **argv) event_test_add("/event/event_b", test_event_b); event_test_add("/event/event_c", test_event_c); event_test_add("/event/event_d", test_event_d); + event_test_add("/event/deprecated", test_event_deprecated); g_test_run(); return 0; diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py index b544af5a1c..95ca4b4753 100644 --- a/scripts/qapi/events.py +++ b/scripts/qapi/events.py @@ -61,7 +61,8 @@ def gen_param_var(typ): return ret -def gen_event_send(name, arg_type, boxed, event_enum_name, event_emit): +def gen_event_send(name, arg_type, features, boxed, + event_enum_name, event_emit): # 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 @@ -86,6 +87,14 @@ def gen_event_send(name, arg_type, boxed, event_enum_name, event_emit): if not boxed: ret += gen_param_var(arg_type) + if 'deprecated' in [f.name for f in features]: + ret += mcgen(''' + + if (compat_policy.deprecated_output == COMPAT_POLICY_OUTPUT_HIDE) { + return; + } +''') + ret += mcgen(''' qmp = qmp_event_build_dict("%(name)s"); @@ -154,6 +163,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor): #include "%(prefix)sqapi-emit-events.h" #include "%(events)s.h" #include "%(visit)s.h" +#include "qapi/compat-policy.h" #include "qapi/error.h" #include "qapi/qmp/qdict.h" #include "qapi/qobject-output-visitor.h" @@ -192,7 +202,7 @@ void %(event_emit)s(%(event_enum)s event, QDict *qdict); def visit_event(self, name, info, ifcond, features, arg_type, boxed): with ifcontext(ifcond, self._genh, self._genc): self._genh.add(gen_event_send_decl(name, arg_type, boxed)) - self._genc.add(gen_event_send(name, arg_type, boxed, + self._genc.add(gen_event_send(name, arg_type, features, boxed, self._event_enum_name, self._event_emit_name)) # Note: we generate the enum member regardless of @ifcond, to
This policy suppresses deprecated bits in output, and thus permits "testing the future". Implement it for QMP events: suppress deprecated ones. No QMP event is deprecated right now. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- tests/test-qmp-event.c | 19 +++++++++++++++++++ scripts/qapi/events.py | 14 ++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-)