Re: [PATCH] qapi: define cleanup function for g_autoptr(Error)

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 13/09/21 07:23, Markus Armbruster wrote:
>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> 
>>>> Allow replacing calls to error_free() with g_autoptr(Error)
>>>> declarations.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>   include/qapi/error.h | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>>> index 4a9260b0cc..8564657baf 100644
>>>> --- a/include/qapi/error.h
>>>> +++ b/include/qapi/error.h
>>>> @@ -437,6 +437,8 @@ Error *error_copy(const Error *err);
>>>>    */
>>>>   void error_free(Error *err);
>>>>   +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
>>>> +
>>>>   /*
>>>>    * Convenience function to assert that *@errp is set, then silently free 
>>>> it.
>>>>    */
>>> I'd like to see at least one actual use.
>>
>> I'll have one soon, I'll Cc you on that one.  (I wrote this because
>> Dan suggested using g_autoptr(Error) in a review, but it doesn't work
>> yet).
>
> I recommend to squash this patch into its first user, or maybe put it
> right before it.

I went for a walk, and now I have more substantial comments.

I'm not sure g_autoptr() is a good match for the Error interface in its
current shape.  Let me explain.

Use of error_free() is relatively rare: a bit over 200 calls outside
tests/, compared to more than 4000 error_setg*().  This is because the
most common ways to clean up are propagation and reporting, not
error_free().

As is, reporting errors doesn't play well with g_autoptr().  Example:

    Error *err = NULL;

    ... code that may set @err ...

    if (error is serious) {
        error_report_err(err);
    } else {
        error_free(err);
    }

Tempting, but wrong:

    g_autoptr(Error) err = NULL;

    ... code that may set @err ...

    if (error is serious) {
        error_report_err(err);
    }

Double-free, since error_report_err() frees its argument.  Correct:

    g_autoptr(Error) err = NULL;

    ... code that may set @err ...

    if (error is serious) {
        error_report_err(err);
        err = NULL;
    }

Hardly an improvement.

Same for propagation: replace error_report_err(err) by
error_propagate(errp, err).

If we decide we want g_autoptr(Error) anyway, then error.h's big comment
needs an update.