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

[Eric Blake]

On 01/05/2016 07:05 AM, Marc-André Lureau wrote:
> 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,
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature