On 9/13/21 3:08 PM, Markus Armbruster wrote:
> Markus Armbruster <arm...@redhat.com> writes:
>
>> Paolo Bonzini <pbonz...@redhat.com> writes:
>>
>>> On 13/09/21 07:23, Markus Armbruster wrote:
>>>> Paolo Bonzini <pbonz...@redhat.com> writes:
>>>>
>>>>> Allow replacing calls to error_free() with g_autoptr(Error)
>>>>> declarations.
>>>>>
>>>>> Signed-off-by: Paolo Bonzini <pbonz...@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);
