Re: [PULL 4/9] migration: Use warn_reportf_err() where appropriate

[Markus Armbruster <armbru@redhat.com>]

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

> On Sat, 22 Nov 2025 at 02:02, Peter Xu <peterx@redhat.com> wrote:
>>
>> From: Markus Armbruster <armbru@redhat.com>
>>
>> Replace
>>
>>     warn_report("...: %s", ..., error_get_pretty(err));
>>
>> by
>>
>>     warn_reportf_err(err, "...: ", ...);
>>
>> Prior art: commit 5217f1887a8 (error: Use error_reportf_err() where
>> appropriate).
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Fabiano Rosas <farosas@suse.de>
>> Link: https://lore.kernel.org/r/20251115083500.2753895-3-armbru@redhat.com
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>  migration/multifd.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index a529c399e4..6210454838 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -464,8 +464,8 @@ static void migration_ioc_shutdown_gracefully(QIOChannel *ioc)
>>           */
>>          migration_tls_channel_end(ioc, &local_err);
>>          if (local_err) {
>> -            warn_report("Failed to gracefully terminate TLS connection: %s",
>> -                        error_get_pretty(local_err));
>> +            warn_reportf_err(local_err,
>> +                        "Failed to gracefully terminate TLS connection: ");
>>          }
>>      }
>
> Hi; Coverity points out (CID 1643463) that this introduces a double-free
> of local_err. In this function local_err is marked up as g_autoptr()
> so it is automatically freed when it goes out of scope. This was needed
> because error_get_pretty() doesn't free its argument. But
> warn_reportf_err() *does* free its error argument, so now we free it twice.
>
> Dropping the g_autoptr markup would be enough, I think.

I'll take care of it.  Hand me the brown paper bag, please.

> The "prior art" commit 5217f1887a8 also seemed to introduce
> some double-frees in hw/usb/dev-mtp.c, but it looks like we
> fixed those in 562a55864 (but with a Fixes: tag that didn't
> point at the commit that introduced them but at a different
> blameless one).

You're right, it's the one that added the error_free(), which only
became wrong later.

I re-checked all of commit 5217f1887a8, and could not find lingering
double-frees.

Thanks!