Marc-André Lureau <marcandre.lur...@gmail.com> writes: > Hi > > On Sun, Mar 15, 2020 at 4:11 PM Markus Armbruster <arm...@redhat.com> wrote: >> >> This policy suppresses deprecated bits in output, and thus permits >> "testing the future". Implement it for QMP command results. Example: >> when QEMU is run with -compat deprecated-output=hide, then >> >> {"execute": "query-cpus-fast"} >> >> yields >> >> {"return": [{"thread-id": 9805, "props": {"core-id": 0, "thread-id": 0, >> "socket-id": 0}, "qom-path": "/machine/unattached/device[0]", "cpu-index": >> 0, "target": "x86_64"}]} >> >> instead of >> >> {"return": [{"arch": "x86", "thread-id": 22436, "props": {"core-id": 0, >> "thread-id": 0, "socket-id": 0}, "qom-path": >> "/machine/unattached/device[0]", "cpu-index": 0, "target": "x86_64"}]} >> >> Note the suppression of deprecated member "arch". >> >> Signed-off-by: Markus Armbruster <arm...@redhat.com> >> --- >> include/qapi/qobject-output-visitor.h | 9 ++++++ >> include/qapi/visitor-impl.h | 3 ++ >> include/qapi/visitor.h | 9 ++++++ >> qapi/qapi-visit-core.c | 9 ++++++ >> qapi/qobject-output-visitor.c | 19 +++++++++++ >> tests/test-qmp-cmds.c | 42 ++++++++++++++++++++++--- >> qapi/trace-events | 1 + >> scripts/qapi/commands.py | 2 +- >> scripts/qapi/visit.py | 12 +++++++ >> tests/qapi-schema/qapi-schema-test.json | 17 +++++----- >> tests/qapi-schema/qapi-schema-test.out | 18 +++++------ >> 11 files changed, 118 insertions(+), 23 deletions(-) >> >> diff --git a/include/qapi/qobject-output-visitor.h >> b/include/qapi/qobject-output-visitor.h >> index 2b1726baf5..29f4ea6aad 100644 >> --- a/include/qapi/qobject-output-visitor.h >> +++ b/include/qapi/qobject-output-visitor.h >> @@ -53,4 +53,13 @@ typedef struct QObjectOutputVisitor QObjectOutputVisitor; >> */ >> Visitor *qobject_output_visitor_new(QObject **result); >> >> +/* >> + * Create a QObject output visitor for @obj for use with QMP >> + * >> + * This is like qobject_output_visitor_new(), except it obeys the >> + * policy for handling deprecated management interfaces set with >> + * -compat. >> + */ >> +Visitor *qobject_output_visitor_new_qmp(QObject **result); >> + >> #endif >> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h >> index 8ccb3b6c20..a6b26b7a5b 100644 >> --- a/include/qapi/visitor-impl.h >> +++ b/include/qapi/visitor-impl.h >> @@ -110,6 +110,9 @@ struct Visitor >> The core takes care of the return type in the public interface. */ >> void (*optional)(Visitor *v, const char *name, bool *present); >> >> + /* Optional */ >> + bool (*deprecated)(Visitor *v, const char *name); >> + >> /* Must be set */ >> VisitorType type; >> >> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h >> index c5b23851a1..c89d51b2a4 100644 >> --- a/include/qapi/visitor.h >> +++ b/include/qapi/visitor.h >> @@ -449,6 +449,15 @@ void visit_end_alternate(Visitor *v, void **obj); >> */ >> bool visit_optional(Visitor *v, const char *name, bool *present); >> >> +/* >> + * Should we visit deprecated member @name? >> + * >> + * @name must not be NULL. This function is only useful between >> + * visit_start_struct() and visit_end_struct(), since only objects >> + * have deprecated members. >> + */ >> +bool visit_deprecated(Visitor *v, const char *name); >> + >> /* >> * Visit an enum value. >> * >> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c >> index 5365561b07..501b3ccdef 100644 >> --- a/qapi/qapi-visit-core.c >> +++ b/qapi/qapi-visit-core.c >> @@ -137,6 +137,15 @@ bool visit_optional(Visitor *v, const char *name, bool >> *present) >> return *present; >> } >> >> +bool visit_deprecated(Visitor *v, const char *name) >> +{ >> + trace_visit_deprecated(v, name); >> + if (v->deprecated) { >> + return v->deprecated(v, name); >> + } >> + return true; >> +} >> + >> bool visit_is_input(Visitor *v) >> { >> return v->type == VISITOR_INPUT; >> diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c >> index 26d7be5ec9..84cee17596 100644 >> --- a/qapi/qobject-output-visitor.c >> +++ b/qapi/qobject-output-visitor.c >> @@ -13,6 +13,7 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qapi/compat-policy.h" >> #include "qapi/qobject-output-visitor.h" >> #include "qapi/visitor-impl.h" >> #include "qemu/queue.h" >> @@ -31,6 +32,8 @@ typedef struct QStackEntry { >> >> struct QObjectOutputVisitor { >> Visitor visitor; >> + CompatPolicyOutput deprecated_policy; >> + >> QSLIST_HEAD(, QStackEntry) stack; /* Stack of unfinished containers */ >> QObject *root; /* Root of the output visit */ >> QObject **result; /* User's storage location for result */ >> @@ -198,6 +201,13 @@ static void qobject_output_type_null(Visitor *v, const >> char *name, >> qobject_output_add(qov, name, qnull()); >> } >> >> +static bool qobject_output_deprecated(Visitor *v, const char *name) >> +{ >> + QObjectOutputVisitor *qov = to_qov(v); >> + >> + return qov->deprecated_policy != COMPAT_POLICY_OUTPUT_HIDE; >> +} >> + >> /* Finish building, and return the root object. >> * The root object is never null. The caller becomes the object's >> * owner, and should use qobject_unref() when done with it. */ >> @@ -247,6 +257,7 @@ Visitor *qobject_output_visitor_new(QObject **result) >> v->visitor.type_number = qobject_output_type_number; >> v->visitor.type_any = qobject_output_type_any; >> v->visitor.type_null = qobject_output_type_null; >> + v->visitor.deprecated = qobject_output_deprecated; >> v->visitor.complete = qobject_output_complete; >> v->visitor.free = qobject_output_free; >> >> @@ -255,3 +266,11 @@ Visitor *qobject_output_visitor_new(QObject **result) >> >> return &v->visitor; >> } >> + >> +Visitor *qobject_output_visitor_new_qmp(QObject **result) >> +{ >> + QObjectOutputVisitor *v = to_qov(qobject_output_visitor_new(result)); >> + >> + v->deprecated_policy = compat_policy.deprecated_output; >> + return &v->visitor; >> +} >> diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c >> index d12ff47e26..82d599630c 100644 >> --- a/tests/test-qmp-cmds.c >> +++ b/tests/test-qmp-cmds.c >> @@ -1,4 +1,5 @@ >> #include "qemu/osdep.h" >> +#include "qapi/compat-policy.h" >> #include "qapi/qmp/qdict.h" >> #include "qapi/qmp/qjson.h" >> #include "qapi/qmp/qnum.h" >> @@ -45,12 +46,17 @@ void qmp_user_def_cmd1(UserDefOne * ud1, Error **errp) >> { >> } >> >> -void qmp_test_features0(FeatureStruct0 *fs0, FeatureStruct1 *fs1, >> - FeatureStruct2 *fs2, FeatureStruct3 *fs3, >> - FeatureStruct4 *fs4, CondFeatureStruct1 *cfs1, >> - CondFeatureStruct2 *cfs2, CondFeatureStruct3 *cfs3, >> - Error **errp) >> +FeatureStruct1 *qmp_test_features0(bool has_fs0, FeatureStruct0 *fs0, >> + bool has_fs1, FeatureStruct1 *fs1, >> + bool has_fs2, FeatureStruct2 *fs2, >> + bool has_fs3, FeatureStruct3 *fs3, >> + bool has_fs4, FeatureStruct4 *fs4, >> + bool has_cfs1, CondFeatureStruct1 *cfs1, >> + bool has_cfs2, CondFeatureStruct2 *cfs2, >> + bool has_cfs3, CondFeatureStruct3 *cfs3, >> + Error **errp) >> { >> + return g_new(FeatureStruct1, 1); >> } >> >> void qmp_test_command_features1(Error **errp) >> @@ -271,6 +277,30 @@ static void test_dispatch_cmd_io(void) >> qobject_unref(ret3); >> } >> >> +static void test_dispatch_cmd_ret_deprecated(void) >> +{ >> + const char *cmd = "{ 'execute': 'test-features0' }"; >> + QDict *ret; >> + >> + memset(&compat_policy, 0, sizeof(compat_policy)); >> + >> + /* default accept */ >> + ret = qobject_to(QDict, do_qmp_dispatch(false, cmd)); >> + assert(ret && qdict_size(ret) == 1); >> + qobject_unref(ret); >> + >> + compat_policy.has_deprecated_output = true; >> + compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_ACCEPT; >> + ret = qobject_to(QDict, do_qmp_dispatch(false, cmd)); >> + assert(ret && qdict_size(ret) == 1); >> + qobject_unref(ret); >> + >> + compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE; >> + ret = qobject_to(QDict, do_qmp_dispatch(false, cmd)); >> + assert(ret && qdict_size(ret) == 0); >> + qobject_unref(ret); >> +} >> + >> /* test generated dealloc functions for generated types */ >> static void test_dealloc_types(void) >> { >> @@ -345,6 +375,8 @@ int main(int argc, char **argv) >> g_test_add_func("/qmp/dispatch_cmd_io", test_dispatch_cmd_io); >> g_test_add_func("/qmp/dispatch_cmd_success_response", >> test_dispatch_cmd_success_response); >> + g_test_add_func("/qmp/dispatch_cmd_ret_deprecated", >> + test_dispatch_cmd_ret_deprecated); >> g_test_add_func("/qmp/dealloc_types", test_dealloc_types); >> g_test_add_func("/qmp/dealloc_partial", test_dealloc_partial); >> >> diff --git a/qapi/trace-events b/qapi/trace-events >> index 5eb4afa110..eff1fbd199 100644 >> --- a/qapi/trace-events >> +++ b/qapi/trace-events >> @@ -17,6 +17,7 @@ visit_start_alternate(void *v, const char *name, void >> *obj, size_t size) "v=%p n >> visit_end_alternate(void *v, void *obj) "v=%p obj=%p" >> >> visit_optional(void *v, const char *name, bool *present) "v=%p name=%s >> present=%p" >> +visit_deprecated(void *v, const char *name) "v=%p name=%s" >> >> visit_type_enum(void *v, const char *name, int *obj) "v=%p name=%s obj=%p" >> visit_type_int(void *v, const char *name, int64_t *obj) "v=%p name=%s >> obj=%p" >> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py >> index bc30876c88..35b79c554d 100644 >> --- a/scripts/qapi/commands.py >> +++ b/scripts/qapi/commands.py >> @@ -69,7 +69,7 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s >> ret_in, QObject **ret_out, >> Error *err = NULL; >> Visitor *v; >> >> - v = qobject_output_visitor_new(ret_out); >> + v = qobject_output_visitor_new_qmp(ret_out); >> visit_type_%(c_name)s(v, "unused", &ret_in, &err); >> if (!err) { >> visit_complete(v, ret_out); >> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py >> index 23d9194aa4..21df3abed2 100644 >> --- a/scripts/qapi/visit.py >> +++ b/scripts/qapi/visit.py >> @@ -56,6 +56,7 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s >> *obj, Error **errp) >> c_type=base.c_name()) >> >> for memb in members: >> + deprecated = 'deprecated' in [f.name for f in memb.features] >> ret += gen_if(memb.ifcond) >> if memb.optional: >> ret += mcgen(''' >> @@ -63,6 +64,12 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s >> *obj, Error **errp) >> ''', >> name=memb.name, c_name=c_name(memb.name)) >> push_indent() >> + if deprecated: >> + ret += mcgen(''' >> + if (visit_deprecated(v, "%(name)s")) { > > Do you have a compelling case where the "name" would change the > deprecation policy? I think that could be more confusing than > necessary, say if x- name wouldn't follow the policy.
Yes, that would be a bad idea. Intended use is error messages, just like we do elsewhere. The output visitor's method qobject_output_deprecated() can't fail, but the input visitor's method can; see PATCH 33. [...]