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

[Eric Blake]

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?

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

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


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

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

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

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

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

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

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

> 
> Else, @name can be a meaningless string or null.

And this sentence is reached only for a root visit.  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".

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, and @name is its member name; or the visited object is
the member of a list and @name is NULL.

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

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

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

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

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


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

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

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

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

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.

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


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

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

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

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.


>>  /**
>     * 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?


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


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

signature.asc
Description: OpenPGP digital signature