Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/28/2015 01:34 AM, Markus Armbruster wrote:
>> Let me rephrase to make sure I understand.
>> 
>> Ignore the (not rets) case, because retval doesn't exist then.
>> 
>> qmp_marshal_output_FOO() visits retval twice.  First, with a QMP output
>> visitor to do the actual marshalling.  Second, with a QAPI dealloc
>> visitor to destroy it.
>
> And the use of the dealloc visitor is buried inside the
> qmp_marshal_output_FOO() call.
>
>> 
>> If we execute the assignment to retval, we must go on to call
>> qmp_marshal_output_FOO(), or else we have a leak.
>> 
>> If we can reach qmp_marshal_output_FOO() without executing the
>> assignment, we must initialize retval.  If we can't, any initialization
>> is unused.
>> 
>> gen_call() generates code of the form
>> 
>>         retval = qmp_FOO(... args ..., &local_err);
>>         if (local_err) {
>>             goto out;
>>         }
>> 
>>         qmp_marshal_output_FOO(retval, ret, &local_err);
>> 
>
>> Observe:
>> 
>> 1. The assignment dominates the only use.  Therefore, the initialization
>>    is unused.  Let's drop it in a separate cleanup patch.
>> 
>> 2. We can leak retval only when qmp_FOO() returns non-null and local_err
>>    is non-null.  This must not happen, because:
>> 
>>    a. local_err must be null before the call, and
>> 
>>    b. the call must not return non-null when it sets local_err.
>
> We don't state that contract anywhere, but I doubt any of the qmp_FOO()
> functions violate it, so it is worth making it part of the contract.

It's a general Error API rule: set an error exactly on failure.  It
applies to any function returning errors through an Error **errp
parameter, and we generally don't bother to spell it out for the
individual functions.

The part that needs to be spelling out is what success and failure mean.
A qmp_FOO() returning an object returns null on failure.

>>    We could right after out: assert(!local_err || !retval).  Not sure
>>    it's worthwhile.
>
> I think it IS worthwhile, because it would catch buggy callers.  Not

We use the same assumption all over the place, without asserting it.

The boilerplate around calls using the Error API is bad enough as it is.
I'm reluctant to make it worse by adding assertions.

Less of an issue in generated code, but violations are even less likely
there.

However, I'm reluctant to assert in some places, but not all, because
inconsistency breeds confusion.

In general, I prefer a function's pre- and post-conditions to be
asserted in the function itself, not in its callers.  Asserting selected
parts of the post-condition in callers can be occasionally helpful to
help human readers and static analyzers lacking insight into the
function.

> sure if after out: is the right place (then you'd need an initializer to
> cover any other code that jumps to out), but this would do the same
>
> retval = qmp_FOO(...);
> if (local_err) {
>     assert(!retval);
>     goto out;
> }
> qmp_marshal_output_FOO(retval, ...);

Not quite the same: it doesn't catch the case !local_err && !retval.
But it doesn't matter, because that case is bound to die in
qmp_marshal_output_FOO().

>> TL;DR: I concur with your analysis.
>
> Is it worth dropping the dead initializer and adding the assert in the
> same pre-req cleanup patch?  Do you want me to submit it since I did the
> analysis?

Let's drop the useless initializer.  As explained above, I don't like
the assertion for reasons explained above, but if you want it, I'm
willing to take it anyway, in a separate follow-up patch.

I'd prefer to drop the initializer myself myself (with you credited in
the commit message), because it's certainly less total work, and quite
possibly less work for just for me.