summaryrefslogtreecommitdiff
path: root/include/qapi
AgeCommit message (Collapse)AuthorFilesLines
2016-03-22Use scripts/clean-includes to drop redundant qemu/typedefs.hMarkus Armbruster1-1/+0
Re-run scripts/clean-includes to apply the previous commit's corrections and updates. Besides redundant qemu/typedefs.h, this only finds a redundant config-host.h include in ui/egl-helpers.c. No idea how that escaped the previous runs. Some manual whitespace trimming around dropped includes squashed in. Signed-off-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
2016-03-22include/qemu/osdep.h: Don't include qapi/error.hMarkus Armbruster1-5/+0
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>
2016-03-17qapi: Drop QERR_UNKNOWN_BLOCK_FORMAT_FEATUREMax Reitz1-3/+0
Just specifying a custom string is simpler in basically all places that used it, and in addition, specifying the BB or node name is something we generally do not do in other error messages when opening a BDS, so we should not do it here. This changes the output for iotest 036 (to the better, in my opinion), so the reference output needs to be changed accordingly. Signed-off-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2016-02-23include: Clean up includesPeter Maydell15-22/+0
Clean up includes so that osdep.h is included first and headers which it implies are not included manually. This commit was created with scripts/clean-includes. NB: If this commit breaks compilation for your out-of-tree patchseries or fork, then you need to make sure you add #include "qemu/osdep.h" to any new .c files that you have. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Eric Blake <eblake@redhat.com>
2016-02-19qapi: Change visit_start_implicit_struct to visit_start_alternateEric Blake2-19/+47
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>
2016-02-19qapi: Don't box branches of flat unionsEric Blake2-3/+0
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>
2016-02-19qapi: Adjust layout of FooList typesEric Blake2-8/+7
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>
2016-02-09Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2016-02-09' ↵Peter Maydell1-4/+11
into staging Error reporting patches for 2016-02-09 # gpg: Signature made Tue 09 Feb 2016 12:38:33 GMT using RSA key ID EB918653 # gpg: Good signature from "Markus Armbruster <armbru@redhat.com>" # gpg: aka "Markus Armbruster <armbru@pond.sub.org>" * remotes/armbru/tags/pull-error-2016-02-09: HACKING: Add a section on error handling and reporting error: Improve documentation some more Use error_fatal to simplify obvious fatal errors (again) Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
2016-02-09error: Improve documentation some moreMarkus Armbruster1-4/+11
Don't claim error_report_err() always reports to stderr. It actually reports to the current monitor when we have one. Clarify intended use of error_abort and error_fatal. Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <1454522628-28294-2-git-send-email-armbru@redhat.com> Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu>
2016-02-08qapi: Drop unused error argument for list and implicit structEric Blake2-6/+11
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>
2016-02-08qapi: Drop unused 'kind' for struct/enum visitEric Blake2-10/+6
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>
2016-02-08qapi: Swap 'name' in visit_* callbacks to match public APIEric Blake1-18/+21
As explained in the previous patches, matching argument order of 'name, &value' to JSON's "name":value makes sense. However, while the last two patches were easy with Coccinelle, I ended up doing this one all by hand. Now all the visitor callbacks match the main interface. The compiler is able to enforce that all clients match the changed interface in visitor-impl.h, even where two pointers are being swapped, because only one of the two pointers is const (if that were not the case, then C's looseness on treating 'char *' like 'void *' would have made review a bit harder). Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <1454075341-13658-21-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-08qapi: Swap visit_* arguments for consistent 'name' placementEric Blake1-21/+31
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 char*const[] 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>
2016-02-08qapi: Consolidate visitor small integer callbacksEric Blake1-7/+1
Commit 4e27e819 introduced optional visitor callbacks for all sorts of int types, but no visitor has supplied any of the callbacks for sizes less than 64 bits. In other words, the generic implementation based on using type_[u]int64() followed by bounds-checking works just fine. In the interest of simplicity, it's easier to make the visitor callback interface not have to worry about the other sizes. Adding some helper functions minimizes the boilerplate required to correct FIXMEs added earlier with regards to questionable reuse of errp, particularly now that we can guarantee from a single file audit that value is unchanged if an error is set. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <1454075341-13658-16-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-08qapi: Make all visitors supply uint64 callbacksEric Blake1-3/+6
Our qapi visitor contract supports multiple integer visitors, but left the type_uint64 visitor as optional (falling back on type_int64); which in turn can lead to awkward behavior with numbers larger than INT64_MAX (the user has to be aware of twos complement, and deal with negatives). This patch does not address the disparity in handling large values as negatives. It merely moves the fallback from uint64 to int64 from the visitor core to the visitors, where the issue can actually be fixed, by implementing the missing type_uint64() callbacks on top of the respective type_int64() callbacks, and with a FIXME comment explaining why that's wrong. With that done, we now have a type_uint64() callback in every driver, so we can make it mandatory from the core. And although the type_int64() callback can cover the entire valid range of type_uint{8,16,32} on valid user input, using type_uint64() to avoid mixed signedness makes more sense. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1454075341-13658-15-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-08qapi: Prefer type_int64 over type_int in visitorsEric Blake1-2/+4
The qapi builtin type 'int' is basically shorthand for the type 'int64'. In fact, since no visitor was providing the optional type_int64() callback, visit_type_int64() was just always falling back to type_int(), cementing the equivalence between the types. However, some visitors are providing a type_uint64() callback. For purposes of code consistency, it is nicer if all visitors use the paired type_int64/type_uint64 names rather than the mismatched type_int/type_uint64. So this patch just renames the signed int callbacks in place, dropping the type_int() callback as redundant, and a later patch will focus on the unsigned int callbacks. Add some FIXMEs to questionable reuse of errp in code touched by the rename, while at it (the reuse works as long as the callbacks don't modify value when setting an error, but it's not a good example to set) - a later patch will then fix those. No change in functionality here, although further cleanups are in the pipeline. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1454075341-13658-14-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2016-02-08qapi-visit: Kill unused visit_end_union()Eric Blake2-2/+0
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>
2016-01-13error: New error_prepend(), error_reportf_err()Markus Armbruster1-2/+29
Instead of simply propagating an error verbatim, we sometimes want to add to its message, like this: frobnicate(arg, &err); error_setg(errp, "Can't frobnicate %s: %s", arg, error_get_pretty(err)); error_free(err); This is suboptimal, because it loses err's hint (if any). Moreover, when errp is &error_abort or is subsequently propagated to &error_abort, the abort message points to the place where we last added to the error, not to the place where it originated. To avoid these issues, provide means to add to an error's message in place: frobnicate(arg, errp); error_prepend(errp, "Can't frobnicate %s: ", arg); Likewise, reporting an error like frobnicate(arg, &err); error_report("Can't frobnicate %s: %s", arg, error_get_pretty(err)); can lose err's hint. To avoid: error_reportf_err(err, "Can't frobnicate %s: ", arg); The next commits will put these functions to use. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <1450452927-8346-10-git-send-email-armbru@redhat.com>
2016-01-13error: Improve documentationMarkus Armbruster1-2/+18
While there, tighten error_append_hint()'s assertion. Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <1450452927-8346-6-git-send-email-armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
2016-01-13error: Document how to accumulate multiple errorsMarkus Armbruster1-0/+17
Suggested-by: Eric Blake <eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <1447776349-2344-1-git-send-email-armbru@redhat.com>
2015-12-17qapi: Shorter visits of optional fieldsEric Blake1-2/+2
For less code, reflect the determined boolean value of an optional visit back to the caller instead of making the caller read the boolean after the fact. The resulting generated code has the following diff: |- visit_optional(v, &has_fdset_id, "fdset-id"); |- if (has_fdset_id) { |+ if (visit_optional(v, &has_fdset_id, "fdset-id")) { | visit_type_int(v, &fdset_id, "fdset-id", &err); | if (err) { | goto out; | } | } Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1449033659-25497-10-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-12-17qapi: Simplify visits of optional fieldsEric Blake2-5/+10
None of the visitor callbacks would set an error when testing if an optional field was present; make this part of the interface contract by eliminating the errp argument. The resulting generated code has a nice diff: |- visit_optional(v, &has_fdset_id, "fdset-id", &err); |- if (err) { |- goto out; |- } |+ visit_optional(v, &has_fdset_id, "fdset-id"); | if (has_fdset_id) { | visit_type_int(v, &fdset_id, "fdset-id", &err); | if (err) { | goto out; | } | } Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1449033659-25497-9-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-12-17qapi: Fix alternates that accept 'number' but not 'int'Eric Blake2-2/+3
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>
2015-12-17qapi: Simplify visiting of alternate typesEric Blake2-2/+9
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>
2015-12-17qapi: Convert QType into QAPI built-in enum typeEric Blake1-16/+5
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>
2015-12-17qobject: Rename qtype_code to QTypeEric Blake1-4/+4
The name QType matches our CODING_STYLE conventions for type names in CamelCase. It also matches the fact that we are already naming all the enum members with a prefix of QTYPE, not QTYPE_CODE. And doing the rename will also make it easier for the next patch to use QAPI for providing the enum, which also wants CamelCase type names. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1449033659-25497-3-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-12-17qobject: Simplify QObjectEric Blake7-16/+21
The QObject hierarchy is small enough, and unlikely to grow further (since we only use it to map to JSON and already cover all JSON types), that we can simplify things by not tracking a separate vtable, but just inline the code element of the vtable QType directly into QObject (renamed to type), and track a separate array of destroy functions. We can drop qnull_destroy_obj() in the process. The remaining QObject subclasses must export their destructor. This also has the nice benefit of moving the typename 'QType' out of the way, so that the next patch can repurpose it for a nicer name for 'qtype_code'. The various objects are still the same size (so no change in cache line pressure), but now have less indirection (although I didn't bother benchmarking to see if there is a noticeable speedup, as we don't have hard evidence that this was in a performance hotspot in the first place). A future patch could drop the refcnt size to 32 bits for a smaller struct on 64-bit architectures, if desired (we have limits on the largest JSON that we are willing to parse, and will probably never need to take full advantage of a 64-bit refcnt). Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1449033659-25497-2-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-12-17qapi: Change munging of CamelCase enum valuesEric Blake1-6/+6
When munging enum values, the fact that we were passing the entire prefix + value through camel_to_upper() meant that enum values spelled with CamelCase could be turned into CAMEL_CASE. However, this provides a potential collision (both OneTwo and One-Two would munge into ONE_TWO) for enum types, when the same two names are valid side-by-side as QAPI member names. By changing the generation of enum constants to always be prefix + '_' + c_name(value, False).upper(), and ensuring that there are no case collisions (in the next patches), we no longer have to worry about names that would be distinct as QAPI members but collide as variant tag names, without having to think about what munging the heuristics in camel_to_upper() will actually perform on an enum value. Making the change will affect enums that did not follow coding conventions, using 'CamelCase' rather than desired 'lower-case'. Thankfully, there are only two culprits: InputButton and ErrorClass. We already tweaked ErrorClass to make it an alias of QapiErrorClass, where only the alias needs changing rather than the whole tree. So the bulk of this change is modifying INPUT_BUTTON_WHEEL_UP to the new INPUT_BUTTON_WHEELUP (and likewise for WHEELDOWN). That part of this commit may later need reverting if we rename the enum constants from 'WheelUp' to 'wheel-up' as part of moving x-input-send-event to a stable interface; but at least we have documentation bread crumbs in place to remind us (commit 513e7cd), and it matches the fact that SDL constants are also spelled SDL_BUTTON_WHEELUP. Suggested by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1447836791-369-27-git-send-email-eblake@redhat.com> [Commit message tweaked] Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-12-17qapi: Add alias for ErrorClassEric Blake1-0/+14
The qapi enum ErrorClass is unusual that it uses 'CamelCase' names, contrary to our documented convention of preferring 'lower-case'. However, this enum is entrenched in the API; we cannot change what strings QMP outputs. Meanwhile, we want to simplify how c_enum_const() is used to generate enum constants, by moving away from the heuristics of camel_to_upper() to a more straightforward c_name(N).upper() - but doing so will rename all of the ErrorClass constants and cause churn to all client files, where the new names are aesthetically less pleasing (ERROR_CLASS_DEVICENOTFOUND looks like we can't make up our minds on whether to break between words). So as always in computer science, solve the problem by some more indirection: rename the qapi type to QapiErrorClass, and add a new enum ErrorClass in error.h whose members are aliases of the qapi type, but with the spelling expected elsewhere in the tree. Then, when c_enum_const() changes the munging, we only have to adjust the one alias spot. Suggested by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1447836791-369-26-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-12-17qapi: Remove dead visitor codeEric Blake1-3/+0
Commit cbc95538 removed unused start_handle() and end_handle(), but forgot to remove their declarations. Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1447836791-369-19-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-26qjson: surprise, allocating 6 QObjects per token is expensivePaolo Bonzini1-0/+7
Replace the contents of the tokens GQueue with a simple struct. This cuts the amount of memory allocated by tests/check-qjson from ~500MB to ~20MB, and the execution time from 600ms to 80ms on my laptop. Still a lot (some could be saved by using an intrusive list, such as QSIMPLEQ, instead of the GQueue), but the savings are already massive and the right thing to do would probably be to get rid of json-streamer completely. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <1448300659-23559-5-git-send-email-pbonzini@redhat.com> [Straightforwardly rebased on my patches] Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
2015-11-26qjson: store tokens in a GQueuePaolo Bonzini2-6/+6
Even though we still have the "streamer" concept, the tokens can now be deleted as they are read. While doing so convert from QList to GQueue, since the next step will make tokens not a QObject and we will have to do the conversion anyway. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <1448300659-23559-4-git-send-email-pbonzini@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
2015-11-26qjson: replace QString in JSONLexer with GStringPaolo Bonzini2-4/+5
JSONLexer only needs a simple resizable buffer. json-streamer.c can allocate memory for each token instead of relying on reference counting of QStrings. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <1448300659-23559-2-git-send-email-pbonzini@redhat.com> [Straightforwardly rebased on my patches, checkpatch made happy] Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
2015-11-26qjson: Give each of the six structural chars its own token typeMarkus Armbruster1-1/+6
Simplifies things, because we always check for a specific one. Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <1448486613-17634-6-git-send-email-armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
2015-11-26qjson: Spell out some silent assumptionsMarkus Armbruster1-1/+2
Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <1448486613-17634-5-git-send-email-armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
2015-11-10qapi: Simplify error cleanup in test-qmp-*Eric Blake1-0/+9
We have several tests that perform multiple sub-actions that are expected to fail. Asserting that an error occurred, then clearing it up to prepare for the next action, turned into enough boilerplate that it was sometimes forgotten (for example, a number of tests added to test-qmp-input-visitor.c in d88f5fd leaked err). Worse, if an error is not reset to NULL, we risk invalidating later use of that error (passing a non-NULL err into a function is generally a bad idea). Encapsulate the boilerplate into a single helper function error_free_or_abort(), and consistently use it. The new function is added into error.c for use everywhere, although it is anticipated that testsuites will be the main client. Signed-off-by: Eric Blake <eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-09qobject: Protect against use-after-free in qobject_decref()Eric Blake1-0/+1
Adding an assertion to qobject_decref() will ensure that a programming error causing use-after-free will result in immediate failure (provided no other thread has started using the memory) instead of silently attempting to wrap refcnt around and leaving the problem to potentially bite later at a harder point to diagnose. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1446791754-23823-4-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-11-06replay: replay blockers for devicesPavel Dovgalyuk1-0/+3
Some devices are not supported by record/replay subsystem. This patch introduces replay blocker which denies starting record/replay if such devices are included into the configuration. Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> Message-Id: <20150917162512.8676.11367.stgit@PASHA-ISP.def.inno> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
2015-10-29qobject: Drop QObject_HEADMarkus Armbruster7-10/+6
QObject_HEAD is a macro expanding into the common part of structs that are sub-types of QObject. It's always been just QObject base, and unlikely to change. Drop the macro, because the code is clearer with out it. Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <1444918537-18107-2-git-send-email-armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
2015-09-21qapi: Introduce a first class 'any' typeMarkus Armbruster2-0/+3
It's first class, because unlike '**', it actually works, i.e. doesn't require 'gen': false. '**' will go away next. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
2015-09-18error: New error_fatalMarkus Armbruster1-0/+11
Similar to error_abort, but doesn't report where the error was created, and terminates the process with exit(1) rather than abort(). Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <1441983105-26376-2-git-send-email-armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
2015-09-18hmp: Allow for error message hints on HMPEric Blake1-0/+7
Commits 7216ae3d and d2828429 disabled some error message hints, all because a change to use modern error reporting meant that the hint would be output prior to the actual error. Fix this by making hints a first-class member of Error. For example, we are now back to the pleasant: $ qemu-system-x86_64 --nodefaults -S --vnc :0 --chardev null,id=, qemu-system-x86_64: --chardev null,id=,: Parameter 'id' expects an identifier Identifiers consist of letters, digits, '-', '.', '_', starting with a letter. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Message-Id: <1441901956-21991-1-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
2015-09-10error: On abort, report where the error was createdMarkus Armbruster1-9/+34
This is particularly useful when we abort in error_propagate(), because there the stack backtrace doesn't lead to where the error was created. Looks like this: Unexpected error in parse_block_error_action() at .../qemu/blockdev.c:322: qemu-system-x86_64: -drive if=none,werror=foo: 'foo' invalid write error action Aborted (core dumped) Note: to get this example output, I monkey-patched drive_new() to pass &error_abort to blockdev_init(). To keep the error handling boiler plate from growing even more, all error_setFOO() become macros expanding into error_setFOO_internal() with additional __FILE__, __LINE__, __func__ arguments. Not exactly pretty, but it works. The macro trickery breaks down when you take the address of an error_setFOO(). Fortunately, we do that in just one place: qemu-ga's Windows VSS provider and requester DLL wants to call error_setg_win32() through a function pointer "to avoid linking glib to the DLL". Use error_setg_win32_internal() there. The use of the function pointer is already wrapped in a macro, so the churn isn't bad. Code size increases by some 35KiB for me (0.7%). Tolerable. Could be less if we passed relative rather than absolute source file names to the compiler, or forwent reporting __func__. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Acked-by: Laszlo Ersek <lersek@redhat.com>
2015-09-10error: Revamp interface documentationMarkus Armbruster1-47/+124
Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
2015-09-10error: error_set_errno() is unused, dropMarkus Armbruster1-5/+2
Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
2015-09-10qga: Clean up unnecessarily dirty castsMarkus Armbruster1-7/+2
qga_vss_fsfreeze() casts error_set_win32() from void (*)(Error **, int, ErrorClass, const char *, ...) to void (*)(void **, int, int, const char *, ...) The result is later called. Since the two types are not compatible, the call is undefined behavior. It works in practice anyway. However, there's no real need for trickery here. Clean it up as follows: * Declare struct Error, and fix the first parameter. * Switch to error_setg_win32(). This gets rid of the troublesome ErrorClass parameter. Requires converting error_setg_win32() from macro to function, but that's trivially easy, because this is the only user of error_set_win32(). Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
2015-09-10error: Make error_setg() a functionMarkus Armbruster1-2/+2
Saves a tiny amount of code at every call site. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
2015-06-22qerror: Move #include out of qerror.hMarkus Armbruster1-3/+0
Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
2015-06-22qerror: Finally unused, clean upMarkus Armbruster2-18/+2
Remove it except for two things in qerror.h: * Two #include to be cleaned up separately to avoid cluttering this patch. * The QERR_ macros. Mark as obsolete. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
2015-06-22qerror: Clean up QERR_ macros to expand into a single stringMarkus Armbruster1-30/+30
These macros expand into error class enumeration constant, comma, string. Unclean. Has been that way since commit 13f59ae. The error class is always ERROR_CLASS_GENERIC_ERROR since the previous commit. Clean up as follows: * Prepend every use of a QERR_ macro by ERROR_CLASS_GENERIC_ERROR, and delete it from the QERR_ macro. No change after preprocessing. * Rewrite error_set(ERROR_CLASS_GENERIC_ERROR, ...) into error_setg(...). Again, no change after preprocessing. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
generated by cgit v1.2.3 (git 2.25.1) at 2024-11-27 03:09:42 +0000