1. Patchew
  2. QEMU
  3. [PATCH v11 00/10] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration
  4. View patch

[PATCH v11 09/10] migration: Implement MigrateChannelList to hmp migration flow.

Het Gala posted 10 patches 2 years, 4 months ago
Maintainers: Juan Quintela <quintela@redhat.com>, Peter Xu <peterx@redhat.com>, Leonardo Bras <leobras@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
[PATCH v11 09/10] migration: Implement MigrateChannelList to hmp migration flow.
Posted by Het Gala 2 years, 4 months ago 
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);
 
-    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 \
-- 
2.22.3
Re: [PATCH v11 09/10] migration: Implement MigrateChannelList to hmp migration flow.
Posted by Fabiano Rosas 2 years, 4 months ago 
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 \
Re: [PATCH v11 09/10] migration: Implement MigrateChannelList to hmp migration flow.
Posted by Het Gala 2 years, 4 months ago 
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
Re: [PATCH v11 09/10] migration: Implement MigrateChannelList to hmp migration flow.
Posted by Fabiano Rosas 2 years, 4 months ago 
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 09/10] migration: Implement MigrateChannelList to hmp migration flow.
Posted by Het Gala 2 years, 4 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.