[v8,22/35] qapi: Add visit_type_null() visitor

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 <eblake@redhat.com>

---
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(+)

Hi

On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>

overall looks good to me:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

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
>
>

On 01/05/2016 07:05 AM, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>>
> 
> overall looks good to me:
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Just a small remark below,
> 

>> +    /* 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.
> 

>> +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?

No one else (yet) uses a visitor that explicitly visits JSON 'null'.  If
someone adds the use of such a visitor, this code will crash at the
attempt to use the missing callback, which will tell the developer to
add the callback.  Adding a conditional would paper over the issue and
make it harder to find.

At any rate, it matches existing pattern of 'must be provided if you
plan to visit X' vs. visitors that lack the callback - see visit_type_any().

Maybe I should write a followup patch that implements it for
qmp-input-visitor, as well as a testsuite that proves we can round-trip
a null literal from JSON to QMP back to JSON.  And when it comes to
blockdev commands for reopening a device (such as changing its
throttling options), we've toyed with the idea of being able to
explicitly call out to leave an option unchanged (omit 'option'
altogether) vs. reset the option to its default (doable with
'option':null, but requires that we allow parsing explicit null).