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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 9/13/21 3:08 PM, Markus Armbruster wrote:
> 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);
>     }

error_report_err() seems always called within an if()
statement, so an alternative is to refactor this pattern as:

  void error_report_err_cond(bool condition, Error *err);