[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v14 19/19] qapi: Change visit_type_FOO() to no l
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v14 19/19] qapi: Change visit_type_FOO() to no longer return partial objects |
Date: |
Wed, 27 Apr 2016 15:51:39 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 04/15/2016 08:49 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
>
>> Returning a partial object on error is an invitation for a careless
>> caller to leak memory. 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.
>
> Hmm, wasn't the "assign null on error" part done in a prior patch?
> Checking... no, only half of it, in PATCH 03: there, we went from "may
> store an incomplete object on error" to "store either an incomplete
> object or null on error". Now we go on to just "store null on error".
> Correct?
Yes. I'll tweak the wording to make it clearer.
>
>> The decision is done by enhancing qapi-visit-core to return true
>> for input visitors (the callbacks themselves do not need
>> modification); since we've documented that visit_end_* must be
>> called after any successful visit_start_*, that is a sufficient
>> point for knowing that something was allocated during start.
>
> I find this sentence a bit confusing. Let me try:
>
> To help visitor-agnostic code, such as the generated qapi-visit.c,
> make the visit_end_FOO() return true when something was allocated.
> Easily done in the visitor core, no need to change the callbacks.
>
> But see my comments on the visit_end_FOO() inline.
Reply below, where your comments are indeed worth thinking about.
>
>> 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).
>
> Should this note be in PATCH 03?
>
> The inconsistency isn't pretty, but tolerable if it simplifies things.
No. Patch 03 fixed visit_start_struct, not visit_type_FOO. Since it is
this patch that is tweaking visit_type_FOO, I have to explain why
visit_type_ENUM preserves values, while visit_type_OBJ sets to NULL.
>> *
>> - * FIXME: At present, input visitors may allocate an incomplete
>> address@hidden
>> - * even when visit_type_FOO() reports an error. Using an output
>> - * visitor with an incomplete object has undefined behavior; callers
>> - * must call qapi_free_FOO() (which uses the dealloc visitor, and
>> - * safely handles an incomplete object) to avoid a memory leak.
>> + * If an error is detected during visit_type_FOO() with an input
>> + * visitor, then address@hidden will be NULL for pointer types, and left
>> + * unchanged for scalar types.
>
> Okay.
And this matches the commit message explaining the difference between
scalar and object (and also applies to visit_type_int being a scalar
that leaves the value unchanged on error).
>
>> + * Using an output visitor with an
>> + * incomplete object has undefined behavior (other than a special case
>> + * for visit_type_str() treating NULL like ""), while the dealloc
>> + * visitor safely handles incomplete objects.
>
> Where do the incomplete objects come from now? I thought this patch
> gets rid of them.
Still possible to create one by manual means, just no longer possible
from a QAPI input visitor. I'll tweak the wording.
>> -void visit_end_struct(Visitor *v);
>> +bool visit_end_struct(Visitor *v);
>
> I generally like functions to return something useful, but not in this
> case, because the function name gives you no clue about its value.
> Consider:
>
> if (visit_end_struct(v) && err) {
> qapi_free_FOO(*obj);
> *obj = NULL;
> }
>
> To find out what this means, a reader not familiar with visitors almost
> certainly needs to refer to visit_end_struct()'s contract or code.
>
> Compare:
>
> visit_end_struct(v);
> if (err && v->type == VISITOR_INPUT) {
v->type is a layering violation...
> qapi_free_FOO(*obj);
> *obj = NULL;
> }
>
> Or:
>
> visit_end_struct(v);
> if (err && visit_is_input(v)) {
...but this is doable by exporting visit_is_input().
> qapi_free_FOO(*obj);
> *obj = NULL;
> }
Makes the generated code have more lines, but who really cares. So
consider it done in v15.
>> +++ b/qapi/qapi-visit-core.c
>> @@ -23,11 +23,17 @@
>> void visit_start_struct(Visitor *v, const char *name, void **obj,
>> size_t size, Error **errp)
>> {
>> + Error *err = NULL;
>> +
>> if (obj) {
>> assert(size);
>> assert(v->type != VISITOR_OUTPUT || *obj);
>> }
>> - v->start_struct(v, name, obj, size, errp);
>> + v->start_struct(v, name, obj, size, &err);
>> + if (obj && v->type == VISITOR_INPUT) {
>> + assert(err || *obj);
>> + }
>> + error_propagate(errp, err);
>
> Sure this belongs to this patch? More of the same below.
Hmm. Patch 3 was the one that tightened semantics on visit_start, so I
can certainly try to hoist the added assertions there.
>> static void test_validate_fail_alternate(TestInputVisitorData *data,
>> const void *unused)
>> {
>> - UserDefAlternate *tmp = NULL;
>> + UserDefAlternate *tmp;
>
> Did this initialization become dead in PATCH 03 already?
Probably :)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v14 18/19] qapi: Simplify semantics of visit_next_list(), (continued)
[Qemu-devel] [PATCH v14 17/19] qmp-input: Require struct push to visit members of top dict, Eric Blake, 2016/04/08
[Qemu-devel] [PATCH v14 19/19] qapi: Change visit_type_FOO() to no longer return partial objects, Eric Blake, 2016/04/08
[Qemu-devel] [PATCH v14 14/19] qapi: Split visit_end_struct() into pieces, Eric Blake, 2016/04/08
Re: [Qemu-devel] [PATCH v14 00/19] qapi visitor cleanups (post-introspection cleanups subset E), Markus Armbruster, 2016/04/15