Daniel P. Berrangé <berra...@redhat.com> writes: > 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.
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!