37f9e0a2 | 09-Jun-2016 |
Eric Blake <eblake@redhat.com> |
sockets: Use new QAPI cloning
Rather than rolling our own clone via an expensive conversion in and back out of QObject, use the new clone visitor.
Signed-off-by: Eric Blake <eblake@redhat.com> Mess
sockets: Use new QAPI cloning
Rather than rolling our own clone via an expensive conversion in and back out of QObject, use the new clone visitor.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1465490926-28625-15-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
show more ...
|
a15fcc3c | 09-Jun-2016 |
Eric Blake <eblake@redhat.com> |
qapi: Add new clone visitor
We have a couple places in the code base that want to deep-clone one QAPI object into another, and they were resorting to serializing the struct out to QObject then repar
qapi: Add new clone visitor
We have a couple places in the code base that want to deep-clone one QAPI object into another, and they were resorting to serializing the struct out to QObject then reparsing it. A much more efficient version can be done by adding a new clone visitor.
Since cloning is still relatively uncommon, expose the use of the new visitor via a QAPI_CLONE() macro that takes care of type-punning the underlying function pointer, rather than generating lots of unused functions for types that won't be cloned. And yes, we're relying on the compiler treating all pointers equally, even though a strict C program cannot portably do so - but we're not the first one in the qemu code base to expect it to work (hello, glib!).
The choice of adding a fourth visitor type deserves some explanation. On the surface, the clone visitor is mostly an input visitor (it takes arbitrary input - in this case, another QAPI object - and creates a new QAPI object during the course of the visit). But ever since commit da72ab0 consolidated enum visits based on the visitor type, using VISITOR_INPUT would cause us to run visit_type_str(), even though for cloning there is nothing to do (we just copy the enum value across, without regards to its mapping to strings). Also, since our input happens to be a QAPI object, we can also satisfy the internal checks for VISITOR_OUTPUT. So in the end, I settled with a new VISITOR_CLONE, and chose its value such that many internal checks can use 'v->type & mask', sticking to 'v->type == value' where the difference matters.
Note that we can only clone objects (including alternates) and lists, not built-ins or enums. The visitor core hides integer width from the actual visitor (since commit 04e070d), and as long as that's the case, we can't clone top-level integers. Then again, those can always be cloned by direct copy, since they are not objects with deep pointers, so it's no real loss. And restricting cloning to just objects and lists is cleaner than restricting it to non-integers. As such, I documented that the clone visitor is for direct use only by code internal to QAPI, and should not be used on incomplete objects (other than a hack to work around the fact that we allow NULL in place of "" in visit_type_str() in other output visitors). Note that as written, the clone visitor will never fail on a complete object.
Scalars (including enums) not at the root of the clone copy just fine with no additional effort while visiting the scalar, by virtue of a g_memdup() each time we push another struct onto the stack. Cloning a string requires deduplication of a pointer, which means it can also provide the guarantee of an input visitor of never producing NULL even when still accepting NULL in place of "" the way the QMP output visitor does.
Cloning an 'any' type could be possible by incrementing the QObject refcnt, but it's not obvious whether that is better than implementing a QObject deep clone. So for now, we document it as unsupported, and intentionally omit the .type_any() callback to let a developer know their usage needs implementation.
Add testsuite coverage for several different clone situations, to ensure that the code is working. I also tested that valgrind was happy with the test.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1465490926-28625-14-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
show more ...
|
3b098d56 | 09-Jun-2016 |
Eric Blake <eblake@redhat.com> |
qapi: Add new visit_complete() function
Making each output visitor provide its own output collection function was the only remaining reason for exposing visitor sub-types to the rest of the code bas
qapi: Add new visit_complete() function
Making each output visitor provide its own output collection function was the only remaining reason for exposing visitor sub-types to the rest of the code base. Add a polymorphic visit_complete() function which is a no-op for input visitors, and which populates an opaque pointer for output visitors. For maximum type-safety, also add a parameter to the output visitor constructors with a type-correct version of the output pointer, and assert that the two uses match.
This approach was considered superior to either passing the output parameter only during construction (action at a distance during visit_free() feels awkward) or only during visit_complete() (defeating type safety makes it easier to use incorrectly).
Most callers were function-local, and therefore a mechanical conversion; the testsuite was a bit trickier, but the previous cleanup patch minimized the churn here.
The visit_complete() function may be called at most once; doing so lets us use transfer semantics rather than duplication or ref-count semantics to get the just-built output back to the caller, even though it means our behavior is not idempotent.
Generated code is simplified as follows for events:
|@@ -26,7 +26,7 @@ void qapi_event_send_acpi_device_ost(ACP | QDict *qmp; | Error *err = NULL; | QMPEventFuncEmit emit; |- QmpOutputVisitor *qov; |+ QObject *obj; | Visitor *v; | q_obj_ACPI_DEVICE_OST_arg param = { | info |@@ -39,8 +39,7 @@ void qapi_event_send_acpi_device_ost(ACP | | qmp = qmp_event_build_dict("ACPI_DEVICE_OST"); | |- qov = qmp_output_visitor_new(); |- v = qmp_output_get_visitor(qov); |+ v = qmp_output_visitor_new(&obj); | | visit_start_struct(v, "ACPI_DEVICE_OST", NULL, 0, &err); | if (err) { |@@ -55,7 +54,8 @@ void qapi_event_send_acpi_device_ost(ACP | goto out; | } | |- qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov)); |+ visit_complete(v, &obj); |+ qdict_put_obj(qmp, "data", obj); | emit(QAPI_EVENT_ACPI_DEVICE_OST, qmp, &err);
and for commands:
| { | Error *err = NULL; |- QmpOutputVisitor *qov = qmp_output_visitor_new(); | Visitor *v; | |- v = qmp_output_get_visitor(qov); |+ v = qmp_output_visitor_new(ret_out); | visit_type_AddfdInfo(v, "unused", &ret_in, &err); |- if (err) { |- goto out; |+ if (!err) { |+ visit_complete(v, ret_out); | } |- *ret_out = qmp_output_get_qobject(qov); |- |-out: | error_propagate(errp, err);
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1465490926-28625-13-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
show more ...
|
1830f22a | 09-Jun-2016 |
Eric Blake <eblake@redhat.com> |
qmp-output-visitor: Favor new visit_free() function
Now that we have a polymorphic visit_free(), we no longer need qmp_output_visitor_cleanup(); however, we still need to expose the subtype for qmp_
qmp-output-visitor: Favor new visit_free() function
Now that we have a polymorphic visit_free(), we no longer need qmp_output_visitor_cleanup(); however, we still need to expose the subtype for qmp_output_get_qobject().
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1465490926-28625-10-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
show more ...
|
e7ca5656 | 09-Jun-2016 |
Eric Blake <eblake@redhat.com> |
string-output-visitor: Favor new visit_free() function
Now that we have a polymorphic visit_free(), we no longer need string_output_visitor_cleanup(); however, we still need to expose the subtype fo
string-output-visitor: Favor new visit_free() function
Now that we have a polymorphic visit_free(), we no longer need string_output_visitor_cleanup(); however, we still need to expose the subtype for string_output_get_string().
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1465490926-28625-9-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
show more ...
|
b70ce101 | 09-Jun-2016 |
Eric Blake <eblake@redhat.com> |
qmp-input-visitor: Favor new visit_free() function
Now that we have a polymorphic visit_free(), we no longer need qmp_input_visitor_cleanup(); which in turn means we no longer need to return a subty
qmp-input-visitor: Favor new visit_free() function
Now that we have a polymorphic visit_free(), we no longer need qmp_input_visitor_cleanup(); which in turn means we no longer need to return a subtype from qmp_input_visitor_new() nor a public upcast function.
Generated code changes to qmp-marshal.c look like:
|@@ -52,11 +52,10 @@ void qmp_marshal_add_fd(QDict *args, QOb | { | Error *err = NULL; | AddfdInfo *retval; |- QmpInputVisitor *qiv = qmp_input_visitor_new(QOBJECT(args), true); | Visitor *v; | q_obj_add_fd_arg arg = {0}; | |- v = qmp_input_get_visitor(qiv); |+ v = qmp_input_visitor_new(QOBJECT(args), true); | visit_start_struct(v, NULL, NULL, 0, &err); | if (err) { | goto out;
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1465490926-28625-8-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
show more ...
|
7a0525c7 | 09-Jun-2016 |
Eric Blake <eblake@redhat.com> |
string-input-visitor: Favor new visit_free() function
Now that we have a polymorphic visit_free(), we no longer need string_input_visitor_cleanup(); which in turn means we no longer need to return a
string-input-visitor: Favor new visit_free() function
Now that we have a polymorphic visit_free(), we no longer need string_input_visitor_cleanup(); which in turn means we no longer need to return a subtype from string_input_visitor_new() nor a public upcast function.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1465490926-28625-7-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
show more ...
|
2c0ef9f4 | 09-Jun-2016 |
Eric Blake <eblake@redhat.com> |
qapi: Add new visit_free() function
Making each visitor provide its own (awkwardly-named) FOO_cleanup() is unusual, when we can instead have a polymorphic visit_free() interface. Over the next few
qapi: Add new visit_free() function
Making each visitor provide its own (awkwardly-named) FOO_cleanup() is unusual, when we can instead have a polymorphic visit_free() interface. Over the next few patches, we can use the polymorphic functions to eliminate the need for a FOO_get_visitor() function for accessing specific visitor functionality, once everything can be accessed directly through the Visitor* interfaces.
The dealloc visitor is the first one converted to completely use the new entry point, since qapi_dealloc_visitor_cleanup() was the only reason that qapi_dealloc_get_visitor() existed, and only generated and testsuite code was even using it. With the new visit_free() entry point in place, we no longer need to expose the QapiDeallocVisitor subtype through qapi_dealloc_visitor_new(), and can get by with less generated code, with diffs that look like:
| void qapi_free_ACPIOSTInfo(ACPIOSTInfo *obj) | { |- QapiDeallocVisitor *qdv; | Visitor *v; | | if (!obj) { | return; | } | |- qdv = qapi_dealloc_visitor_new(); |- v = qapi_dealloc_get_visitor(qdv); |+ v = qapi_dealloc_visitor_new(); | visit_type_ACPIOSTInfo(v, NULL, &obj, NULL); |- qapi_dealloc_visitor_cleanup(qdv); |+ visit_free(v); |}
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1465490926-28625-5-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
show more ...
|
1158bb2a | 09-Jun-2016 |
Eric Blake <eblake@redhat.com> |
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 ju
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>
show more ...
|
68ab47e4 | 28-Apr-2016 |
Eric Blake <eblake@redhat.com> |
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 pa
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>
show more ...
|
d9f62dde | 28-Apr-2016 |
Eric Blake <eblake@redhat.com> |
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); p
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>
show more ...
|
15c2f669 | 28-Apr-2016 |
Eric Blake <eblake@redhat.com> |
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 ma
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, ¶m, &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>
show more ...
|
3bc97fd5 | 28-Apr-2016 |
Eric Blake <eblake@redhat.com> |
qapi: Add visit_type_null() visitor
Right now, qmp-output-visitor happens to produce a QNull result if nothing is actually visited between the creation of the visitor and the request for the resulti
qapi: Add visit_type_null() visitor
Right now, qmp-output-visitor happens to produce a QNull result if nothing is actually visited between the creation of the visitor and the request for the resulting QObject. A stronger protocol would require that a QMP output visit MUST visit something. But to still be able to produce a JSON 'null' output, we need a new visitor function that states our intentions. Yes, we could say that such a visit must go through visit_type_any(), but that feels clunky.
So this patch introduces the new visit_type_null() interface and its no-op interface in the dealloc visitor, and stubs in the qmp visitors (the next patch will finish the implementation). For the visitors that will not implement the callback, document the situation. The code in qapi-visit-core unconditionally dereferences the callback pointer, so that a segfault will inform a developer if they need to implement the callback for their choice of visitor.
Note that JSON has a primitive null type, with the single value null; likewise with the QNull type for QObject; but for QAPI, we just have the 'null' value without a null type. We may eventually want to add more support in QAPI for null (most likely, we'd use it via an alternate type that permits 'null' or an object); but we'll create that usage when we need it.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-15-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
show more ...
|
adfb264c | 28-Apr-2016 |
Eric Blake <eblake@redhat.com> |
qapi: Document visitor interfaces, add assertions
The visitor interface for mapping between QObject/QemuOpts/string and QAPI is scandalously under-documented, making changes to visitor core, individ
qapi: Document visitor interfaces, add assertions
The visitor interface for mapping between QObject/QemuOpts/string and QAPI is scandalously under-documented, making changes to visitor core, individual visitors, and users of visitors difficult to coordinate. Among other questions: when is it safe to pass NULL, vs. when a string must be provided; which visitors implement which callbacks; the difference between concrete and virtual visits.
Correct this by retrofitting proper contracts, and document where some of the interface warts remain (for example, we may want to modify visit_end_* to require the same 'obj' as the visit_start counterpart, so the dealloc visitor can be simplified). Later patches in this series will tackle some, but not all, of these warts.
Add assertions to (partially) enforce the contract. Some of these were only made possible by recent cleanup commits.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-13-git-send-email-eblake@redhat.com> [Doc fix from Eric squashed in] Signed-off-by: Markus Armbruster <armbru@redhat.com>
show more ...
|
fc471c18 | 28-Apr-2016 |
Eric Blake <eblake@redhat.com> |
qapi: Consolidate QMP input visitor creation
Rather than having two separate ways to create a QMP input visitor, where the safer approach has the more verbose name, it is better to consolidate thing
qapi: Consolidate QMP input visitor creation
Rather than having two separate ways to create a QMP input visitor, where the safer approach has the more verbose name, it is better to consolidate things into a single function where the caller must explicitly choose whether to be strict or to ignore excess input. This patch is the strictly mechanical conversion; the next patch will then audit which uses can be made stricter.
Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <1461879932-9020-6-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com>
show more ...
|