Re: [PATCH v3 28/34] qapi: Implement deprecated-output=hide for QMP command results

[Marc-André Lureau]

Hi

On Sun, Mar 15, 2020 at 4:11 PM Markus Armbruster <address@hidden> 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 <address@hidden>
> ---
>  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.

> +''',
> +                         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
> --
> 2.21.1
>
>


-- 
Marc-André Lureau