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

[Eric Blake <eblake@redhat.com>]

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 <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

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

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);
-- 
2.5.0