On 04/15/2016 08:49 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> 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 *@obj
>> - * 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 *@obj 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
