| Message ID | 20240722131611.2820041-2-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | crypto: improve error reporting detail | expand |
On 22/7/24 15:16, Daniel P. Berrangé wrote: > 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(+) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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. 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.
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. > 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
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!
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. */
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(+)