Re: [Qemu-devel] [PATCH v9 23/37] qmp: Support explicit null during input visit

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Implement the new type_null() callback for the qmp input visitor.
> While we don't yet have a use for this in qapi (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).
>
> At any rate, we can test this without the use of generated qapi
> by manually using visit_start_struct()/visit_end_struct().

Well, we test by calling visit_type_null() manually.  We choose to wrap
it in a visit_start_struct() ... visit_end_struct() pair, but that's
detail.  Actually, we do an unwrapped root visit first, and then a
struct-wrapped visit.

Suggest "by calling visit_type_null() manually."

> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v9: new patch
> ---
>  include/qapi/visitor-impl.h    |  2 +-
>  qapi/qmp-input-visitor.c       | 14 ++++++++++++++
>  tests/test-qmp-input-visitor.c | 26 ++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 8705136..95408a5 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -76,7 +76,7 @@ struct Visitor
>      void (*type_any)(Visitor *v, const char *name, QObject **obj,
>                       Error **errp);
>      /* Must be provided to visit explicit null values (right now, only the
> -     * dealloc visitor supports this).  */
> +     * dealloc and qmp-input visitors support this).  */

Will need updating.

>      void (*type_null)(Visitor *v, const char *name, Error **errp);
>
>      /* May be NULL; most useful for input visitors. */
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 597652c..ad23bec 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -315,6 +315,19 @@ 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) {
> +        return;
> +    }
> +
> +    error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> +               "null");

Recommend to put the error in the conditional:

    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);
> @@ -358,6 +371,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;
>      v->visitor.get_next_type = qmp_input_get_next_type;
>
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index f6bd408..6489e4a 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -278,6 +278,30 @@ static void test_visitor_in_any(TestInputVisitorData 
> *data,
>      qobject_decref(res);
>  }
>
> +static void test_visitor_in_null(TestInputVisitorData *data,
> +                                 const void *unused)
> +{
> +    Visitor *v;
> +    QObject *null;
> +
> +    v = visitor_input_test_init(data, "null");
> +    visit_type_null(v, NULL, &error_abort);
> +
> +    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);
> +
> +    /* Check that qnull reference counting is sane:
> +     * 1 for global use, 1 for our qnull() use, and 1 still owned by 'v'
> +     * until it is torn down */
> +    null = qnull();
> +    g_assert(null->refcnt == 3);
> +    visitor_input_teardown(data, NULL);
> +    g_assert(null->refcnt == 2);
> +    qobject_decref(null);

For other kinds of QObject, we leave the testing of reference counting
to the check-qKIND.c, and don't bother with it when testing the
visitors.  Any particular reason to do null differently?

> +}
> +
>  static void test_visitor_in_union_flat(TestInputVisitorData *data,
>                                         const void *unused)
>  {
> @@ -792,6 +816,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",