Re: [PATCH 2.5/6] error: Explain why we don't g_autoptr(Error)
Peter Xu writes:
> On Wed, Nov 26, 2025 at 04:14:55PM +0100, Cédric Le Goater wrote:
>> On 11/26/25 15:34, Markus Armbruster wrote:
>> > Suggested-by: Peter Maydell
>> > Signed-off-by: Markus Armbruster
>> > ---
>> > include/qapi/error.h | 17 +
>> > 1 file changed, 17 insertions(+)
>> >
>> > diff --git a/include/qapi/error.h b/include/qapi/error.h
>> > index f3ce4a4a2d..fc018b4c59 100644
>> > --- a/include/qapi/error.h
>> > +++ b/include/qapi/error.h
>> > @@ -437,6 +437,23 @@ Error *error_copy(const Error *err);
>> >*/
>> > void error_free(Error *err);
>> > +/*
>> > + * Note: we intentionally do not enable g_autoptr(Error) with
>> > + * G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(Error, error_free).
>> > + *
>> > + * Functions that report or propagate an error take ownership of the
>> > + * Error object. Explicit error_free() is needed when you handle an
>> > + * error in some other way. This is rare.
>> > + *
>> > + * g_autoptr(Error) would call error_free() automatically on return.
>> > + * To avoid a double-free, we'd have to manually clear the pointer
>> > + * every time we propagate or report.
>> > + *
>> > + * Thus, g_autoptr(Error) would make the rare case easier to get right
>> > + * (less prone to leaks), and the common case easier to get wrong
>> > + * (more prone to double-free).
>
> How about we further poison the auto free altogether?
>
> IIUC this should work:
>
> +extern void
> +__attribute__((error("Error should not be used with g_autoptr")))
> +error_free_poisoned(Error *err);
> +
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free_poisoned)
Cute. Why not. I'll post a new patch.
Re: [PATCH 2.5/6] error: Explain why we don't g_autoptr(Error)
On Wed, Nov 26, 2025 at 04:14:55PM +0100, Cédric Le Goater wrote:
> On 11/26/25 15:34, Markus Armbruster wrote:
> > Suggested-by: Peter Maydell
> > Signed-off-by: Markus Armbruster
> > ---
> > include/qapi/error.h | 17 +
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/include/qapi/error.h b/include/qapi/error.h
> > index f3ce4a4a2d..fc018b4c59 100644
> > --- a/include/qapi/error.h
> > +++ b/include/qapi/error.h
> > @@ -437,6 +437,23 @@ Error *error_copy(const Error *err);
> >*/
> > void error_free(Error *err);
> > +/*
> > + * Note: we intentionally do not enable g_autoptr(Error) with
> > + * G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(Error, error_free).
> > + *
> > + * Functions that report or propagate an error take ownership of the
> > + * Error object. Explicit error_free() is needed when you handle an
> > + * error in some other way. This is rare.
> > + *
> > + * g_autoptr(Error) would call error_free() automatically on return.
> > + * To avoid a double-free, we'd have to manually clear the pointer
> > + * every time we propagate or report.
> > + *
> > + * Thus, g_autoptr(Error) would make the rare case easier to get right
> > + * (less prone to leaks), and the common case easier to get wrong
> > + * (more prone to double-free).
How about we further poison the auto free altogether?
IIUC this should work:
+extern void
+__attribute__((error("Error should not be used with g_autoptr")))
+error_free_poisoned(Error *err);
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free_poisoned)
> > + */
> > +
> > /*
> >* Convenience function to assert that *@errp is set, then silently free
> > it.
> >*/
>
>
> Reviewed-by: Cédric Le Goater
>
> Thanks,
>
> C.
>
>
--
Peter Xu
Re: [PATCH 2.5/6] error: Explain why we don't g_autoptr(Error)
On 11/26/25 15:34, Markus Armbruster wrote: Suggested-by: Peter Maydell Signed-off-by: Markus Armbruster --- include/qapi/error.h | 17 + 1 file changed, 17 insertions(+) diff --git a/include/qapi/error.h b/include/qapi/error.h index f3ce4a4a2d..fc018b4c59 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -437,6 +437,23 @@ Error *error_copy(const Error *err); */ void error_free(Error *err); +/* + * Note: we intentionally do not enable g_autoptr(Error) with + * G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(Error, error_free). + * + * Functions that report or propagate an error take ownership of the + * Error object. Explicit error_free() is needed when you handle an + * error in some other way. This is rare. + * + * g_autoptr(Error) would call error_free() automatically on return. + * To avoid a double-free, we'd have to manually clear the pointer + * every time we propagate or report. + * + * Thus, g_autoptr(Error) would make the rare case easier to get right + * (less prone to leaks), and the common case easier to get wrong + * (more prone to double-free). + */ + /* * Convenience function to assert that *@errp is set, then silently free it. */ Reviewed-by: Cédric Le Goater Thanks, C.
