Het Gala <het.g...@nutanix.com> writes: > On 10/4/2023 8:55 PM, Fabiano Rosas wrote: >> Het Gala<het.g...@nutanix.com> writes: >> >>> Integrate MigrateChannelList with all transport backends >>> (socket, exec and rdma) for both src and dest migration >>> endpoints for hmp migration. >>> >>> Suggested-by: Aravind Retnakaran<aravind.retnaka...@nutanix.com> >>> Signed-off-by: Het Gala<het.g...@nutanix.com> >>> Reviewed-by: Daniel P. Berrangé<berra...@redhat.com> >>> --- >>> migration/migration-hmp-cmds.c | 15 +++++++++++++-- >>> migration/migration.c | 5 ++--- >>> migration/migration.h | 3 ++- >>> 3 files changed, 17 insertions(+), 6 deletions(-) >>> >>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c >>> index a2e6a5c51e..a1657f3d37 100644 >>> --- a/migration/migration-hmp-cmds.c >>> +++ b/migration/migration-hmp-cmds.c >>> @@ -441,9 +441,14 @@ void hmp_migrate_incoming(Monitor *mon, const QDict >>> *qdict) >>> { >>> Error *err = NULL; >>> const char *uri = qdict_get_str(qdict, "uri"); >>> + MigrationChannelList *caps = NULL; >>> + g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1); >> Just the pointer here. If I remember correctly the g_autoptr here would >> cause a double free when freeing the caps. > > Yes, we'll just have 'g_autoptr(MigrationChannel) channel = NULL'. > > Is it because inside QAPI_LIST_PREPEND, caps will be refrencing to the > same memory as 'channel', we don't need to free channel ?
Slightly different scenario here. Here the issue is that we will free the caps with qapi_free_MigrationChannel() before returning. Then, at function exit the g_autoptr will try to free 'channel', which has already been freed along with 'caps'. That's a double free, I think it hits an assert inside glib, if I remember correctly. > I am still not > sure what is the right place to use g_steal_pointer(), is this a right > place to use (non-error paths) ? It doesn't look like we need it here. As long as the qapi list has a reference and we're freeing the caps, then channel should be freed by that function already. >>> >>> - qmp_migrate_incoming(uri, false, NULL, &err); >>> + migrate_uri_parse(uri, &channel, &err); >>> + QAPI_LIST_PREPEND(caps, channel); >>> >>> + qmp_migrate_incoming(NULL, true, caps, &err); >>> + qapi_free_MigrationChannelList(caps); >>> hmp_handle_error(mon, err); >>> } >>> >>> @@ -730,9 +735,15 @@ void hmp_migrate(Monitor *mon, const QDict *qdict) >>> bool resume = qdict_get_try_bool(qdict, "resume", false); >>> const char *uri = qdict_get_str(qdict, "uri"); >>> Error *err = NULL; >>> + MigrationChannelList *caps = NULL; >>> + g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1); >> Same here. We free the channel via caps and we attribute it below, no >> need to allocate. > Ack. >>> >>> - qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc, >>> + migrate_uri_parse(uri, &channel, &err); >>> + QAPI_LIST_PREPEND(caps, channel); >>> + >>> + qmp_migrate(NULL, true, caps, !!blk, blk, !!inc, inc, >>> false, false, true, resume, &err); >>> + qapi_free_MigrationChannelList(caps); >>> if (hmp_handle_error(mon, err)) { > Regards, > Het Gala