[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v14 08/19] qapi: Document visitor interfaces, add assertions
Date: Tue, 26 Apr 2016 15:50:13 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

On 04/14/2016 09:22 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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

> 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: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]