This reverts commit 18eb55546a54e443d94a4c49286348176ad4b00a.  Discussion
can be seen at:

https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com

Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Cc: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qapi/error.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index b16c6303f8..f3ce4a4a2d 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -437,8 +437,6 @@ 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.
  */
-- 
2.50.1

On 25.11.2025 21:46, Peter Xu wrote:
> This reverts commit 18eb55546a54e443d94a4c49286348176ad4b00a.  Discussion
> can be seen at:
> 
> https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com
> 
> Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Cc: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/qapi/error.h | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index b16c6303f8..f3ce4a4a2d 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -437,8 +437,6 @@ 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.
>    */

Acked-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Thanks,
Maciej

On 11/25/25 21:46, Peter Xu wrote:
> This reverts commit 18eb55546a54e443d94a4c49286348176ad4b00a.  Discussion
> can be seen at:
> 
> https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com
> 
> Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Cc: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/qapi/error.h | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index b16c6303f8..f3ce4a4a2d 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -437,8 +437,6 @@ 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.
>    */

Is that related to CID 1643463 issue ?

anyhow,


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Cédric Le Goater <clg@redhat.com> writes:

> On 11/25/25 21:46, Peter Xu wrote:
>> This reverts commit 18eb55546a54e443d94a4c49286348176ad4b00a.  Discussion
>> can be seen at:
>> https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com
>> Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>> Cc: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>  include/qapi/error.h | 2 --
>>  1 file changed, 2 deletions(-)
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index b16c6303f8..f3ce4a4a2d 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -437,8 +437,6 @@ 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.
>>   */
>
> Is that related to CID 1643463 issue ?

g_autoptr(Error) is a bad idea, as discussed in

    Subject: g_autoptr(Error) (was: [PATCH] migration: Fix double-free on error path) 
    Date: Tue, 25 Nov 2025 08:40:07 +0100
    Message-ID: <871plmk1bc.fsf@pond.sub.org>

We have three instances of g_autoptr(Error) in master.  CID 1643463 made
me see them.

One is removed by my fix to CID 1643463:

    Subject: [PATCH] migration: Fix double-free on error path
    Date: Tue, 25 Nov 2025 08:05:54 +0100
    Message-ID: <20251125070554.2256181-1-armbru@redhat.com>

The remaining two get removed in PATCH 1.  This patch deletes the code
that makes g_autoptr(Error) work.

> anyhow,
>
>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thank you!

Peter Xu <peterx@redhat.com> writes:

> This reverts commit 18eb55546a54e443d94a4c49286348176ad4b00a.  Discussion
> can be seen at:
>
> https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com

Suggest to steal from the previous commit like this:

  Due to the nature of how Error should be used (normally ownership will be
  passed over to Error APIs, like error_report_err), auto-free functions may
  be error prone on its own.  The auto cleanup function was merged without 
  proper review as pointed out by Dan and Markus:

  https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com

> Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Cc: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qapi/error.h | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index b16c6303f8..f3ce4a4a2d 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -437,8 +437,6 @@ 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.
>   */

Reviewed-by: Markus Armbruster <armbru@redhat.com>

The previous commit reverted support for g_autoptr(Error).  This one
should stop it from coming back.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/error.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index f3ce4a4a2d..2356b84bb3 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -437,6 +437,26 @@ Error *error_copy(const Error *err);
  */
 void error_free(Error *err);
 
+/*
+ * Poison g_autoptr(Error) to prevent its use.
+ *
+ * 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).
+ */
+extern void
+__attribute__((error("Do not use g_autoptr() to declare Error * variables")))
+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.
  */
-- 
2.49.0

On 11/27/25 08:10, Markus Armbruster wrote:
> The previous commit reverted support for g_autoptr(Error).  This one
> should stop it from coming back.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   include/qapi/error.h | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index f3ce4a4a2d..2356b84bb3 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -437,6 +437,26 @@ Error *error_copy(const Error *err);
>    */
>   void error_free(Error *err);
>   
> +/*
> + * Poison g_autoptr(Error) to prevent its use.
> + *
> + * 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).
> + */
> +extern void
> +__attribute__((error("Do not use g_autoptr() to declare Error * variables")))
> +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.
>    */

broken build, even better.

Tested-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 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.
  */
-- 
2.49.0

On 11/26/25 15:34, Markus Armbruster wrote:
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   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 <clg@redhat.com>

Thanks,

C.

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 <peter.maydell@linaro.org>
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >   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 <clg@redhat.com>
> 
> Thanks,
> 
> C.
> 
> 

-- 
Peter Xu

Peter Xu <peterx@redhat.com> 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 <peter.maydell@linaro.org>
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> > ---
>> >   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.