Re: [Qemu-devel] [PATCH v9 21/37] qapi: Document visitor interfaces, add assertions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 01/21/2016 01:08 PM, Markus Armbruster wrote:
>> All right, this one's a bear.  Not because the patch is bad, but because
>> what it tries to do is bloody difficult.
>
> Is there any reasonable split (such as adding some of the assertions in
> earlier patches) that would make it any easier? Or do we just bite the
> bullet and do it?

I think the difficulty is in finding a contract that fits the current
code and makes sense.

Assertions and contract overlap: assertions partly enforce the contract.
Separating the two won't help.

We could try to split along sub-interface boundaries, say scalars,
structs, unions, alternates, lists.  Several smaller patches, but to
ensure overall consistency, you have to mentally merge them again.
Doesn't seem helpful.

Let's bite the bullet.

>> Eric Blake <address@hidden> writes:
>> 
>>> The visitor interface for mapping between QObject/QemuOpts/string
>>> and qapi has formerly been documented only by reading source code,
>> 
>> Polite way to say "is scandalously undocumented".
>
> Indeed.
>
>> 
>>> making it difficult to propose changes to either scripts/qapi*.py
>>> or to clients without knowing whether those changes would be safe.
>>> This adds documentation, including mentioning when parameters can
>>> be NULL, and where there are still some interface warts that would
>>> be nice to remove.  In particular, I have plans to remove
>>> visit_start_union() in a future patch.
>> 
>> Suggest
>> 
>>     The visitor interface for mapping between QObject/QemuOpts/string
>>     and QAPI is pretty much undocumented, making changes to visitor
>>     core, individual visitors, and users of visitors difficult.
>> 
>>     Correct this by retrofitting proper contracts.  Document some
>>     interface warts that would be nice to remove.  In particular, I have
>>     plans to remove visit_start_union() in a future patch.
>
> Your suggestions here, and elsewhere, are good and will be in my next
> spin.  I'll trim to just the places where you have more than just a
> wording suggestion.
>
>
>>>  include/qapi/visitor-impl.h |  31 ++++++-
>>>  include/qapi/visitor.h      | 196 
>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>  qapi/qapi-visit-core.c      |  39 ++++++++-
>>>  3 files changed, 262 insertions(+), 4 deletions(-)
>>>
>> 
>> My review probably makes more sense if you skip ahead to visitor.h, then
>> come back here.
>
> If I remember, I'll use -O when generating v10 to force visitor.h first,
> other .h second, and .c last (I don't always remember to do it; maybe I
> should add it into my handy .git alias that I use for firing off long
> series).  [I really wish the git people would make it possible to
> automate -O via 'git config', and to make it easier to have a
> per-project preferred order file]
>
>> 
>>> +
>>>  struct Visitor
>>>  {
>>> -    /* Must be set */
>>> +    /* Must be provided to visit structs (the string visitors do not
>>> +     * currently visit structs). */
>> 
>> Uh, the string visitors don't decide what gets visited, their users do.
>> The string visitors don't support visiting structs.  The restriction
>> needs to be documented, but this isn't the place to do it, is it?
>
> Good point.  I think what I will do is split out a separate patch that
> documents, per visitor with limitation, what callers cannot (yet) do
> with that visitor.

Makes sense.

>>> +    /* May be NULL; most useful for input visitors. */
>> 
>> "Optional" would be a bit terser than "May be NULL".
>> 
>> Why is it "most useful for input visitors"?  For what it's worth, the
>> dealloc visitor finds it useful, too...
>
> and a later patch adds it to the QemuOpts input visitor (Zoltan's patch
> has been sitting for how many months now?).  I'll come up with something.
>
>> 
>>>      void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
>>>                                    Error **errp);
>>>      /* May be NULL */
>>>      void (*end_implicit_struct)(Visitor *v);
>>>
>>> +    /* Must be set */
>>>      void (*start_list)(Visitor *v, const char *name, Error **errp);
>>> +    /* Must be set */
>>>      GenericList *(*next_list)(Visitor *v, GenericList **list, Error 
>>> **errp);
>>>      /* Must be set */
>>>      void (*end_list)(Visitor *v);
>> 
>> A visitor could omit these two with similar consequences to omitting
>> start_struct() and end_struct(): attempts to visit lists crash then.  In
>> fact, the string visitors omitted them until commit 659268f and 69e2556,
>> respectively.
>
> Which is why, as you pointed out, it may be better to document the
> limitations in the string visitor rather than here, and in this file
> maybe just mention at the top something along the lines that "must be
> set" really means that "only needs to be set if your callers are
> expecting a visit to encounter this type; the corresponding crash on
> calling NULL is your hint to write missing functionality in your visitor".

Good idea.

Perhaps say "Must be set for $TYPE visits to work" in each applicable
place, and explain what that means just once.

>>>      /* May be NULL; only needed for input visitors. */
>>        void (*get_next_type)(Visitor *v, const char *name, QType *type,
>>                              bool promote_int, Error **errp);
>> 
>> I figure it must be set for input visitors, because without it, the
>> generated visit function will assume QTYPE_NONE, and fail.
>> 
>> Suggest "Must be set for input visitors."
>
> Correct, and nice wording.
>
>> 
>> Looks like we say "must be set to visit this" when at least one visitor
>> doesn't provide, and "must be set" when all our current visitors
>> provide.  Hmm.
>> 
>>>      void (*type_any)(Visitor *v, const char *name, QObject **obj,
>>>                       Error **errp);
>>>
>>>      /* May be NULL; most useful for input visitors. */
>>>      void (*optional)(Visitor *v, const char *name, bool *present);
>>>
>>> +    /* FIXME - needs to be removed */
>>>      bool (*start_union)(Visitor *v, bool data_present, Error **errp);
>>> +    /* FIXME - needs to be removed */
>>>      void (*end_union)(Visitor *v, bool data_present, Error **errp);
>>>  };
>>>
>>> +/**
>> 
>> The /** is a marker for GTK-Doc.  We don't actually follow GTK-Doc
>> conventions, however (they suck).  Sure we want the extra * anyway?
>
> I don't mind dropping it.

Please do.

>>> +++ b/include/qapi/visitor.h
>>> @@ -18,6 +18,20 @@
>>>  #include "qapi/error.h"
>>>  #include <stdlib.h>
>>>
>>> +/* This file describes the client view for visiting a map between
>>> + * generated QAPI C structs and another representation (command line
>>> + * options, strings, or QObjects).
>> 
>> "visiting a map"?
>
> I'm a victim of too much rebasing.  I think I meant:
>
> ...the client view for mapping between generated QAPI C structs and
> another representation...

That makes more sense.  It's rather terse, though.  I explained the
purpose and structur more verbosely below.  Perhaps we can and adapt
that into an introductory comment.  But let's figure out the contract
before we worry about the introduction.

>>>                                      An input visitor converts from
>>> + * some other form into QAPI representation; an output visitor
>>> + * converts from QAPI back into another form.
>> 
>> Let me try to work out what this visitor thingy is about.
>> 
>> The QAPI schema defines a set of QAPI types.  QAPI types can have
>> members of QAPI type.  A value of such a type is actually a root of a
>> graph of QAPI values.
>> 
>> QAPI generates visitor functions to walk these graphs.  They currently
>> work only for trees, because that's all we need.
>> 
>> Walking a tree (or graph for that matter) is useful only to actually do
>> something with the nodes.  The generated visitor functions do the
>> walking and the visitor classes do the doing.  This is the visitor
>> pattern at work: we factor walking the data structure out of the various
>> tasks that need to walk it to do something.
>
> Additionally, it is possible to use the visitor without a qapi struct,
> purely for their side effects of translation to or from the alternative
> representation of that visitor, by manually calling visitor functions in
> the same order that generated QAPI structs would use.

This is like walking a virtual tree.

Virtual walks add complexity:

* When we walk a real tree, we pass real @obj pointers.  When we walk a
  virtual tree, we pass null pointers.

  Example: we pass null to visit_start_struct() when we have no real
  QAPI struct.  However, we *can't* pass null to visits of scalars like
  visit_type_int().  Scalars must be real.

  When exactly are null pointers okay?  Or in other words, which parts
  of the tree may be virtual?  Needs to be spelled out.

  Does "may be virtual" depend on the actual visitor?

* When we walk a real tree, we use in-tree information to find optional
  stuff: visit_optional() for optional members, visit_next_list() for
  remaining list members, visit_get_next_type() to help find the
  alternate member in use.  When we walk a virtual tree, we make those
  decisions ourselves.

* Anything else?

Not documenting interfaces breeds interface complexity.

Related complication: the generated visitor functions accept
half-constructed trees, because we need to be able to destroy such trees
with the dealloc visitor.  If you visit a half-constructed tree with an
output visitor, the visit function doesn't flag the misuse, and the
output will be garbage.

>> Three kinds of visitor classes exist: two output visitors (QMP and
>> string), three input visitors (QMP, string and QemuOpts), and the
>> dealloc visitor.
>> 
>> With an output visitor, the visitor functions walk a tree, and the
>> output visitor builds up some output.  QmpOutputVisitor builds a QObject
>> matching QMP wire format,, and StringOutputVisitor builds a C string.
>> Both convert a QAPI tree to another representation.
>
> Or, when passed NULL obj, create the other representation out of thin
> air from the manual visit.

Mind, I'm talking about the generated visitor

With an output visitor, the visitor functions walk a tree (real or
virtual), and...

>> Similarly with the QapiDeallocVisitor, except nothing gets built.
>> Instead, the tree gets destroyed node by node.
>> 
>> With an input visitor, the visitor functions walk a tree as the input
>> visitor constructs it.  QmpInputVisitor constructs it from a QObject
>> matching QMP wire format, StringInputVisitor from a C string, and
>> OptsVisitor from a QemuOpts.  All three convert from another
>> representation to a QAPI tree.
>
> Or, when passed NULL obj, parse the other representation via a manual
> visit with no QAPI object involved.

Actually, the generated visitor functions assume real trees.  Possibly
half-constructed ones (see "related complication" above).  Null should
only occur in a half-constructed tree.  The generated visitor functions
then skip over that part.

To walk the virtual part of a tree, you need to stitch together calls of
the visitor.h interface yourself.  I guess that's what you call "manual
visit".

I think the introduction should explain visiting real trees first, and
virtual trees second, to keep complexity under control.

>> The Qmp visitors and the dealloc visitor a general: they can walk /
>> build any QAPI tree.
>> 
>> The string visitors and the QemuOpts visitor have restrictions: they can
>> walk / build only a subset.
>
> Yes.
>
>>> +/**
>>> + * Prepare to visit an object tied to key @name.
>> 
>> Not just any object, but a *struct*.  Suggest:
>> 
>>  * Start visiting struct value @obj.
>> 
>>> + * @name will be NULL if this is visited as part of a list.
>> 
>> I'm afraid reality is a bit messier.
>> 
>> If the visited object is a member of struct or union, @name is its
>> member name.
>> 
>> If it's a member of a list, @name is null.
>
> [side thread in earlier message about possibly using "name[0]" instead
> of NULL, as a later improvement]
>
>> 
>> If it's a command argument, @name is its parameter name.
>
> But this is a special case of "visiting as part of a struct", since we
> have a (possibly-implicit) QAPI struct for parameters.

Conceptually, yes.  In actual code, the struct need not exist, and then
the name comes from different code, which could result in different
@name.  That's why I made it a separate clause.  Turns out @name is the
same, i.e. we didn't screw that one up.

>> If it's a command's result, @name is a meaningless string (qmp-marshal.c
>> currently uses "unused").
>
> But this is a special case of a root visit (the command result is the
> top of a visit, so @name is meaningless).

In actual code, this is a differnet root visit, and it actually does
things differently: "unused" rather than NULL.  The difference is of
course pointless.

>> Else, @name can be a meaningless string or null.
>
> And this sentence is reached only for a root visit.

Unless I missed a case.  I wasn't quite sure, so I used a catch-all
clause.

>                                                      So now I'm thinking
> along these lines:
>
> As the first object in a visit (the root of a QAPI struct), @name is
> meaningless. It is typically set to NULL or a descriptive string,
> although some callers pass "unused".

Scratch "or a descriptive string".

> At all other points of the visit, @name reflects the relationship of
> this visit to the parent.  Either the visited object is a member of a
> struct or union,

alternate?

>                  and @name is its member name; or the visited object is
> the member of a list and @name is NULL.

Works for me.

We'll likely have to change it to get sane error messages, but let's not
worry about that now.

>> Same for other visit functions.
>
> Is it okay to write it out once, and then have all other functions refer
> back to the common location?

Yes, please.

>>>                                                               The
>>> + * caller then makes a series of visit calls for each key expected in
>>> + * the object, where those visits set their respective obj parameter
>>> + * to the address of a member of the qapi struct, and follows
>>> + * everything by a call to visit_end_struct() to clean up resources.
>> 
>> I'd explain intended use after the parameters.
>> 
>>> + *
>>> + * @obj can be NULL (in which case @size should also be 0) to indicate
>> 
>> "must be 0", because you assert that below.
>> 
>> Actually, I find this @size contract weird.  Passing the type's actual
>> size would make at least as much sense as passing 0.  We generally pass
>> 0 when the size isn't used, but that's just because it's the easiest
>> value to pass.
>
> We pass 0 precisely when @obj is NULL because that is the usage pattern
> where we do NOT have a QAPI struct, but are manually using the visitor
> for its side effects.  It's hard to have a non-zero size of a
> non-existent struct.

"I don't have a struct, and therefore I don't have a size, and therefore
the size must be zero" isn't exactly impeccable logic, but defensible.

Equally defensible, at least for me: "my struct is virtual, and its size
is the same as if it was real".

Why constrain @size when it's not used?  Just document that it's not
used.

>>> + * that there is no qapi C struct, and that the upcoming visit calls
>>> + * are parsing input to or creating output from some other
>>> + * representation.
>> 
>> This is very vague.
>> 
>> Can you point me to code passing null @obj?
>
> Easiest example: hw/ppc/spapr_drc.c.  Maybe I should follow danpb's lead
> and actually put some <example> usage of the visitors in the comments.
>
>> I suspect only some visitors accept null @obj.  Which ones?
>
> I didn't check that; will do.
>
>> 
>>> + *
>>> + * If @obj is not NULL, then input visitors malloc a qapi struct of
>>> + * @size bytes into address@hidden on success, and output visitors expect 
>>> address@hidden
>>> + * to be a fully-populated qapi struct.
>> 
>> Only input visitors and the dealloc visitor use @obj.  The dealloc
>> visitor doesn't need it in start_struct(), but has to save it for
>> end_struct(), because that one doesn't get it passed.  Aside: feels
>> stupid.
>
> Probably possible to change, although none of my patches do it yet.

Please think twice before from growing the QAPI patch queue further.  We
really, really need to clear it.  A TODO comment would be welcome,
though.

>> Only input visitors use @size, and they use it only when @obj isn't
>> null.
>> 
>> We could make visitors check the member pointers passed to the member
>> visits actually point into (@obj, @size).  Then *all* visitors would use
>> @obj and @size.  I'm not asking for that to be done, I'm just providing
>> an argument for a tighter contract.  The simplest one would be "Some
>> visitors permit @obj to be null.  @size must be the struct value's
>> size."  Except that doesn't match existing usage.  Unless we change it
>> to match, we need to hedge "except @size is unused and may be anything
>> when @obj is null", or "except @size is unused and must be zero when
>> @obj is null".
>
> My intent was the latter - @size is unused and must be zero when @obj is
> NULL.  Also, rather than making every visitor track that @obj for the
> current visit lies within (@obj, @size) of the parent struct, I'm
> wondering if it would be better to do that tracking at the
> qapi-visit-core level - but then we'd have two levels of code tracking a
> stack of information instead of one.

I don't think we should do this tracking now.  This is just about
figuring out a sensible contract without undue clinging to what the
current code does.

>> This method is an awful mess.  Probably because it's serving quite
>> different purposes in the different visitors.
>
> Indeed.  visit_type_str() is the best example of the conflict of
> interest between input and output visitors, in that we can't be
> const-correct.
>
> At one point, I was half-tempted to split input and output visitors into
> two separate contracts, rather than trying to merge both types of visits
> through a single interface and having the interface become a bit
> unwieldy.  But as currently written, it's kind of convenient that a
> single function in generated qapi-visit.c can handle all the visitors.

It's a tradeoff.  The generated visitor code is repetitive enough as it
is.

>>> + *
>>> + * Set address@hidden on failure; for example, if the input stream does not
>>> + * have a member @name or if the member is not an object.
>> 
>> What do you mean by "if the member is not an object"?
>
> s/object/struct/
>
> If I'm using the QMP input visitor to parse the string "{ \"a\": 1 }",
> and call visit_start_struct("a"), I expect an error because "1" is not a
> struct.
>
>> 
>> Any other ways to fail?
>
> I don't think so.
>
>> 
>> This is the only function comment that says anything about @errp.
>> Should we document the various failure modes everywhere?
>
> Probably useful.  More work, but worth doing.

I think documenting the failure modes is something we could split off
this patch.

>>> + *
>>> + * FIXME: For input visitors, address@hidden can be assigned here even if 
>>> later
>>> + * visits will fail; this can lead to memory leaks if clients aren't
>>> + * careful.
>> 
>> Why is this a FIXME?  How could it be fixed?  More of the same below.
>
> Fixed by 35/37, where we change the return type here, and fix all the
> visit_type_FOO() functions to use that return type to properly use the
> dealloc visitor on any partial address@hidden, so that the callers of
> visit_type_FOO() no longer have to worry about partial allocation.
>
> Maybe my wording could be improved here.

>From the current wording, I gather the interface is needlessly hard to
use correctly, but I don't understand how exactly users can screw up.  I
hope I will after review of PATCH 35.  Perhaps I can suggest clearer
wording then.

>> My attempt at explaining intended use:
>> 
>>     After visit_start_struct() succeeds, the caller may visit its
>>     members one after the other, passing the member's name and address
>>     within the struct.  Finally, visit_end_struct() needs to be called
>>     to clean up.
>> 
>> I guess what the reader really needs here to understand intended use is
>> example code.  qapi-code-gen.txt has some.  Add a pointer?
>
> Hmm, a pointer to qapi-code-gen would be shorter than an inline
> <example> blurb.  But it only lists the generated usage; I may also want
> to document the no-QAPI-struct usage (our friend spapr_drc.c again).

That's an argument against doing it in qapi-code-gen.txt, because
walking virtual trees is probably out of scope there.

>>> + */
>>>  void visit_start_struct(Visitor *v, const char *name, void **obj,
>>>                          size_t size, Error **errp);
>> 
>> Please separate each commented declaration with a blank line.  Not
>> flagging further instances.
>
> I was trying to group related functions; will switch to one blank in
> related sets, and two blanks between sets (instead of zero/one).
>
>> 
>>> +/**
>>> + * Complete a struct visit started earlier.
>>> + * Must be called after any successful use of visit_start_struct(),
>>> + * even if intermediate processing was skipped due to errors, to allow
>>> + * the backend to release any resources.
>> 
>> Actually, destroying the visitor is safe even without calling the
>> remaining visit_end_struct().  If we introduce a visitor reset as
>> discussed elsewhere, that could probably be made safe, too.
>
> Except that patch 33/37 asserts that destroying the visitor is only ever
> done after all visit_end_struct()s have been paired, as a tighter
> contract which will then let us free up some resources during the
> end_struct() without having to track them for a potentially-early
> visitor destruction.

Requiring all outstanding end_FOO()s to be called before you may destroy
or reset makes the interface harder to use.  Moreover, not being able to
destroy at any time is unusual.  I can accept this if it makes the code
substantially simpler.

> And yes, I already have a followup series posted that introduces a
> visitor reset, at least for the QMP output visitor (I wasn't sure
> whether to generalize it to all visitors).
>
>
>>> +/**
>>> + * Prepare to visit an implicit struct.
>>> + * Similar to visit_start_struct(), except that an implicit struct
>>> + * represents a subset of keys that are present at the same nesting level
>>> + * of a common object but as a separate qapi C struct, rather than a new
>>> + * object at a deeper nesting level.
>> 
>> I'm afraid this is impenetrable.
>> 
>> I tried to refresh my memory on visit_start_implicit_struct()'s purpose
>> by reading code, but that's pretty impenetrable, too.
>
> Consider the input string { "a":1, "b":2 }.
>
> With a normal QAPI struct, all fields of the JSON object are part of the
> same C struct:
> Foo { int a; int b; };
>
> and it is visited with:
> visit_start_struct(); visit_type_int("a", &a); visit_type_int("b", &b);
> visit_end_struct();
>
> But with an implicit struct (namely, a branch of a QAPI union type or a
> member of a QAPI alternate type; we used to also do it for base classes
> but changed that to inline fields instead), we have more than one C
> struct that map to the same flat JSON object:
> Bar { int b; };
> Foo { int a; Bar *sub; }
>
> which is visited with:
> visit_start_struct(); visit_type_int("a", &b);
> visit_start_implicit_struct(&sub); visit_type_int("b", &sub.b);
> visit_end_implicit_struct(); visit_end_struct();
>
> Suggestions on how to better word it are welcome.  I'm basically trying
> to describe that this function marks the start of a new C struct used as
> a sub-object while still conceptually parsing the same top-level QAPI
> struct.

Thinking aloud...

QAPI defines both C data types and a QMP wire format.

The work of mapping between the two is split between generated visitor
functions and the QMP visitors.  Roughly, the visitor functions direct
the mapping of the tree structure, and the QMP visitors take care of the
leaves.

The QAPI tree structure and the QMP tree structure are roughly the same.
Exception: some structs are inlined into a parent struct in QMP, but
stored as a separate, boxed struct in QAPI.  We call them "implicit"
structs.  We got rid of one case (base structs), but we still got two
(flat union and alternate members).  I dislike the exception, but we
need to document what we've got.

It's mostly handled by the visitor functions, but two visitors need to
hook into their handling, because they allocate / free the boxed struct:
the QMP input visitor and the dealloc visitor.

The QMP output visitor doesn't.  The effect is that the members of the
implicit struct get inlined into the explicit struct that surrounds it.

I oversimplified when I said "and a QMP wire format": since we acquired
string and QemuOpts visitors, we also have a string and QemuOpts format.
Both are partial: they don't support all of QAPI.

However, these formats aren't independent of the QMP wire format.  Since
we use the same visitor functions, and the visitor functions map the
tree structure, the tree structure gets mapped just like for QMP.
Almost theoretical, because these visitors don't support most of the
structure.

If we wanted a format that doesn't inline implicit structs, the visitor
would want to implement visit_start_implicit_struct() and
visit_end_implicit_struct() just like visit_start_struct() and
visit_end_struct().  Differences:

* start_implicit_struct() lacks the @name parameter.

* end_implicit_struct() lacks the @errp now.  Later in this series, this
  becomes: there is no check_implicit_struct().

Not inlining implicit structs seems impossible with the current API.
I'm *not* asking for a change that makes it possible.  Instead, my point
is: the inlining of implicit structs is baked into the visitor
interfaces.

I'm afraid I can't turn this into a reasonable function comment without
first amending the introduction comment to cover tree structure mapping.
Ugh.

Crazy thought: unboxing the implicit struct should make this interface
wart go away.  If we commit to do that later, we can "solve" our
documentation problem the same way as for visit_start_union(): FIXME
should not be needed.

>>> + *
>>> + * @obj must not be NULL, since this function is only called when
>>> + * visiting with qapi structs.
>> 
>> Really?  Why does qmp_input_start_implicit_struct() check for null then?
>
> Probably worth an independent cleanup.  The assertions added in this
> patch prove that that check is dead.

Please do that; such contradictions can be terribly confusing.

>>> +/**
>>> + * Prepare to visit a list tied to an object key @name.
>>> + * @name will be NULL if this is visited as part of another list.
>>> + * After calling this, the elements must be collected until
>>> + * visit_next_list() returns NULL, then visit_end_list() must be
>>> + * used to complete the visit.
>> 
>> I'm afraid this doesn't really tell me how to visit a list.  Perhaps:
>> 
>>     After visit_start_list() succeeds, the caller may visit its members
>>     one after the other, passing the value of visit_next_list() as @obj,
>>     until visit_next_list() returns NULL.  Finally, visit_end_list()
>>     needs to be called to clean up.
>> 
>> May want to add a pointer to example code in qapi-code-gen.txt.
>
> And it changes again in 34/37.

Reviewers are too myopic to see beyond the current patch, let alone 13
patches ahead :)

> We have two styles; our generated code, which is roughly:
>
> visit_start_list(&obj);
> while (visit_next_list()) {
>     visit_type_FOO(element);
> }
> visit_end_list()

Here, the list is real, i.e. an instance of a QAPI list type.

> and the manual style of our friend hw/ppc/spapr_drc.c:
>
> visit_start_list(NULL);
> while (some other way to decide if data available) {
>     visit_type_FOO(element);
> }
> visit_end_list()

Here, the list is virtual, but the list elements are real.

> That is, the use of visit_next_list() is optional depending on whether a
> QAPI list is in use, but the visit_type_FOO() per element is necessary.
>  We'll see if I can get the next wording attempt a bit easier to understand.
>> 
>>> + */
>>>  void visit_start_list(Visitor *v, const char *name, Error **errp);
>>> +/**
>
>>> + *
>>> + * Note that for some visitors (qapi-dealloc and qmp-output), when a
>>> + * qapi GenericList linked list is not being used (comparable to when
>>> + * a NULL obj is used for visit_start_struct()), it is acceptable to
>>> + * bypass the use of visit_next_list() and just directly call the
>>> + * appropriate visit_type_*() for each element in between the
>>> + * visit_start_list() and visit_end_list() calls.
>> 
>> I'm confused.  Can you point me to code bypassing visit_next_list()?
>
> See above; spapr_drc.c.

I see clearer now, thanks.

I think the documentation needs to explain the difference between
walking a real tree and walking a virtual tree, how to do either, and
what visitors need to implement to support either.

>>>  /**
>>     * Check if an optional member @name of an object needs visiting.
>>     * For input visitors, set address@hidden according to whether the
>>     * corresponding visit_type_*() needs calling; for other visitors,
>>     * leave address@hidden unchanged.  Return address@hidden for convenience.
>>    bool visit_optional(Visitor *v, const char *name, bool *present);
>> 
>> 
>> Suggest to clarify:
>> 
>>     Does optional struct member @name need visiting?
>>     @present points to the optional member's has_ flag.
>>     Input visitors set address@hidden according to their input.  Other
>>     visitors leave it unchanged.
>>     In either case, return address@hidden
>
> Thanks. I originally added all the docs in one pass, but some of the
> docs snuck in via earlier patches during rebase churn, so reviewing it
> again here helps.
>
>>> +/**
>>> + * Visit a string value tied to @name in the current object visit.
>>> + * @name will be NULL if this is visited as part of a list.
>>> + * @obj must be non-NULL.
>> 
>> Same for the other visit_type_T(), but not specified there.
>> 
>>>                             Input visitors set address@hidden to the parsed
>>> + * string (never NULL); while output visitors leave address@hidden 
>>> unchanged,
>>> + * except that a NULL address@hidden will be treated the same as "".
>> 
>> Suggest:
>> 
>>     Input visitors set address@hidden according to their input (never NULL).
>>     Other visitors leave it unchanged.  They commonly treat NULL like "".
>> 
>>> + *
>>> + * FIXME: Unfortunately not const-correct for output visitors.
>> 
>> Is that fixable?
>
> Not easily, and certainly not as part of this series.  The best I can
> think of is either two interfaces:
>
> char *visit_input_type_str(Visitor *v, const char *name, Error **errp);
> void visit_output_type_str(Visitor *v, const char *name, const char
> *value, Error **errp);
>
> used as:
>
> obj.str = visit_input_type_str(v, "foo", &err);
> visit_output_type_str(v, "foo", obj.str, &err);
>
> or to make the single interface have more parameters:
>
> void visit_type_str(Visitor *v, const char *name, const char *value_in,
> char **value_out, Error **errp);
>
> used as:
>
> visit_type_str(v, "foo", obj.str, &obj.str, &err);
>
> where input visits could pass NULL instead of value_in, and output
> visits could pass NULL instead of value_out.
>
> But either way, consistency would then argue that ALL visit_type_FOO()
> interfaces should either have two forms (one for input, one for output)
> or have more parameters (with input distinct from output), and it would
> allow const-correctness on output visits of more than just strings.  And
> which form would a dealloc visitor use?

What a headache...

I asked whether its fixable, because I don't like FIXMEs we can't or
don't intend to fix.

>>> +/**
>>> + * Mark the start of visiting the branches of a union. Return true if
>>> + * @data_present.
>> 
>> Not quite.  Actually returns true when the visitor doesn't provide this
>> callback, or else whatever its callback returns.  Only the dealloc
>> visitor provides it, and it returns @data_present.
>> 
>> You remove the function along with visit_end_union() in PATCH 32.  I'd
>> be okay with leaving them undocumented apart from the FIXME until then.
>> But if we add a contract now, it better be correct.
>
> I like the 'FIXME'-only approach.

Me too.