[Qemu-devel] [PATCH v5 24/46] qapi: Document visitor interfaces

[Eric Blake <eblake@redhat.com>]

The visitor interface for mapping between QObject 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 tries to add 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 <eblake@redhat.com>
---
 include/qapi/visitor-impl.h |  36 +++++++-
 include/qapi/visitor.h      | 195 ++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 220 insertions(+), 11 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 370935a..090b19b 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -15,40 +15,66 @@
 #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; most useful for input visitors. */
     void (*get_next_type)(Visitor *v, qtype_code *type, bool promote_int,
                           const char *name, Error **errp);

+    /* Must be set */
     void (*type_int)(Visitor *v, int64_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 */
+    /* May be NULL; most useful for input visitors. */
     void (*optional)(Visitor *v, bool *present, const char *name,
                      Error **errp);

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

     /* Only required to visit uint64 differently than (*type_int)().  */
@@ -59,8 +85,14 @@ struct Visitor
                       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 a5ec44e..2883cf4 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -18,6 +18,17 @@
 #include "qapi/error.h"
 #include <stdlib.h>

+/* This file describes the client view for visiting a map between
+ * QObjects and another representation (command line options, strings,
+ * or generated QAPI C structs).  An input visitor converts from
+ * QObject to another form; an output visitor converts from the other
+ * form back into QObjects.  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 +38,77 @@ typedef struct GenericList
     struct GenericList *next;
 } GenericList;

+/**
+ * Prepare to visit a QDict with C type @kind tied to QDict key @name.
+ * @name will be NULL if this is visited as part of a QList.
+ * The caller then makes a series of visit calls for each key expected
+ * in the dictionary, followed by a call to visit_end_struct(). For an
+ * input visitor, @obj can be NULL to validate that the visit will
+ * succeed; otherwise, *@obj is assigned with an allocation of @size
+ * bytes. For other visitors, *@obj is the object to visit. Set *@errp
+ * on failure.
+ *
+ * FIXME: *@obj 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 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 this will visit a
+ * C pointer pointing to @size bytes, and where the QDict fields are
+ * part of the parent object.
+ *
+ * FIXME: *@obj 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 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 QList tied to QDict key @name.
+ * @name will be NULL if this is visited as part of a QList.
+ * 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 *@list.
+ * Start with *@list of NULL, then subsequent iterations should pass
+ * *@list 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.
+ */
 void visit_end_list(Visitor *v, Error **errp);
+
+/**
+ * Check if an optional member @name of a QDict needs visiting.
+ * For input visitors, set *@present according to whether the
+ * corresponding visit_type_*() needs calling; for other visitors,
+ * leave *@present unchanged.
+ */
 void visit_optional(Visitor *v, bool *present, const char *name,
                     Error **errp);

@@ -47,23 +120,127 @@ void visit_optional(Visitor *v, bool *present, const char *name,
  */
 void visit_get_next_type(Visitor *v, qtype_code *type, bool promote_int,
                          const char *name, Error **errp);
+
+/**
+ * Visit an enum value tied to @name in the current QDict visit.
+ * @name will be NULL if this is visited as part of a QList.
+ * For input visitors, parse a string and set *@obj to the numeric value
+ * of the enum type @kind using @strings as the mapping; for output
+ * visitors, reverse the mapping and visit the output string determined
+ * by *@obj.
+ */
 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 QDict visit.
+ * @name will be NULL if this is visited as part of a QList.
+ * For input visitors, set *@obj to the parsed value; for other visitors,
+ * leave *@obj 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 QDict 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 QDict 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 QDict 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 QDict 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 QDict 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 QDict 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 QDict 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 QDict 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 QDict 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 QDict visit.
+ * @name will be NULL if this is visited as part of a QList.
+ * Input visitors set *@obj to the value; other visitors will leave
+ * *@obj unchanged.
+ */
 void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
+
+/**
+ * Visit a string value tied to @name in the current QDict visit.
+ * @name will be NULL if this is visited as part of a QList.
+ * @obj must be non-NULL.  Input visitors set *@obj to the parsed string;
+ * while output visitors leave *@obj unchanged, except that a NULL *@obj
+ * 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 QDict visit.
+ * @name will be NULL if this is visited as part of a QList.
+ * Input visitors set *@obj to the value; other visitors will leave
+ * *@obj 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 QDict visit.
+ * @name will be NULL if this is visited as part of a QList.
+ * Input visitors set *@obj to the value; other visitors will leave
+ * *@obj 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