From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40982) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aMPcj-00025E-TO for qemu-devel@nongnu.org; Thu, 21 Jan 2016 19:30:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aMPcg-0006mI-KS for qemu-devel@nongnu.org; Thu, 21 Jan 2016 19:30:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:32867) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aMPcg-0006mC-7a for qemu-devel@nongnu.org; Thu, 21 Jan 2016 19:30:42 -0500 References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-22-git-send-email-eblake@redhat.com> <87vb6m7l25.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56A17830.7010305@redhat.com> Date: Thu, 21 Jan 2016 17:30:40 -0700 MIME-Version: 1.0 In-Reply-To: <87vb6m7l25.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="A9rfan33Ih4sr5MXno6JiFcTJqrANo0bp" Subject: Re: [Qemu-devel] [PATCH v9 21/37] qapi: Document visitor interfaces, add assertions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --A9rfan33Ih4sr5MXno6JiFcTJqrANo0bp Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/21/2016 01:08 PM, Markus Armbruster wrote: > All right, this one's a bear. Not because the patch is bad, but becaus= e > what it tries to do is bloody difficult. Is there any reasonable split (such as adding some of the assertions in earlier patches) that would make it any easier? Or do we just bite the bullet and do it? >=20 > Eric Blake writes: >=20 >> The visitor interface for mapping between QObject/QemuOpts/string >> and qapi has formerly been documented only by reading source code, >=20 > Polite way to say "is scandalously undocumented". Indeed. >=20 >> 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. >=20 > Suggest >=20 > 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. >=20 > Correct this by retrofitting proper contracts. Document some > interface warts that would be nice to remove. In particular, I hav= e > plans to remove visit_start_union() in a future patch. Your suggestions here, and elsewhere, are good and will be in my next spin. I'll trim to just the places where you have more than just a wording suggestion. >> include/qapi/visitor-impl.h | 31 ++++++- >> include/qapi/visitor.h | 196 +++++++++++++++++++++++++++++++++++= +++++++++ >> qapi/qapi-visit-core.c | 39 ++++++++- >> 3 files changed, 262 insertions(+), 4 deletions(-) >> >=20 > My review probably makes more sense if you skip ahead to visitor.h, the= n > come back here. If I remember, I'll use -O when generating v10 to force visitor.h first, other .h second, and .c last (I don't always remember to do it; maybe I should add it into my handy .git alias that I use for firing off long series). [I really wish the git people would make it possible to automate -O via 'git config', and to make it easier to have a per-project preferred order file] >=20 >> + >> struct Visitor >> { >> - /* Must be set */ >> + /* Must be provided to visit structs (the string visitors do not >> + * currently visit structs). */ >=20 > 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? Good point. I think what I will do is split out a separate patch that documents, per visitor with limitation, what callers cannot (yet) do with that visitor. >> + /* May be NULL; most useful for input visitors. */ >=20 > "Optional" would be a bit terser than "May be NULL". >=20 > Why is it "most useful for input visitors"? For what it's worth, the > dealloc visitor finds it useful, too... and a later patch adds it to the QemuOpts input visitor (Zoltan's patch has been sitting for how many months now?). I'll come up with something.= >=20 >> 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); >=20 > A visitor could omit these two with similar consequences to omitting > start_struct() and end_struct(): attempts to visit lists crash then. I= n > fact, the string visitors omitted them until commit 659268f and 69e2556= , > respectively. Which is why, as you pointed out, it may be better to document the limitations in the string visitor rather than here, and in this file maybe just mention at the top something along the lines that "must be set" really means that "only needs to be set if your callers are expecting a visit to encounter this type; the corresponding crash on calling NULL is your hint to write missing functionality in your visitor"= =2E >> /* May be NULL; only needed for input visitors. */ > void (*get_next_type)(Visitor *v, const char *name, QType *type,= > bool promote_int, Error **errp); >=20 > I figure it must be set for input visitors, because without it, the > generated visit function will assume QTYPE_NONE, and fail. >=20 > Suggest "Must be set for input visitors." Correct, and nice wording. >=20 > 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. >=20 >> 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); >> }; >> >> +/** >=20 > The /** is a marker for GTK-Doc. We don't actually follow GTK-Doc > conventions, however (they suck). Sure we want the extra * anyway? I don't mind dropping it. >> +++ b/include/qapi/visitor.h >> @@ -18,6 +18,20 @@ >> #include "qapi/error.h" >> #include >> >> +/* This file describes the client view for visiting a map between >> + * generated QAPI C structs and another representation (command line >> + * options, strings, or QObjects). >=20 > "visiting a map"? I'm a victim of too much rebasing. I think I meant: =2E..the client view for mapping between generated QAPI C structs and another representation... >=20 >> An input visitor converts from >> + * some other form into QAPI representation; an output visitor >> + * converts from QAPI back into another form. >=20 > Let me try to work out what this visitor thingy is about. >=20 > 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. >=20 > QAPI generates visitor functions to walk these graphs. They currently > work only for trees, because that's all we need. >=20 > 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 variou= s > tasks that need to walk it to do something. Additionally, it is possible to use the visitor without a qapi struct, purely for their side effects of translation to or from the alternative representation of that visitor, by manually calling visitor functions in the same order that generated QAPI structs would use. >=20 > Three kinds of visitor classes exist: two output visitors (QMP and > string), three input visitors (QMP, string and QemuOpts), and the > dealloc visitor. >=20 > With an output visitor, the visitor functions walk a tree, and the > output visitor builds up some output. QmpOutputVisitor builds a QObjec= t > matching QMP wire format,, and StringOutputVisitor builds a C string. > Both convert a QAPI tree to another representation. Or, when passed NULL obj, create the other representation out of thin air from the manual visit. >=20 > Similarly with the QapiDeallocVisitor, except nothing gets built. > Instead, the tree gets destroyed node by node. >=20 > 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. Or, when passed NULL obj, parse the other representation via a manual visit with no QAPI object involved. >=20 > The Qmp visitors and the dealloc visitor a general: they can walk / > build any QAPI tree. >=20 > The string visitors and the QemuOpts visitor have restrictions: they ca= n > walk / build only a subset. Yes. >> +/** >> + * Prepare to visit an object tied to key @name. >=20 > Not just any object, but a *struct*. Suggest: >=20 > * Start visiting struct value @obj. >=20 >> + * @name will be NULL if this is visited as part of a list. >=20 > I'm afraid reality is a bit messier. >=20 > If the visited object is a member of struct or union, @name is its > member name. >=20 > If it's a member of a list, @name is null. [side thread in earlier message about possibly using "name[0]" instead of NULL, as a later improvement] >=20 > If it's a command argument, @name is its parameter name. But this is a special case of "visiting as part of a struct", since we have a (possibly-implicit) QAPI struct for parameters. >=20 > If it's a command's result, @name is a meaningless string (qmp-marshal.= c > currently uses "unused"). But this is a special case of a root visit (the command result is the top of a visit, so @name is meaningless). >=20 > Else, @name can be a meaningless string or null. And this sentence is reached only for a root visit. So now I'm thinking along these lines: As the first object in a visit (the root of a QAPI struct), @name is meaningless. It is typically set to NULL or a descriptive string, although some callers pass "unused". At all other points of the visit, @name reflects the relationship of this visit to the parent. Either the visited object is a member of a struct or union, and @name is its member name; or the visited object is the member of a list and @name is NULL. >=20 > Same for other visit functions. Is it okay to write it out once, and then have all other functions refer back to the common location? >=20 >> 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. >=20 > I'd explain intended use after the parameters. >=20 >> + * >> + * @obj can be NULL (in which case @size should also be 0) to indicat= e >=20 > "must be 0", because you assert that below. >=20 > 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. We pass 0 precisely when @obj is NULL because that is the usage pattern where we do NOT have a QAPI struct, but are manually using the visitor for its side effects. It's hard to have a non-zero size of a non-existent struct. >=20 >> + * that there is no qapi C struct, and that the upcoming visit calls >> + * are parsing input to or creating output from some other >> + * representation. >=20 > This is very vague. >=20 > Can you point me to code passing null @obj? Easiest example: hw/ppc/spapr_drc.c. Maybe I should follow danpb's lead and actually put some usage of the visitors in the comments. >=20 > I suspect only some visitors accept null @obj. Which ones? I didn't check that; will do. >=20 >> + * >> + * If @obj is not NULL, then input visitors malloc a qapi struct of >> + * @size bytes into *@obj on success, and output visitors expect *@ob= j >> + * to be a fully-populated qapi struct. >=20 > 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. Probably possible to change, although none of my patches do it yet. >=20 > Only input visitors use @size, and they use it only when @obj isn't > null. >=20 > We could make visitors check the member pointers passed to the member > visits actually point into (@obj, @size). Then *all* visitors would us= e > @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". My intent was the latter - @size is unused and must be zero when @obj is NULL. Also, rather than making every visitor track that @obj for the current visit lies within (@obj, @size) of the parent struct, I'm wondering if it would be better to do that tracking at the qapi-visit-core level - but then we'd have two levels of code tracking a stack of information instead of one. >=20 > This method is an awful mess. Probably because it's serving quite > different purposes in the different visitors. Indeed. visit_type_str() is the best example of the conflict of interest between input and output visitors, in that we can't be const-correct. At one point, I was half-tempted to split input and output visitors into two separate contracts, rather than trying to merge both types of visits through a single interface and having the interface become a bit unwieldy. But as currently written, it's kind of convenient that a single function in generated qapi-visit.c can handle all the visitors. >=20 >> + * >> + * Set *@errp on failure; for example, if the input stream does not >> + * have a member @name or if the member is not an object. >=20 > What do you mean by "if the member is not an object"? s/object/struct/ If I'm using the QMP input visitor to parse the string "{ \"a\": 1 }", and call visit_start_struct("a"), I expect an error because "1" is not a struct. >=20 > Any other ways to fail? I don't think so. >=20 > This is the only function comment that says anything about @errp. > Should we document the various failure modes everywhere? Probably useful. More work, but worth doing. >=20 >> + * >> + * FIXME: For input visitors, *@obj can be assigned here even if late= r >> + * visits will fail; this can lead to memory leaks if clients aren't >> + * careful. >=20 > Why is this a FIXME? How could it be fixed? More of the same below. Fixed by 35/37, where we change the return type here, and fix all the visit_type_FOO() functions to use that return type to properly use the dealloc visitor on any partial *@obj, so that the callers of visit_type_FOO() no longer have to worry about partial allocation. Maybe my wording could be improved here. >=20 > My attempt at explaining intended use: >=20 > 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. >=20 > I guess what the reader really needs here to understand intended use is= > example code. qapi-code-gen.txt has some. Add a pointer? Hmm, a pointer to qapi-code-gen would be shorter than an inline blurb. But it only lists the generated usage; I may also want to document the no-QAPI-struct usage (our friend spapr_drc.c again). >=20 >> + */ >> void visit_start_struct(Visitor *v, const char *name, void **obj, >> size_t size, Error **errp); >=20 > Please separate each commented declaration with a blank line. Not > flagging further instances. I was trying to group related functions; will switch to one blank in related sets, and two blanks between sets (instead of zero/one). >=20 >> +/** >> + * 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 allo= w >> + * the backend to release any resources. >=20 > 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. Except that patch 33/37 asserts that destroying the visitor is only ever done after all visit_end_struct()s have been paired, as a tighter contract which will then let us free up some resources during the end_struct() without having to track them for a potentially-early visitor destruction. And yes, I already have a followup series posted that introduces a visitor reset, at least for the QMP output visitor (I wasn't sure whether to generalize it to all visitors). >> +/** >> + * 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 l= evel >> + * of a common object but as a separate qapi C struct, rather than a = new >> + * object at a deeper nesting level. >=20 > I'm afraid this is impenetrable. >=20 > I tried to refresh my memory on visit_start_implicit_struct()'s purpose= > by reading code, but that's pretty impenetrable, too. Consider the input string { "a":1, "b":2 }. With a normal QAPI struct, all fields of the JSON object are part of the same C struct: Foo { int a; int b; }; and it is visited with: visit_start_struct(); visit_type_int("a", &a); visit_type_int("b", &b); visit_end_struct(); But with an implicit struct (namely, a branch of a QAPI union type or a member of a QAPI alternate type; we used to also do it for base classes but changed that to inline fields instead), we have more than one C struct that map to the same flat JSON object: Bar { int b; }; Foo { int a; Bar *sub; } which is visited with: visit_start_struct(); visit_type_int("a", &b); visit_start_implicit_struct(&sub); visit_type_int("b", &sub.b); visit_end_implicit_struct(); visit_end_struct(); Suggestions on how to better word it are welcome. I'm basically trying to describe that this function marks the start of a new C struct used as a sub-object while still conceptually parsing the same top-level QAPI struct. >=20 >> + * >> + * @obj must not be NULL, since this function is only called when >> + * visiting with qapi structs. >=20 > Really? Why does qmp_input_start_implicit_struct() check for null then= ? Probably worth an independent cleanup. The assertions added in this patch prove that that check is dead. >> +/** >> + * 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. >=20 > I'm afraid this doesn't really tell me how to visit a list. Perhaps: >=20 > 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. >=20 > May want to add a pointer to example code in qapi-code-gen.txt. And it changes again in 34/37. We have two styles; our generated code, which is roughly: visit_start_list(&obj); while (visit_next_list()) { visit_type_FOO(element); } visit_end_list() and the manual style of our friend hw/ppc/spapr_drc.c: visit_start_list(NULL); while (some other way to decide if data available) { visit_type_FOO(element); } visit_end_list() That is, the use of visit_next_list() is optional depending on whether a QAPI list is in use, but the visit_type_FOO() per element is necessary. We'll see if I can get the next wording attempt a bit easier to understa= nd. >=20 >> + */ >> void visit_start_list(Visitor *v, const char *name, Error **errp); >> +/** >> + * >> + * 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. >=20 > I'm confused. Can you point me to code bypassing visit_next_list()? See above; spapr_drc.c. >> /** > * Check if an optional member @name of an object needs visiting. > * For input visitors, set *@present according to whether the > * corresponding visit_type_*() needs calling; for other visitors, > * leave *@present unchanged. Return *@present for convenience. > bool visit_optional(Visitor *v, const char *name, bool *present); >=20 >=20 > Suggest to clarify: >=20 > Does optional struct member @name need visiting? > @present points to the optional member's has_ flag. > Input visitors set *@present according to their input. Other > visitors leave it unchanged. > In either case, return *@present. Thanks. I originally added all the docs in one pass, but some of the docs snuck in via earlier patches during rebase churn, so reviewing it again here helps. >> +/** >> + * 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. >=20 > Same for the other visit_type_T(), but not specified there. >=20 >> 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 "". >=20 > Suggest: >=20 > Input visitors set *@obj according to their input (never NULL). > Other visitors leave it unchanged. They commonly treat NULL like "= ". >=20 >> + * >> + * FIXME: Unfortunately not const-correct for output visitors. >=20 > Is that fixable? Not easily, and certainly not as part of this series. The best I can think of is either two interfaces: char *visit_input_type_str(Visitor *v, const char *name, Error **errp); void visit_output_type_str(Visitor *v, const char *name, const char *value, Error **errp); used as: obj.str =3D visit_input_type_str(v, "foo", &err); visit_output_type_str(v, "foo", obj.str, &err); or to make the single interface have more parameters: void visit_type_str(Visitor *v, const char *name, const char *value_in, char **value_out, Error **errp); used as: visit_type_str(v, "foo", obj.str, &obj.str, &err); where input visits could pass NULL instead of value_in, and output visits could pass NULL instead of value_out. But either way, consistency would then argue that ALL visit_type_FOO() interfaces should either have two forms (one for input, one for output) or have more parameters (with input distinct from output), and it would allow const-correctness on output visits of more than just strings. And which form would a dealloc visitor use? >> +/** >> + * Mark the start of visiting the branches of a union. Return true if= >> + * @data_present. >=20 > 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. >=20 > 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. I like the 'FIXME'-only approach. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --A9rfan33Ih4sr5MXno6JiFcTJqrANo0bp Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWoXgwAAoJEKeha0olJ0Nqkz8IAKRcO3O99lj8N02vy19ykWR7 UHeMS+hOlGhkXqHeWJQ3AqtCqsEMfhyGT+inUTf+Z+S89iMGY32HGDp6wWLH+pqq hjgHNo3iEwx0PuC7wO1bgFpc/ZKlybz9PtNucZjgL5YLe8czxdlvtt1kENAz2za7 5gHjmpc6lVyTUYyFm2laM1U96308eOtuDV4GrDDOAdHN5oWMOmyOgcPGw741Xsiw 76nOOzQAWjInzcAWoxVER7MqGBb3SgX6G/gtQNId1oKV5FjmoF6tQxtgMrIm4r8b yP0RpnxohbJBDk2z1QHW5SDosNUYwtsz2Ejpa4oD2bFv6mgRs3Dei3xq/VILsjc= =nU6Q -----END PGP SIGNATURE----- --A9rfan33Ih4sr5MXno6JiFcTJqrANo0bp--