Re: [Qemu-devel] [PATCH v14 10/19] qapi: Add visit_type_null() visitor

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Right now, qmp-output-visitor happens to produce a QNull result
> if nothing is actually visited between the creation of the visitor
> and the request for the resulting QObject.  A stronger protocol
> would require that a QMP output visit MUST visit something.  But
> to still be able to produce a JSON 'null' output, we need a new
> visitor function that states our intentions.  Yes, we could say
> that such a visit must go through visit_type_any(), but that
> feels clunky.
>
> So this patch introduces the new visit_type_null() interface and
> its no-op interface in the dealloc visitor, and the next patch
> will then wire it up into the qmp visitors.  For the visitors
> that will not implement the callback, document the situation.
> The code in qapi-visit-core unconditionally dereferences the
> callback pointer, so that a segfault will inform a developer if
> they need to implement the callback for their choice of visitor.
>
> If QAPI had a 'null' type, we'd also have to use visit_type_null()

JSON has a primitive null type with the single value null.

QAPI has the null value.  It doesn't have a (builtin) null type (yet).

> in the generated visitor functions (most likely, we'd use it via
> an alternate type that permits 'null' or an object); we'll create
> that usage when we need it.

Agreed.

> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v14: no change
> v13: no change
> v12: rebase to earlier changes, drop R-b due to better documentation
> [no v10, v11]
> v9: no change
> v8: rebase to 'name' motion
> v7: new patch, based on discussion about spapr_drc.c
> ---
>  include/qapi/visitor.h               | 12 ++++++++++++
>  include/qapi/visitor-impl.h          |  3 +++
>  include/qapi/opts-visitor.h          |  2 +-
>  include/qapi/string-input-visitor.h  |  2 +-
>  include/qapi/string-output-visitor.h |  2 +-
>  qapi/qapi-visit-core.c               |  5 +++++
>  qapi/qapi-dealloc-visitor.c          |  5 +++++
>  7 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index faf67d2..735b389 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -507,4 +507,16 @@ void visit_type_number(Visitor *v, const char *name, 
> double *obj,
>   */
>  void visit_type_any(Visitor *v, const char *name, QObject **obj, Error 
> **errp);
>
> +/*
> + * Visit a JSON null value.
> + *
> + * @name expresses the relationship of the null value to its parent
> + * container; see the general description of @name above.
> + *
> + * Unlike all other visit_type_* functions, no obj parameter is
> + * needed; rather, this is a witness that an explicit null value is
> + * expected rather than any other type.
> + */
> +void visit_type_null(Visitor *v, const char *name, Error **errp);
> +
>  #endif
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index d967b18..90bcaec 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -86,6 +86,9 @@ struct Visitor
>      void (*type_any)(Visitor *v, const char *name, QObject **obj,
>                       Error **errp);
>
> +    /* Must be set to visit explicit null values.  */
> +    void (*type_null)(Visitor *v, const char *name, Error **errp);
> +
>      /* Must be set for input visitors, optional otherwise.  The core
>       * takes care of the return type in the public interface.  */
>      void (*optional)(Visitor *v, const char *name, bool *present);
> diff --git a/include/qapi/opts-visitor.h b/include/qapi/opts-visitor.h
> index 2002e37..3fcd327 100644
> --- a/include/qapi/opts-visitor.h
> +++ b/include/qapi/opts-visitor.h
> @@ -31,7 +31,7 @@ typedef struct OptsVisitor OptsVisitor;
>   * - values above INT64_MAX or LLONG_MAX are rejected.
>   *
>   * The Opts input visitor does not yet implement support for visiting
> - * QAPI alternates, numbers (other than integers), or arbitrary
> + * QAPI alternates, numbers (other than integers), null, or arbitrary
>   * QTypes.
>   */
>  OptsVisitor *opts_visitor_new(const QemuOpts *opts);
> diff --git a/include/qapi/string-input-visitor.h 
> b/include/qapi/string-input-visitor.h
> index 4c8d1ea..1a34c52 100644
> --- a/include/qapi/string-input-visitor.h
> +++ b/include/qapi/string-input-visitor.h
> @@ -19,7 +19,7 @@ typedef struct StringInputVisitor StringInputVisitor;
>
>  /*
>   * The string input visitor does not yet implement support for
> - * visiting QAPI structs, alternates, or arbitrary QTypes.
> + * visiting QAPI structs, alternates, null, or arbitrary QTypes.
>   */
>  StringInputVisitor *string_input_visitor_new(const char *str);
>  void string_input_visitor_cleanup(StringInputVisitor *v);
> diff --git a/include/qapi/string-output-visitor.h 
> b/include/qapi/string-output-visitor.h
> index 094a11e..2564833 100644
> --- a/include/qapi/string-output-visitor.h
> +++ b/include/qapi/string-output-visitor.h
> @@ -19,7 +19,7 @@ typedef struct StringOutputVisitor StringOutputVisitor;
>
>  /*
>   * The string output visitor does not yet implement support for
> - * visiting QAPI structs, alternates, or arbitrary QTypes.
> + * visiting QAPI structs, alternates, null, or arbitrary QTypes.
>   */
>  StringOutputVisitor *string_output_visitor_new(bool human);
>  void string_output_visitor_cleanup(StringOutputVisitor *v);
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 71f3b5d..064b9f1 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -219,6 +219,11 @@ void visit_type_any(Visitor *v, const char *name, 
> QObject **obj, Error **errp)
>      v->type_any(v, name, obj, errp);
>  }
>
> +void visit_type_null(Visitor *v, const char *name, Error **errp)
> +{
> +    v->type_null(v, name, errp);
> +}
> +
>  static void output_type_enum(Visitor *v, const char *name, int *obj,
>                               const char *const strings[], Error **errp)
>  {
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index c19a459..413d525 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -163,6 +163,10 @@ static void qapi_dealloc_type_anything(Visitor *v, const 
> char *name,
>      }
>  }
>
> +static void qapi_dealloc_type_null(Visitor *v, const char *name, Error 
> **errp)
> +{
> +}
> +
>  Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v)
>  {
>      return &v->visitor;
> @@ -193,6 +197,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
>      v->visitor.type_str = qapi_dealloc_type_str;
>      v->visitor.type_number = qapi_dealloc_type_number;
>      v->visitor.type_any = qapi_dealloc_type_anything;
> +    v->visitor.type_null = qapi_dealloc_type_null;
>
>      QTAILQ_INIT(&v->stack);

Let's stick stub implementations marked FIXME into the QMP visitors in
this patch, and fill them out in the next one.