[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v11 09/10] migration: Implement MigrateChannelList to hmp mig
From: |
Fabiano Rosas |
Subject: |
Re: [PATCH v11 09/10] migration: Implement MigrateChannelList to hmp migration flow. |
Date: |
Mon, 09 Oct 2023 11:35:40 -0300 |
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
- Re: [PATCH v11 02/10] migration: convert migration 'uri' into 'MigrateAddress', (continued)
- [PATCH v11 03/10] migration: convert socket backend to accept MigrateAddress, Het Gala, 2023/10/04
- [PATCH v11 04/10] migration: convert rdma backend to accept MigrateAddress, Het Gala, 2023/10/04
- [PATCH v11 05/10] migration: convert exec backend to accept MigrateAddress., Het Gala, 2023/10/04
- [PATCH v11 06/10] migration: New migrate and migrate-incoming argument 'channels', Het Gala, 2023/10/04
- [PATCH v11 09/10] migration: Implement MigrateChannelList to hmp migration flow., Het Gala, 2023/10/04
[PATCH v11 10/10] migration: modify test_multifd_tcp_none() to use new QAPI syntax., Het Gala, 2023/10/04
[PATCH v11 08/10] migration: Implement MigrateChannelList to qmp migration flow., Het Gala, 2023/10/04
[PATCH v11 07/10] migration: modify migration_channels_and_uri_compatible() for new QAPI syntax, Het Gala, 2023/10/04