[v9,21/37] qapi: Document visitor interfaces, add assertions

Commit Message

Eric Blake Jan. 19, 2016, 4:10 p.m. UTC

The visitor interface for mapping between QObject/QemuOpts/string
and qapi has formerly been documented only by reading source code,
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.

Add some asserts to strengthen the claims of the documentation; some
of these were only made possible by recent cleanup commits.  These
were made easier with the addition of a new visit_is_output()
helper (since all 2 output visitors of our 6 overall visitors use
the same .type_enum() callback).

Signed-off-by: Eric Blake <[e-mail]>
Reviewed-by: Marc-André Lureau <[e-mail]>

---
v9: no change
v8: rebase to 'name' motion
v7: retitle; more wording changes, add asserts to enforce the
wording, place later in series to rebase on fixes that would
otherwise trip the new assertions
v6: mention that input visitors blindly assign over *obj; wording
improvements
---
 include/qapi/visitor-impl.h |  31 ++++++-
 include/qapi/visitor.h      | 196 ++++++++++++++++++++++++++++++++++++++++++++
 qapi/qapi-visit-core.c      |  39 ++++++++-
 3 files changed, 262 insertions(+), 4 deletions(-)

Comments

Markus Armbruster Jan. 21, 2016, 8:08 p.m. UTC | #1

All right, this one's a bear.  Not because the patch is bad, but because
what it tries to do is bloody difficult.

Eric Blake <[e-mail]> 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".

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

> Add some asserts to strengthen the claims of the documentation; some
> of these were only made possible by recent cleanup commits.  These
> were made easier with the addition of a new visit_is_output()
> helper (since all 2 output visitors of our 6 overall visitors use
> the same .type_enum() callback).

I find past tense confusing here, it makes me think visit_is_output()
was added in a previous patch.  Perhaps:

    Add assertions to (partially) enforce the contract.  Some of these
    were only made possible by recent cleanup commits.

    Add a new visit_is_output() helper to make the assertions more
    readable.  Its implementation is a bit hacky: it relies on the fact
    that all output visitors use the same .type_enum() callback.

> Signed-off-by: Eric Blake <[e-mail]>
> Reviewed-by: Marc-André Lureau <[e-mail]>
>
> ---
> v9: no change
> v8: rebase to 'name' motion
> v7: retitle; more wording changes, add asserts to enforce the
> wording, place later in series to rebase on fixes that would
> otherwise trip the new assertions
> v6: mention that input visitors blindly assign over *obj; wording
> improvements
> ---
>  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.

> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 7f512cf..aab46bc 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -15,23 +15,37 @@
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
>
> +/* This file describes the callback interface for implementing a
> + * QObject visitor.  For the client interface, see visitor.h.  When

"QAPI visitor", for consistency with visitor.h's and qapi-visit-core.c's
file comment.

> + * implementing the callbacks, it is easiest to declare a struct with
> + * 'Visitor visitor;' as the first member.  Semantics for the
> + * callbacks are generally similar to the counterpart public
> + * interface.  */

Thanks for adding this overview comment.

Suggest to say "A callback's contract matches the corresponding public
functions' contract unless stated otherwise."  Then state otherwise
where needed.

> +
>  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?
Suggest to drop the parenthesis.

>      void (*start_struct)(Visitor *v, const char *name, void **obj,
>                           size_t size, Error **errp);
> +    /* Must be provided if start_struct is present. */
>      void (*end_struct)(Visitor *v, Error **errp);
>
> +    /* 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...

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

>
> +    /* Must be set, although the helpers input_type_enum() and
> +     * output_type_enum() should be used if appropriate.  */

Suggest

    Must be set.  See also helpers input_type_enum(),
    output_type_enum().

>      void (*type_enum)(Visitor *v, const char *name, int *obj,
>                        const char *const strings[], Error **errp);
>      /* 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."

> @@ -47,23 +61,38 @@ struct Visitor
>      /* Optional; fallback is type_uint64().  */
>      void (*type_size)(Visitor *v, const char *name, uint64_t *obj,
>                        Error **errp);
> +
>      /* Must be set. */
>      void (*type_bool)(Visitor *v, const char *name, bool *obj, Error **errp);
> +    /* Must be set */
>      void (*type_str)(Visitor *v, const char *name, char **obj, Error **errp);
> +
> +    /* Must be provided to visit numbers (the opts visitor does not
> +     * currently visit non-integers). */

The restriction needs to be documented, but this isn't the place to do
it.  Suggest to drop the parenthesis.

>      void (*type_number)(Visitor *v, const char *name, double *obj,
>                          Error **errp);
> +    /* Must be provided to visit arbitrary QTypes (the opts and string
> +     * visitors do not currently visit arbitrary types).  */

Likewise.

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?

> + * A generic visitor.type_enum suitable for input visitors.
> + */
>  void input_type_enum(Visitor *v, const char *name, int *obj,
>                       const char *const strings[], Error **errp);
> +/**
> + * A generic visitor.type_enum suitable for output visitors.
> + */
>  void output_type_enum(Visitor *v, const char *name, int *obj,
>                        const char *const strings[], Error **errp);
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 10390d2..5349a64 100644
> --- a/include/qapi/visitor.h
> +++ 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"?

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

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.

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.

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.

>                                                 In the descriptions
> + * below, an object or dictionary refers to a JSON '{}', and a list
> + * refers to a JSON '[]'.

We usually call these "JSON object" and "JSON array", following RFC
7159.

>                             These functions seldom need to be called
> + * directly, but are instead used by code generated by
> + * scripts/qapi-visit.py.  For the visitor callback contracts, see
> + * visitor-impl.h.
> + */
> +
> +/* This struct is layout-compatible with all other *List structs
> + * created by the qapi generator. */

QAPI

More instances elsewhere.

>  typedef struct GenericList
>  {
>      union {
> @@ -27,15 +41,101 @@ typedef struct GenericList
>      struct GenericList *next;
>  } GenericList;
>
> +/**
> + * 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.

If it's a command argument, @name is its parameter name.

If it's a command's result, @name is a meaningless string (qmp-marshal.c
currently uses "unused").

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

Same for other visit functions.

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

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

I suspect only some visitors accept null @obj.  Which ones?

> + *
> + * If @obj is not NULL, then input visitors malloc a qapi struct of
> + * @size bytes into *@obj on success, and output visitors expect *@obj
> + * 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.

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

This method is an awful mess.  Probably because it's serving quite
different purposes in the different visitors.

> + *
> + * Set *@errp 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"?

Any other ways to fail?

This is the only function comment that says anything about @errp.
Should we document the various failure modes everywhere?

> + *
> + * FIXME: For input visitors, *@obj 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.

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?

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

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

Same for the other visit_end_FOO().

> + */
>  void visit_end_struct(Visitor *v, Error **errp);
> +
> +/**
> + * 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.

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

> + *
> + * FIXME: For input visitors, *@obj can be assigned here even if later
> + * visits will fail; this can lead to memory leaks if clients aren't
> + * careful.
> + */
>  void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
>                                   Error **errp);
> +/**
> + * Complete an implicit struct visit started earlier.
> + * Must be called after any successful use of visit_start_implicit_struct(),
> + * even if intermediate processing was skipped due to errors, to allow
> + * the backend to release any resources.  Unlike visit_end_struct(), this
> + * does not need to check for errors (detection of unused keys is only
> + * possible for the overall struct, not a subset).
> + */
>  void visit_end_implicit_struct(Visitor *v);
>
> +/**
> + * 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.

> + */
>  void visit_start_list(Visitor *v, const char *name, Error **errp);
> +/**
> + * Iterate over a GenericList during a list visit.
> + * @list must not be NULL; on the first call, @list contains the
> + * address of the list head, and on subsequent calls *@list must be
> + * the previously returned value.  Must be called in a loop until a
> + * NULL return or error occurs; for each non-NULL return, the caller
> + * must then call the appropriate visit_type_*() for the element type
> + * of the list, with that function's name parameter set to NULL.

Isn't must too strong?  Anything horrible's going to happen when you
stop visiting list members early?  I believe not visiting some list
members behaves the same not visiting some struct members: you won't get
the skipped visits' side effects (obviously).  While that may be bad,
it's not *necessarily* bad.  The visitor machinery should cope just fine
regardless.

> + *
> + * 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()?

> + *
> + * FIXME: For input visitors, *@list can be assigned here even if
> + * later visits will fail; this can lead to memory leaks if clients
> + * aren't careful.
> + */
>  GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
> +/**
> + * Complete the list started earlier.
> + * Must be called after any successful use of visit_start_list(),
> + * even if intermediate processing was skipped due to errors, to allow
> + * the backend to release any resources.
> + */
>  void visit_end_list(Visitor *v);
>
>  /**
    * Check if an optional member @name of an object needs visiting.
    * For input visitors, set *@present according to whether the
    * corresponding visit_type_*() needs calling; for other visitors,
    * leave *@present unchanged.  Return *@present 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 *@present according to their input.  Other
    visitors leave it unchanged.
    In either case, return *@present.

> @@ -54,32 +154,128 @@ bool visit_optional(Visitor *v, const char *name, bool *present);
   /**
    * Determine the qtype of the item @name in the current object visit.
    * For input visitors, set *@type to the correct qtype of a qapi
    * alternate type; for other visitors, leave *@type unchanged.
    * If @promote_int, treat integers as QTYPE_FLOAT.
>   */
>  void visit_get_next_type(Visitor *v, const char *name, QType *type,
>                           bool promote_int, Error **errp);

Suggest to clarify:

    Determine a visited alternate's QType.
    [Common description of @name goes here]
    @type points to the alternate's type member.
    Input visitors set *@type according to their input.  If
    @promote_int, integer input results in QTYPE_FLOAT.
    Other visitors leave it unchanged.

> +
> +/**
> + * Visit an enum value tied to @name in the current object visit.
> + * @name will be NULL if this is visited as part of a list.
> + * For input visitors, parse a string and set *@obj to the numeric
> + * value of the enum type using @strings as the mapping, leaving @obj
> + * unchanged on error; for output visitors, reverse the mapping and
> + * visit the output string determined by *@obj.

Not covered: dealloc visitor.  It does nothing, of course.

Our current input / output visitors indeed parse from / unparse to a
string, and but isn't this a *visitor* detail that doesn't belong here?
We could theoretically do binary serialization with a pair of visitors,
where the output visitor writes the numeric encoding, and the input
visitor reads it back.

Suggest:

    Input visitors set *@obj according to their input.  Other visitors
    leave it unchanged.

> + */
>  void visit_type_enum(Visitor *v, const char *name, int *obj,
>                       const char *const strings[], Error **errp);
> +
> +/**
> + * Visit an integer value tied to @name in the current object visit.
> + * @name will be NULL if this is visited as part of a list.
> + * For input visitors, set *@obj to the parsed value; for other visitors,
> + * leave *@obj unchanged.

Here, your text is very close to what I suggested for enums :)

> + */
>  void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp);
> +/**
> + * Visit a uint8_t value tied to @name in the current object visit.
> + * Like visit_type_int(), except clamps the value to uint8_t range.
> + */
>  void visit_type_uint8(Visitor *v, const char *name, uint8_t *obj,
>                        Error **errp);
> +/**
> + * Visit a uint16_t value tied to @name in the current object visit.
> + * Like visit_type_int(), except clamps the value to uint16_t range.
> + */
>  void visit_type_uint16(Visitor *v, const char *name, uint16_t *obj,
>                         Error **errp);
> +/**
> + * Visit a uint32_t value tied to @name in the current object visit.
> + * Like visit_type_int(), except clamps the value to uint32_t range.
> + */
>  void visit_type_uint32(Visitor *v, const char *name, uint32_t *obj,
>                         Error **errp);
> +/**
> + * Visit a uint64_t value tied to @name in the current object visit.
> + * Like visit_type_int(), except clamps the value to uint64_t range
> + * (that is, ensures it is unsigned).

I'd scratch the parenthesis.

> + */
>  void visit_type_uint64(Visitor *v, const char *name, uint64_t *obj,
>                         Error **errp);
> +/**
> + * Visit an int8_t value tied to @name in the current object visit.
> + * Like visit_type_int(), except clamps the value to int8_t range.
> + */
>  void visit_type_int8(Visitor *v, const char *name, int8_t *obj, Error **errp);
> +/**
> + * Visit an int16_t value tied to @name in the current object visit.
> + * Like visit_type_int(), except clamps the value to int16_t range.
> + */
>  void visit_type_int16(Visitor *v, const char *name, int16_t *obj,
>                        Error **errp);
> +/**
> + * Visit an int32_t value tied to @name in the current object visit.
> + * Like visit_type_int(), except clamps the value to int32_t range.
> + */
>  void visit_type_int32(Visitor *v, const char *name, int32_t *obj,
>                        Error **errp);
> +/**
> + * Visit an int64_t value tied to @name in the current object visit.
> + * Identical to visit_type_int().
> + */
>  void visit_type_int64(Visitor *v, const char *name, int64_t *obj,
>                        Error **errp);
> +/**
> + * Visit a uint64_t value tied to @name in the current object visit.
> + * Like visit_type_uint64(), except that some visitors may choose to
> + * recognize additional suffixes for easily scaling input values.

"use different syntax, say suffixes for scaling values."

> + */
>  void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
>                       Error **errp);
> +
> +/**
> + * Visit a boolean value tied to @name in the current object visit.
> + * @name will be NULL if this is visited as part of a list.
> + * Input visitors set *@obj to the value; other visitors will leave
> + * *@obj unchanged.

You're getting closer and closer to my version of this clause :)

> + */
>  void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp);
> +
> +/**
> + * 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 *@obj to the parsed
> + * string (never NULL); while output visitors leave *@obj unchanged,
> + * except that a NULL *@obj will be treated the same as "".

Suggest:

    Input visitors set *@obj 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?

> + * FIXME: Callers that try to output NULL *obj should not be allowed.
> + */
>  void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp);
> +
> +/**
> + * Visit a number value tied to @name in the current object visit.
> + * @name will be NULL if this is visited as part of a list.
> + * Input visitors set *@obj to the value; other visitors will leave
> + * *@obj unchanged.
> + */
>  void visit_type_number(Visitor *v, const char *name, double *obj,
>                         Error **errp);
> +
> +/**
> + * Visit an arbitrary qtype value tied to @name in the current object visit.

Scratch qtype?

> + * @name will be NULL if this is visited as part of a list.
> + * Input visitors set *@obj to the value; other visitors will leave
> + * *@obj (which must not be NULL) unchanged.

Should specify that input visitors never set it to null.  Suggest:

    Input visitors set *@obj according to their input (never NULL).
    Other visitors leave it unchanged.  It must not be NULL then.

> + */
>  void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp);
> +
> +/**
> + * 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.

> + * FIXME: Should not be needed
> + */
>  bool visit_start_union(Visitor *v, bool data_present, Error **errp);
> +/**
> + * Mark the end of union branches, after visit_start_union().
> + * FIXME: Should not be needed
> + */
>  void visit_end_union(Visitor *v, bool data_present, Error **errp);
>
>  #endif
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 2d3743b..1612d0d 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -18,9 +18,21 @@
>  #include "qapi/visitor.h"
>  #include "qapi/visitor-impl.h"
>
> +/* Determine if this is an output visitor.
> + * Useful for making some tighter assertions that hold for output
> + * visitors, but not for input or dealloc visitors. */
> +static bool visit_is_output(Visitor *v)
> +{
> +    return v->type_enum == output_type_enum;

This is a hack.  As mentioned above, we could have an output visitor
doing binary serialization, which would need a different type_enum.  I'm
okay with the hack for now, but it needs a scary comment.

> +}
> +
>  void visit_start_struct(Visitor *v, const char *name, void **obj,
>                          size_t size, Error **errp)
>  {
> +    assert(obj ? size : !size);
> +    if (obj && visit_is_output(v)) {
> +        assert(*obj);
> +    }

This needs to match the contract specified in visitor.h, but so far the
contract is too confusing for me to judge.  I'm skipping review of this
and the following assertion changes for now.

>      v->start_struct(v, name, obj, size, errp);
>  }
>
> @@ -32,6 +44,10 @@ void visit_end_struct(Visitor *v, Error **errp)
>  void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
>                                   Error **errp)
>  {
> +    assert(obj && size);
> +    if (visit_is_output(v)) {
> +        assert(*obj);
> +    }
>      if (v->start_implicit_struct) {
>          v->start_implicit_struct(v, obj, size, errp);
>      }
> @@ -51,6 +67,7 @@ void visit_start_list(Visitor *v, const char *name, Error **errp)
>
>  GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp)
>  {
> +    assert(list);
>      return v->next_list(v, list, errp);
>  }
>
> @@ -85,6 +102,7 @@ bool visit_optional(Visitor *v, const char *name, bool *present)
>  void visit_get_next_type(Visitor *v, const char *name, QType *type,
>                           bool promote_int, Error **errp)
>  {
> +    assert(type);
>      if (v->get_next_type) {
>          v->get_next_type(v, name, type, promote_int, errp);
>      }
> @@ -93,11 +111,13 @@ void visit_get_next_type(Visitor *v, const char *name, QType *type,
>  void visit_type_enum(Visitor *v, const char *name, int *obj,
>                       const char *const strings[], Error **errp)
>  {
> +    assert(obj && strings);
>      v->type_enum(v, name, obj, strings, errp);
>  }
>
>  void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp)
>  {
> +    assert(obj);
>      v->type_int64(v, name, obj, errp);
>  }
>
> @@ -145,6 +165,7 @@ void visit_type_uint32(Visitor *v, const char *name, uint32_t *obj,
>  void visit_type_uint64(Visitor *v, const char *name, uint64_t *obj,
>                         Error **errp)
>  {
> +    assert(obj);
>      v->type_uint64(v, name, obj, errp);
>  }
>
> @@ -192,12 +213,14 @@ void visit_type_int32(Visitor *v, const char *name, int32_t *obj,
>  void visit_type_int64(Visitor *v, const char *name, int64_t *obj,
>                        Error **errp)
>  {
> +    assert(obj);
>      v->type_int64(v, name, obj, errp);
>  }
>
>  void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
>                       Error **errp)
>  {
> +    assert(obj);
>      if (v->type_size) {
>          v->type_size(v, name, obj, errp);
>      } else {
> @@ -207,22 +230,35 @@ void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
>
>  void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp)
>  {
> +    assert(obj);
>      v->type_bool(v, name, obj, errp);
>  }
>
>  void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp)
>  {
> +    assert(obj);
> +    /* TODO: Fix callers to not pass NULL when they mean "", so that we
> +     * can enable:
> +    if (visit_is_output(v)) {
> +        assert(*obj);
> +    }
> +     */
>      v->type_str(v, name, obj, errp);
>  }
>
>  void visit_type_number(Visitor *v, const char *name, double *obj,
>                         Error **errp)
>  {
> +    assert(obj);
>      v->type_number(v, name, obj, errp);
>  }
>
>  void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp)
>  {
> +    assert(obj);
> +    if (visit_is_output(v)) {
> +        assert(*obj);
> +    }
>      v->type_any(v, name, obj, errp);
>  }
>
> @@ -233,7 +269,6 @@ void output_type_enum(Visitor *v, const char *name, int *obj,
>      int value = *obj;
>      char *enum_str;
>
> -    assert(strings);
>      while (strings[i++] != NULL);
>      if (value < 0 || value >= i - 1) {
>          error_setg(errp, QERR_INVALID_PARAMETER, name ? name : "null");
> @@ -251,8 +286,6 @@ void input_type_enum(Visitor *v, const char *name, int *obj,
>      int64_t value = 0;
>      char *enum_str;
>
> -    assert(strings);
> -
>      visit_type_str(v, name, &enum_str, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);

Eric Blake Jan. 22, 2016, 12:30 a.m. UTC | #2

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 <[e-mail]> 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 *@obj on success, and output visitors expect *@obj
>> + * 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 *@errp 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, *@obj 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 *@obj, 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 *@present according to whether the
>     * corresponding visit_type_*() needs calling; for other visitors,
>     * leave *@present unchanged.  Return *@present 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 *@present according to their input.  Other
>     visitors leave it unchanged.
>     In either case, return *@present.

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 *@obj to the parsed
>> + * string (never NULL); while output visitors leave *@obj unchanged,
>> + * except that a NULL *@obj will be treated the same as "".
> 
> Suggest:
> 
>     Input visitors set *@obj 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.

Markus Armbruster Jan. 22, 2016, 12:18 p.m. UTC | #3

Eric Blake <[e-mail]> 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 <[e-mail]> 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 *@obj on success, and output visitors expect *@obj
>>> + * 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 *@errp 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, *@obj 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 *@obj, 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 *@present according to whether the
>>     * corresponding visit_type_*() needs calling; for other visitors,
>>     * leave *@present unchanged.  Return *@present 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 *@present according to their input.  Other
>>     visitors leave it unchanged.
>>     In either case, return *@present.
>
> 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 *@obj to the parsed
>>> + * string (never NULL); while output visitors leave *@obj unchanged,
>>> + * except that a NULL *@obj will be treated the same as "".
>> 
>> Suggest:
>> 
>>     Input visitors set *@obj 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.

Eric Blake Feb. 10, 2016, 12:23 a.m. UTC | #4

On 01/22/2016 05:18 AM, Markus Armbruster wrote:

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

Yes, especially with 2.6 soft freeze fast approaching.


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

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

I _want_ to get rid of the boxing.  But as it is not in my current queue
of pending patches, it will have to wait until the current queue is
flushed; so I'm going for documenting it with FIXMEs for now.

Basically, our current flat union representation is:

struct Union {
    Type tag;
    union {
        Subtype1 *one;
        Subtype2 *two;
    } u;
};

which requires two malloc's to completely populate the struct, and where
we access union->u.one->member, or pass union->u.one to a function
taking Subtype1*.  But it _should_ be:

struct Union {
    Type tag;
    union {
        Subtype1 one;
        Subtype2 two;
    } u;
};

where a single malloc is sufficient, and where we access
union->u.one.member, or pass &union->u.one to a function taking Subtype1*.

It's a tree-wide conversion; and may be easier if done in stages (fix
the generator to take a temporary boolean flag on whether a particular
structure uses inline or boxing, then a series of patches adding that
flag to a few QMP commands at a time, then a final patch to clear out
the temporary flag support) rather than all at once.  I'm not sure how
much Coccinelle will help, because I specifically haven't started the
conversion work until after we can get the current backlog flushed.

Markus Armbruster Feb. 10, 2016, 7:38 a.m. UTC | #5

Eric Blake <[e-mail]> writes:

> On 01/22/2016 05:18 AM, Markus Armbruster wrote:
[...]
>> 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.
>
> I _want_ to get rid of the boxing.  But as it is not in my current queue
> of pending patches, it will have to wait until the current queue is
> flushed; so I'm going for documenting it with FIXMEs for now.
>
> Basically, our current flat union representation is:
>
> struct Union {
>     Type tag;
>     union {
>         Subtype1 *one;
>         Subtype2 *two;
>     } u;
> };
>
> which requires two malloc's to completely populate the struct, and where
> we access union->u.one->member, or pass union->u.one to a function
> taking Subtype1*.  But it _should_ be:
>
> struct Union {
>     Type tag;
>     union {
>         Subtype1 one;
>         Subtype2 two;
>     } u;
> };
>
> where a single malloc is sufficient, and where we access
> union->u.one.member, or pass &union->u.one to a function taking Subtype1*.
>
> It's a tree-wide conversion; and may be easier if done in stages (fix
> the generator to take a temporary boolean flag on whether a particular
> structure uses inline or boxing, then a series of patches adding that
> flag to a few QMP commands at a time, then a final patch to clear out
> the temporary flag support) rather than all at once.  I'm not sure how
> much Coccinelle will help, because I specifically haven't started the
> conversion work until after we can get the current backlog flushed.

I hope the use of unions in C code is localized enough to do it in one
step.

Patch

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 7f512cf..aab46bc 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -15,23 +15,37 @@ 
 #include "qapi/error.h"
 #include "qapi/visitor.h"

+/* This file describes the callback interface for implementing a
+ * QObject visitor.  For the client interface, see visitor.h.  When
+ * implementing the callbacks, it is easiest to declare a struct with
+ * 'Visitor visitor;' as the first member.  Semantics for the
+ * callbacks are generally similar to the counterpart public
+ * interface.  */
+
 struct Visitor
 {
-    /* Must be set */
+    /* Must be provided to visit structs (the string visitors do not
+     * currently visit structs). */
     void (*start_struct)(Visitor *v, const char *name, void **obj,
                          size_t size, Error **errp);
+    /* Must be provided if start_struct is present. */
     void (*end_struct)(Visitor *v, Error **errp);

+    /* May be NULL; most useful for input visitors. */
     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);

+    /* Must be set, although the helpers input_type_enum() and
+     * output_type_enum() should be used if appropriate.  */
     void (*type_enum)(Visitor *v, const char *name, int *obj,
                       const char *const strings[], Error **errp);
     /* May be NULL; only needed for input visitors. */
@@ -47,23 +61,38 @@  struct Visitor
     /* Optional; fallback is type_uint64().  */
     void (*type_size)(Visitor *v, const char *name, uint64_t *obj,
                       Error **errp);
+
     /* Must be set. */
     void (*type_bool)(Visitor *v, const char *name, bool *obj, Error **errp);
+    /* Must be set */
     void (*type_str)(Visitor *v, const char *name, char **obj, Error **errp);
+
+    /* Must be provided to visit numbers (the opts visitor does not
+     * currently visit non-integers). */
     void (*type_number)(Visitor *v, const char *name, double *obj,
                         Error **errp);
+    /* Must be provided to visit arbitrary QTypes (the opts and string
+     * visitors do not currently visit arbitrary types).  */
     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);
 };

+/**
+ * A generic visitor.type_enum suitable for input visitors.
+ */
 void input_type_enum(Visitor *v, const char *name, int *obj,
                      const char *const strings[], Error **errp);
+/**
+ * A generic visitor.type_enum suitable for output visitors.
+ */
 void output_type_enum(Visitor *v, const char *name, int *obj,
                       const char *const strings[], Error **errp);

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 10390d2..5349a64 100644
--- a/include/qapi/visitor.h
+++ 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).  An input visitor converts from
+ * some other form into QAPI representation; an output visitor
+ * converts from QAPI back into another form.  In the descriptions
+ * below, an object or dictionary refers to a JSON '{}', and a list
+ * refers to a JSON '[]'.  These functions seldom need to be called
+ * directly, but are instead used by code generated by
+ * scripts/qapi-visit.py.  For the visitor callback contracts, see
+ * visitor-impl.h.
+ */
+
+/* This struct is layout-compatible with all other *List structs
+ * created by the qapi generator. */
 typedef struct GenericList
 {
     union {
@@ -27,15 +41,101 @@  typedef struct GenericList
     struct GenericList *next;
 } GenericList;

+/**
+ * Prepare to visit an object tied to key @name.
+ * @name will be NULL if this is visited as part of a list.  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.
+ *
+ * @obj can be NULL (in which case @size should also be 0) to indicate
+ * that there is no qapi C struct, and that the upcoming visit calls
+ * are parsing input to or creating output from some other
+ * representation.
+ *
+ * If @obj is not NULL, then input visitors malloc a qapi struct of
+ * @size bytes into *@obj on success, and output visitors expect *@obj
+ * to be a fully-populated qapi struct.
+ *
+ * Set *@errp on failure; for example, if the input stream does not
+ * have a member @name or if the member is not an object.
+ *
+ * FIXME: For input visitors, *@obj can be assigned here even if later
+ * visits will fail; this can lead to memory leaks if clients aren't
+ * careful.
+ */
 void visit_start_struct(Visitor *v, const char *name, void **obj,
                         size_t size, Error **errp);
+/**
+ * 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.
+ */
 void visit_end_struct(Visitor *v, Error **errp);
+
+/**
+ * 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.
+ *
+ * @obj must not be NULL, since this function is only called when
+ * visiting with qapi structs.
+ *
+ * FIXME: For input visitors, *@obj can be assigned here even if later
+ * visits will fail; this can lead to memory leaks if clients aren't
+ * careful.
+ */
 void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
                                  Error **errp);
+/**
+ * Complete an implicit struct visit started earlier.
+ * Must be called after any successful use of visit_start_implicit_struct(),
+ * even if intermediate processing was skipped due to errors, to allow
+ * the backend to release any resources.  Unlike visit_end_struct(), this
+ * does not need to check for errors (detection of unused keys is only
+ * possible for the overall struct, not a subset).
+ */
 void visit_end_implicit_struct(Visitor *v);

+/**
+ * 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.
+ */
 void visit_start_list(Visitor *v, const char *name, Error **errp);
+/**
+ * Iterate over a GenericList during a list visit.
+ * @list must not be NULL; on the first call, @list contains the
+ * address of the list head, and on subsequent calls *@list must be
+ * the previously returned value.  Must be called in a loop until a
+ * NULL return or error occurs; for each non-NULL return, the caller
+ * must then call the appropriate visit_type_*() for the element type
+ * of the list, with that function's name parameter set to NULL.
+ *
+ * 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.
+ *
+ * FIXME: For input visitors, *@list can be assigned here even if
+ * later visits will fail; this can lead to memory leaks if clients
+ * aren't careful.
+ */
 GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
+/**
+ * Complete the list started earlier.
+ * Must be called after any successful use of visit_start_list(),
+ * even if intermediate processing was skipped due to errors, to allow
+ * the backend to release any resources.
+ */
 void visit_end_list(Visitor *v);

 /**
@@ -54,32 +154,128 @@  bool visit_optional(Visitor *v, const char *name, bool *present);
  */
 void visit_get_next_type(Visitor *v, const char *name, QType *type,
                          bool promote_int, Error **errp);
+
+/**
+ * Visit an enum value tied to @name in the current object visit.
+ * @name will be NULL if this is visited as part of a list.
+ * For input visitors, parse a string and set *@obj to the numeric
+ * value of the enum type using @strings as the mapping, leaving @obj
+ * unchanged on error; for output visitors, reverse the mapping and
+ * visit the output string determined by *@obj.
+ */
 void visit_type_enum(Visitor *v, const char *name, int *obj,
                      const char *const strings[], Error **errp);
+
+/**
+ * Visit an integer value tied to @name in the current object visit.
+ * @name will be NULL if this is visited as part of a list.
+ * For input visitors, set *@obj to the parsed value; for other visitors,
+ * leave *@obj unchanged.
+ */
 void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp);
+/**
+ * Visit a uint8_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to uint8_t range.
+ */
 void visit_type_uint8(Visitor *v, const char *name, uint8_t *obj,
                       Error **errp);
+/**
+ * Visit a uint16_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to uint16_t range.
+ */
 void visit_type_uint16(Visitor *v, const char *name, uint16_t *obj,
                        Error **errp);
+/**
+ * Visit a uint32_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to uint32_t range.
+ */
 void visit_type_uint32(Visitor *v, const char *name, uint32_t *obj,
                        Error **errp);
+/**
+ * Visit a uint64_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to uint64_t range
+ * (that is, ensures it is unsigned).
+ */
 void visit_type_uint64(Visitor *v, const char *name, uint64_t *obj,
                        Error **errp);
+/**
+ * Visit an int8_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to int8_t range.
+ */
 void visit_type_int8(Visitor *v, const char *name, int8_t *obj, Error **errp);
+/**
+ * Visit an int16_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to int16_t range.
+ */
 void visit_type_int16(Visitor *v, const char *name, int16_t *obj,
                       Error **errp);
+/**
+ * Visit an int32_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to int32_t range.
+ */
 void visit_type_int32(Visitor *v, const char *name, int32_t *obj,
                       Error **errp);
+/**
+ * Visit an int64_t value tied to @name in the current object visit.
+ * Identical to visit_type_int().
+ */
 void visit_type_int64(Visitor *v, const char *name, int64_t *obj,
                       Error **errp);
+/**
+ * Visit a uint64_t value tied to @name in the current object visit.
+ * Like visit_type_uint64(), except that some visitors may choose to
+ * recognize additional suffixes for easily scaling input values.
+ */
 void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
                      Error **errp);
+
+/**
+ * Visit a boolean value tied to @name in the current object visit.
+ * @name will be NULL if this is visited as part of a list.
+ * Input visitors set *@obj to the value; other visitors will leave
+ * *@obj unchanged.
+ */
 void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp);
+
+/**
+ * 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.  Input visitors set *@obj to the parsed
+ * string (never NULL); while output visitors leave *@obj unchanged,
+ * except that a NULL *@obj will be treated the same as "".
+ *
+ * FIXME: Unfortunately not const-correct for output visitors.
+ * FIXME: Callers that try to output NULL *obj should not be allowed.
+ */
 void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp);
+
+/**
+ * Visit a number value tied to @name in the current object visit.
+ * @name will be NULL if this is visited as part of a list.
+ * Input visitors set *@obj to the value; other visitors will leave
+ * *@obj unchanged.
+ */
 void visit_type_number(Visitor *v, const char *name, double *obj,
                        Error **errp);
+
+/**
+ * Visit an arbitrary qtype value tied to @name in the current object visit.
+ * @name will be NULL if this is visited as part of a list.
+ * Input visitors set *@obj to the value; other visitors will leave
+ * *@obj (which must not be NULL) unchanged.
+ */
 void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp);
+
+/**
+ * Mark the start of visiting the branches of a union. Return true if
+ * @data_present.
+ * FIXME: Should not be needed
+ */
 bool visit_start_union(Visitor *v, bool data_present, Error **errp);
+/**
+ * Mark the end of union branches, after visit_start_union().
+ * FIXME: Should not be needed
+ */
 void visit_end_union(Visitor *v, bool data_present, Error **errp);

 #endif
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 2d3743b..1612d0d 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -18,9 +18,21 @@ 
 #include "qapi/visitor.h"
 #include "qapi/visitor-impl.h"

+/* Determine if this is an output visitor.
+ * Useful for making some tighter assertions that hold for output
+ * visitors, but not for input or dealloc visitors. */
+static bool visit_is_output(Visitor *v)
+{
+    return v->type_enum == output_type_enum;
+}
+
 void visit_start_struct(Visitor *v, const char *name, void **obj,
                         size_t size, Error **errp)
 {
+    assert(obj ? size : !size);
+    if (obj && visit_is_output(v)) {
+        assert(*obj);
+    }
     v->start_struct(v, name, obj, size, errp);
 }

@@ -32,6 +44,10 @@  void visit_end_struct(Visitor *v, Error **errp)
 void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
                                  Error **errp)
 {
+    assert(obj && size);
+    if (visit_is_output(v)) {
+        assert(*obj);
+    }
     if (v->start_implicit_struct) {
         v->start_implicit_struct(v, obj, size, errp);
     }
@@ -51,6 +67,7 @@  void visit_start_list(Visitor *v, const char *name, Error **errp)

 GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp)
 {
+    assert(list);
     return v->next_list(v, list, errp);
 }

@@ -85,6 +102,7 @@  bool visit_optional(Visitor *v, const char *name, bool *present)
 void visit_get_next_type(Visitor *v, const char *name, QType *type,
                          bool promote_int, Error **errp)
 {
+    assert(type);
     if (v->get_next_type) {
         v->get_next_type(v, name, type, promote_int, errp);
     }
@@ -93,11 +111,13 @@  void visit_get_next_type(Visitor *v, const char *name, QType *type,
 void visit_type_enum(Visitor *v, const char *name, int *obj,
                      const char *const strings[], Error **errp)
 {
+    assert(obj && strings);
     v->type_enum(v, name, obj, strings, errp);
 }

 void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp)
 {
+    assert(obj);
     v->type_int64(v, name, obj, errp);
 }

@@ -145,6 +165,7 @@  void visit_type_uint32(Visitor *v, const char *name, uint32_t *obj,
 void visit_type_uint64(Visitor *v, const char *name, uint64_t *obj,
                        Error **errp)
 {
+    assert(obj);
     v->type_uint64(v, name, obj, errp);
 }

@@ -192,12 +213,14 @@  void visit_type_int32(Visitor *v, const char *name, int32_t *obj,
 void visit_type_int64(Visitor *v, const char *name, int64_t *obj,
                       Error **errp)
 {
+    assert(obj);
     v->type_int64(v, name, obj, errp);
 }

 void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
                      Error **errp)
 {
+    assert(obj);
     if (v->type_size) {
         v->type_size(v, name, obj, errp);
     } else {
@@ -207,22 +230,35 @@  void visit_type_size(Visitor *v, const char *name, uint64_t *obj,

 void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp)
 {
+    assert(obj);
     v->type_bool(v, name, obj, errp);
 }

 void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp)
 {
+    assert(obj);
+    /* TODO: Fix callers to not pass NULL when they mean "", so that we
+     * can enable:
+    if (visit_is_output(v)) {
+        assert(*obj);
+    }
+     */
     v->type_str(v, name, obj, errp);
 }

 void visit_type_number(Visitor *v, const char *name, double *obj,
                        Error **errp)
 {
+    assert(obj);
     v->type_number(v, name, obj, errp);
 }

 void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp)
 {
+    assert(obj);
+    if (visit_is_output(v)) {
+        assert(*obj);
+    }
     v->type_any(v, name, obj, errp);
 }

@@ -233,7 +269,6 @@  void output_type_enum(Visitor *v, const char *name, int *obj,
     int value = *obj;
     char *enum_str;

-    assert(strings);
     while (strings[i++] != NULL);
     if (value < 0 || value >= i - 1) {
         error_setg(errp, QERR_INVALID_PARAMETER, name ? name : "null");
@@ -251,8 +286,6 @@  void input_type_enum(Visitor *v, const char *name, int *obj,
     int64_t value = 0;
     char *enum_str;

-    assert(strings);
-
     visit_type_str(v, name, &enum_str, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);