Re: [Qemu-devel] [PATCH v14 08/19] qapi: Document visitor interfaces, add assertions

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2908 bytes --]

On 04/14/2016 09:22 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> The visitor interface for mapping between QObject/QemuOpts/string
>> and QAPI is scandalously under-documented, making changes to visitor
>> core, individual visitors, and users of visitors difficult to
>> coordinate.  Among other questions: when is it safe to pass NULL,
>> vs. when a string must be provided; which visitors implement which
>> callbacks; the difference between concrete and virtual visits.
>>
>> Correct this by retrofitting proper contracts, and document where some
>> of the interface warts remain (for example, we may want to modify
>> visit_end_* to require the same 'obj' as the visit_start counterpart,
>> so the dealloc visitor can be simplified).  Later patches in this
>> series will tackle some, but not all, of these warts.
>>
>> Add assertions to (partially) enforce the contract.  Some of these
>> were only made possible by recent cleanup commits.
>>
>> +/*
>> + * The QAPI schema defines both a set of C data types, and a QMP wire
>> + * format.  A QAPI object is formed as a directed acyclic graph of
>> + * QAPI values.
> 
> I understand what you're trying to say, but I find the value / object
> dichotomy odd.  For me, A QAPI object isn't a DAG, it's a node in a DAG.
> Perhaps: "QAPI objects can contain references to other QAPI objects,
> resulting in a directed acyclic graph."

Thanks for a lot of good comments. I'm replying only to the questions
you left amidst all the good review.


>> +++ b/qapi/qapi-visit-core.c
>> @@ -23,6 +23,10 @@
>>  void visit_start_struct(Visitor *v, const char *name, void **obj,
>>                          size_t size, Error **errp)
>>  {
>> +    if (obj) {
>> +        assert(size);
> 
> Yes, because the generator puts a dummy member into empty structs.
> 
>> +        assert(v->type != VISITOR_OUTPUT || *obj);
> 
> Can you point me to the spot in the contract that requires this?

Translation of the assert: If you are using an output visitor, and not
doing a virtual walk (obj is non-NULL), then the object must be
completely built (*obj is non-NULL).  For an input visitor, *obj is NULL
on entry (we're allocating it, after all); for the dealloc visitor, *obj
may or may not be NULL (since we handle cleanup of partial allocation).

In the text, "output visitors (QMP and string) take a completed QAPI
graph", but maybe I can further clarify that a completed object means
that *obj is non-NULL and all 'has_member' and 'member' members are
complete.

> 
> Unlike last time, my remarks are pretty much only about how to say
> things, not about what to say.  Progress!

Yay!  Hopefully I'll post v15 soon and we can get this in at the start
of the 2.7 cycle.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]