[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 21/37] qapi: Document visitor interfaces, add
|
From: |
Markus Armbruster |
|
Subject: |
Re: [Qemu-devel] [PATCH v9 21/37] qapi: Document visitor interfaces, add assertions |
|
Date: |
Thu, 21 Jan 2016 21:08:02 +0100 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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 <address@hidden> 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 <address@hidden>
> Reviewed-by: Marc-André Lureau <address@hidden>
>
> ---
> 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 address@hidden on success, and output visitors expect
> address@hidden
> + * 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 address@hidden 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, address@hidden 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, address@hidden 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 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
> + * 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, address@hidden 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 address@hidden according to whether the
* corresponding visit_type_*() needs calling; for other visitors,
* leave address@hidden unchanged. Return address@hidden 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 address@hidden according to their input. Other
visitors leave it unchanged.
In either case, return address@hidden
> @@ -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 address@hidden to the correct qtype of a qapi
* alternate type; for other visitors, leave address@hidden 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 address@hidden 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 address@hidden 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 address@hidden
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 address@hidden 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 address@hidden to the parsed value; for other
> visitors,
> + * leave address@hidden 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 address@hidden to the value; other visitors will leave
> + * address@hidden 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 address@hidden to the parsed
> + * string (never NULL); while output visitors leave address@hidden unchanged,
> + * except that a NULL address@hidden will be treated the same as "".
Suggest:
Input visitors set address@hidden 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 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 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 address@hidden to the value; other visitors will leave
> + * address@hidden (which must not be NULL) unchanged.
Should specify that input visitors never set it to null. Suggest:
Input visitors set address@hidden 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);
- Re: [Qemu-devel] [PATCH v9 11/37] qapi: Consolidate visitor small integer callbacks, (continued)
- [Qemu-devel] [PATCH v9 09/37] qapi: Prefer type_int64 over type_int in visitors, Eric Blake, 2016/01/19
- [Qemu-devel] [PATCH v9 20/37] qmp: Don't abuse stack to track qmp-output root, Eric Blake, 2016/01/19
- [Qemu-devel] [PATCH v9 21/37] qapi: Document visitor interfaces, add assertions, Eric Blake, 2016/01/19
- Re: [Qemu-devel] [PATCH v9 21/37] qapi: Document visitor interfaces, add assertions,
Markus Armbruster <=
[Qemu-devel] [PATCH v9 26/37] qapi: Simplify excess input reporting in input visitors, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH 0/3] qapi-visit: Unify struct and union visit, Markus Armbruster, 2016/01/27