On Tue, Nov 25, 2025 at 01:59:54PM +0100, Markus Armbruster wrote: > Marc-André Lureau <[email protected]> writes: > > > Hi > > > > On Tue, Nov 25, 2025 at 11:06 AM Markus Armbruster <[email protected]> > > wrote: > >> > >> Fixes: ffaa1b50a879 (migration: Use warn_reportf_err() where appropriate) > >> Resolves: Coverity CID 1643463 > >> Signed-off-by: Markus Armbruster <[email protected]> > > > > Reviewed-by: Marc-André Lureau <[email protected]> > > > >> --- > >> 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
