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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> The visitor interface for mapping between QObject/QemuOpts/string
> and QAPI is scandalously under-documented, making changes to visitor
> core, individual visitors, and users of visitors difficult to
> coordinate.  Among other questions: when is it safe to pass NULL,
> vs. when a string must be provided; which visitors implement which
> callbacks; the difference between concrete and virtual visits.
>
> Correct this by retrofitting proper contracts, and document where some
> of the interface warts remain (for example, we may want to modify
> visit_end_* to require the same 'obj' as the visit_start counterpart,
> so the dealloc visitor can be simplified).  Later patches in this
> series will tackle some, but not all, of these warts.
>
> Add assertions to (partially) enforce the contract.  Some of these
> were only made possible by recent cleanup commits.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v14: rebase to master context
> v13: minor wording tweaks for consistency
> v12: major rework based on Markus' comments, drop R-b
> [no v10, v11]
> 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.h               | 433 
> +++++++++++++++++++++++++++++++++--
>  include/qapi/visitor-impl.h          |  42 +++-
>  include/qapi/dealloc-visitor.h       |   4 +
>  include/qapi/opts-visitor.h          |   4 +
>  include/qapi/qmp-input-visitor.h     |   8 +
>  include/qapi/string-input-visitor.h  |   4 +
>  include/qapi/string-output-visitor.h |   4 +
>  qapi/qapi-visit-core.c               |  19 +-
>  8 files changed, 494 insertions(+), 24 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 9a8d010..faf67d2 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -16,8 +16,185 @@
>
>  #include "qapi/qmp/qobject.h"
>
> +/*
> + * The QAPI schema defines both a set of C data types, and a QMP wire
> + * format.  A QAPI object is formed as a directed acyclic graph of
> + * QAPI values.

I understand what you're trying to say, but I find the value / object
dichotomy odd.  For me, A QAPI object isn't a DAG, it's a node in a DAG.
Perhaps: "QAPI objects can contain references to other QAPI objects,
resulting in a directed acyclic graph."

>                   QAPI also generates visitor functions to walk these
> + * graphs.  This file represents the interface for doing work at each
> + * point of a QAPI graph; it can also be used for a virtual walk,

Point?  Common graph terminology would be node or edge.

> + * where there is no actual QAPI C struct.
> + *
> + * There are three kinds of visitor classes: input visitors parse an
> + * external representation and allocate the corresponding QAPI graph

"Create" or "build"?  We're not merely allocating...

> + * (QMP, string, and QemuOpts),

The parenthesis applies "external representation, not to "QAPI graph".
Move it there?

> + *                              output visitors take a completed QAPI
> + * graph and generate an external representation (QMP and string), and
> + * the dealloc visitor can take a (partial) QAPI graph and recursively

Suggest (possibly partially constructed).

> + * free its resources.  While the dealloc and QMP input/output
> + * visitors are general, the string and QemuOpts visitors have some
> + * implementation limitations; see the documentation for each visitor
> + * for more details on what it supports.  Also, see visitor-impl.h for
> + * the callback contracts implemented by each visitor, and
> + * docs/qapi-code-gen.txt for more about the QAPI code generator.

Nice directory of resources.

> + *
> + * All QAPI types have a corresponding function with a signature
> + * roughly compatible with this:
> + *
> + * void visit_type_FOO(Visitor *v, const char *name, void *obj, Error 
> **errp);
> + *
> + * except that address@hidden is typed correctly as a pointer or scalar,
> + * depending on the type of FOO.

Perhaps we could say

    * void visit_type_FOO(Visitor *v, const char *name, T *obj, Error **errp);
    *
    * where T is FOO for scalar FOOs, and FOO * otherwise.

>                                    The scalar visitors are declared
> + * here; the remaining visitors are generated in qapi-visit.h.
> + *
> + * The @name parameter of visit_type_FOO() describes the relation
> + * between this QAPI value and its parent container.  When visiting
> + * the root of a tree, @name is usually ignored (although some
> + * visitors require it to be NULL);

The inconsistent behavior at roots is a bit of a wart.  Is it addressed
later in the series?  Shoult we call out the wartiness here?

> +                                    when visiting a member of an
> + * object, @name is the key associated with the value; and when
> + * visiting a member of a list, @name is NULL.
> + *
> + * The visit_type_FOO() functions expect a non-NULL @obj argument;

I'd spell it non-null.

> + * they allocate address@hidden during input visits, leave it unchanged on
> + * output visits, and recursively free any resources during a dealloc
> + * visit.  Each function also has an @errp argument which may be NULL
> + * to ignore errors, or point to a NULL Error object on entry for
> + * reporting any errors (such as if a member @name is not present, or
> + * is present but not the specified type).

We don't normally explain the Error ** parameter in any detail, unless
it deviates from common usage (which it really shouldn't).  Perhaps:
"Each function also takes the customary @errp argument.  See
qapi/error.h for details."

> + *
> + * FIXME: Clients must pass NULL for @name when visiting a member of a
> + * list, but this leads to poor error messages; it might be nicer to
> + * require a non-NULL name such as "key.0" for '{ "key": [ "value" ]
> + * }' if an error is encountered on "value" (or to have the visitor
> + * core auto-generate the nicer name).
> + *
> + * FIXME: At present, input visitors may allocate an incomplete 
> address@hidden
> + * even when visit_type_FOO() reports an error.  Using an output
> + * visitor with an incomplete object has undefined behavior; callers
> + * must call qapi_free_FOO() (which uses the dealloc visitor, and
> + * safely handles an incomplete object) to avoid a memory leak.
> + *
> + * Likewise, the QAPI object types (structs, unions, and alternates)
> + * have a generated function in qapi-visit.h compatible with:
> + *
> + * void visit_type_FOO_members(Visitor *v, FOO *obj, Error **errp);

This confused me briefly, until I spotted the _members part.  Perhaps we
can better prime the reader:

    * For QAPI object types (structs, unions, and alternates), we
    * additionally have a generated function in qapi-visit.h compatible
    * with

> + *
> + * for visiting the members of a type without also allocating the QAPI
> + * struct.
> + *
> + * Additionally, in qapi-types.h, all QAPI pointer types (structs,
> + * unions, alternates, and lists) have a generated function compatible
> + * with:
> + *
> + * void qapi_free_FOO(FOO *obj);
> + *
> + * which behaves like free() (@obj can be NULL);

Golden opportunity to terminate the already long sentence with a period
;)

>                                                   because of these
> + * functions, the dealloc visitor is seldom used directly outside of
> + * generated code.  QAPI types can also inherit from a base class;
> + * when this happens, a function is generated for easily going from
> + * the derived type to the base type:
> + *
> + * BASE *qapi_CHILD_base(CHILD *obj);
> + *
> + * For a real QAPI struct, a typical input life cycle is thus:

"Life cycle" suggests the example ranges from creation to destruction.

> + *
> + * <example>
> + *  Foo *f;
> + *  FooList *l;
> + *  Error *err = NULL;
> + *  Visitor *v;
> + *
> + *  v = ...obtain input visitor...
> + *  visit_type_Foo(v, NULL, &f, &err);
> + *  if (err) {
> + *      qapi_free_Foo(f);
> + *      ...handle error...
> + *  } else {
> + *      ...use f...
> + *  }

... clean up v ... missing?

The example stops before destruction.  You can either append
qapi_free_Foo() to it, or rephrase the introduction not to claim "life
cycle".  Same for list below.

Suggest to split the example right here:

    * </example>
    *
    * For a list, it's:
    * <example>
    *  FooList *l;
    *  Error *err = NULL;
    *  Visitor *v;
    *
> + *  v = ...obtain input visitor...
> + *  visit_type_FooList(v, NULL, &l, &err);
> + *  if (err) {
> + *      qapi_free_FooList(l);
> + *      ...handle error...
> + *  } else {
> + *      while (l) {

I like to collect sufficiently simple loop control in one place, like this:

           for (; l; l->next) {

> + *          ...use l->value...
> + *          l = l->next;
> + *      }
> + *  }
> + *  ...clean up v...
> + * </example>
> + *
> + * Similarly, a typical output life cycle is:

Here, a full "life cycle" makes the example needlessly artificial,
because...

> + *
> + * <example>
> + *  Foo *f = g_new0(Foo, 1);

... you have to stub out creation here (a /* stub */ comment would make
sense), and ...

> + *  Error *err = NULL;
> + *  Visitor *v;
> + *
> + *  ...populate f...
> + *  v = ...obtain output visitor...
> + *  visit_type_Foo(v, NULL, &f, &err);
> + *  if (err) {
> + *      ...handle error...
> + *  }
> + *  qapi_free_Foo(f);

... you have to have a qapi_free_Foo(f) here.  In contrast, common
output visitor use neither creates nor frees, it simply receives an
object as argument.

... clean up v ... missing?

> + * </example>
> + *
> + * When visiting a real QAPI struct, this file provides several
> + * helpers that rely on in-tree information to control the walk:
> + * visit_optional() for the 'has_member' field associated with
> + * optional 'member' in the C struct; and visit_next_list() for
> + * advancing through a FooList linked list.  Only the generated
> + * visit_type functions need to use these helpers.
> + *
> + * It is also possible to use the visitors to do a virtual walk, where
> + * no actual QAPI struct is present.  In this situation, decisions
> + * about what needs to be walked are made by the calling code, and
> + * structured visits are split between pairs of start and end methods
> + * (where the end method must be called if the start function
> + * succeeded, even if an intermediate visit encounters an error).
> + * Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks
> + * like:
> + *
> + * <example>
> + *  Visitor *v;
> + *  Error *err = NULL;
> + *  int value;
> + *
> + *  v = ...obtain visitor...
> + *  visit_start_struct(v, NULL, NULL, 0, &err);
> + *  if (err) {
> + *      goto outobj;
> + *  }
> + *  visit_start_list(v, "list", &err);
> + *  if (err) {
> + *      goto outobj;
> + *  }
> + *  value = 1;
> + *  visit_type_int(v, NULL, &value, &err);
> + *  if (err) {
> + *      goto outlist;
> + *  }
> + *  value = 2;
> + *  visit_type_int(v, NULL, &value, &err);
> + *  if (err) {
> + *      goto outlist;
> + *  }
> + * outlist:
> + *  visit_end_list(v);
> + * outobj:
> + *  error_propagate(errp, err);
> + *  err = NULL;
> + *  visit_end_struct(v, &err);
> + *  error_propagate(errp, err);
> + *  ...clean up v...
> + * </example>
> + */
> +
> +/* === Useful types */
> +

Structuring the header with headlines can be useful, but I haven't seen
the /* === Headline text */ decoration style in QEMU.  We have a few
occurences of /*** Headline text ***/.

>  /* This struct is layout-compatible with all other *List structs
> - * created by the qapi generator.  It is used as a typical
> + * created by the QAPI generator.  It is used as a typical
>   * singly-linked list. */
>  typedef struct GenericList {
>      struct GenericList *next;
> @@ -25,26 +202,117 @@ typedef struct GenericList {
>  } GenericList;
>
>  /* This struct is layout-compatible with all Alternate types
> - * created by the qapi generator. */
> + * created by the QAPI generator. */
>  typedef struct GenericAlternate {
>      QType type;
>      char padding[];
>  } GenericAlternate;
>
> +/* === Visiting structures */
> +
> +/*
> + * Start visiting an object value @obj (struct or union).

"value" feels redundant.

> + *
> + * @name expresses the relationship of this object to its parent
> + * container; see the general description of @name above.
> + *
> + * @obj must be non-NULL for a real walk, in which case @size
> + * determines how much memory an input visitor will allocate into
> + * address@hidden  @obj may also be NULL for a virtual walk, in which case
> + * @size is ignored.
> + *
> + * @errp must be NULL-initialized,

Not really.  It may be NULL (nothing to initialize), &error_abort
(happens to contain null, but that doesn't matter), &error_fatal
(likewise).  Suggest to drop "must be NULL-initialized," and rely on the
common @errp rules implicitly.

More of the same below.

>                                     and is set if an error is detected
> + * (such as if a member @name is not present, or is present but not an
> + * object).  On error, input visitors set address@hidden to NULL.
> + *
> + * 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, even if intermediate visits fail.  See the examples
> + * above.
> + *
> + * FIXME Should this be named visit_start_object, since it is also
> + * used for QAPI unions, and maps to JSON objects?
> + */
>  void visit_start_struct(Visitor *v, const char *name, void **obj,
>                          size_t size, Error **errp);
> +
> +/*
> + * Complete an object visit started earlier.
> + *
> + * @errp must be NULL-initialized, and is set if an error is detected
> + * (such as unparsed keys remaining in the input stream).
> + *
> + * 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.  Destroying the visitor may
> + * behave as if this was implicitly called.

"may" is of course pretty useless :)  Do we intend to nail it down one
way or the other?

> + */
>  void visit_end_struct(Visitor *v, Error **errp);
>
> +
> +/* === Visiting lists */
> +
> +/*
> + * Start visiting a list.
> + *
> + * @name expresses the relationship of this list to its parent
> + * container; see the general description of @name above.
> + *
> + * @errp must be NULL-initialized, and is set if an error is detected
> + * (such as if a member @name is not present, or is present but not a
> + * list).
> + *
> + * After visit_start_list() succeeds, the caller may visit its members
> + * one after the other.  A real visit uses visit_next_list() for
> + * traversing the linked list, while a virtual visit uses other means.
> + * For each list element, call the appropriate visit_type_FOO() with
> + * name set to NULL and obj set to the address of the value member of
> + * the list element.  Finally, visit_end_list() needs to be called to
> + * clean up, even if intermediate visits fail.  See the examples
> + * above.
> + */
>  void visit_start_list(Visitor *v, const char *name, Error **errp);
> +
> +/*
> + * Iterate over a GenericList during a list visit.
> + *
> + * @size represents the size of a linked list node.
> + *
> + * @list must not be NULL; on the first call, @list contains the
> + * address of the list head, and on subsequent calls address@hidden 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

Does anything bad happen if you elect to stop the loop before NULL
return or error?

If no, then the two last "must" are misleading.

> + * of the list, with that function's name parameter set to NULL and
> + * obj set to the address of (address@hidden)->value.
> + *
> + * FIXME: This interface is awkward; it requires all callbacks to
> + * track whether it is the first or a subsequent call.  A better
> + * interface would pass the head of the list through
> + * visit_start_list().
> + */
>  GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size);
> +
> +/*
> + * Complete a list visit 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.  Destroying the visitor may
> + * behave as if this was implicitly called.

Remark on visit_end_struct()'s "may" applies.

> + */
>  void visit_end_list(Visitor *v);
>
> +
> +/* === Visiting alternates */
> +
>  /*
> - * Start the visit of an alternate @obj with the given @size.
> + * Start the visit of an alternate @obj with the given @size (at least
> + * sizeof(GenericAlternate)).

I like to limit the first paragraph of the contract to a single line.
Easy enough here: document @size further down, like you do for
visit_start_struct().

When is @size used?  In visit_start_struct(), it's used only by an input
visitor with non-null @obj.

>   *
> - * @name specifies the relationship to the containing struct (ignored
> - * for a top level visit, the name of the key if this alternate is
> - * part of an object, or NULL if this alternate is part of a list).
> + * @name expresses the relationship of this alternate to its parent
> + * container; see the general description of @name above.
>   *
>   * @obj must not be NULL. Input visitors will allocate @obj and
>   * determine the qtype of the next thing to be visited, stored in
> @@ -52,8 +320,8 @@ void visit_end_list(Visitor *v);
>   *
>   * If @promote_int, treat integers as QTYPE_FLOAT.
>   *
> - * If successful, this must be paired with visit_end_alternate(), even
> - * if visiting the contents of the alternate fails.
> + * If successful, this must be paired with visit_end_alternate() to
> + * clean up, even if visiting the contents of the alternate fails.
>   */
>  void visit_start_alternate(Visitor *v, const char *name,
>                             GenericAlternate **obj, size_t size,
> @@ -62,46 +330,181 @@ void visit_start_alternate(Visitor *v, const char *name,
>  /*
>   * Finish visiting an alternate type.
>   *
> - * Must be called after a successful visit_start_alternate(), even if
> - * an error occurred in the meantime.
> + * Must be called after any successful use of visit_start_alternate(),
> + * even if intermediate processing was skipped due to errors, to allow
> + * the backend to release any resources.  Destroying the visitor may
> + * behave as if this was implicitly called.

Remark on visit_end_struct()'s "may" applies.

>   *
>   * TODO: Should all the visit_end_* interfaces take obj parameter, so
>   * that dealloc visitor need not track what was passed in visit_start?
>   */
>  void visit_end_alternate(Visitor *v);
>
> -/**
> - * Check if an optional member @name of an object needs visiting.
> - * For input visitors, set address@hidden according to whether the
> - * corresponding visit_type_*() needs calling; for other visitors,
> - * leave address@hidden unchanged.  Return address@hidden for convenience.
> +
> +/* === Other helpers */
> +
> +/*
> + * Does optional struct member @name need visiting?
> + *
> + * @name must not be NULL.  This function is only useful between
> + * visit_start_struct() and visit_end_struct(), since only objects
> + * have optional keys.
> + *
> + * @present points to the address of the optional member's has_ flag.
> + *
> + * Input visitors set address@hidden according to input; other visitors
> + * leave it unchanged.  In either case, return address@hidden for
> + * convenience.
>   */
>  bool visit_optional(Visitor *v, const char *name, bool *present);
>
> +/*
> + * Visit an enum value.
> + *
> + * @name expresses the relationship of this enum to its parent
> + * container; see the general description of @name above.
> + *
> + * @strings expresses the mapping between C enum values and QAPI enum
> + * names; it should be the ENUM_lookup array from visit-types.h.
> + *
> + * @obj must be non-NULL.  For input visitors, parse a string and set
> + * address@hidden to the numeric value of the enum type using @strings as the
> + * mapping, leaving @obj unchanged on error; other visitors leave
> + * address@hidden unchanged.  Output visitors use address@hidden to reverse 
> the mapping
> + * and visit the appropriate output string.

Our current input visitors' input and our output visitor's output is
text, but that's detail.  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.  Except we can't anymore
since PATCH 02 baked the text assumption into the visitor core.  Hmm.

I guess baking it into the core is okay.  If we ever need more, we can
bring back the type_enum() callback.

Suggest to relax the contract a bit:

    * @obj must be non-NULL.  Input visitors parse input and set address@hidden
    * to the enumeration value, leaving @obj unchanged on error; other
    * visitors leave address@hidden unchanged.  Output visitors use 
address@hidden
    *
    * Currently, all input visitors parse text input, and all output
    * visitors produce text output.  The mapping between enumeration
    * values and strings is done by the visitor core, using @strings.
    * The core calls visit_type_str() to read / write the strings.

> + */
>  void visit_type_enum(Visitor *v, const char *name, int *obj,
>                       const char *const strings[], Error **errp);
> +
> +
> +/* === Visiting built-in types */
> +
> +/*
> + * Visit an integer value.
> + *
> + * @name expresses the relationship of this integer to its parent
> + * container; see the general description of @name above.
> + *
> + * @obj must be non-NULL.  Input visitors set address@hidden to the value;
> + * other visitors will leave address@hidden unchanged.
> + */
>  void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error 
> **errp);
> +
> +/*
> + * Visit a uint8_t value.
> + * 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.
> + * 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.
> + * 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.
> + * 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.
> + * 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.
> + * 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.
> + * 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.
> + * Identical to visit_type_int().
> + */
>  void visit_type_int64(Visitor *v, const char *name, int64_t *obj,
>                        Error **errp);
> +
> +/*
> + * Visit a uint64_t value.
> + * Like visit_type_uint64(), except that some visitors may choose to
> + * recognize additional syntax, such as suffixes for easily scaling
> + * values.
> + */
>  void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
>                       Error **errp);
> +
> +/*
> + * Visit a boolean value.
> + *
> + * @name expresses the relationship of this boolean to its parent
> + * container; see the general description of @name above.
> + *
> + * @obj must be non-NULL.  Input visitors set address@hidden to the value;
> + * other visitors will leave address@hidden unchanged.
> + */
>  void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp);
> +
> +/*
> + * Visit a string value.
> + *
> + * @name expresses the relationship of this string to its parent
> + * container; see the general description of @name above.
> + *
> + * @obj must be non-NULL.  Input visitors set address@hidden to the value
> + * (never NULL).  Other visitors leave address@hidden unchanged, and commonly
> + * treat NULL like "".
> + *
> + * Note that using an output visitor along with a (const char *) value
> + * requires casting away const when computing @obj.

I'm pretty sure the user can figure that out himself with the compiler's
help :)

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

Suggest "a number (i.e. double) value"

> + *
> + * @name expresses the relationship of this number to its parent
> + * container; see the general description of @name above.
> + *
> + * @obj must be non-NULL.  Input visitors set address@hidden to the value;
> + * other visitors will leave address@hidden unchanged.
> + */
>  void visit_type_number(Visitor *v, const char *name, double *obj,
>                         Error **errp);
> +
> +/*
> + * Visit an arbitrary value.
> + *
> + * @name expresses the relationship of this value to its parent
> + * container; see the general description of @name above.
> + *
> + * @obj must be non-NULL.  Input visitors set address@hidden to the value;
> + * other visitors will leave address@hidden unchanged.  address@hidden must 
> be non-NULL
> + * for output visitors.
> + */
>  void visit_type_any(Visitor *v, const char *name, QObject **obj, Error 
> **errp);
>
>  #endif
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 228a2a6..d967b18 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -14,6 +14,16 @@
>
>  #include "qapi/visitor.h"
>
> +/* This file describes the callback interface for implementing a QAPI
> + * 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.  A callback's contract
> + * matches the corresponding public functions' contract unless stated
> + * otherwise.  In the comments below, some callbacks are marked "must
> + * be set for $TYPE visits to work"; if a visitor implementation omits
> + * that callback, it should also document that it is only useful for a
> + * subset of QAPI.  */
> +
>  /* There are three classes of visitors; setting the class determines
>   * how QAPI enums are visited, as well as what additional restrictions
>   * can be asserted.  */
> @@ -25,18 +35,24 @@ typedef enum VisitorType {
>
>  struct Visitor
>  {
> -    /* Must be set */
> +    /* Must be set to visit structs.  */

I actually prefer to do short one line comments as phrases, not
sentences, i.e. without a period at the end.

>      void (*start_struct)(Visitor *v, const char *name, void **obj,
>                           size_t size, Error **errp);
> +
> +    /* Must be set to visit structs.  */
>      void (*end_struct)(Visitor *v, Error **errp);
>
> +    /* Must be set.  */

I guess we say /* Must be set to visit FOOs */ when at least one visitor
doesn't implement it.  Else we say /* Must be set */.  Correct?

>      void (*start_list)(Visitor *v, const char *name, Error **errp);
> -    /* Must be set */
> +
> +    /* Must be set.  */
>      GenericList *(*next_list)(Visitor *v, GenericList **list, size_t size);
> -    /* Must be set */
> +
> +    /* Must be set.  */
>      void (*end_list)(Visitor *v);
>
> -    /* Optional, needed for input and dealloc visitors.  */
> +    /* Must be set by input and dealloc visitors to visit alternates;
> +     * optional for output visitors.  */
>      void (*start_alternate)(Visitor *v, const char *name,
>                              GenericAlternate **obj, size_t size,
>                              bool promote_int, Error **errp);
> @@ -44,24 +60,34 @@ struct Visitor
>      /* Optional, needed for dealloc visitor.  */
>      void (*end_alternate)(Visitor *v);
>
> -    /* Must be set. */
> +    /* Must be set.  */
>      void (*type_int64)(Visitor *v, const char *name, int64_t *obj,
>                         Error **errp);
> -    /* Must be set. */
> +
> +    /* Must be set.  */
>      void (*type_uint64)(Visitor *v, const char *name, uint64_t *obj,
>                          Error **errp);
> +
>      /* Optional; fallback is type_uint64().  */
>      void (*type_size)(Visitor *v, const char *name, uint64_t *obj,
>                        Error **errp);
> -    /* Must be set. */
> +
> +    /* 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 set to visit numbers.  */
>      void (*type_number)(Visitor *v, const char *name, double *obj,
>                          Error **errp);
> +
> +    /* Must be set to visit arbitrary QTypes.  */
>      void (*type_any)(Visitor *v, const char *name, QObject **obj,
>                       Error **errp);
>
> -    /* May be NULL; most useful for input visitors. */
> +    /* Must be set for input visitors, optional otherwise.  The core
> +     * takes care of the return type in the public interface.  */
>      void (*optional)(Visitor *v, const char *name, bool *present);
>
>      /* Must be set.  */
> diff --git a/include/qapi/dealloc-visitor.h b/include/qapi/dealloc-visitor.h
> index cf4c36d..7aa8293 100644
> --- a/include/qapi/dealloc-visitor.h
> +++ b/include/qapi/dealloc-visitor.h
> @@ -18,6 +18,10 @@
>
>  typedef struct QapiDeallocVisitor QapiDeallocVisitor;
>
> +/* The dealloc visitor is primarly used only by generated
> + * qapi_free_FOO() functions, and is the only visitor designed to work
> + * correctly in the face of a partially-constructed QAPI tree.
> + */

Wing your winged comments at both ends, please.

>  QapiDeallocVisitor *qapi_dealloc_visitor_new(void);
>  void qapi_dealloc_visitor_cleanup(QapiDeallocVisitor *d);
>
> diff --git a/include/qapi/opts-visitor.h b/include/qapi/opts-visitor.h
> index fd48c14..2002e37 100644
> --- a/include/qapi/opts-visitor.h
> +++ b/include/qapi/opts-visitor.h
> @@ -29,6 +29,10 @@ typedef struct OptsVisitor OptsVisitor;
>   * - string representations of negative numbers yield negative values,
>   * - values below INT64_MIN or LLONG_MIN are rejected,
>   * - values above INT64_MAX or LLONG_MAX are rejected.
> + *
> + * The Opts input visitor does not yet implement support for visiting
> + * QAPI alternates, numbers (other than integers), or arbitrary
> + * QTypes.

"Yet" suggests this is likely to change.  I doubt it is.

>   */
>  OptsVisitor *opts_visitor_new(const QemuOpts *opts);
>  void opts_visitor_cleanup(OptsVisitor *nv);
> diff --git a/include/qapi/qmp-input-visitor.h 
> b/include/qapi/qmp-input-visitor.h
> index 3ed499c..d75ff98 100644
> --- a/include/qapi/qmp-input-visitor.h
> +++ b/include/qapi/qmp-input-visitor.h
> @@ -19,6 +19,14 @@
>
>  typedef struct QmpInputVisitor QmpInputVisitor;
>
> +/*
> + * FIXME: When visiting a QDict, passing a non-NULL @name for the
> + * first visit_type_FOO() when the root is a QDict will find that
> + * particular key within the QDict.  In the future, the contract may
> + * be tightened to require visit_start_struct() with ignored @name as
> + * the first visit; in the meantime, the first visit is safest when
> + * using NULL for @name.
> + */

Can't grok this right now.  I guess I'm getting tired...

>  QmpInputVisitor *qmp_input_visitor_new(QObject *obj);
>  QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj);
>
> diff --git a/include/qapi/string-input-visitor.h 
> b/include/qapi/string-input-visitor.h
> index 089243c..4c8d1ea 100644
> --- a/include/qapi/string-input-visitor.h
> +++ b/include/qapi/string-input-visitor.h
> @@ -17,6 +17,10 @@
>
>  typedef struct StringInputVisitor StringInputVisitor;
>
> +/*
> + * The string input visitor does not yet implement support for
> + * visiting QAPI structs, alternates, or arbitrary QTypes.
> + */

"Yet" suggests this is likely to change.  Is it?

>  StringInputVisitor *string_input_visitor_new(const char *str);
>  void string_input_visitor_cleanup(StringInputVisitor *v);
>
> diff --git a/include/qapi/string-output-visitor.h 
> b/include/qapi/string-output-visitor.h
> index d99717f..094a11e 100644
> --- a/include/qapi/string-output-visitor.h
> +++ b/include/qapi/string-output-visitor.h
> @@ -17,6 +17,10 @@
>
>  typedef struct StringOutputVisitor StringOutputVisitor;
>
> +/*
> + * The string output visitor does not yet implement support for
> + * visiting QAPI structs, alternates, or arbitrary QTypes.
> + */

"Yet" suggests this is likely to change.  Is it?

>  StringOutputVisitor *string_output_visitor_new(bool human);
>  void string_output_visitor_cleanup(StringOutputVisitor *v);
>
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 3cd7edc..71f3b5d 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -23,6 +23,10 @@
>  void visit_start_struct(Visitor *v, const char *name, void **obj,
>                          size_t size, Error **errp)
>  {
> +    if (obj) {
> +        assert(size);

Yes, because the generator puts a dummy member into empty structs.

> +        assert(v->type != VISITOR_OUTPUT || *obj);

Can you point me to the spot in the contract that requires this?

> +    }
>      v->start_struct(v, name, obj, size, errp);
>  }
>
> @@ -74,6 +78,7 @@ bool visit_optional(Visitor *v, const char *name, bool 
> *present)
>
>  void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp)
>  {
> +    assert(obj);
>      v->type_int64(v, name, obj, errp);
>  }
>
> @@ -121,6 +126,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);
>  }
>
> @@ -168,12 +174,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 {
> @@ -183,22 +191,31 @@ 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:
> +    assert(v->type != VISITOR_OUTPUT || *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);
> +    assert(v->type != VISITOR_OUTPUT || *obj);
>      v->type_any(v, name, obj, errp);
>  }
>
> @@ -252,7 +269,7 @@ static void input_type_enum(Visitor *v, const char *name, 
> int *obj,
>  void visit_type_enum(Visitor *v, const char *name, int *obj,
>                       const char *const strings[], Error **errp)
>  {
> -    assert(strings);
> +    assert(obj && strings);
>      if (v->type == VISITOR_INPUT) {
>          input_type_enum(v, name, obj, strings, errp);
>      } else if (v->type == VISITOR_OUTPUT) {

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