Re: [Qemu-devel] [PATCH v14 11/19] qmp: Support explicit null during visits

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> Implement the new type_null() callback for the qmp input and
> output visitors. While we don't yet have a use for this in QAPI
> input (the generator will need some tweaks first), one usage
> is already envisioned: when changing blockdev parameters, it
> would be nice to have a difference between leaving a tuning
> parameter unchanged (omit that parameter from the struct) and
> to explicitly reset the parameter to its default without having
> to know what the default value is (specify the parameter with
> an explicit null value, which will require us to allow a QAPI
> alternate that chooses between the normal value and an explicit
> null).

I'm not sure we'll do it that way.  More cautious would be:

    one possible usage has been discussed: when changing blockdev
    parameters, it would be nice to have a difference between leaving a
    tuning parameter unchanged and to explicitly reset the parameter to
    its default without having to know what the default value is.  One
    way to do that would be omitting the parameter to leave it
    unchanged, and setting it to null to reset to the default.

But I'd simply refrain from going into detail here: shorten the whole
thing to "possible usages have been discussed."

>         Meanwhile, the output visitor could already output
> explicit null via type_any, but this gives us finer control.
>
> At any rate, it's easy to test that we can round-trip an explicit
> null through manual use of visit_type_null().  Repurpose the
> test_visitor_out_empty test, particularly since a future patch
> will tighten semantics to forbid immediate use of
> qmp_output_get_qobject() without at least one visit_type_*.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v14: rebase to header inclusion cleanups
> v13: no change
> v12: retitle and merge in output visitor support, move refcount
> tests to separate file
> [no v10, v11]
> v9: new patch
> ---
>  qapi/qmp-input-visitor.c        | 12 ++++++++++++
>  qapi/qmp-output-visitor.c       |  7 +++++++
>  tests/check-qnull.c             | 11 ++++++++++-
>  tests/test-qmp-input-visitor.c  | 16 ++++++++++++++++
>  tests/test-qmp-output-visitor.c |  9 +++++----
>  5 files changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 1820360..e5b412a 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -339,6 +339,17 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj,
>      *obj = qobj;
>  }
>
> +static void qmp_input_type_null(Visitor *v, const char *name, Error **errp)
> +{
> +    QmpInputVisitor *qiv = to_qiv(v);
> +    QObject *qobj = qmp_input_get_object(qiv, name, true);
> +
> +    if (qobject_type(qobj) != QTYPE_QNULL) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> +                   "null");
> +    }
> +}
> +
>  static void qmp_input_optional(Visitor *v, const char *name, bool *present)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> @@ -382,6 +393,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>      v->visitor.type_str = qmp_input_type_str;
>      v->visitor.type_number = qmp_input_type_number;
>      v->visitor.type_any = qmp_input_type_any;
> +    v->visitor.type_null = qmp_input_type_null;
>      v->visitor.optional = qmp_input_optional;
>
>      qmp_input_push(v, obj, NULL, NULL);
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index 1f2a7ba..5681ad3 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -196,6 +196,12 @@ static void qmp_output_type_any(Visitor *v, const char *name, QObject **obj,
>      qmp_output_add_obj(qov, name, *obj);
>  }
>
> +static void qmp_output_type_null(Visitor *v, const char *name, Error **errp)
> +{
> +    QmpOutputVisitor *qov = to_qov(v);
> +    qmp_output_add_obj(qov, name, qnull());
> +}
> +
>  /* Finish building, and return the root object. Will not be NULL. */
>  QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
>  {
> @@ -246,6 +252,7 @@ QmpOutputVisitor *qmp_output_visitor_new(void)
>      v->visitor.type_str = qmp_output_type_str;
>      v->visitor.type_number = qmp_output_type_number;
>      v->visitor.type_any = qmp_output_type_any;
> +    v->visitor.type_null = qmp_output_type_null;
>
>      QTAILQ_INIT(&v->stack);
>
> diff --git a/tests/check-qnull.c b/tests/check-qnull.c
> index b0fb1e6..43a222d 100644
> --- a/tests/check-qnull.c
> +++ b/tests/check-qnull.c
> @@ -11,7 +11,9 @@
>
>  #include "qapi/qmp/qobject.h"
>  #include "qemu-common.h"
> +#include "qapi/qmp-input-visitor.h"
>  #include "qapi/qmp-output-visitor.h"
> +#include "qapi/error.h"
>
>  /*
>   * Public Interface test-cases
> @@ -37,15 +39,22 @@ static void qnull_visit_test(void)
>  {
>      QObject *obj;
>      QmpOutputVisitor *qov;
> +    QmpInputVisitor *qiv;
>
>      g_assert(qnull_.refcnt == 1);
> +    obj = qnull();
> +    qiv = qmp_input_visitor_new(obj);
> +    qobject_decref(obj);
> +    visit_type_null(qmp_input_get_visitor(qiv), NULL, &error_abort);

I'd destroy qiv here...

> +
>      qov = qmp_output_visitor_new();
> -    /* FIXME: Empty visits are ugly, we should have a visit_type_null(). */
> +    visit_type_null(qmp_output_get_visitor(qov), NULL, &error_abort);
>      obj = qmp_output_get_qobject(qov);
>      g_assert(obj == &qnull_);
>      qobject_decref(obj);
>
>      qmp_output_visitor_cleanup(qov);
> +    qmp_input_visitor_cleanup(qiv);

... instead of here.

>      g_assert(qnull_.refcnt == 1);
>  }
>

See discussion of the split between check-qnull and test-qmp-*-visitor
elsewhere in this thread.

> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index 80527eb..d06383a 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -279,6 +279,20 @@ static void test_visitor_in_any(TestInputVisitorData *data,
>      qobject_decref(res);
>  }
>
> +static void test_visitor_in_null(TestInputVisitorData *data,
> +                                 const void *unused)
> +{
> +    Visitor *v;
> +
> +    v = visitor_input_test_init(data, "null");
> +    visit_type_null(v, NULL, &error_abort);

Effectively a duplicate of qnull_visit_test()'s first half.

Unlike the other test_visitor_in_FOO(), this one ignores the result
instead of checking it.

> +
> +    v = visitor_input_test_init(data, "{ 'a': null }");
> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
> +    visit_type_null(v, "a", &error_abort);
> +    visit_end_struct(v, &error_abort);
> +}
> +
>  static void test_visitor_in_union_flat(TestInputVisitorData *data,
>                                         const void *unused)
>  {
> @@ -829,6 +843,8 @@ int main(int argc, char **argv)
>                             &in_visitor_data, test_visitor_in_list);
>      input_visitor_test_add("/visitor/input/any",
>                             &in_visitor_data, test_visitor_in_any);
> +    input_visitor_test_add("/visitor/input/null",
> +                           &in_visitor_data, test_visitor_in_null);
>      input_visitor_test_add("/visitor/input/union-flat",
>                             &in_visitor_data, test_visitor_in_union_flat);
>      input_visitor_test_add("/visitor/input/alternate",
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index fddb5a6..e656d99 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -477,11 +477,12 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
>      qobject_decref(arg);
>  }
>
> -static void test_visitor_out_empty(TestOutputVisitorData *data,
> -                                   const void *unused)
> +static void test_visitor_out_null(TestOutputVisitorData *data,
> +                                  const void *unused)
>  {
>      QObject *arg;
>
> +    visit_type_null(data->ov, NULL, &error_abort);
>      arg = qmp_output_get_qobject(data->qov);
>      g_assert(qobject_type(arg) == QTYPE_QNULL);
>      qobject_decref(arg);

Effectively a duplicate of qnull_visit_test()'s second half, less the
final reference count check.

> @@ -837,8 +838,8 @@ int main(int argc, char **argv)
>                              &out_visitor_data, test_visitor_out_union_flat);
>      output_visitor_test_add("/visitor/output/alternate",
>                              &out_visitor_data, test_visitor_out_alternate);
> -    output_visitor_test_add("/visitor/output/empty",
> -                            &out_visitor_data, test_visitor_out_empty);
> +    output_visitor_test_add("/visitor/output/null",
> +                            &out_visitor_data, test_visitor_out_null);
>      output_visitor_test_add("/visitor/output/native_list/int",
>                              &out_visitor_data,
>                              test_visitor_out_native_list_int);