Re: [PATCH 1/5] qapi: allow for g_autoptr(Error) usage

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Jul 23, 2024 at 01:36:32PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > While common error propagation practice does not require manually
>> > free'ing of local 'Error' objects, there are some cases where this
>> > is needed. One example is where the 'Error' object is only used
>> > for providing info to a trace event probe. Supporting g_autoptr
>> > avoids the need to manually call 'error_free'.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@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 71f8fb2c50..6e429809d8 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.
>> >   */
>> 
>> The Error interface is designed for a certain way of using it: an Error
>> object flows from the spot detecting the error to a spot handling it.
>> Failure to handle the error is a memory leak.  Our tooling can help with
>> tracking these down.
>> 
>> The interface tries to make the intended use easy: functions that report
>> an error consume the Error object.  Explicit error_free() should only
>> needed when you handle an error in some other way.
>> 
>> When such an explicit error_free() is needed on all paths to return,
>> then replacing it with auto-freeing is nice.  But what if it isn't?
>> 
>> Say we add a new error path and use error_report_err(err) there.  This
>> has always been just fine.  No more: if @err is auto-freed, this is a
>> double-free.  We have to also add err = NULL.  Feels like a trap for
>> developers to me.
>> 
>> Your use of auto-freeing is in the next patch.  It's this pattern:
>> 
>>     g_autoptr(Error) err = NULL;
>> 
>>     if (!frobnicate(args, &err)) {
>>         trace_frobnicate_err(..., error_get_pretty(err));
>>     }
>> 
>> You want to report the error to a trace point.  That's perfectly
>> legitimate.  The problem is that this kind of error reporting function
>> does not free, unlike the ones provided by qapi/error.h.
>> 
>> We could extend tracing to accept Error values, so that
>> 
>>         trace_frobnicate_err(..., err);
>> 
>> does free.  Doesn't seem worthwhile unless we find quite a few more uses
>> for it.
>
> That is awkward because the trace calls expand to nothing at all
> when tracing is disabled, so we can't rely on them to free any
> args.

True.

Another idea:

    g_autofree char *errmsg = error_to_pretty(err);
    trace_frobnicate_err(..., errmsg);

where error_to_pretty() frees the error except for err->msg, which it
returns.

>> If we conclude we want to provide auto-free as an option, we at least
>> need to point out the trap in a comment.  A bit of a pain to write, and
>> whether people will read, understand, and remember it is uncertain.
>> 
>> My gut feeling right now: stick to the design, and free manually.  If
>> you think my gut is wrong, tell me.
>
> I'll drop this since there's only one place benefitting right now.

Sensible.  Thanks!