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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 07/01/2015 02:22 PM, Markus Armbruster wrote:
>> is_c_ptr() looks whether the end of the C text for the type looks like
>> a pointer.  Works, but is fragile.
>> 
>> We now have a better tool: use QAPISchemaType method c_null().  The
>> initializers for non-pointers become prettier: 0, false or the
>> enumeration constant with the value 0 instead of {0}.
>> 
>> One place looks suspicious: we initialize pointers, but not
>> non-pointers.  Either the initialization is superfluous and should be
>> deleted, or the non-pointers need it as well, or something subtle is
>> going on and needs a comment.  Since I lack the time to figure it out
>> now, mark it FIXME.
>
> I commented on that in another mail - either we have a leak on failure
> (and need the initializer only for pointers), or we should add an assert
> (and don't need the initializer at all), depending on what semantics we
> want to enforce on all handler functions that set their error parameter.
>  Probably worth cleaning that up in a separate pre-req patch.

A function call should either fail completely or succeed completely.

On success, it must not set an error (d'oh!).

On failure, it must set an error (d'oh again!), and should not return
stuff for the caller to clean up.

The rules on setting errors together with the rule that a &local_err
argument must contain null before the call ensures that afterwards
local_err is set exactly when the call failed.

The rule on returning stuff lets us keep the error path simple: no need
for cleaning up anything returned by the function.  This is pervasive in
the code.  We generally don't bother to assert() the function actually
obeys the rule.

Perhaps I should fix up my pending patch "error: Revamp interface
documentation"[*] to cover the "should not return stuff for the caller
to clean up" part.

Apply to handler functions returning objects in need of clean up:

        retval = qmp_FOO(... args ..., &local_err);
        if (local_err) {
            goto out;
        }

        qmp_marshal_output_FOO(retval, ret, &local_err);

On success, qmp_FOO() must not touch local_err.

On failure, qmp_FOO() must set local_err, and return null.

Bypassing the cleanup qmp_marshal_output_FOO() is therfore perfectly
fine.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi-commands.py | 19 +++++++------------
>>  scripts/qapi.py          |  3 ---
>>  2 files changed, 7 insertions(+), 15 deletions(-)
>> 
>
> Nice reduction in size.  If the marshal code FIXME disappears due to a
> separate cleanup, then the rest of this commit is good to go:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!


[*] https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg04756.html