[PATCH] migration: Fix double-free on error path

[PATCH] migration: Fix double-free on error path

[Markus Armbruster]

Fixes: ffaa1b50a879 (migration: Use warn_reportf_err() where appropriate)
Resolves: Coverity CID 1643463
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/multifd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 6210454838..3203dc98e1 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -450,7 +450,7 @@ static void multifd_send_set_error(Error *err)
  */
 static void migration_ioc_shutdown_gracefully(QIOChannel *ioc)
 {
-    g_autoptr(Error) local_err = NULL;
+    Error *local_err = NULL;
 
     if (!migration_has_failed(migrate_get_current()) &&
         object_dynamic_cast((Object *)ioc, TYPE_QIO_CHANNEL_TLS)) {
-- 
2.49.0

Re: [PATCH] migration: Fix double-free on error path

[Markus Armbruster]

I can include this in a PR I indend to send later today, but do feel
free to commit it directly.

Re: [PATCH] migration: Fix double-free on error path

[Marc-André Lureau]

Hi

On Tue, Nov 25, 2025 at 11:06 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Fixes: ffaa1b50a879 (migration: Use warn_reportf_err() where appropriate)
> Resolves: Coverity CID 1643463
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  migration/multifd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 6210454838..3203dc98e1 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -450,7 +450,7 @@ static void multifd_send_set_error(Error *err)
>   */
>  static void migration_ioc_shutdown_gracefully(QIOChannel *ioc)
>  {
> -    g_autoptr(Error) local_err = NULL;
> +    Error *local_err = NULL;
>
>      if (!migration_has_failed(migrate_get_current()) &&
>          object_dynamic_cast((Object *)ioc, TYPE_QIO_CHANNEL_TLS)) {
> --
> 2.49.0
>

Maybe warn_reportf_err() should take a Error **err instead, and clear
it (and accept NULL values)


-- 
Marc-André Lureau

Re: [PATCH] migration: Fix double-free on error path

[Philippe Mathieu-Daudé]

On 25/11/25 08:05, Markus Armbruster wrote:
> Fixes: ffaa1b50a879 (migration: Use warn_reportf_err() where appropriate)
> Resolves: Coverity CID 1643463

That was fast :)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   migration/multifd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

g_autoptr(Error) (was: [PATCH] migration: Fix double-free on error path)

[Markus Armbruster]

g_autoptr(T) is quite useful when the object's extent matches the
function's.

This isn't the case for an Error object the function propagates to its
caller.  It is the case for an Error object the function reports or
handles itself.  However, the functions to report Error also free it.

Thus, g_autoptr(Error) is rarely applicable.  We have just three
instances out of >1100 local Error variables, all in migration code.

Two want to move the error to the MigrationState for later handling /
reporting.  Since migrate_set_error() doesn't move, but stores a copy,
the original needs to be freed, and g_autoptr() is correct there.  We
have 17 more that instead manually free with error_free() or
error_report_err() right after migrate_set_error().

We recently discussed storing a copy vs. move the original:

    From: Peter Xu <peterx@redhat.com>
    Subject: Re: [PATCH 0/3] migration: Error fixes and improvements
    Date: Mon, 17 Nov 2025 11:03:37 -0500
    Message-ID: <aRtHWbWcTh3OF2wY@x1.local>

The two g_autoptr() gave me pause when I investigated this topic, simply
because they deviate from the common pattern migrate_set_error(s, err)
followed by error_free() or error_report_err().

The third one became wrong when I cleaned up the reporting (missed in
the cleanup patch, fixed in the patch I'm replying to).  I suspect my
mistake escaped review for the same reason I made it: g_autoptr(Error)
is unusual and not visible in the patch hunk.

Would you like me to replace the two correct uses of g_autoptr(Error) by
more common usage?

Re: g_autoptr(Error) (was: [PATCH] migration: Fix double-free on error path)

[Daniel P. Berrangé]

On Tue, Nov 25, 2025 at 08:40:07AM +0100, Markus Armbruster wrote:
> g_autoptr(T) is quite useful when the object's extent matches the
> function's.
> 
> This isn't the case for an Error object the function propagates to its
> caller.  It is the case for an Error object the function reports or
> handles itself.  However, the functions to report Error also free it.
> 
> Thus, g_autoptr(Error) is rarely applicable.  We have just three
> instances out of >1100 local Error variables, all in migration code.
> 
> Two want to move the error to the MigrationState for later handling /
> reporting.  Since migrate_set_error() doesn't move, but stores a copy,
> the original needs to be freed, and g_autoptr() is correct there.  We
> have 17 more that instead manually free with error_free() or
> error_report_err() right after migrate_set_error().
> 
> We recently discussed storing a copy vs. move the original:
> 
>     From: Peter Xu <peterx@redhat.com>
>     Subject: Re: [PATCH 0/3] migration: Error fixes and improvements
>     Date: Mon, 17 Nov 2025 11:03:37 -0500
>     Message-ID: <aRtHWbWcTh3OF2wY@x1.local>
> 
> The two g_autoptr() gave me pause when I investigated this topic, simply
> because they deviate from the common pattern migrate_set_error(s, err)
> followed by error_free() or error_report_err().
> 
> The third one became wrong when I cleaned up the reporting (missed in
> the cleanup patch, fixed in the patch I'm replying to).  I suspect my
> mistake escaped review for the same reason I made it: g_autoptr(Error)
> is unusual and not visible in the patch hunk.
> 
> Would you like me to replace the two correct uses of g_autoptr(Error) by
> more common usage?

I had previously proposed g_autoptr(Error) a year or two back and you
rejected it then, so I'm surprised to see that it got into the code,
because it requires explicit opt-in via a G_DEFINE_AUTOPTR_CLEANUP_FUNC.

Unfortunately it appears exactly that was added earlier this year in

  commit 18eb55546a54e443d94a4c49286348176ad4b00a
  Author: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
  Date:   Tue Mar 4 23:03:35 2025 +0100

    error: define g_autoptr() cleanup function for the Error type
    
    Automatic memory management helps avoid memory safety issues.
    
    Reviewed-by: Peter Xu <peterx@redhat.com>
    Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
    Link: https://lore.kernel.org/qemu-devel/a5843c5fa64d7e5239a4316092ec0ef0d10c2320.1741124640.git.maciej.szmigiero@oracle.com
    Signed-off-by: Cédric Le Goater <clg@redhat.com>


When removing this usage, ensure that commit is reverted too, which
will prevent anyone unwittingly re-introducing g_autoptr(Error)
usage

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 :|

Re: g_autoptr(Error)

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Nov 25, 2025 at 08:40:07AM +0100, Markus Armbruster wrote:
>> g_autoptr(T) is quite useful when the object's extent matches the
>> function's.
>> 
>> This isn't the case for an Error object the function propagates to its
>> caller.  It is the case for an Error object the function reports or
>> handles itself.  However, the functions to report Error also free it.
>> 
>> Thus, g_autoptr(Error) is rarely applicable.  We have just three
>> instances out of >1100 local Error variables, all in migration code.
>> 
>> Two want to move the error to the MigrationState for later handling /
>> reporting.  Since migrate_set_error() doesn't move, but stores a copy,
>> the original needs to be freed, and g_autoptr() is correct there.  We
>> have 17 more that instead manually free with error_free() or
>> error_report_err() right after migrate_set_error().
>> 
>> We recently discussed storing a copy vs. move the original:
>> 
>>     From: Peter Xu <peterx@redhat.com>
>>     Subject: Re: [PATCH 0/3] migration: Error fixes and improvements
>>     Date: Mon, 17 Nov 2025 11:03:37 -0500
>>     Message-ID: <aRtHWbWcTh3OF2wY@x1.local>
>> 
>> The two g_autoptr() gave me pause when I investigated this topic, simply
>> because they deviate from the common pattern migrate_set_error(s, err)
>> followed by error_free() or error_report_err().
>> 
>> The third one became wrong when I cleaned up the reporting (missed in
>> the cleanup patch, fixed in the patch I'm replying to).  I suspect my
>> mistake escaped review for the same reason I made it: g_autoptr(Error)
>> is unusual and not visible in the patch hunk.
>> 
>> Would you like me to replace the two correct uses of g_autoptr(Error) by
>> more common usage?
>
> I had previously proposed g_autoptr(Error) a year or two back and you
> rejected it then, so I'm surprised to see that it got into the code,
> because it requires explicit opt-in via a G_DEFINE_AUTOPTR_CLEANUP_FUNC.
>
> Unfortunately it appears exactly that was added earlier this year in
>
>   commit 18eb55546a54e443d94a4c49286348176ad4b00a
>   Author: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>   Date:   Tue Mar 4 23:03:35 2025 +0100
>
>     error: define g_autoptr() cleanup function for the Error type
>     
>     Automatic memory management helps avoid memory safety issues.
>     
>     Reviewed-by: Peter Xu <peterx@redhat.com>
>     Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>     Link: https://lore.kernel.org/qemu-devel/a5843c5fa64d7e5239a4316092ec0ef0d10c2320.1741124640.git.maciej.szmigiero@oracle.com
>     Signed-off-by: Cédric Le Goater <clg@redhat.com>

I missed it.  Not he submitter's fault; it was cc'ed to me.

> When removing this usage, ensure that commit is reverted too, which
> will prevent anyone unwittingly re-introducing g_autoptr(Error)
> usage

Thanks for the pointer!

Re: [PATCH] migration: Fix double-free on error path

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Tue, Nov 25, 2025 at 11:06 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Fixes: ffaa1b50a879 (migration: Use warn_reportf_err() where appropriate)
>> Resolves: Coverity CID 1643463
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>> ---
>>  migration/multifd.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index 6210454838..3203dc98e1 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -450,7 +450,7 @@ static void multifd_send_set_error(Error *err)
>>   */
>>  static void migration_ioc_shutdown_gracefully(QIOChannel *ioc)
>>  {
>> -    g_autoptr(Error) local_err = NULL;
>> +    Error *local_err = NULL;
>>
>>      if (!migration_has_failed(migrate_get_current()) &&
>>          object_dynamic_cast((Object *)ioc, TYPE_QIO_CHANNEL_TLS)) {
>> --
>> 2.49.0
>>
>
> Maybe warn_reportf_err() should take a Error **err instead, and clear
> it (and accept NULL values)

Our deallocating functions don't work that way.

Having them take a pointer by reference and clear it gets rid of *one*
dangling reference.  There may be more.

Coverity is fairly good at finding the kind of use after free this could
avoid.

Error ** parameters are almost always for returning errors.  Not having
to wonder what such a parameter is for makes code easier to read.

Thanks for your review!

Re: [PATCH] migration: Fix double-free on error path

[Peter Xu]

On Tue, Nov 25, 2025 at 08:11:34AM +0100, Markus Armbruster wrote:
> I can include this in a PR I indend to send later today, but do feel
> free to commit it directly.

Works on my side:

Acked-by: Peter Xu <peterx@redhat.com>

Note: I think it's Richard's turn to collect pulls for 10.2.

Thanks,

-- 
Peter Xu

Re: [PATCH] migration: Fix double-free on error path

[Peter Xu]

On Tue, Nov 25, 2025 at 01:59:54PM +0100, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> 
> > Hi
> >
> > On Tue, Nov 25, 2025 at 11:06 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Fixes: ffaa1b50a879 (migration: Use warn_reportf_err() where appropriate)
> >> Resolves: Coverity CID 1643463
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> >> ---
> >>  migration/multifd.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/migration/multifd.c b/migration/multifd.c
> >> index 6210454838..3203dc98e1 100644
> >> --- a/migration/multifd.c
> >> +++ b/migration/multifd.c
> >> @@ -450,7 +450,7 @@ static void multifd_send_set_error(Error *err)
> >>   */
> >>  static void migration_ioc_shutdown_gracefully(QIOChannel *ioc)
> >>  {
> >> -    g_autoptr(Error) local_err = NULL;
> >> +    Error *local_err = NULL;
> >>
> >>      if (!migration_has_failed(migrate_get_current()) &&
> >>          object_dynamic_cast((Object *)ioc, TYPE_QIO_CHANNEL_TLS)) {
> >> --
> >> 2.49.0
> >>
> >
> > Maybe warn_reportf_err() should take a Error **err instead, and clear
> > it (and accept NULL values)
> 
> Our deallocating functions don't work that way.
> 
> Having them take a pointer by reference and clear it gets rid of *one*
> dangling reference.  There may be more.

True.  However I need to confess I like Marc-André's proposal.. Normally we
only have one Error object, or >1 objects.

The only thing I'm not sure is such design doesn't match with the error API
(e.g. current form matches the more famous error_report_err(), and likely
others that I'm not familiar).  So at least this will need some more
thoughts before all the code churns.

Thanks,

> 
> Coverity is fairly good at finding the kind of use after free this could
> avoid.
> 
> Error ** parameters are almost always for returning errors.  Not having
> to wonder what such a parameter is for makes code easier to read.

-- 
Peter Xu

Re: g_autoptr(Error)

[Peter Xu]

On Tue, Nov 25, 2025 at 12:46:01PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Nov 25, 2025 at 08:40:07AM +0100, Markus Armbruster wrote:
> >> g_autoptr(T) is quite useful when the object's extent matches the
> >> function's.
> >> 
> >> This isn't the case for an Error object the function propagates to its
> >> caller.  It is the case for an Error object the function reports or
> >> handles itself.  However, the functions to report Error also free it.

I'd confess I didn't pay enough attention on how the error API was designed
deliberately to always free the Error objects before almost whenever
possible.  But I see now, thanks for the write up.

> >> 
> >> Thus, g_autoptr(Error) is rarely applicable.  We have just three
> >> instances out of >1100 local Error variables, all in migration code.
> >> 
> >> Two want to move the error to the MigrationState for later handling /
> >> reporting.  Since migrate_set_error() doesn't move, but stores a copy,
> >> the original needs to be freed, and g_autoptr() is correct there.  We
> >> have 17 more that instead manually free with error_free() or
> >> error_report_err() right after migrate_set_error().
> >> 
> >> We recently discussed storing a copy vs. move the original:
> >> 
> >>     From: Peter Xu <peterx@redhat.com>
> >>     Subject: Re: [PATCH 0/3] migration: Error fixes and improvements
> >>     Date: Mon, 17 Nov 2025 11:03:37 -0500
> >>     Message-ID: <aRtHWbWcTh3OF2wY@x1.local>
> >> 
> >> The two g_autoptr() gave me pause when I investigated this topic, simply
> >> because they deviate from the common pattern migrate_set_error(s, err)
> >> followed by error_free() or error_report_err().
> >> 
> >> The third one became wrong when I cleaned up the reporting (missed in
> >> the cleanup patch, fixed in the patch I'm replying to).  I suspect my
> >> mistake escaped review for the same reason I made it: g_autoptr(Error)
> >> is unusual and not visible in the patch hunk.
> >> 
> >> Would you like me to replace the two correct uses of g_autoptr(Error) by
> >> more common usage?

Works for me.

Now I also think it should be good migrate_set_error() follow QEMU's Error
API design if we decide to stick with it freeing errors in such APIs.

Said that, I wonder if you think we could still consider passing Error**
into migrate_set_error(), though, which will be a merged solution of
current Error API and what Marc-Andre proposed on resetting pointers to
avoid any possible UAF, which I would still slightly prefer personally.

If we rework migrate_set_error() to take ownership first, then we can
naturally drop the two use cases, and remove the cleanup function.

Markus, please also let me know if you want me to do it.

> >
> > I had previously proposed g_autoptr(Error) a year or two back and you
> > rejected it then, so I'm surprised to see that it got into the code,
> > because it requires explicit opt-in via a G_DEFINE_AUTOPTR_CLEANUP_FUNC.
> >
> > Unfortunately it appears exactly that was added earlier this year in
> >
> >   commit 18eb55546a54e443d94a4c49286348176ad4b00a
> >   Author: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> >   Date:   Tue Mar 4 23:03:35 2025 +0100
> >
> >     error: define g_autoptr() cleanup function for the Error type
> >     
> >     Automatic memory management helps avoid memory safety issues.
> >     
> >     Reviewed-by: Peter Xu <peterx@redhat.com>
> >     Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> >     Link: https://lore.kernel.org/qemu-devel/a5843c5fa64d7e5239a4316092ec0ef0d10c2320.1741124640.git.maciej.szmigiero@oracle.com
> >     Signed-off-by: Cédric Le Goater <clg@redhat.com>
> 
> I missed it.  Not he submitter's fault; it was cc'ed to me.

If someone to blame, it's the reviewer.

Thanks!

-- 
Peter Xu

Re: [PATCH] migration: Fix double-free on error path

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Nov 25, 2025 at 01:59:54PM +0100, Markus Armbruster wrote:
>> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>> 
>> > Hi
>> >
>> > On Tue, Nov 25, 2025 at 11:06 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> Fixes: ffaa1b50a879 (migration: Use warn_reportf_err() where appropriate)
>> >> Resolves: Coverity CID 1643463
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >
>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> >> ---
>> >>  migration/multifd.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/migration/multifd.c b/migration/multifd.c
>> >> index 6210454838..3203dc98e1 100644
>> >> --- a/migration/multifd.c
>> >> +++ b/migration/multifd.c
>> >> @@ -450,7 +450,7 @@ static void multifd_send_set_error(Error *err)
>> >>   */
>> >>  static void migration_ioc_shutdown_gracefully(QIOChannel *ioc)
>> >>  {
>> >> -    g_autoptr(Error) local_err = NULL;
>> >> +    Error *local_err = NULL;
>> >>
>> >>      if (!migration_has_failed(migrate_get_current()) &&
>> >>          object_dynamic_cast((Object *)ioc, TYPE_QIO_CHANNEL_TLS)) {
>> >> --
>> >> 2.49.0
>> >>
>> >
>> > Maybe warn_reportf_err() should take a Error **err instead, and clear
>> > it (and accept NULL values)
>> 
>> Our deallocating functions don't work that way.

g_free(), g_realloc(), freeaddrinfo(), qapi_free_T(), visit_free(),
qcrypto_FOO_free(), aio_task_pool_free(), qemu_opts_free(),
timer_free(), ...

>> Having them take a pointer by reference and clear it gets rid of *one*
>> dangling reference.  There may be more.
>
> True.  However I need to confess I like Marc-André's proposal.. Normally we
> only have one Error object, or >1 objects.
>
> The only thing I'm not sure is such design doesn't match with the error API
> (e.g. current form matches the more famous error_report_err(), and likely
> others that I'm not familiar).  So at least this will need some more
> thoughts before all the code churns.

The error.h functions and macros that free an Error object are:

* error_free(), error_free_or_abort()

  These take a single Error * argument.

* error_report_err(), warn_report_err(), error_reportf_err(),
  warn_reportf_err(). warn_report_err_once_cond(),
  warn_report_err_once()

  These take also a single Error * argument.

* error_propagate(), error_propagate_prepend()

  These take an Error ** destination, and an Error * object to be
  propagated.  They take ownership of the latter.  They either store it
  in the destination, or free it.

  Changing the latter to Error ** makes these functions vulnerable to
  swapped arguments.  See "Error ** parameters are almost always for
  returning errors" below.

* ERRP_GUARD()

  Includes automatic error_propagate() on return, immune to use after
  free.

>
> Thanks,
>
>> 
>> Coverity is fairly good at finding the kind of use after free this could
>> avoid.
>> 
>> Error ** parameters are almost always for returning errors.  Not having
>> to wonder what such a parameter is for makes code easier to read.

My main argument remains this one: our deallocating functions don't work
that way.  For better or worse.

I don't want the Error API free differently.

Re: g_autoptr(Error)

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Nov 25, 2025 at 12:46:01PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Tue, Nov 25, 2025 at 08:40:07AM +0100, Markus Armbruster wrote:
>> >> g_autoptr(T) is quite useful when the object's extent matches the
>> >> function's.
>> >> 
>> >> This isn't the case for an Error object the function propagates to its
>> >> caller.  It is the case for an Error object the function reports or
>> >> handles itself.  However, the functions to report Error also free it.
>
> I'd confess I didn't pay enough attention on how the error API was designed
> deliberately to always free the Error objects before almost whenever
> possible.  But I see now, thanks for the write up.

You're welcome!

>> >> 
>> >> Thus, g_autoptr(Error) is rarely applicable.  We have just three
>> >> instances out of >1100 local Error variables, all in migration code.
>> >> 
>> >> Two want to move the error to the MigrationState for later handling /
>> >> reporting.  Since migrate_set_error() doesn't move, but stores a copy,
>> >> the original needs to be freed, and g_autoptr() is correct there.  We
>> >> have 17 more that instead manually free with error_free() or
>> >> error_report_err() right after migrate_set_error().
>> >> 
>> >> We recently discussed storing a copy vs. move the original:
>> >> 
>> >>     From: Peter Xu <peterx@redhat.com>
>> >>     Subject: Re: [PATCH 0/3] migration: Error fixes and improvements
>> >>     Date: Mon, 17 Nov 2025 11:03:37 -0500
>> >>     Message-ID: <aRtHWbWcTh3OF2wY@x1.local>
>> >> 
>> >> The two g_autoptr() gave me pause when I investigated this topic, simply
>> >> because they deviate from the common pattern migrate_set_error(s, err)
>> >> followed by error_free() or error_report_err().
>> >> 
>> >> The third one became wrong when I cleaned up the reporting (missed in
>> >> the cleanup patch, fixed in the patch I'm replying to).  I suspect my
>> >> mistake escaped review for the same reason I made it: g_autoptr(Error)
>> >> is unusual and not visible in the patch hunk.
>> >> 
>> >> Would you like me to replace the two correct uses of g_autoptr(Error) by
>> >> more common usage?
>
> Works for me.
>
> Now I also think it should be good migrate_set_error() follow QEMU's Error
> API design if we decide to stick with it freeing errors in such APIs.
>
> Said that, I wonder if you think we could still consider passing Error**
> into migrate_set_error(), though, which will be a merged solution of
> current Error API and what Marc-Andre proposed on resetting pointers to
> avoid any possible UAF, which I would still slightly prefer personally.
>
> If we rework migrate_set_error() to take ownership first, then we can
> naturally drop the two use cases, and remove the cleanup function.
>
> Markus, please also let me know if you want me to do it.

I think the first step should replace the two g_autoptr() by
error_free(), then delete g_autoptr() support.

A possible second step is to replace migrate_set_error() by a function
that takes ownership.  "Replace" because I think migrate_set_error()
would be a bad name for such a function.  What's a better name?  Naming
is hard...  migrate_error_propagate_to_state()?  Because there's
similarity:

    error_propagate(errp, err);

stores @err in @errp, or else frees it, and

    migrate_error_propagate_to_state(s, err)

stores @err in @s, or else frees it.

We could also forgo encapsulation and simply use

    error_propagate(&s->error, err);

Matter of taste, which means migration maintainers decide.

I can do just the first step, or both.  Up to you.

[...]

Re: [PATCH] migration: Fix double-free on error path

[Peter Xu]

On Tue, Nov 25, 2025 at 07:48:44PM +0100, Markus Armbruster wrote:
> My main argument remains this one: our deallocating functions don't work
> that way.  For better or worse.
> 
> I don't want the Error API free differently.

Yes, it's fair.

IMHO, it's a matter of taste, and maybe that's also why I liked
g_clear_pointer() but not everyone likes it.. said that, when we have
g_clear_pointer() it also makes the current form more flexible, because we
can apply g_clear_pointer() on top when we need a reset..

I'll follow your advise on error reporting for migration.

Thanks,

-- 
Peter Xu

Re: g_autoptr(Error)

[Peter Xu]

On Tue, Nov 25, 2025 at 08:02:55PM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Nov 25, 2025 at 12:46:01PM +0100, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Tue, Nov 25, 2025 at 08:40:07AM +0100, Markus Armbruster wrote:
> >> >> g_autoptr(T) is quite useful when the object's extent matches the
> >> >> function's.
> >> >> 
> >> >> This isn't the case for an Error object the function propagates to its
> >> >> caller.  It is the case for an Error object the function reports or
> >> >> handles itself.  However, the functions to report Error also free it.
> >
> > I'd confess I didn't pay enough attention on how the error API was designed
> > deliberately to always free the Error objects before almost whenever
> > possible.  But I see now, thanks for the write up.
> 
> You're welcome!
> 
> >> >> 
> >> >> Thus, g_autoptr(Error) is rarely applicable.  We have just three
> >> >> instances out of >1100 local Error variables, all in migration code.
> >> >> 
> >> >> Two want to move the error to the MigrationState for later handling /
> >> >> reporting.  Since migrate_set_error() doesn't move, but stores a copy,
> >> >> the original needs to be freed, and g_autoptr() is correct there.  We
> >> >> have 17 more that instead manually free with error_free() or
> >> >> error_report_err() right after migrate_set_error().
> >> >> 
> >> >> We recently discussed storing a copy vs. move the original:
> >> >> 
> >> >>     From: Peter Xu <peterx@redhat.com>
> >> >>     Subject: Re: [PATCH 0/3] migration: Error fixes and improvements
> >> >>     Date: Mon, 17 Nov 2025 11:03:37 -0500
> >> >>     Message-ID: <aRtHWbWcTh3OF2wY@x1.local>
> >> >> 
> >> >> The two g_autoptr() gave me pause when I investigated this topic, simply
> >> >> because they deviate from the common pattern migrate_set_error(s, err)
> >> >> followed by error_free() or error_report_err().
> >> >> 
> >> >> The third one became wrong when I cleaned up the reporting (missed in
> >> >> the cleanup patch, fixed in the patch I'm replying to).  I suspect my
> >> >> mistake escaped review for the same reason I made it: g_autoptr(Error)
> >> >> is unusual and not visible in the patch hunk.
> >> >> 
> >> >> Would you like me to replace the two correct uses of g_autoptr(Error) by
> >> >> more common usage?
> >
> > Works for me.
> >
> > Now I also think it should be good migrate_set_error() follow QEMU's Error
> > API design if we decide to stick with it freeing errors in such APIs.
> >
> > Said that, I wonder if you think we could still consider passing Error**
> > into migrate_set_error(), though, which will be a merged solution of
> > current Error API and what Marc-Andre proposed on resetting pointers to
> > avoid any possible UAF, which I would still slightly prefer personally.
> >
> > If we rework migrate_set_error() to take ownership first, then we can
> > naturally drop the two use cases, and remove the cleanup function.
> >
> > Markus, please also let me know if you want me to do it.
> 
> I think the first step should replace the two g_autoptr() by
> error_free(), then delete g_autoptr() support.
> 
> A possible second step is to replace migrate_set_error() by a function
> that takes ownership.  "Replace" because I think migrate_set_error()
> would be a bad name for such a function.  What's a better name?  Naming
> is hard...  migrate_error_propagate_to_state()?  Because there's
> similarity:
> 
>     error_propagate(errp, err);
> 
> stores @err in @errp, or else frees it, and
> 
>     migrate_error_propagate_to_state(s, err)

I took this one but dropped to_state to make it shorter (and also dropped
"s" to make it g_clear_pointer() friendly).

> 
> stores @err in @s, or else frees it.
> 
> We could also forgo encapsulation and simply use
> 
>     error_propagate(&s->error, err);
> 
> Matter of taste, which means migration maintainers decide.
> 
> I can do just the first step, or both.  Up to you.

I sent the patches here for both of the issues discussed (I should still
owe some other patches; I'll do them separately..):

https://lore.kernel.org/r/20251125204648.857018-1-peterx@redhat.com

Comments more than welcomed.

Thanks,

-- 
Peter Xu

Should functions that free memory clear the pointer? (was: [PATCH] migration: Fix double-free on error path)

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Nov 25, 2025 at 07:48:44PM +0100, Markus Armbruster wrote:
>> My main argument remains this one: our deallocating functions don't work
>> that way.  For better or worse.
>> 
>> I don't want the Error API free differently.
>
> Yes, it's fair.
>
> IMHO, it's a matter of taste,

Depends on context.

Modula-2's DISPOSE takes the pointer by reference and clears it.  C's
free() takes it by value.  Matter of taste when these were designed.

But when one style has become overwhelmingly prevalent, it's no longer a
matter of taste.  This is arguably the case in C.

>                               and maybe that's also why I liked
> g_clear_pointer() but not everyone likes it.. said that, when we have
> g_clear_pointer() it also makes the current form more flexible, because we
> can apply g_clear_pointer() on top when we need a reset..

I'm no friend of g_clear_pointer().  I find

    g_clear_pointer(&ptr, free_frob);

a rather roundabout way to say

    free_frob(ptr);
    ptr = NULL;

Any C programmer will immediately understand the latter.  For the
former, you need to know one more little thing.  Yes, we can all get
used to these little things, but it's one more little thing new people
have to learn and internalize.  Even little things add up.

If an entire project gets into the habit of using it religiously, it may
reduce "forgot to zap the reference" bugs some.  Until then, it feels
like a net negative to me.

> I'll follow your advise on error reporting for migration.

Thank you!

Re: g_autoptr(Error)

[Cédric Le Goater]

On 11/25/25 17:15, Peter Xu wrote:
> On Tue, Nov 25, 2025 at 12:46:01PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>>> On Tue, Nov 25, 2025 at 08:40:07AM +0100, Markus Armbruster wrote:
>>>> g_autoptr(T) is quite useful when the object's extent matches the
>>>> function's.
>>>>
>>>> This isn't the case for an Error object the function propagates to its
>>>> caller.  It is the case for an Error object the function reports or
>>>> handles itself.  However, the functions to report Error also free it.
> 
> I'd confess I didn't pay enough attention on how the error API was designed
> deliberately to always free the Error objects before almost whenever
> possible.  But I see now, thanks for the write up.
> 
>>>>
>>>> Thus, g_autoptr(Error) is rarely applicable.  We have just three
>>>> instances out of >1100 local Error variables, all in migration code.
>>>>
>>>> Two want to move the error to the MigrationState for later handling /
>>>> reporting.  Since migrate_set_error() doesn't move, but stores a copy,
>>>> the original needs to be freed, and g_autoptr() is correct there.  We
>>>> have 17 more that instead manually free with error_free() or
>>>> error_report_err() right after migrate_set_error().
>>>>
>>>> We recently discussed storing a copy vs. move the original:
>>>>
>>>>      From: Peter Xu <peterx@redhat.com>
>>>>      Subject: Re: [PATCH 0/3] migration: Error fixes and improvements
>>>>      Date: Mon, 17 Nov 2025 11:03:37 -0500
>>>>      Message-ID: <aRtHWbWcTh3OF2wY@x1.local>
>>>>
>>>> The two g_autoptr() gave me pause when I investigated this topic, simply
>>>> because they deviate from the common pattern migrate_set_error(s, err)
>>>> followed by error_free() or error_report_err().
>>>>
>>>> The third one became wrong when I cleaned up the reporting (missed in
>>>> the cleanup patch, fixed in the patch I'm replying to).  I suspect my
>>>> mistake escaped review for the same reason I made it: g_autoptr(Error)
>>>> is unusual and not visible in the patch hunk.
>>>>
>>>> Would you like me to replace the two correct uses of g_autoptr(Error) by
>>>> more common usage?
> 
> Works for me.
> 
> Now I also think it should be good migrate_set_error() follow QEMU's Error
> API design if we decide to stick with it freeing errors in such APIs.
> 
> Said that, I wonder if you think we could still consider passing Error**
> into migrate_set_error(), though, which will be a merged solution of
> current Error API and what Marc-Andre proposed on resetting pointers to
> avoid any possible UAF, which I would still slightly prefer personally.
> 
> If we rework migrate_set_error() to take ownership first, then we can
> naturally drop the two use cases, and remove the cleanup function.
> 
> Markus, please also let me know if you want me to do it.
> 
>>>
>>> I had previously proposed g_autoptr(Error) a year or two back and you
>>> rejected it then, so I'm surprised to see that it got into the code,
>>> because it requires explicit opt-in via a G_DEFINE_AUTOPTR_CLEANUP_FUNC.
>>>
>>> Unfortunately it appears exactly that was added earlier this year in
>>>
>>>    commit 18eb55546a54e443d94a4c49286348176ad4b00a
>>>    Author: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>    Date:   Tue Mar 4 23:03:35 2025 +0100
>>>
>>>      error: define g_autoptr() cleanup function for the Error type
>>>      
>>>      Automatic memory management helps avoid memory safety issues.
>>>      
>>>      Reviewed-by: Peter Xu <peterx@redhat.com>
>>>      Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>      Link: https://lore.kernel.org/qemu-devel/a5843c5fa64d7e5239a4316092ec0ef0d10c2320.1741124640.git.maciej.szmigiero@oracle.com
>>>      Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>
>> I missed it.  Not he submitter's fault; it was cc'ed to me.
> 
> If someone to blame, it's the reviewer.
At end, I was the one who merged this stuff. My bad.

I felt confident at the time, as it was only a single-line change reviewed
by a subsystem maintainer and the patch was large enough that this didn't
raise my attention.

But it should have been treated with greater caution, global features must
be introduced together with concrete usage proposals. I think this would
have raised some unconscious red flags.

Thanks,

C.

Re: Should functions that free memory clear the pointer?

[Cédric Le Goater]

On 11/26/25 07:12, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
>> On Tue, Nov 25, 2025 at 07:48:44PM +0100, Markus Armbruster wrote:
>>> My main argument remains this one: our deallocating functions don't work
>>> that way.  For better or worse.
>>>
>>> I don't want the Error API free differently.
>>
>> Yes, it's fair.
>>
>> IMHO, it's a matter of taste,
> 
> Depends on context.
> 
> Modula-2's DISPOSE takes the pointer by reference and clears it.  C's
> free() takes it by value.  Matter of taste when these were designed.
> 
> But when one style has become overwhelmingly prevalent, it's no longer a
> matter of taste.  This is arguably the case in C.
> 
>>                                and maybe that's also why I liked
>> g_clear_pointer() but not everyone likes it.. said that, when we have
>> g_clear_pointer() it also makes the current form more flexible, because we
>> can apply g_clear_pointer() on top when we need a reset..
> 
> I'm no friend of g_clear_pointer().  I find
> 
>      g_clear_pointer(&ptr, free_frob);
> 
> a rather roundabout way to say
> 
>      free_frob(ptr);
>      ptr = NULL;
> 
> Any C programmer will immediately understand the latter.  For the
> former, you need to know one more little thing.  Yes, we can all get
> used to these little things, but it's one more little thing new people
> have to learn and internalize.  Even little things add up.
> 
> If an entire project gets into the habit of using it religiously, it may
> reduce "forgot to zap the reference" bugs some.  Until then, it feels
> like a net negative to me.

I'm not a big fan of g_clear_pointer() either; I prefer being explicit,
even if it means a bit of redundancy.


Thanks,

C.



> 
>> I'll follow your advise on error reporting for migration.
> 
> Thank you!
> 
> 

Re: g_autoptr(Error)

[Cédric Le Goater]

On 11/26/25 09:19, Cédric Le Goater wrote:
> On 11/25/25 17:15, Peter Xu wrote:
>> On Tue, Nov 25, 2025 at 12:46:01PM +0100, Markus Armbruster wrote:
>>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>>
>>>> On Tue, Nov 25, 2025 at 08:40:07AM +0100, Markus Armbruster wrote:
>>>>> g_autoptr(T) is quite useful when the object's extent matches the
>>>>> function's.
>>>>>
>>>>> This isn't the case for an Error object the function propagates to its
>>>>> caller.  It is the case for an Error object the function reports or
>>>>> handles itself.  However, the functions to report Error also free it.
>>
>> I'd confess I didn't pay enough attention on how the error API was designed
>> deliberately to always free the Error objects before almost whenever
>> possible.  But I see now, thanks for the write up.
>>
>>>>>
>>>>> Thus, g_autoptr(Error) is rarely applicable.  We have just three
>>>>> instances out of >1100 local Error variables, all in migration code.
>>>>>
>>>>> Two want to move the error to the MigrationState for later handling /
>>>>> reporting.  Since migrate_set_error() doesn't move, but stores a copy,
>>>>> the original needs to be freed, and g_autoptr() is correct there.  We
>>>>> have 17 more that instead manually free with error_free() or
>>>>> error_report_err() right after migrate_set_error().
>>>>>
>>>>> We recently discussed storing a copy vs. move the original:
>>>>>
>>>>>      From: Peter Xu <peterx@redhat.com>
>>>>>      Subject: Re: [PATCH 0/3] migration: Error fixes and improvements
>>>>>      Date: Mon, 17 Nov 2025 11:03:37 -0500
>>>>>      Message-ID: <aRtHWbWcTh3OF2wY@x1.local>
>>>>>
>>>>> The two g_autoptr() gave me pause when I investigated this topic, simply
>>>>> because they deviate from the common pattern migrate_set_error(s, err)
>>>>> followed by error_free() or error_report_err().
>>>>>
>>>>> The third one became wrong when I cleaned up the reporting (missed in
>>>>> the cleanup patch, fixed in the patch I'm replying to).  I suspect my
>>>>> mistake escaped review for the same reason I made it: g_autoptr(Error)
>>>>> is unusual and not visible in the patch hunk.
>>>>>
>>>>> Would you like me to replace the two correct uses of g_autoptr(Error) by
>>>>> more common usage?
>>
>> Works for me.
>>
>> Now I also think it should be good migrate_set_error() follow QEMU's Error
>> API design if we decide to stick with it freeing errors in such APIs.
>>
>> Said that, I wonder if you think we could still consider passing Error**
>> into migrate_set_error(), though, which will be a merged solution of
>> current Error API and what Marc-Andre proposed on resetting pointers to
>> avoid any possible UAF, which I would still slightly prefer personally.
>>
>> If we rework migrate_set_error() to take ownership first, then we can
>> naturally drop the two use cases, and remove the cleanup function.
>>
>> Markus, please also let me know if you want me to do it.
>>
>>>>
>>>> I had previously proposed g_autoptr(Error) a year or two back and you
>>>> rejected it then, so I'm surprised to see that it got into the code,
>>>> because it requires explicit opt-in via a G_DEFINE_AUTOPTR_CLEANUP_FUNC.
>>>>
>>>> Unfortunately it appears exactly that was added earlier this year in
>>>>
>>>>    commit 18eb55546a54e443d94a4c49286348176ad4b00a
>>>>    Author: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>>    Date:   Tue Mar 4 23:03:35 2025 +0100
>>>>
>>>>      error: define g_autoptr() cleanup function for the Error type
>>>>      Automatic memory management helps avoid memory safety issues.
>>>>      Reviewed-by: Peter Xu <peterx@redhat.com>
>>>>      Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>>      Link: https://lore.kernel.org/qemu-devel/a5843c5fa64d7e5239a4316092ec0ef0d10c2320.1741124640.git.maciej.szmigiero@oracle.com
>>>>      Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>>
>>> I missed it.  Not he submitter's fault; it was cc'ed to me.
>>
>> If someone to blame, it's the reviewer.
> At end, I was the one who merged this stuff. My bad.
> 
> I felt confident at the time, as it was only a single-line change reviewed
> by a subsystem maintainer and the patch was large enough that this didn't

s/patch/series/ makes more sense.

Sorry for the noise.

C.


> raise my attention.
> 
> But it should have been treated with greater caution, global features must
> be introduced together with concrete usage proposals. I think this would
> have raised some unconscious red flags.
> 
> Thanks,
> 
> C.
> 
> 
> 
> 
> 
> 

Re: g_autoptr(Error)

[Markus Armbruster]

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

> On 11/26/25 09:19, Cédric Le Goater wrote:
>> On 11/25/25 17:15, Peter Xu wrote:
>>> On Tue, Nov 25, 2025 at 12:46:01PM +0100, Markus Armbruster wrote:

[...]

On the review and merging of commit 18eb55546a5 (error: define
g_autoptr() cleanup function for the Error type):

>>>> I missed it.  Not he submitter's fault; it was cc'ed to me.
>>>
>>> If someone to blame, it's the reviewer.
>> At end, I was the one who merged this stuff. My bad.
>>
>> I felt confident at the time, as it was only a single-line change reviewed
>> by a subsystem maintainer and the patch was large enough that this didn't
>
> s/patch/series/ makes more sense.
>
> Sorry for the noise.

> C.
>
>
>> raise my attention.
>>
>> But it should have been treated with greater caution, global features must
>> be introduced together with concrete usage proposals. I think this would
>> have raised some unconscious red flags.
>> Thanks,
>> C.

Don't worry about it!  From my point of view, the process worked okay.
A big series got reviewed by maintainers, except for one little patch
touching another subsystem, where that subsystem's maintainer (me)
remained silent.  The series was then merged without further delay.

Would I have appreciate a timely nudge on that little patch?  Sure.  Is
not nudging me a failure of sorts?  Nope.

Re: g_autoptr(Error)

[Daniel P. Berrangé]

On Wed, Nov 26, 2025 at 12:46:40PM +0100, Markus Armbruster wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
> > On 11/26/25 09:19, Cédric Le Goater wrote:
> >> On 11/25/25 17:15, Peter Xu wrote:
> >>> On Tue, Nov 25, 2025 at 12:46:01PM +0100, Markus Armbruster wrote:
> 
> [...]
> 
> On the review and merging of commit 18eb55546a5 (error: define
> g_autoptr() cleanup function for the Error type):
> 
> >>>> I missed it.  Not he submitter's fault; it was cc'ed to me.
> >>>
> >>> If someone to blame, it's the reviewer.
> >> At end, I was the one who merged this stuff. My bad.
> >>
> >> I felt confident at the time, as it was only a single-line change reviewed
> >> by a subsystem maintainer and the patch was large enough that this didn't
> >
> > s/patch/series/ makes more sense.
> >
> > Sorry for the noise.
> 
> > C.
> >
> >
> >> raise my attention.
> >>
> >> But it should have been treated with greater caution, global features must
> >> be introduced together with concrete usage proposals. I think this would
> >> have raised some unconscious red flags.
> >> Thanks,
> >> C.
> 
> Don't worry about it!  From my point of view, the process worked okay.
> A big series got reviewed by maintainers, except for one little patch
> touching another subsystem, where that subsystem's maintainer (me)
> remained silent.  The series was then merged without further delay.
> 
> Would I have appreciate a timely nudge on that little patch?  Sure.  Is
> not nudging me a failure of sorts?  Nope.

The other thing that plays in here is that we actively encourage use of
g_autoptr everywhere. It is very unusual for "Error" to be a type that
does NOT want g_autoptr, and thus the mistake is very much on the cards.

I've proposed it before myself & Markus caught it. I also caught one
other proposals to add it since my attempt. This third time it slipped
through review. I expect we'll see a 4th attempt to add it at some point.

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 :|

Re: g_autoptr(Error)

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Nov 26, 2025 at 12:46:40PM +0100, Markus Armbruster wrote:

[...]

>> Don't worry about it!  From my point of view, the process worked okay.
>> A big series got reviewed by maintainers, except for one little patch
>> touching another subsystem, where that subsystem's maintainer (me)
>> remained silent.  The series was then merged without further delay.
>> 
>> Would I have appreciate a timely nudge on that little patch?  Sure.  Is
>> not nudging me a failure of sorts?  Nope.
>
> The other thing that plays in here is that we actively encourage use of
> g_autoptr everywhere.

We do, and for good reasons.

>                       It is very unusual for "Error" to be a type that
> does NOT want g_autoptr, and thus the mistake is very much on the cards.
>
> I've proposed it before myself & Markus caught it. I also caught one
> other proposals to add it since my attempt. This third time it slipped
> through review. I expect we'll see a 4th attempt to add it at some point.

%-}

Re: g_autoptr(Error)

[Peter Maydell]

On Wed, 26 Nov 2025 at 12:01, Daniel P. Berrangé <berrange@redhat.com> wrote:
> The other thing that plays in here is that we actively encourage use of
> g_autoptr everywhere. It is very unusual for "Error" to be a type that
> does NOT want g_autoptr, and thus the mistake is very much on the cards.
>
> I've proposed it before myself & Markus caught it. I also caught one
> other proposals to add it since my attempt. This third time it slipped
> through review. I expect we'll see a 4th attempt to add it at some point.

We could add a comment to the header explaining the rationale for
not providing a g_autoptr handler, so that at least we have it
written down, and are more likely to remember it when it comes
up again in future...

-- PMM

Re: g_autoptr(Error)

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 26 Nov 2025 at 12:01, Daniel P. Berrangé <berrange@redhat.com> wrote:
>> The other thing that plays in here is that we actively encourage use of
>> g_autoptr everywhere. It is very unusual for "Error" to be a type that
>> does NOT want g_autoptr, and thus the mistake is very much on the cards.
>>
>> I've proposed it before myself & Markus caught it. I also caught one
>> other proposals to add it since my attempt. This third time it slipped
>> through review. I expect we'll see a 4th attempt to add it at some point.
>
> We could add a comment to the header explaining the rationale for
> not providing a g_autoptr handler, so that at least we have it
> written down, and are more likely to remember it when it comes
> up again in future...

Good idea.  I'll take care of it.

Re: [PATCH] migration: Fix double-free on error path

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Fixes: ffaa1b50a879 (migration: Use warn_reportf_err() where appropriate)
> Resolves: Coverity CID 1643463
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Queued for 10.2.