[Qemu-devel] [PATCH v6 07/23] qapi: Document visitor interfaces

[Eric Blake]

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.

Signed-off-by: Eric Blake <address@hidden>

---
v6: mention that input visitors blindly assign over *obj; wording
improvements
---
 include/qapi/visitor-impl.h |  33 +++++++-
 include/qapi/visitor.h      | 192 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 215 insertions(+), 10 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index d071c06..2f4c89b 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -15,21 +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, void **obj, const char *kind,
                          const char *name, 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, Error **errp);

+    /* 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, Error **errp);

+    /* Must be set, although the helpers input_type_enum() and
+     * output_type_enum() can be used.  */
     void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
                       const char *kind, const char *name, Error **errp);
     /* May be NULL; only needed for input visitors. */
@@ -45,23 +61,38 @@ struct Visitor
     /* Only required to visit sizes differently than (*type_uint64)().  */
     void (*type_size)(Visitor *v, uint64_t *obj, const char *name,
                       Error **errp);
+
     /* Must be set. */
     void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp);
+    /* Must be set */
     void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
+
+    /* Must be provided to visit numbers (the opts visitor does not
+     * currently visit non-integers). */
     void (*type_number)(Visitor *v, double *obj, const char *name,
                         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, QObject **obj, const char *name,
                      Error **errp);

     /* May be NULL; most useful for input visitors. */
     void (*optional)(Visitor *v, bool *present, const char *name);

+    /* 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, int *obj, const char * const strings[],
                      const char *kind, const char *name, Error **errp);
+/**
+ * A generic visitor.type_enum suitable for output visitors.
+ */
 void output_type_enum(Visitor *v, int *obj, const char * const strings[],
                       const char *kind, const char *name, Error **errp);

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index a14a16d..74159c6 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,14 +41,70 @@ typedef struct GenericList
     struct GenericList *next;
 } GenericList;

+/**
+ * Prepare to visit an object with C type @kind 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, followed by a call to visit_end_struct(). For an
+ * input visitor, @obj can be NULL to validate that the visit will
+ * succeed; otherwise, address@hidden is assigned with an allocation of @size
+ * bytes, without regards to the previous value of address@hidden For other
+ * visitors, address@hidden is the object to visit. Set address@hidden on 
failure.
+ *
+ * FIXME: address@hidden can be modified even on error; this can lead to
+ * memory leaks if clients aren't careful.
+ */
 void visit_start_struct(Visitor *v, void **obj, const char *kind,
                         const char *name, 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.
+ */
 void visit_end_struct(Visitor *v, Error **errp);
+
+/**
+ * Prepare to visit an implicit struct.
+ * Similar to visit_start_struct(), except that in implicit struct
+ * represents a subset of keys that are present at the same nesting level
+ * of a common object, rather than a new object at a deeper nesting level.
+ *
+ * FIXME: address@hidden can be modified even on error; 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.
+ */
 void visit_end_implicit_struct(Visitor *v, Error **errp);
+
+/**
+ * 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);
+/**
+ * Collect the next list member and append it to address@hidden
+ * Start with address@hidden of NULL, then subsequent iterations should pass
+ * address@hidden pointing to the previous return 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.
+ */
 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, Error **errp);

 /**
@@ -53,23 +123,127 @@ bool visit_optional(Visitor *v, bool *present, const char 
*name);
  */
 void visit_get_next_type(Visitor *v, QType *type, bool promote_int,
                          const char *name, 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 address@hidden to the numeric 
value
+ * of the enum type @kind using @strings as the mapping, leaving @obj
+ * unchanged on error; for output visitors, reverse the mapping and visit
+ * the output string determined by address@hidden
+ */
 void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
                      const char *kind, const char *name, 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 address@hidden to the parsed value; for other 
visitors,
+ * leave address@hidden unchanged.
+ */
 void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp);
-void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error 
**errp);
-void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error 
**errp);
-void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error 
**errp);
-void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, 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, uint8_t *obj, const char *name,
+                      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, uint16_t *obj, const char *name,
+                       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, uint32_t *obj, const char *name,
+                       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, uint64_t *obj, const char *name,
+                       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, int8_t *obj, const char *name, Error **errp);
-void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error 
**errp);
-void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error 
**errp);
-void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error 
**errp);
-void visit_type_size(Visitor *v, uint64_t *obj, const char *name, 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, int16_t *obj, const char *name,
+                      Error **errp);
+/**
+ * Visit an uint32_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, int32_t *obj, const char *name,
+                      Error **errp);
+/**
+ * Visit an int64_t value tied to @name in the current object visit.
+ * Like visit_type_int(), except clamps the value to int64_t range.
+ */
+void visit_type_int64(Visitor *v, int64_t *obj, const char *name,
+                      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, uint64_t *obj, const char *name,
+                     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 address@hidden to the value; other visitors will leave
+ * address@hidden unchanged.
+ */
 void visit_type_bool(Visitor *v, bool *obj, const char *name, 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 address@hidden to the parsed 
string;
+ * while output visitors leave address@hidden unchanged, except that a NULL 
address@hidden
+ * must be treated the same as "".
+ *
+ * FIXME: Unfortunately not const-correct for output visitors.
+ */
 void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
-void visit_type_number(Visitor *v, double *obj, const char *name, 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 address@hidden to the value; other visitors will leave
+ * address@hidden unchanged.
+ */
+void visit_type_number(Visitor *v, double *obj, const char *name,
+                       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 address@hidden to the value; other visitors will leave
+ * address@hidden unchanged.
+ */
 void visit_type_any(Visitor *v, QObject **obj, const char *name, 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
-- 
2.4.3