Re: [Qemu-devel] [PATCH v14 19/19] qapi: Change visit_type_FOO() to no longer return partial objects

[Eric Blake]

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