Re: [Qemu-devel] [PATCH v8 22/35] qapi: Add visit_type_null() visitor

[Marc-André Lureau]

Hi

On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <address@hidden> wrote:
> 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.
>
> This patch introduces the new visit_type_null() interface, and
> a later patch will then wire it up into the qmp output visitor.
>
> Signed-off-by: Eric Blake <address@hidden>
>

overall looks good to me:
Reviewed-by: Marc-André Lureau <address@hidden>

Just a small remark below,

> ---
> v8: rebase to 'name' motion
> v7: new patch, based on discussion about spapr_drc.c
> ---
>  include/qapi/visitor-impl.h | 3 +++
>  include/qapi/visitor.h      | 8 ++++++++
>  qapi/qapi-dealloc-visitor.c | 5 +++++
>  qapi/qapi-visit-core.c      | 5 +++++
>  4 files changed, 21 insertions(+)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 8110ebd..d301901 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -75,6 +75,9 @@ struct Visitor
>       * visitors do not currently visit arbitrary types).  */
>      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).  */
> +    void (*type_null)(Visitor *v, const char *name, Error **errp);
>

It's not clear to me what you mean by "Must be provided" if only one
visitor implements it.

>      /* May be NULL; most useful for input visitors. */
>      void (*optional)(Visitor *v, const char *name, bool *present);
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 5349a64..6e49b51 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -267,6 +267,14 @@ 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 tied to @name in the current object visit.
> + * @name will be NULL if this is visited as part of a list.
> + * 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);
> +
> +/**
>   * Mark the start of visiting the branches of a union. Return true if
>   * @data_present.
>   * FIXME: Should not be needed
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 560feb3..ede1703 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -162,6 +162,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)
> +{
> +}
> +
>  static void qapi_dealloc_type_enum(Visitor *v, const char *name, int *obj,
>                                     const char * const strings[], Error 
> **errp)
>  {
> @@ -222,6 +226,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;
>      v->visitor.start_union = qapi_dealloc_start_union;
>
>      QTAILQ_INIT(&v->stack);
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 1612d0d..399256b 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -262,6 +262,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);
> +}

Shouldn't it be optional then and a if (v->type_null) be added?

> +
>  void output_type_enum(Visitor *v, const char *name, int *obj,
>                        const char * const strings[], Error **errp)
>  {
> --
> 2.4.3
>
>



-- 
Marc-André Lureau