riscv/qemu - qemu - Gitea: Git with a cup of tea

Commit Graph

Author	SHA1	Message	Date
Markus Armbruster	c261394978	qapi: Make pylint a bit happier Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <1489582656-31133-47-git-send-email-armbru@redhat.com>	10 years ago
Markus Armbruster	ef801a9bb1	qapi: Prefer single-quoted strings more consistently PEP 8 advises: In Python, single-quoted strings and double-quoted strings are the same. This PEP does not make a recommendation for this. Pick a rule and stick to it. When a string contains single or double quote characters, however, use the other one to avoid backslashes in the string. It improves readability. The QAPI generators succeed at picking a rule, but fail at sticking to it. Convert a bunch of double-quoted strings to single-quoted ones. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <1489582656-31133-20-git-send-email-armbru@redhat.com>	10 years ago
Markus Armbruster	a4a1c70dc7	qapi: Make input visitors detect unvisited list tails Fix the design flaw demonstrated in the previous commit: new method check_list() lets input visitors report that unvisited input remains for a list, exactly like check_struct() lets them report that unvisited input remains for a struct or union. Implement the method for the qobject input visitor (straightforward), and the string input visitor (less so, due to the magic list syntax there). The opts visitor's list magic is even more impenetrable, and all I can do there today is a stub with a FIXME comment. No worse than before. Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <1488544368-30622-26-git-send-email-armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>	10 years ago
Eric Blake	fa274ed6fb	qapi: Drop useless gen_err_check() Ever since commit `12f254f` removed the last parameterization of gen_err_check(), it no longer makes sense to hide the three lines of generated C code behind a macro call. Just inline it into the remaining users. No change to generated code. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1468468228-27827-7-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	10 years ago
Eric Blake	1158bb2a05	qapi: Add parameter to visit_end_* Rather than making the dealloc visitor track of stack of pointers remembered during visit_start_* in order to free them during visit_end_, it's a lot easier to just make all callers pass the same pointer to visit_end_. The generated code has access to the same pointer, while all other users are doing virtual walks and can pass NULL. The dealloc visitor is then greatly simplified. All three visit_end_() functions intentionally take a void, even though the visit_start_() functions differ between void, GenericList, and GenericAlternate*. This is done for several reasons: when doing a virtual walk, passing NULL doesn't care what the type is, but when doing a generated walk, we already have to cast the caller's specific FOO to call visit_start, while using void** lets us use visit_end without a cast. Also, an upcoming patch will add a clone visitor that wants to use the same implementation for all three visit_end callbacks, which is made easier if all three share the same signature. For visitors with already track per-object state (the QMP visitors via a stack, and the string visitors which do not allow nesting), add an assertion that the caller is indeed passing the same pointer to paired calls. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1465490926-28625-4-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	10 years ago
Eric Blake	9b4e38fe6a	qapi: Fix crash on missing alternate member of QAPI struct If a QAPI struct has a mandatory alternate member which is not present on input, the input visitor reports an error for the missing alternate without setting the discriminator, but the cleanup code for the struct still tries to use the dealloc visitor to clean up the alternate. Commit `dbf11922` changed visit_start_alternate to set obj to NULL when an error occurs, where it was previously left untouched. Thus, before the patch, the dealloc visitor is blindly trying to cleanup whatever branch corresponds to (obj)->type == 0 (that is, QTYPE_NONE, because obj still pointed to zeroed memory), which selects the default branch of the switch and sets an error, but this second error is ignored by the way the dealloc visitor is used; but after the patch, the attempt to switch dereferences NULL. When cleaning up after a partial object parse, we specifically check for !obj after visit_start_struct() (see gen_visit_object()); doing the same for alternates fixes the crash. Enhance the testsuite to give coverage for both missing struct and missing alternate members. Also add an abort - we expect visit_start_alternate() to either set an error or to set (obj)->type to a valid QType that corresponds to actual user input, and QTYPE_NONE should never be reachable from valid input. Had the abort() been in place earlier, we might have noticed the dealloc visitor dereferencing bogus zeroed memory prior to when commit `dbf11922` forced our hand by setting obj to NULL and causing a fault. Test case: {'execute':'blockdev-add', 'arguments':{'options':{'driver':'raw'}}} The choice of 'driver':'raw' selects a BlockdevOptionsGenericFormat struct, which has a mandatory 'file':'BlockdevRef' in QAPI. Since 'file' is missing as a sibling of 'driver', this should report a graceful error rather than fault. After this patch, we are back to: {"error": {"class": "GenericError", "desc": "Parameter 'file' is missing"}} Generated code in qapi-visit.c changes as: \|@@ -2444,6 +2444,9 @@ void visit_type_BlockdevRef(Visitor v, \| if (err) { \| goto out; \| } \|+ if (!obj) { \|+ goto out_obj; \|+ } \| switch ((obj)->type) { \| case QTYPE_QDICT: \| visit_start_struct(v, name, NULL, 0, &err); \|@@ -2459,10 +2462,13 @@ void visit_type_BlockdevRef(Visitor v, \| case QTYPE_QSTRING: \| visit_type_str(v, name, &(*obj)->u.reference, &err); \| break; \|+ case QTYPE_NONE: \|+ abort(); \| default: \| error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", \| "BlockdevRef"); \| } \|+out_obj: \| visit_end_alternate(v); Reported by Kashyap Chamarthy <kchamart@redhat.com> CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1466012271-5204-1-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Tested-by: Kashyap Chamarthy <kchamart@redhat.com> [Commit message tweaked] Signed-off-by: Markus Armbruster <armbru@redhat.com>	10 years ago
Eric Blake	68ab47e4b4	qapi: Change visit_type_FOO() to no longer return partial objects Returning a partial object on error is an invitation for a careless caller to leak memory. We already fixed things in an earlier patch to guarantee NULL if visit_start fails ("qapi: Guarantee NULL obj on input visitor callback error"), but that does not help the case where visit_start succeeds but some other failure happens before visit_end, such that we leak a partially constructed object outside visit_type_FOO(). As no one outside the testsuite was actually relying on these semantics, it is cleaner to just document and guarantee that ALL pointer-based visit_type_FOO() functions always leave a safe value in obj during an input visitor (either the new object on success, or NULL if an error is encountered), so callers can now unconditionally use qapi_free_FOO() to clean up regardless of whether an error occurred. The decision is done by adding visit_is_input(), then updating the generated code to check if additional cleanup is needed based on the type of visitor in use. Note that we still leave obj unchanged after a scalar-based visit_type_FOO(); I did not feel like auditing all uses of visit_type_Enum() to see if the callers would tolerate a specific sentinel value (not to mention having to decide whether it would be better to use 0 or ENUM__MAX as that sentinel). Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-25-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	10 years ago
Eric Blake	d9f62dde13	qapi: Simplify semantics of visit_next_list() The semantics of the list visit are somewhat baroque, with the following pseudocode when FooList is used: start() for (prev = head; cur = next(prev); prev = &cur) { visit(&cur->value) } Note that these semantics (advance before visit) requires that the first call to next() return the list head, while all other calls return the next element of the list; that is, every visitor implementation is required to track extra state to decide whether to return the input as-is, or to advance. It also requires an argument of 'GenericList *' to next(), solely because the first iteration might need to modify the caller's GenericList head, so that all other calls have to do a layer of dereferencing. Thankfully, we only have two uses of list visits in the entire code base: one in spapr_drc (which completely avoids visit_next_list(), feeding in integers from a different source than uint8List), and one in qapi-visit.py. That is, all other list visitors are generated in qapi-visit.c, and share the same paradigm based on a qapi FooList type, so we can refactor how lists are laid out with minimal churn among clients. We can greatly simplify things by hoisting the special case into the start() routine, and flipping the order in the loop to visit before advance: start(head) for (tail = head; tail; tail = next(tail)) { visit(&tail->value) } With the simpler semantics, visitors have less state to track, the argument to next() is reduced to 'GenericList *', and it also becomes obvious whether an input visitor is allocating a FooList during visit_start_list() (rather than the old way of not knowing if an allocation happened until the first visit_next_list()). As a minor drawback, we now allocate in two functions instead of one, and have to pass the size to both functions (unless we were to tweak the input visitors to cache the size to start_list for reuse during next_list, but that defeats the goal of less visitor state). The signature of visit_start_list() is chosen to match visit_start_struct(), with the new parameters after 'name'. The spapr_drc case is a virtual visit, done by passing NULL for list, similarly to how NULL is passed to visit_start_struct() when a qapi type is not used in those visits. It was easy to provide these semantics for qmp-output and dealloc visitors, and a bit harder for qmp-input (several prerequisite patches refactored things to make this patch straightforward). But it turned out that the string and opts visitors munge enough other state during visit_next_list() to make it easier to just document and require a GenericList visit for now; an assertion will remind us to adjust things if we need the semantics in the future. Several pre-requisite cleanup patches made the reshuffling of the various visitors easier; particularly the qmp input visitor. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-24-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	10 years ago
Eric Blake	15c2f669e3	qapi: Split visit_end_struct() into pieces As mentioned in previous patches, we want to call visit_end_struct() functions unconditionally, so that visitors can release resources tied up since the matching visit_start_struct() without also having to worry about error priority if more than one error occurs. Even though error_propagate() can be safely used to ignore a second error during cleanup caused by a first error, it is simpler if the cleanup cannot set an error. So, split out the error checking portion (basically, input visitors checking for unvisited keys) into a new function visit_check_struct(), which can be safely skipped if any earlier errors are encountered, and leave the cleanup portion (which never fails, but must be called unconditionally if visit_start_struct() succeeded) in visit_end_struct(). Generated code in qapi-visit.c has diffs resembling: \|@@ -59,10 +59,12 @@ void visit_type_ACPIOSTInfo(Visitor *v, \| goto out_obj; \| } \| visit_type_ACPIOSTInfo_members(v, obj, &err); \|- error_propagate(errp, err); \|- err = NULL; \|+ if (err) { \|+ goto out_obj; \|+ } \|+ visit_check_struct(v, &err); \| out_obj: \|- visit_end_struct(v, &err); \|+ visit_end_struct(v); \| out: and in qapi-event.c: @@ -47,7 +47,10 @@ void qapi_event_send_acpi_device_ost(ACP \| goto out; \| } \| visit_type_q_obj_ACPI_DEVICE_OST_arg_members(v, &param, &err); \|- visit_end_struct(v, err ? NULL : &err); \|+ if (!err) { \|+ visit_check_struct(v, &err); \|+ } \|+ visit_end_struct(v); \| if (err) { \| goto out; Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-20-git-send-email-eblake@redhat.com> [Conflict with a doc fixup resolved] Signed-off-by: Markus Armbruster <armbru@redhat.com>	10 years ago
Markus Armbruster	da34e65cb4	include/qemu/osdep.h: Don't include qapi/error.h Commit `57cb38b` included qapi/error.h into qemu/osdep.h to get the Error typedef. Since then, we've moved to include qemu/osdep.h everywhere. Its file comment explains: "To avoid getting into possible circular include dependencies, this file should not include any other QEMU headers, with the exceptions of config-host.h, compiler.h, os-posix.h and os-win32.h, all of which are doing a similar job to this file and are under similar constraints." qapi/error.h doesn't do a similar job, and it doesn't adhere to similar constraints: it includes qapi-types.h. That's in excess of 100KiB of crap most .c files don't actually need. Add the typedef to qemu/typedefs.h, and include that instead of qapi/error.h. Include qapi/error.h in .c files that need it and don't get it now. Include qapi-types.h in qom/object.h for uint16List. Update scripts/clean-includes accordingly. Update it further to match reality: replace config.h by config-target.h, add sysemu/os-posix.h, sysemu/os-win32.h. Update the list of includes in the qemu/osdep.h comment quoted above similarly. This reduces the number of objects depending on qapi/error.h from "all of them" to less than a third. Unfortunately, the number depending on qapi-types.h shrinks only a little. More work is needed for that one. Signed-off-by: Markus Armbruster <armbru@redhat.com> [Fix compilation without the spice devel packages. - Paolo] Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>	11 years ago
Eric Blake	32bafa8fdd	qapi: Don't special-case simple union wrappers Simple unions were carrying a special case that hid their 'data' QMP member from the resulting C struct, via the hack method QAPISchemaObjectTypeVariant.simple_union_type(). But by using the work we started by unboxing flat union and alternate branches, coupled with the ability to visit the members of an implicit type, we can now expose the simple union's implicit type in qapi-types.h: \| struct q_obj_ImageInfoSpecificQCow2_wrapper { \| ImageInfoSpecificQCow2 data; \| }; \| \| struct q_obj_ImageInfoSpecificVmdk_wrapper { \| ImageInfoSpecificVmdk data; \| }; ... \| struct ImageInfoSpecific { \| ImageInfoSpecificKind type; \| union { /* union tag is @type / \| void data; \|- ImageInfoSpecificQCow2 qcow2; \|- ImageInfoSpecificVmdk vmdk; \|+ q_obj_ImageInfoSpecificQCow2_wrapper qcow2; \|+ q_obj_ImageInfoSpecificVmdk_wrapper vmdk; \| } u; \| }; Doing this removes asymmetry between QAPI's QMP side and its C side (both sides now expose 'data'), and means that the treatment of a simple union as sugar for a flat union is now equivalent in both languages (previously the two approaches used a different layer of dereferencing, where the simple union could be converted to a flat union with equivalent C layout but different {} on the wire, or to an equivalent QMP wire form but with different C representation). Using the implicit type also lets us get rid of the simple_union_type() hack. Of course, now all clients of simple unions have to adjust from using su->u.member to using su->u.member.data; while this touches a number of files in the tree, some earlier cleanup patches helped minimize the change to the initialization of a temporary variable rather than every single member access. The generated qapi-visit.c code is also affected by the layout change: \|@@ -7393,10 +7393,10 @@ void visit_type_ImageInfoSpecific_member \| } \| switch (obj->type) { \| case IMAGE_INFO_SPECIFIC_KIND_QCOW2: \|- visit_type_ImageInfoSpecificQCow2(v, "data", &obj->u.qcow2, &err); \|+ visit_type_q_obj_ImageInfoSpecificQCow2_wrapper_members(v, &obj->u.qcow2, &err); \| break; \| case IMAGE_INFO_SPECIFIC_KIND_VMDK: \|- visit_type_ImageInfoSpecificVmdk(v, "data", &obj->u.vmdk, &err); \|+ visit_type_q_obj_ImageInfoSpecificVmdk_wrapper_members(v, &obj->u.vmdk, &err); \| break; \| default: \| abort(); Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1458254921-17042-13-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	12f254fd5f	qapi: Inline gen_visit_members() into lone caller Commit `82ca8e46` noticed that we had multiple implementations of visiting every member of a struct, and consolidated it into gen_visit_fields() (now gen_visit_members()) with enough parameters to cater to slight differences between the clients. But recent exposure of implicit types has meant that we are now down to a single use of that method, so we can clean up the unused conditionals and just inline it into the remaining caller: gen_visit_object_members(). Likewise, gen_err_check() no longer needs optional parameters, as the lone use of non-defaults was via gen_visit_members(). No change to generated code. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1458254921-17042-11-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	7ce106a96f	qapi: Emit implicit structs in generated C We already have several places that want to visit all the members of an implicit object within a larger context (simple union variant, event with anonymous data, command with anonymous arguments struct); and will be adding another one soon (the ability to declare an anonymous base for a flat union). Having a C struct declared for these implicit types, along with a visit_type_FOO_members() helper function, will make for fewer special cases in our generator. We do not, however, need qapi_free_FOO() or visit_type_FOO() functions for implicit types, because they should not be used directly outside of the generated code. This is done by adding a conditional in visit_object_type() for both qapi-types.py and qapi-visit.py based on the object name. The comparison of "name.startswith('q_')" is a bit hacky (it's basically duplicating what .is_implicit() already uses), but beats changing the signature of the visit_object_type() callback to pass a new 'implicit' flag. The hack should be temporary: we are considering adding a future patch that consolidates the narrow visit_object_type(..., base, local_members, variants) and visit_object_type_flat(..., all_members, variants) [where different sets of information are already broken out, and the QAPISchemaObjectType is no longer available] into a broader visit_object_type(obj_type) [where the visitor can query the needed fields from obj_type directly]. Also, now that we WANT to output C code for implicits, we no longer need the visit_needed() filter, leaving 'q_empty' as the only object still needing a special case. Remember, 'q_empty' is the only built-in generated object, which means that without a special case it would be emitted in multiple files (the main qapi-types.h and in qga-qapi-types.h) causing compilation failure due to redefinition. But since it has no members, it's easier to just avoid an attempt to visit that particular type; since gen_object() is called recursively, we also prime the objects_seen set to cover any recursion into the empty type. The patch relies on the changed naming of implicit types in the previous patch. It is a bit unfortunate that the generated struct names and visit_type_FOO_members() don't match normal naming conventions, but it's not too bad, since they will only be used in generated code. The generated code grows substantially in size: the implicit '-wrapper' types must be emitted in qapi-types.h before any union can include an unboxed member of that type. Arguably, the '-args' types could be emitted in a private header for just qapi-visit.c and qmp-marshal.c, rather than polluting qapi-types.h; but adding complexity to the generator to split the output location according to role doesn't seem worth the maintenance costs. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1458254921-17042-6-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	4d91e9115c	qapi-visit: Expose visit_type_FOO_members() Dan Berrange reported a case where he needs to work with a QCryptoBlockOptions union type using the OptsVisitor, but only visit one of the branches of that type (the discriminator is not visited directly, but learned externally). When things were boxed, it was easy: just visit the variant directly, which took care of both allocating the variant and visiting its members, then store that pointer in the union type. But now that things are unboxed, we need a way to visit the members without allocation, done by exposing visit_type_FOO_members() to the user. Before the patch, we had quite a bit of code associated with object_members_seen to make sure that a declaration of the helper was in scope before any use of the function. But now that the helper is public and declared in the header, the .c file no longer needs to worry about topological sorting (the helper is always in scope), which leads to some nice cleanups. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1457021813-10704-4-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	c81200b014	qapi: Rename 'fields' to 'members' in generated C code C types and JSON objects don't have fields, but members. We shouldn't gratuitously invent terminology. This patch is a strict renaming of static genarated functions, plus the naming of the dummy filler member for empty structs, before the next patch exposes some of that naming to the rest of the code base. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1457021813-10704-3-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	14f00c6c49	qapi: Rename 'fields' to 'members' in generator C types and JSON objects don't have fields, but members. We shouldn't gratuitously invent terminology. This patch is a strict renaming of generator code internals (including testsuite comments), before later patches rename C interfaces. No change to generated code with this patch. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1457021813-10704-2-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	dbf1192262	qapi: Change visit_start_implicit_struct to visit_start_alternate After recent changes, the only remaining use of visit_start_implicit_struct() is for allocating the space needed when visiting an alternate. Since the term 'implicit struct' is hard to explain, rename the function to its current usage. While at it, we can merge the functionality of visit_get_next_type() into the same function, making it more like visit_start_struct(). Generated code is now slightly smaller: \| { \| Error err = NULL; \| \|- visit_start_implicit_struct(v, (void) obj, sizeof(BlockdevRef), &err); \|+ visit_start_alternate(v, name, (GenericAlternate )obj, sizeof(obj), \|+ true, &err); \| if (err) { \| goto out; \| } \|- visit_get_next_type(v, name, &(obj)->type, true, &err); \|- if (err) { \|- goto out_obj; \|- } \| switch ((*obj)->type) { \| case QTYPE_QDICT: \| visit_start_struct(v, name, NULL, 0, &err); ... \| } \|-out_obj: \|- visit_end_implicit_struct(v); \|+ visit_end_alternate(v); \| out: \| error_propagate(errp, err); \| } Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-16-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	544a373159	qapi: Don't box branches of flat unions There's no reason to do two malloc's for a flat union; let's just inline the branch struct directly into the C union branch of the flat union. Surprisingly, fewer clients were actually using explicit references to the branch types in comparison to the number of flat unions thus modified. This lets us reduce the hack in qapi-types:gen_variants() added in the previous patch; we no longer need to distinguish between alternates and flat unions. The change to unboxed structs means that u.data (added in commit `cee2dedb`) is now coincident with random fields of each branch of the flat union, whereas beforehand it was only coincident with pointers (since all branches of a flat union have to be objects). Note that this was already the case for simple unions - but there we got lucky. Remember, visit_start_union() blindly returns true for all visitors except for the dealloc visitor, where it returns the value !!obj->u.data, and that this result then controls whether to proceed with the visit to the variant. Pre-patch, this meant that flat unions were testing whether the boxed pointer was still NULL, and thereby skipping visit_end_implicit_struct() and avoiding a NULL dereference if the pointer had not been allocated. The same was true for simple unions where the current branch had pointer type, except there we bypassed visit_type_FOO(). But for simple unions where the current branch had scalar type, the contents of that scalar meant that the decision to call visit_type_FOO() was data-dependent - the reason we got lucky there is that visit_type_FOO() for all scalar types in the dealloc visitor is a no-op (only the pointer variants had anything to free), so it did not matter whether the dealloc visit was skipped. But with this patch, we would risk leaking memory if we could skip a call to visit_type_FOO_fields() based solely on a data-dependent decision. But notice: in the dealloc visitor, visit_type_FOO() already handles a NULL obj - it was only the visit_type_implicit_FOO() that was failing to check for NULL. And now that we have refactored things to have the branch be part of the parent struct, we no longer have a separate pointer that can be NULL in the first place. So we can just delete the call to visit_start_union() altogether, and blindly visit the branch type; there is no change in behavior except to the dealloc visitor, where we now unconditionally visit the branch, but where that visit is now always safe (for a flat union, we can no longer dereference NULL, and for a simple union, visit_type_FOO() was already safely handling NULL on pointer types). Unfortunately, simple unions are not as easy to switch to unboxed layout; because we are special-casing the hidden implicit type with a single 'data' member, we really DO need to keep calling another layer of visit_start_struct(), with a second malloc; although there are some cleanups planned for simple unions in later patches. visit_start_union() and gen_visit_implicit_struct() are now unused. Drop them. Note that after this patch, the only remaining use of visit_start_implicit_struct() is for alternate types; the next patch will do further cleanup based on that fact. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-14-git-send-email-eblake@redhat.com> [Dead code deletion squashed in, commit message updated accordingly] Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	becceedc4d	qapi: Don't box struct branch of alternate There's no reason to do two malloc's for an alternate type visiting a QAPI struct; let's just inline the struct directly as the C union branch of the struct. Surprisingly, no clients were actually using the struct member prior to this patch outside of the testsuite; an earlier patch in the series added some testsuite coverage to make the effect of this patch more obvious. In qapi.py, c_type() gains a new is_unboxed flag to control when we are emitting a C struct unboxed within the context of an outer struct (different from our other two modes of usage with no flags for normal local variable declarations, and with is_param for adding 'const' in a parameter list). I don't know if there is any more pythonic way of collapsing the two flags into a single parameter, as we never have a caller setting both flags at once. Ultimately, we want to also unbox branches for QAPI unions, but as that touches a lot more client code, it is better as separate patches. But since unions and alternates share gen_variants(), I had to hack in a way to test if we are visiting an alternate type for setting the is_unboxed flag: look for a non-object branch. This works because alternates have at least two branches, with at most one object branch, while unions have only object branches. The hack will go away in a later patch. The generated code difference to qapi-types.h is relatively small: \| struct BlockdevRef { \| QType type; \| union { /* union tag is @type / \| void data; \|- BlockdevOptions definition; \|+ BlockdevOptions definition; \| char reference; \| } u; \| }; The corresponding spot in qapi-visit.c calls visit_type_FOO(), which first calls visit_start_struct() to allocate or deallocate the member and handle a layer of {} from the JSON stream, then visits the members. To peel off the indirection and the memory management that comes with it, we inline this call, then suppress allocation / deallocation by passing NULL to visit_start_struct(), and adjust the member visit: \| switch ((obj)->type) { \| case QTYPE_QDICT: \|- visit_type_BlockdevOptions(v, name, &(obj)->u.definition, &err); \|+ visit_start_struct(v, name, NULL, 0, &err); \|+ if (err) { \|+ break; \|+ } \|+ visit_type_BlockdevOptions_fields(v, &(obj)->u.definition, &err); \|+ error_propagate(errp, err); \|+ err = NULL; \|+ visit_end_struct(v, &err); \| break; \| case QTYPE_QSTRING: \| visit_type_str(v, name, &(obj)->u.reference, &err); The visit of non-object fields is unchanged. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-13-git-send-email-eblake@redhat.com> [Commit message tweaked] Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	2208d64998	qapi-visit: Use common idiom in gen_visit_fields_decl() We have several instances of methods that do an early exit if output is not needed, then log that output is being generated, and finally produce the output; see qapi-types.py:gen_object() and qapi-visit.py:gen_visit_implicit_struct(). The odd man out was gen_visit_fields_decl(); rearrange it to be more like the others. No semantic change or difference to generated code. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-12-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	e65d89bf1a	qapi: Adjust layout of FooList types By sticking the next pointer first, we don't need a union with 64-bit padding for smaller types. On 32-bit platforms, this can reduce the size of uint8List from 16 bytes (or 12, depending on whether 64-bit ints can tolerate 4-byte alignment) down to 8. It has no effect on 64-bit platforms (where alignment still dictates a 16-byte struct); but fewer anonymous unions is still a win in my book. It requires visit_next_list() to gain a size parameter, to know what size element to allocate; comparable to the size parameter of visit_start_struct(). I debated about going one step further, to allow for fewer casts, by doing: typedef GenericList GenericList; struct GenericList { GenericList next; }; struct FooList { GenericList base; Foo value; }; so that you convert to 'GenericList ' by '&foolist->base', and back by 'container_of(generic, GenericList, base)' (as opposed to the existing '(GenericList )foolist' and '(FooList )generic'). But doing that would require hoisting the declaration of GenericList prior to inclusion of qapi-types.h, rather than its current spot in visitor.h; it also makes iteration a bit more verbose through 'foolist->base.next' instead of 'foolist->next'. Note that for lists of objects, the 'value' payload is still hidden behind a boxed pointer. Someday, it would be nice to do: struct FooList { FooList next; Foo value; }; for one less level of malloc for each list element. This patch is a step in that direction (now that 'next' is no longer at a fixed non-zero offset within the struct, we can store more than just a pointer's-worth of data as the value payload), but the actual conversion would be a task for another series, as it will touch a lot of code. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-10-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	655519030b	qapi-visit: Less indirection in visit_type_Foo_fields() We were passing 'Foo *obj' to the internal helper function, but all uses within the helper were via reads of 'obj'. Refactor things to pass one less level of indirection, by having the callers dereference before calling. For an example of the generated code change: \|-static void visit_type_BalloonInfo_fields(Visitor v, BalloonInfo obj, Error errp) \|+static void visit_type_BalloonInfo_fields(Visitor v, BalloonInfo obj, Error errp) \| { \| Error err = NULL; \| \|- visit_type_int(v, "actual", &(obj)->actual, &err); \|+ visit_type_int(v, "actual", &obj->actual, &err); \| error_propagate(errp, err); \| } \| \|@@ -261,7 +261,7 @@ void visit_type_BalloonInfo(Visitor v, \| if (!obj) { \| goto out_obj; \| } \|- visit_type_BalloonInfo_fields(v, obj, &err); \|+ visit_type_BalloonInfo_fields(v, obj, &err); \| out_obj: The refactoring will also make it easier to reuse the helpers in a future patch when implicit structs are stored directly in the parent struct rather than boxed through a pointer. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-9-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Markus Armbruster	59d9e84cc9	qapi-visit: Unify struct and union visit gen_visit_union() is now just like gen_visit_struct(). Rename it to gen_visit_object(), use it for structs, and drop gen_visit_struct(). Output is unchanged. Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <1453902888-20457-4-git-send-email-armbru@redhat.com> [split out variant handling, rebase to earlier changes] Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-8-git-send-email-eblake@redhat.com>	11 years ago
Eric Blake	9a5cd424d5	qapi: Visit variants in visit_type_FOO_fields() We initially created the static visit_type_FOO_fields() helper function for reuse of code - we have cases where the initial setup for a visit has different allocation (depending on whether the fields represent a stand-alone type or are embedded as part of a larger type), but where the actual field visits are identical once a pointer is available. Up until the previous patch, visit_type_FOO_fields() was only used for structs (no variants), so it was covering every field for each type where it was emitted. Meanwhile, the code for visiting unions looks like: static visit_type_U_fields() { visit base; visit local_members; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit variants; visit_end_struct(); } which splits the fields of the union visit across two functions. Move the code to visit variants to live inside visit_type_U_fields(), while making it conditional on having variants so that all other instances of the helper function remain unchanged. This is also a step closer towards unifying struct and union visits, and towards allowing one union type to be the branch of another flat union. The resulting diff to the generated code is a bit hard to read, but it can be verified that it touches only union types, and that the end result is the following general structure: static visit_type_U_fields() { visit base; visit local_members; visit variants; } visit_type_U() { visit_start_struct(); visit_type_U_fields(); visit_end_struct(); } Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-7-git-send-email-eblake@redhat.com> [gen_visit_struct_fields() parameter variants made mandatory] Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Markus Armbruster	d7445b57f4	qapi-visit: Simplify how we visit common union members For a simple union SU, gen_visit_union() generates a visit of its single tag member, like this: visit_type_SUKind(v, "type", &(obj)->type, &err); For a flat union FU with base B, it generates a visit of its base fields: visit_type_B_fields(v, (B )obj, &err); Instead, we can simply visit the common members using the same fields visit function we use for structs, generated with gen_visit_struct_fields(). This function visits the base if any, then the local members. For a simple union SU, visit_type_SU_fields() contains exactly the old tag member visit, because there is no base, and the tag member is the only member. For instance, the code generated for qapi-schema.json's KeyValue changes like this: +static void visit_type_KeyValue_fields(Visitor v, KeyValue obj, Error errp) +{ + Error err = NULL; + + visit_type_KeyValueKind(v, "type", &(obj)->type, &err); + if (err) { + goto out; + } + +out: + error_propagate(errp, err); +} + void visit_type_KeyValue(Visitor v, const char name, KeyValue obj, Error errp) { Error err = NULL; @@ -4863,7 +4911,7 @@ void visit_type_KeyValue(Visitor v, con if (!obj) { goto out_obj; } - visit_type_KeyValueKind(v, "type", &(obj)->type, &err); + visit_type_KeyValue_fields(v, obj, &err); if (err) { goto out_obj; } For a flat union FU, visit_type_FU_fields() contains exactly the old base fields visit, because there is a base, but no members. For instance, the code generated for qapi-schema.json's CpuInfo changes like this: static void visit_type_CpuInfoBase_fields(Visitor v, CpuInfoBase obj, Error errp); +static void visit_type_CpuInfo_fields(Visitor v, CpuInfo obj, Error errp) +{ + Error err = NULL; + + visit_type_CpuInfoBase_fields(v, (CpuInfoBase )obj, &err); + if (err) { + goto out; + } + +out: + error_propagate(errp, err); +} + static void visit_type_CpuInfoX86_fields(Visitor v, CpuInfoX86 obj, Error errp) ... @@ -3485,7 +3509,7 @@ void visit_type_CpuInfo(Visitor v, cons if (!obj) { goto out_obj; } - visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, &err); + visit_type_CpuInfo_fields(v, obj, &err); if (err) { goto out_obj; } As you see, the generated code grows a bit, but in practice, it's lost in the noise: qapi-schema.json's qapi-visit.c gains roughly 1%. This simplification became possible with commit `441cbac` "qapi-visit: Convert to QAPISchemaVisitor, fixing bugs". It's a step towards unifying gen_struct() and gen_union(). Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <1453902888-20457-2-git-send-email-armbru@redhat.com> [improve commit message examples] Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455778109-6278-6-git-send-email-eblake@redhat.com> [Commit message tweaked]	11 years ago
Eric Blake	9d3524b39e	qapi-visit: Honor prefix of discriminator enum When we added support for a user-specified prefix for an enum type (commit `351d36e`), we forgot to teach the qapi-visit code to honor that prefix in the case of using a prefixed enum as the discriminator for a flat union. While there is still some on-list debate on whether we want to keep prefixes, we should at least make it work as long as it is still part of the code base. Reported-by: Daniel P. Berrange <berrange@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1455665965-27638-1-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	9167ebd98f	qapi: Clean up includes in generated files As a followup to commit `cbf2115`, clean up the includes in files generated by QAPI so that osdep.h is included first in .c files, and headers which it implies are not included manually. This patch is done manually, since Coccinelle (and therefore scripts/clean-includes) doesn't see into the generator scripts. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Eric Blake <eblake@redhat.com>	11 years ago
Eric Blake	08f9541dec	qapi: Drop unused error argument for list and implicit struct No backend was setting an error when ending the visit of a list or implicit struct, or when moving to the next list node. Make the callers a bit easier to follow by making this a part of the contract, and removing the errp argument - callers can then unconditionally end an object as part of cleanup without having to think about whether a second error is dominated by a first, because there is no second error. A later patch will then tackle the larger task of splitting visit_end_struct(), which can indeed set an error. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1454075341-13658-24-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	337283dffb	qapi: Drop unused 'kind' for struct/enum visit visit_start_struct() and visit_type_enum() had a 'kind' argument that was usually set to either the stringized version of the corresponding qapi type name, or to NULL (although some clients didn't even get that right). But nothing ever used the argument. It's even hard to argue that it would be useful in a debugger, as a stack backtrace also tells which type is being visited. Therefore, drop the 'kind' argument as dead. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <1454075341-13658-22-git-send-email-eblake@redhat.com> [Harmless rebase mistake cleaned up] Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	51e72bc1dd	qapi: Swap visit_* arguments for consistent 'name' placement JSON uses "name":value, but many of our visitor interfaces were called with visit_type_FOO(v, &value, name, errp). This can be a bit confusing to have to mentally swap the parameter order to match JSON order. It's particularly bad for visit_start_struct(), where the 'name' parameter is smack in the middle of the otherwise-related group of 'obj, kind, size' parameters! It's time to do a global swap of the parameter ordering, so that the 'name' parameter is always immediately after the Visitor argument. Additional reason in favor of the swap: the existing include/qjson.h prefers listing 'name' first in json_prop_(), and I have plans to unify that file with the qapi visitors; listing 'name' first in qapi will minimize churn to the (admittedly few) qjson.h clients. Later patches will then fix docs, object.h, visitor-impl.h, and those clients to match. Done by first patching scripts/qapi.py by hand to make generated files do what I want, then by running the following Coccinelle script to affect the rest of the code base: $ spatch --sp-file script `git grep -l '\bvisit_' -- '*/.[ch]'` I then had to apply some touchups (Coccinelle insisted on TAB indentation in visitor.h, and botched the signature of visit_type_enum() by rewriting 'const char const strings[]' to the syntactically invalid 'const charconst[] strings'). The movement of parameters is sufficient to provoke compiler errors if any callers were missed. // Part 1: Swap declaration order @@ type TV, TErr, TObj, T1, T2; identifier OBJ, ARG1, ARG2; @@ void visit_start_struct -(TV v, TObj OBJ, T1 ARG1, const char name, T2 ARG2, TErr errp) +(TV v, const char name, TObj OBJ, T1 ARG1, T2 ARG2, TErr errp) { ... } @@ type bool, TV, T1; identifier ARG1; @@ bool visit_optional -(TV v, T1 ARG1, const char name) +(TV v, const char name, T1 ARG1) { ... } @@ type TV, TErr, TObj, T1; identifier OBJ, ARG1; @@ void visit_get_next_type -(TV v, TObj OBJ, T1 ARG1, const char name, TErr errp) +(TV v, const char name, TObj OBJ, T1 ARG1, TErr errp) { ... } @@ type TV, TErr, TObj, T1, T2; identifier OBJ, ARG1, ARG2; @@ void visit_type_enum -(TV v, TObj OBJ, T1 ARG1, T2 ARG2, const char name, TErr errp) +(TV v, const char name, TObj OBJ, T1 ARG1, T2 ARG2, TErr errp) { ... } @@ type TV, TErr, TObj; identifier OBJ; identifier VISIT_TYPE =~ "^visit_type_"; @@ void VISIT_TYPE -(TV v, TObj OBJ, const char name, TErr errp) +(TV v, const char name, TObj OBJ, TErr errp) { ... } // Part 2: swap caller order @@ expression V, NAME, OBJ, ARG1, ARG2, ERR; identifier VISIT_TYPE =~ "^visit_type_"; @@ ( -visit_start_struct(V, OBJ, ARG1, NAME, ARG2, ERR) +visit_start_struct(V, NAME, OBJ, ARG1, ARG2, ERR) \| -visit_optional(V, ARG1, NAME) +visit_optional(V, NAME, ARG1) \| -visit_get_next_type(V, OBJ, ARG1, NAME, ERR) +visit_get_next_type(V, NAME, OBJ, ARG1, ERR) \| -visit_type_enum(V, OBJ, ARG1, ARG2, NAME, ERR) +visit_type_enum(V, NAME, OBJ, ARG1, ARG2, ERR) \| -VISIT_TYPE(V, OBJ, NAME, ERR) +VISIT_TYPE(V, NAME, OBJ, ERR) ) Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <1454075341-13658-19-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	395a233f7c	qapi: Don't cast Enum* to int* C compilers are allowed to represent enums as a smaller type than int, if all enum values fit in the smaller type. There are even compiler flags that force the use of this smaller representation, although using them changes the ABI of a binary. Therefore, our generated code for visit_type_ENUM() (for all qapi enums) was wrong for casting Enum* to int* when calling visit_type_enum(). It appears that no one has been using compiler ABI switches for qemu, because if they had, we are potentially dereferencing beyond bounds or even risking a SIGBUS on platforms where unaligned pointer dereferencing is fatal. But it is still better to avoid the practice entirely, and just use the correct types. This matches the fix for alternate qapi types, done earlier in commit `0426d53` "qapi: Simplify visiting of alternate types", with generated code changing as: \| void visit_type_QType(Visitor v, QType obj, const char name, Error errp) \| { \|- visit_type_enum(v, (int )obj, QType_lookup, "QType", name, errp); \|+ int value = obj; \|+ visit_type_enum(v, &value, QType_lookup, "QType", name, errp); \|+ obj = value; \| } Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <1454075341-13658-17-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	7c91aabd89	qapi-visit: Kill unused visit_end_union() The generated code can call visit_end_union() without having called visit_start_union(). Example: if (!obj) { goto out_obj; } visit_type_CpuInfoBase_fields(v, (CpuInfoBase )obj, &err); if (err) { goto out_obj; // if we go from here... } if (!visit_start_union(v, !!(obj)->u.data, &err) \|\| err) { goto out_obj; } switch ((obj)->arch) { [...] } out_obj: // ... then obj is true, and ... error_propagate(errp, err); err = NULL; if (obj) { // we end up here visit_end_union(v, !!(obj)->u.data, &err); } error_propagate(errp, err); Harmless only because no visitor implements end_union(). Clean it up anyway, by deleting the function as useless. Messed up since we have visit_end_union (commit `cee2ded`). Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <1453902888-20457-3-git-send-email-armbru@redhat.com> [expand scope of patch to delete rather than repair] Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1454075341-13658-13-git-send-email-eblake@redhat.com>	11 years ago
Eric Blake	92b09babc1	qapi: Track all failures between visit_start/stop Inside the generated code between visit_start_struct() and visit_end_struct(), we were blindly setting the error into the caller's errp parameter. But a future patch to split visit_end_struct() will require that we take action based on whether an error has occurred, which requires us to track all actions through a local err. Rewrite the visits to be more in line with the other generated calls. Generated code changes look like: \| visit_start_struct(v, (void *)obj, "Abort", name, sizeof(Abort), &err); \|- if (!err) { \|- if (obj) { \|- visit_type_Abort_fields(v, obj, errp); \|- } \|- visit_end_struct(v, &err); \|+ if (err) { \|+ goto out; \| } \|+ if (!*obj) { \|+ goto out_obj; \|+ } \|+ visit_type_Abort_fields(v, obj, &err); \|+ error_propagate(errp, err); \|+ err = NULL; \|+out_obj: \|+ visit_end_struct(v, &err); \|+out: \| error_propagate(errp, err); \| } Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1454075341-13658-12-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	d00341af38	qapi: Fix alternates that accept 'number' but not 'int' The QMP input visitor allows integral values to be assigned by promotion to a QTYPE_QFLOAT. However, when parsing an alternate, we did not take this into account, such that an alternate that accepts 'number' and some other type, but not 'int', would reject integral values. With this patch, we now have the following desirable table: alternate has case selected for 'int' 'number' QTYPE_QINT QTYPE_QFLOAT no no error error no yes 'number' 'number' yes no 'int' error yes yes 'int' 'number' While it is unlikely that we will ever use 'number' in an alternate other than in the testsuite, it never hurts to be more precise in what we allow. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1449033659-25497-8-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	0426d53c65	qapi: Simplify visiting of alternate types Previously, working with alternates required two lookup arrays and some indirection: for type Foo, we created Foo_qtypes[] which maps each qtype to a value of the generated FooKind enum, then look up that value in FooKind_lookup[] like we do for other union types. This has a couple of subtle bugs. First, the generator was creating a call with a parameter '(int ) &(obj)->type' where type is an enum type; this is unsafe if the compiler chooses to store the enum type in a different size than int, where assigning through the wrong size pointer can corrupt data or cause a SIGBUS. Related bug, not not fixed in this patch: qapi-visit.py's gen_visit_enum() generates a cast of its enum * argument to int . Marked FIXME. Second, since the values of the FooKind enum start at zero, all entries of the Foo_qtypes[] array that were not explicitly initialized will map to the same branch of the union as the first member of the alternate, rather than triggering a desired failure in visit_get_next_type(). Fortunately, the bug seldom bites; the very next thing the input visitor does is try to parse the incoming JSON with the wrong parser, which normally fails; the output visitor is not used with a C struct in that state, and the dealloc visitor has nothing to clean up (so there is no leak). However, the second bug IS observable in one case: parsing an integer causes unusual behavior in an alternate that contains at least a 'number' member but no 'int' member, because the 'number' parser accepts QTYPE_QINT in addition to the expected QTYPE_QFLOAT (that is, since 'int' is not a member, the type QTYPE_QINT accidentally maps to FooKind 0; if this enum value is the 'number' branch the integer parses successfully, but if the 'number' branch is not first, some other branch tries to parse the integer and rejects it). A later patch will worry about fixing alternates to always parse all inputs that a non-alternate 'number' would accept, for now this is still marked FIXME in the updated test-qmp-input-visitor.c, to merely point out that new undesired behavior of 'ans' matches the existing undesired behavior of 'asn'. This patch fixes the default-initialization bug by deleting the indirection, and modifying get_next_type() to directly assign a QTypeCode parameter. This in turn fixes the type-casting bug, as we are no longer casting a pointer to enum to a questionable size. There is no longer a need to generate an implicit FooKind enum associated with the alternate type (since the QMP wire format never uses the stringized counterparts of the C union member names). Since the updated visit_get_next_type() does not know which qtypes are expected, the generated visitor is modified to generate an error statement if an unexpected type is encountered. Callers now have to know the QTYPE_ mapping when looking at the discriminator; but so far, only the testsuite was even using the C struct of an alternate types. I considered the possibility of keeping the internal enum FooKind, but initialized differently than most generated arrays, as in: typedef enum FooKind { FOO_KIND_A = QTYPE_QDICT, FOO_KIND_B = QTYPE_QINT, } FooKind; to create nicer aliases for knowing when to use foo->a or foo->b when inspecting foo->type; but it turned out to add too much complexity, especially without a client. There is a user-visible side effect to this change, but I consider it to be an improvement. Previously, the invalid QMP command: {"execute":"blockdev-add", "arguments":{"options": {"driver":"raw", "id":"a", "file":true}}} failed with: {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'file', expected: QDict"}} (visit_get_next_type() succeeded, and the error comes from the visit_type_BlockdevOptions() expecting {}; there is no mention of the fact that a string would also work). Now it fails with: {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'file', expected: BlockdevRef"}} (the error when the next type doesn't match any expected types for the overall alternate). Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1449033659-25497-5-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	7264f5c50c	qapi: Convert QType into QAPI built-in enum type What's more meta than using qapi to define qapi? :) Convert QType into a full-fledged[] builtin qapi enum type, so that a subsequent patch can then use it as the discriminator type of qapi alternate types. Fortunately, the judicious use of 'prefix' in the qapi definition avoids churn to the spelling of the enum constants. To avoid circular definitions, we have to flip the order of inclusion between "qobject.h" vs. "qapi-types.h". Back in commit `28770e0`, we had the latter include the former, so that we could use 'QObject ' for our implementation of 'any'. But that usage also works with only a forward declaration, whereas the definition of QObject requires QType to be a complete type. [*] The type has to be builtin, rather than declared in qapi/common.json, because we want to use it for alternates even when common.json is not included. But since it is the first builtin enum type, we have to add special cases to qapi-types and qapi-visit to only emit definitions once, even when two qapi files are being compiled into the same binary (the way we already handled builtin list types like 'intList'). We may need to revisit how multiple qapi files share common types, but that's a project for another day. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1449033659-25497-4-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	da34a9bd99	qapi: Track simple union tag in object.local_members We were previously creating all unions with an empty list for local_members. However, it will make it easier to unify struct and union generation if we include the generated tag member in local_members. That way, we can have a common code pattern: visit the base (if any), visit the local members (if any), visit the variants (if any). The local_members of a flat union remains empty (because the discriminator is already visited as part of the base). Then, by visiting tag_member.check() during AlternateType.check(), we no longer need to call it during Variants.check(). The various front end entities now exist as follows: struct: optional base, optional local_members, no variants simple union: no base, one-element local_members, variants with tag_member from local_members flat union: base, no local_members, variants with tag_member from base alternate: no base, no local_members, variants With the new local members, we require a bit of finesse to avoid assertions in the clients. No change to generated code. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1447836791-369-2-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	dd5ee2c2d3	qapi: Test failure in middle of array parse Our generated list visitors have the same problem as has been mentioned elsewhere (see commit `2f52e20`): they allocate data even on failure. An upcoming patch will correct things to provide saner guarantees, but first we need to expose the behavior in the testsuite to ensure we aren't introducing any memory usage bugs. There are more test cases throughout the test-qmp-input-* tests that already deal with partial allocation; a later commit will clean up all visit_type_FOO(), without marking all of the tests with FIXME at this time. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1446791754-23823-10-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	150d0564a4	qapi-visit: Convert to new qapi union layout We have two issues with our qapi union layout: 1) Even though the QMP wire format spells the tag 'type', the C code spells it 'kind', requiring some hacks in the generator. 2) The C struct uses an anonymous union, which places all tag values in the same namespace as all non-variant members. This leads to spurious collisions if a tag value matches a non-variant member's name. Make the conversion to the new layout for qapi-visit.py. Generated code changes look like: \|@@ -4912,16 +4912,16 @@ void visit_type_MemoryDeviceInfo(Visitor \| if (!obj) { \| goto out_obj; \| } \|- visit_type_MemoryDeviceInfoKind(v, &(obj)->kind, "type", &err); \|+ visit_type_MemoryDeviceInfoKind(v, &(obj)->type, "type", &err); \| if (err) { \| goto out_obj; \| } \|- if (!visit_start_union(v, !!(obj)->data, &err) \|\| err) { \|+ if (!visit_start_union(v, !!(obj)->u.data, &err) \|\| err) { \| goto out_obj; \| } \|- switch ((obj)->kind) { \|+ switch ((obj)->type) { \| case MEMORY_DEVICE_INFO_KIND_DIMM: \|- visit_type_PCDIMMDeviceInfo(v, &(obj)->dimm, "data", &err); \|+ visit_type_PCDIMMDeviceInfo(v, &(obj)->u.dimm, "data", &err); \| break; \| default: \| abort(); \|@@ -4930,7 +4930,7 @@ out_obj: \| error_propagate(errp, err); \| err = NULL; \| if (obj) { \|- visit_end_union(v, !!(obj)->data, &err); \|+ visit_end_union(v, !!(obj)->u.data, &err); \| } \| error_propagate(errp, err); \| err = NULL; Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1445898903-12082-14-git-send-email-eblake@redhat.com> [Commit message tweaked slightly] Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	5c5e51a05b	qapi-visit: Remove redundant functions for flat union base The code for visiting the base class of a child struct created visit_type_Base_fields() which covers all fields of Base; while the code for visiting the base class of a flat union created visit_type_Union_fields() covering all fields of the base except the discriminator. But since the base class includes the discriminator of a flat union, we can just visit the entire base, without needing a separate visit of the discriminator. Not only is consistently visiting all fields easier to understand, it lets us share code. The generated code in qapi-visit.c loses several now-unused visit_type_UNION_fields(), along with changes like: \|@@ -1654,11 +1557,7 @@ void visit_type_BlockdevOptions(Visitor \| if (!obj) { \| goto out_obj; \| } \|- visit_type_BlockdevOptions_fields(v, obj, &err); \|- if (err) { \|- goto out_obj; \|- } \|- visit_type_BlockdevDriver(v, &(obj)->driver, "driver", &err); \|+ visit_type_BlockdevOptionsBase_fields(v, (BlockdevOptionsBase )obj, &err); \| if (err) { \| goto out_obj; \| } and forward declarations where needed. Note that the cast of obj to BASE is necessary to call visit_type_BASE_fields() (and we can't use our upcast wrappers, because those work on pointers while we have a pointer-to-pointer). Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1445898903-12082-12-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	ddf2190896	qapi: Unbox base members Rather than storing a base class as a pointer to a box, just store the fields of that base class in the same order, so that a child struct can be directly cast to its parent. This gives less malloc overhead, less pointer dereferencing, and even less generated code. Compare to the earlier commit `1e6c1616a` "qapi: Generate a nicer struct for flat unions" (although that patch had fewer places to change, as less of qemu was directly using qapi structs for flat unions). It also allows us to turn on automatic type-safe wrappers for upcasting to the base class of a struct. Changes to the generated code look like this in qapi-types.h: \| struct SpiceChannel { \|- SpiceBasicInfo base; \|+ / Members inherited from SpiceBasicInfo: / \|+ char host; \|+ char port; \|+ NetworkAddressFamily family; \|+ / Own members: / \| int64_t connection_id; as well as additional upcast functions like qapi_SpiceChannel_base(). Meanwhile, changes to qapi-visit.c look like: \| static void visit_type_SpiceChannel_fields(Visitor v, SpiceChannel obj, Error errp) \| { \| Error err = NULL; \| \|- visit_type_implicit_SpiceBasicInfo(v, &(obj)->base, &err); \|+ visit_type_SpiceBasicInfo_fields(v, (SpiceBasicInfo *)obj, &err); \| if (err) { (the cast is necessary, since our upcast wrappers only deal with a single pointer, not pointer-to-pointer); plus the wholesale elimination of some now-unused visit_type_implicit_FOO() functions. Without boxing, the corner case of one empty struct having another empty struct as its base type now requires inserting a dummy member (previously, the 'Base base' member sufficed). And now that we no longer consume a 'base' member in the generated C struct, we can delete the former negative struct-base-clash-base test. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1445898903-12082-11-git-send-email-eblake@redhat.com> [Commit message tweaked slightly] Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	d02cf37766	qapi-visit: Split off visit_type_FOO_fields forward decl We generate a static visit_type_FOO_fields() for every type FOO. However, sometimes we need a forward declaration. Split the code to generate the forward declaration out of gen_visit_implicit_struct() into a new gen_visit_fields_decl(), and also prepare for a forward declaration to be emitted during gen_visit_struct(), so that a future patch can switch from using visit_type_FOO_implicit() to the simpler visit_type_FOO_fields() as part of unboxing the base class of a struct. No change to generated code. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1445898903-12082-8-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	f9e6102b48	qapi: More robust conditions for when labels are needed We were using regular expressions to see if ret included any earlier text that emitted a 'goto out;' line, to decide whether we needed to output an 'out:' label. But this is fragile, if the ret text can possibly combine more than one generated function body, where the first function used a goto but the second does not. Change the code to just check for the known conditions which cause an error check to be needed. Besides, it's slightly more efficient to use plain checks than regular expression searching. No change to generated code. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1445898903-12082-4-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	49823c4b43	qapi: Don't use info as witness of implicit object type A future patch will enable error reporting from the various QAPISchema*.check() methods. But to report an error related to an implicit type, we'll need to associate a location with the type (the same location as the top-level entity that is causing the creation of the implicit type), and once we do that, keying off of whether foo.info exists is no longer a viable way to determine if foo is an implicit type. Instead, add an is_implicit() method to QAPISchemaEntity, and use it. It can be overridden later for ObjectType and EnumType, when implicit instances of those classes gain info. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1444710158-8723-8-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	25a0d9c977	qapi: Use predicate callback to determine visit filtering Previously, qapi-types and qapi-visit filtered out implicit objects during visit_object_type() by using 'info' (works since implicit objects do not [yet] have associated info); meanwhile qapi-introspect filtered out all schema types on the first pass by returning a python type from visit_begin(), which was then used at a distance in QAPISchema.visit() to do the filtering. Rather than keeping these ad hoc approaches, add a new visitor callback visit_needed() which returns False to skip a given entity, and which defaults to True unless overridden. Use the new mechanism to simplify all three filtering visitors. No change to the generated code. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1444710158-8723-2-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	d08ac81a45	qapi: Fix regression with '-netdev help' Commit `e36c714e` causes 'qemu -netdev help' to dump core, because the call to visit_end_union() is no longer conditional on whether *obj was allocated. Reported by Marc-André Lureau <marcandre.lureau@gmail.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1444861825-19256-1-git-send-email-eblake@redhat.com> [Commit message tweaked to say 'help' instead of '?'] Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	82ca8e4696	qapi: Share gen_visit_fields() Consolidate the code between visit, command marshalling, and event generation that iterates over the members of a struct. It reduces code duplication in the generator, so that a future patch can reduce the size of generated code while touching only one instead of three locations. There are no changes to the generated marshal code. The visitor code becomes slightly more verbose, but remains semantically equivalent, and is actually easier to read as it follows a more common idiom: \| visit_optional(v, &(obj)->has_device, "device", &err); \|- if (!err && (obj)->has_device) { \|- visit_type_str(v, &(obj)->device, "device", &err); \|- } \| if (err) { \| goto out; \| } \|+ if ((obj)->has_device) { \|+ visit_type_str(v, &(*obj)->device, "device", &err); \|+ if (err) { \|+ goto out; \|+ } \|+ } The event code becomes slightly more verbose, but this is arguably a bug fix: although the visitors are not well documented, use of an optional member should not be attempted unless guarded by a prior call to visit_optional(). Works only because the output qmp visitor has a no-op visit_optional(): \|+ visit_optional(v, &has_offset, "offset", &err); \|+ if (err) { \|+ goto out; \|+ } \| if (has_offset) { \| visit_type_int(v, &offset, "offset", &err); Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1443565276-4535-17-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	1f35334489	qapi: Share gen_err_check() qapi-commands has a nice helper gen_err_check(), but did not use it everywhere. In fact, using it in more places makes it easier to reduce the lines of code used for generating error checks. This in turn will make it easier for later patches to consolidate another common pattern among the generators. The generated code has fewer blank lines in qapi-event.c functions, but has no semantic difference. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1443565276-4535-16-git-send-email-eblake@redhat.com> [Drop another blank line for symmetry] Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	05372f708a	qapi: Consistent generated code: minimize push_indent() usage We had some pointless differences in the generated code for visit, command marshalling, and events; unifying them makes it easier for future patches to consolidate to common helper functions. This is one patch of a series to clean up these differences. This patch reduces the number of push_indent()/pop_indent() pairs so that generated code is typically already at its natural output indentation in the python files. It is easier to reason about generated code if the reader does not have to track how much spacing will be inserted alongside the code, and moreso when all of the generators use the same patterns (qapi-type and qapi-event were already using in-place indentation). Arguably, the resulting python may be a bit harder to read with C code at the same indentation as python; on the other hand, not having to think about push_indent() is a win, and most decent editors provide syntax highlighting that makes it easier to visually distinguish python code from string literals that will become C code. There is no change to the generated output. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1443565276-4535-15-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago
Eric Blake	e36c714e6a	qapi: Consistent generated code: prefer common indentation We had some pointless differences in the generated code for visit, command marshalling, and events; unifying them makes it easier for future patches to consolidate to common helper functions. This is one patch of a series to clean up these differences. This patch adjusts gen_visit_union() to use the same indentation as other functions, namely, by jumping early to the error label if the object was not set rather than placing the rest of the body inside an if for when it is set. No change in semantics to the generated code. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1443565276-4535-14-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>	11 years ago

1 2 3

119 Commits (243e0fe5506d6b830e01d8da727ff7f20fa38502)