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);
}
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.
