On Tue, Jul 23, 2024 at 01:36:32PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berra...@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é <berra...@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.
> 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.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
