diff mbox series

[v4,28/34] qapi: Implement deprecated-output=hide for QMP command results

Message ID 20200317115459.31821-29-armbru@redhat.com (mailing list archive)
State New, archived
Headers show
Series Configurable policy for handling deprecated interfaces | expand

Commit Message

Markus Armbruster March 17, 2020, 11:54 a.m. UTC
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 <armbru@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(-)

Comments

Eric Blake March 18, 2020, 10:40 a.m. UTC | #1
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 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 <armbru@redhat.com>
> ---

> +++ 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);

Should this be using g_new0, rather than random contents?

>   }
>   
>   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;

Of course, if we ever enable defaults in QAPI, we can get rid of 
has_deprecated_output by recording proper defaults for bools.  But 
that's a different project ;)

> +    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);
> +}
> +

Otherwise,
Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster March 18, 2020, 4:47 p.m. UTC | #2
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 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 <armbru@redhat.com>
>> ---
>
>> +++ 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);
>
> Should this be using g_new0, rather than random contents?

Accident.  It's not actually used.

>>   }
>>     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;
>
> Of course, if we ever enable defaults in QAPI, we can get rid of
> has_deprecated_output by recording proper defaults for bools.  But
> that's a different project ;)

Yes.  The has_FOO have been annoying me since forever.  I just never get
around to doing anything about it.

>> +    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);
>> +}
>> +
>
> Otherwise,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!
diff mbox series

Patch

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")) {
+''',
+                         name=memb.name)
+            push_indent()
         ret += mcgen('''
     visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, &err);
     if (err) {
@@ -71,6 +78,11 @@  void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
 ''',
                      c_type=memb.type.c_name(), name=memb.name,
                      c_name=c_name(memb.name))
+        if deprecated:
+            pop_indent()
+            ret += mcgen('''
+    }
+''')
         if memb.optional:
             pop_indent()
             ret += mcgen('''
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 6b1f05afa7..e4cce0d5b0 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -297,14 +297,15 @@ 
   'features': [ 'feature1' ] }
 
 { 'command': 'test-features0',
-  'data': { 'fs0': 'FeatureStruct0',
-            'fs1': 'FeatureStruct1',
-            'fs2': 'FeatureStruct2',
-            'fs3': 'FeatureStruct3',
-            'fs4': 'FeatureStruct4',
-            'cfs1': 'CondFeatureStruct1',
-            'cfs2': 'CondFeatureStruct2',
-            'cfs3': 'CondFeatureStruct3' },
+  'data': { '*fs0': 'FeatureStruct0',
+            '*fs1': 'FeatureStruct1',
+            '*fs2': 'FeatureStruct2',
+            '*fs3': 'FeatureStruct3',
+            '*fs4': 'FeatureStruct4',
+            '*cfs1': 'CondFeatureStruct1',
+            '*cfs2': 'CondFeatureStruct2',
+            '*cfs3': 'CondFeatureStruct3' },
+  'returns': 'FeatureStruct1',
   'features': [] }
 
 { 'command': 'test-command-features1',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 891b4101e0..cd53323abd 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -407,15 +407,15 @@  alternate FeatureAlternate1
     case eins: FeatureStruct1
     feature feature1
 object q_obj_test-features0-arg
-    member fs0: FeatureStruct0 optional=False
-    member fs1: FeatureStruct1 optional=False
-    member fs2: FeatureStruct2 optional=False
-    member fs3: FeatureStruct3 optional=False
-    member fs4: FeatureStruct4 optional=False
-    member cfs1: CondFeatureStruct1 optional=False
-    member cfs2: CondFeatureStruct2 optional=False
-    member cfs3: CondFeatureStruct3 optional=False
-command test-features0 q_obj_test-features0-arg -> None
+    member fs0: FeatureStruct0 optional=True
+    member fs1: FeatureStruct1 optional=True
+    member fs2: FeatureStruct2 optional=True
+    member fs3: FeatureStruct3 optional=True
+    member fs4: FeatureStruct4 optional=True
+    member cfs1: CondFeatureStruct1 optional=True
+    member cfs2: CondFeatureStruct2 optional=True
+    member cfs3: CondFeatureStruct3 optional=True
+command test-features0 q_obj_test-features0-arg -> FeatureStruct1
     gen=True success_response=True boxed=False oob=False preconfig=False
 command test-command-features1 None -> None
     gen=True success_response=True boxed=False oob=False preconfig=False