| Message ID | 20231004075851.219173-10-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration | expand |
Het Gala <het.gala@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.retnakaran@nutanix.com> > Signed-off-by: Het Gala <het.gala@nutanix.com> > Reviewed-by: Daniel P. Berrangé <berrange@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. > > - 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. > > - 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)) { > return; > } > diff --git a/migration/migration.c b/migration/migration.c > index 3eae32e616..7d2d5ae329 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -431,9 +431,8 @@ void migrate_add_address(SocketAddress *address) > QAPI_CLONE(SocketAddress, address)); > } > > -static bool migrate_uri_parse(const char *uri, > - MigrationChannel **channel, > - Error **errp) > +bool migrate_uri_parse(const char *uri, MigrationChannel **channel, > + Error **errp) > { > g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1); > g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); > diff --git a/migration/migration.h b/migration/migration.h > index 972597f4de..f9127707f5 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -511,7 +511,8 @@ bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm, > Error **errp); > > void migrate_add_address(SocketAddress *address); > - > +bool migrate_uri_parse(const char *uri, MigrationChannel **channel, > + Error **errp); > int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque); > > #define qemu_ram_foreach_block \
On 10/4/2023 8:55 PM, Fabiano Rosas wrote: > Het Gala<het.gala@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.retnakaran@nutanix.com> >> Signed-off-by: Het Gala<het.gala@nutanix.com> >> Reviewed-by: Daniel P. Berrangé<berrange@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 ? 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) ? >> >> - 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
Het Gala <het.gala@nutanix.com> writes: > On 10/4/2023 8:55 PM, Fabiano Rosas wrote: >> Het Gala<het.gala@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.retnakaran@nutanix.com> >>> Signed-off-by: Het Gala<het.gala@nutanix.com> >>> Reviewed-by: Daniel P. Berrangé<berrange@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
On 10/9/2023 8:05 PM, Fabiano Rosas wrote: > Het Gala<het.gala@nutanix.com> writes: > >> On 10/4/2023 8:55 PM, Fabiano Rosas wrote: >>> Het Gala<het.gala@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.retnakaran@nutanix.com> >>>> Signed-off-by: Het Gala<het.gala@nutanix.com> >>>> Reviewed-by: Daniel P. Berrangé<berrange@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. Ack. Yes, with the discussion in earlier patches, I also don't think we need g_autoptr too here. Normal pointer is enough as we are freeing the memory after the function is returned. >>>> >>>> - 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 Regards, Het Gala
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); - 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); - 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)) { return; } diff --git a/migration/migration.c b/migration/migration.c index 3eae32e616..7d2d5ae329 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -431,9 +431,8 @@ void migrate_add_address(SocketAddress *address) QAPI_CLONE(SocketAddress, address)); } -static bool migrate_uri_parse(const char *uri, - MigrationChannel **channel, - Error **errp) +bool migrate_uri_parse(const char *uri, MigrationChannel **channel, + Error **errp) { g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1); g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); diff --git a/migration/migration.h b/migration/migration.h index 972597f4de..f9127707f5 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -511,7 +511,8 @@ bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm, Error **errp); void migrate_add_address(SocketAddress *address); - +bool migrate_uri_parse(const char *uri, MigrationChannel **channel, + Error **errp); int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque); #define qemu_ram_foreach_block \