[PATCH v11 00/10] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration

[PATCH v11 00/10] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration

[Het Gala]

This is v11 patchset of modified 'migrate' and 'migrate-incoming' QAPI design
for upstream review.

Update: Daniel has reviewed all patches and is okay with them. Markus has also
        given Acked-by tag for patches related to QAPI syntax change.
Fabiano, Juan and other migration maintainers, let me know if there are still
improvements to be made in this patchset series.

Link to previous upstream community patchset links:
v1: https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg04339.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg02106.html
v3: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg02473.html
v4: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03064.html
v5: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04845.html
v6: https://lists.gnu.org/archive/html/qemu-devel/2023-06/msg01251.html
v7: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg02027.html
v8: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg02770.html
v9: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg04216.html
v10: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg05022.html

v10 -> v11 changelog:
-------------------
- Resolved make check errors as its been almost two months since v10
  version of this patchset series went out. Till date migration workflow
  might have changed which caused make check errors.

Abstract:
---------

Current QAPI 'migrate' command design (for initiating a migration
stream) contains information regarding different migrate transport mechanism
(tcp / unix / exec), dest-host IP address, and binding port number in form of
a string. Thus the design does seem to have some design issues. Some of the
issues, stated below are:

1. Use of string URIs is a data encoding scheme within a data encoding scheme.
   QEMU code should directly be able to work with the results from QAPI,
   without resorting to do a second level of parsing (eg. socket_parse()).
2. For features / parameters related to migration, the migration tunables needs
   to be defined and updated upfront. For example, 'migrate-set-capability'
   and 'migrate-set-parameter' is required to enable multifd capability and
   multifd-number of channels respectively. Instead, 'Multifd-channels' can
   directly be represented as a single additional parameter to 'migrate'
   QAPI. 'migrate-set-capability' and 'migrate-set-parameter' commands could
   be used for runtime tunables that need setting after migration has already
   started.

The current patchset focuses on solving the first problem of multi-level
encoding of URIs. The patch defines 'migrate' command as a QAPI discriminated
union for the various transport backends (like socket, exec and rdma), and on
basis of transport backends, different migration parameters are defined.

(uri) string -->  (channel) Channel-type
                            Transport-type
                            Migration parameters based on transport type
------------------------------------------------------------------------------

Het Gala (10):
  migration: New QAPI type 'MigrateAddress'
  migration: convert migration 'uri' into 'MigrateAddress'
  migration: convert socket backend to accept MigrateAddress
  migration: convert rdma backend to accept MigrateAddress
  migration: convert exec backend to accept MigrateAddress.
  migration: New migrate and migrate-incoming argument 'channels'
  migration: modify migration_channels_and_uri_compatible() for new QAPI
    syntax
  migration: Implement MigrateChannelList to qmp migration flow.
  migration: Implement MigrateChannelList to hmp migration flow.
  migration: modify test_multifd_tcp_none() to use new QAPI syntax.

 migration/exec.c               |  72 +++++++++----
 migration/exec.h               |   8 +-
 migration/migration-hmp-cmds.c |  17 ++-
 migration/migration.c          | 190 ++++++++++++++++++++++++++-------
 migration/migration.h          |   3 +-
 migration/rdma.c               |  34 +++---
 migration/rdma.h               |   6 +-
 migration/socket.c             |  39 ++-----
 migration/socket.h             |   7 +-
 qapi/migration.json            | 150 +++++++++++++++++++++++++-
 softmmu/vl.c                   |   2 +-
 tests/qtest/migration-test.c   |   7 +-
 12 files changed, 409 insertions(+), 126 deletions(-)

-- 
2.22.3

[PATCH v11 01/10] migration: New QAPI type 'MigrateAddress'

[Het Gala]

This patch introduces well defined MigrateAddress struct
and its related child objects.

The existing argument of 'migrate' and 'migrate-incoming' QAPI
- 'uri' is of type string. The current implementation follows
double encoding scheme for fetching migration parameters like
'uri' and this is not an ideal design.

Motive for intoducing struct level design is to prevent double
encoding of QAPI arguments, as Qemu should be able to directly
use the QAPI arguments without any level of encoding.

Note: this commit only adds the type, and actual uses comes
in later commits.

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Acked-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/qapi/migration.json b/qapi/migration.json
index 8843e74b59..4e4c39a9bd 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1478,6 +1478,47 @@
 ##
 { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
 
+##
+# @MigrationAddressType:
+#
+# The migration stream transport mechanisms.
+#
+# @socket: Migrate via socket.
+#
+# @exec: Direct the migration stream to another process.
+#
+# @rdma: Migrate via RDMA.
+#
+# Since 8.2
+##
+{ 'enum': 'MigrationAddressType',
+  'data': ['socket', 'exec', 'rdma'] }
+
+##
+# @MigrationExecCommand:
+#
+# @args: command (list head) and arguments to execute.
+#
+# Since 8.2
+##
+{ 'struct': 'MigrationExecCommand',
+  'data': {'args': [ 'str' ] } }
+
+##
+# @MigrationAddress:
+#
+# Migration endpoint configuration.
+#
+# Since 8.2
+##
+{ 'union': 'MigrationAddress',
+  'base': { 'transport' : 'MigrationAddressType'},
+  'discriminator': 'transport',
+  'data': {
+    'socket': 'SocketAddress',
+    'exec': 'MigrationExecCommand',
+    'rdma': 'InetSocketAddress' } }
+
 ##
 # @migrate:
 #
-- 
2.22.3

[PATCH v11 02/10] migration: convert migration 'uri' into 'MigrateAddress'

[Het Gala]

This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri'
string containing migration connection related information
and stores them inside well defined 'MigrateAddress' struct.

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/exec.c      |  1 -
 migration/exec.h      |  4 ++++
 migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/migration/exec.c b/migration/exec.c
index 2bf882bbe1..32f5143dfd 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -27,7 +27,6 @@
 #include "qemu/cutils.h"
 
 #ifdef WIN32
-const char *exec_get_cmd_path(void);
 const char *exec_get_cmd_path(void)
 {
     g_autofree char *detected_path = g_new(char, MAX_PATH);
diff --git a/migration/exec.h b/migration/exec.h
index b210ffde7a..736cd71028 100644
--- a/migration/exec.h
+++ b/migration/exec.h
@@ -19,6 +19,10 @@
 
 #ifndef QEMU_MIGRATION_EXEC_H
 #define QEMU_MIGRATION_EXEC_H
+
+#ifdef WIN32
+const char *exec_get_cmd_path(void);
+#endif
 void exec_start_incoming_migration(const char *host_port, Error **errp);
 
 void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
diff --git a/migration/migration.c b/migration/migration.c
index 6d3cf5d5cd..dcbd509d56 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -65,6 +65,7 @@
 #include "sysemu/qtest.h"
 #include "options.h"
 #include "sysemu/dirtylimit.h"
+#include "qemu/sockets.h"
 
 static NotifierList migration_state_notifiers =
     NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
@@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address)
                       QAPI_CLONE(SocketAddress, address));
 }
 
+static bool migrate_uri_parse(const char *uri,
+                              MigrationAddress **channel,
+                              Error **errp)
+{
+    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
+    SocketAddress *saddr = &addr->u.socket;
+    InetSocketAddress *isock = &addr->u.rdma;
+    strList **tail = &addr->u.exec.args;
+
+    if (strstart(uri, "exec:", NULL)) {
+        addr->transport = MIGRATION_ADDRESS_TYPE_EXEC;
+#ifdef WIN32
+        QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
+        QAPI_LIST_APPEND(tail, g_strdup("/c"));
+#else
+        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
+        QAPI_LIST_APPEND(tail, g_strdup("-c"));
+#endif
+        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
+    } else if (strstart(uri, "rdma:", NULL)) {
+        if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
+            qapi_free_InetSocketAddress(isock);
+            return false;
+        }
+        addr->transport = MIGRATION_ADDRESS_TYPE_RDMA;
+    } else if (strstart(uri, "tcp:", NULL) ||
+                strstart(uri, "unix:", NULL) ||
+                strstart(uri, "vsock:", NULL) ||
+                strstart(uri, "fd:", NULL)) {
+        addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET;
+        saddr = socket_parse(uri, errp);
+        if (!saddr) {
+            qapi_free_SocketAddress(saddr);
+            return false;
+        }
+    } else {
+        error_setg(errp, "unknown migration protocol: %s", uri);
+        return false;
+    }
+
+    *channel = addr;
+    return true;
+}
+
 static void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p = NULL;
+    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
 
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
         return;
     }
 
+    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
+        return;
+    }
+
     qapi_event_send_migration(MIGRATION_STATUS_SETUP);
     if (strstart(uri, "tcp:", &p) ||
         strstart(uri, "unix:", NULL) ||
@@ -1671,12 +1721,17 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
     const char *p = NULL;
+    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
 
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
         return;
     }
 
+    if (!migrate_uri_parse(uri, &channel, errp)) {
+        return;
+    }
+
     resume_requested = has_resume && resume;
     if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
                          resume_requested, errp)) {
-- 
2.22.3

[PATCH v11 03/10] migration: convert socket backend to accept MigrateAddress

[Het Gala]

Socket transport backend for 'migrate'/'migrate-incoming' QAPIs accept
new wire protocol of MigrateAddress struct.

It is achived by parsing 'uri' string and storing migration parameters
required for socket connection into well defined SocketAddress struct.

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.c | 30 ++++++++++++++++++------------
 migration/socket.c    | 39 +++++++++------------------------------
 migration/socket.h    |  7 ++++---
 3 files changed, 31 insertions(+), 45 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index dcbd509d56..b773f0110f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -487,18 +487,21 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
     }
 
     qapi_event_send_migration(MIGRATION_STATUS_SETUP);
-    if (strstart(uri, "tcp:", &p) ||
-        strstart(uri, "unix:", NULL) ||
-        strstart(uri, "vsock:", NULL)) {
-        socket_start_incoming_migration(p ? p : uri, errp);
+    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
+        SocketAddress *saddr = &channel->u.socket;
+        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
+            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
+            socket_start_incoming_migration(saddr, errp);
+        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
+            fd_start_incoming_migration(saddr->u.fd.str, errp);
+        }
 #ifdef CONFIG_RDMA
     } else if (strstart(uri, "rdma:", &p)) {
         rdma_start_incoming_migration(p, errp);
 #endif
     } else if (strstart(uri, "exec:", &p)) {
         exec_start_incoming_migration(p, errp);
-    } else if (strstart(uri, "fd:", &p)) {
-        fd_start_incoming_migration(p, errp);
     } else {
         error_setg(errp, "unknown migration protocol: %s", uri);
     }
@@ -1745,18 +1748,21 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         }
     }
 
-    if (strstart(uri, "tcp:", &p) ||
-        strstart(uri, "unix:", NULL) ||
-        strstart(uri, "vsock:", NULL)) {
-        socket_start_outgoing_migration(s, p ? p : uri, &local_err);
+    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
+        SocketAddress *saddr = &channel->u.socket;
+        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
+            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
+            socket_start_outgoing_migration(s, saddr, &local_err);
+        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
+            fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
+        }
 #ifdef CONFIG_RDMA
     } else if (strstart(uri, "rdma:", &p)) {
         rdma_start_outgoing_migration(s, p, &local_err);
 #endif
     } else if (strstart(uri, "exec:", &p)) {
         exec_start_outgoing_migration(s, p, &local_err);
-    } else if (strstart(uri, "fd:", &p)) {
-        fd_start_outgoing_migration(s, p, &local_err);
     } else {
         error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
                    "a valid migration protocol");
diff --git a/migration/socket.c b/migration/socket.c
index 1b6f5baefb..98e3ea1514 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -28,6 +28,8 @@
 #include "trace.h"
 #include "postcopy-ram.h"
 #include "options.h"
+#include "qapi/clone-visitor.h"
+#include "qapi/qapi-visit-sockets.h"
 
 struct SocketOutgoingArgs {
     SocketAddress *saddr;
@@ -108,19 +110,19 @@ out:
     object_unref(OBJECT(sioc));
 }
 
-static void
-socket_start_outgoing_migration_internal(MigrationState *s,
-                                         SocketAddress *saddr,
-                                         Error **errp)
+void socket_start_outgoing_migration(MigrationState *s,
+                                     SocketAddress *saddr,
+                                     Error **errp)
 {
     QIOChannelSocket *sioc = qio_channel_socket_new();
     struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
+    SocketAddress *addr = QAPI_CLONE(SocketAddress, saddr);
 
     data->s = s;
 
     /* in case previous migration leaked it */
     qapi_free_SocketAddress(outgoing_args.saddr);
-    outgoing_args.saddr = saddr;
+    outgoing_args.saddr = addr;
 
     if (saddr->type == SOCKET_ADDRESS_TYPE_INET) {
         data->hostname = g_strdup(saddr->u.inet.host);
@@ -135,18 +137,6 @@ socket_start_outgoing_migration_internal(MigrationState *s,
                                      NULL);
 }
 
-void socket_start_outgoing_migration(MigrationState *s,
-                                     const char *str,
-                                     Error **errp)
-{
-    Error *err = NULL;
-    SocketAddress *saddr = socket_parse(str, &err);
-    if (!err) {
-        socket_start_outgoing_migration_internal(s, saddr, &err);
-    }
-    error_propagate(errp, err);
-}
-
 static void socket_accept_incoming_migration(QIONetListener *listener,
                                              QIOChannelSocket *cioc,
                                              gpointer opaque)
@@ -172,9 +162,8 @@ socket_incoming_migration_end(void *opaque)
     object_unref(OBJECT(listener));
 }
 
-static void
-socket_start_incoming_migration_internal(SocketAddress *saddr,
-                                         Error **errp)
+void socket_start_incoming_migration(SocketAddress *saddr,
+                                     Error **errp)
 {
     QIONetListener *listener = qio_net_listener_new();
     MigrationIncomingState *mis = migration_incoming_get_current();
@@ -213,13 +202,3 @@ socket_start_incoming_migration_internal(SocketAddress *saddr,
     }
 }
 
-void socket_start_incoming_migration(const char *str, Error **errp)
-{
-    Error *err = NULL;
-    SocketAddress *saddr = socket_parse(str, &err);
-    if (!err) {
-        socket_start_incoming_migration_internal(saddr, &err);
-    }
-    qapi_free_SocketAddress(saddr);
-    error_propagate(errp, err);
-}
diff --git a/migration/socket.h b/migration/socket.h
index dc54df4e6c..5e4c33b8ea 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -19,13 +19,14 @@
 
 #include "io/channel.h"
 #include "io/task.h"
+#include "qemu/sockets.h"
 
 void socket_send_channel_create(QIOTaskFunc f, void *data);
 QIOChannel *socket_send_channel_create_sync(Error **errp);
 int socket_send_channel_destroy(QIOChannel *send);
 
-void socket_start_incoming_migration(const char *str, Error **errp);
+void socket_start_incoming_migration(SocketAddress *saddr, Error **errp);
 
-void socket_start_outgoing_migration(MigrationState *s, const char *str,
-                                     Error **errp);
+void socket_start_outgoing_migration(MigrationState *s,
+                                     SocketAddress *saddr, Error **errp);
 #endif
-- 
2.22.3

[PATCH v11 04/10] migration: convert rdma backend to accept MigrateAddress

[Het Gala]

RDMA based transport backend for 'migrate'/'migrate-incoming' QAPIs
accept new wire protocol of MigrateAddress struct.

It is achived by parsing 'uri' string and storing migration parameters
required for RDMA connection into well defined InetSocketAddress struct.

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.c |  8 ++++----
 migration/rdma.c      | 34 ++++++++++++----------------------
 migration/rdma.h      |  6 ++++--
 3 files changed, 20 insertions(+), 28 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b773f0110f..b41fda6f80 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -497,8 +497,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
             fd_start_incoming_migration(saddr->u.fd.str, errp);
         }
 #ifdef CONFIG_RDMA
-    } else if (strstart(uri, "rdma:", &p)) {
-        rdma_start_incoming_migration(p, errp);
+    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
+        rdma_start_incoming_migration(&channel->u.rdma, errp);
 #endif
     } else if (strstart(uri, "exec:", &p)) {
         exec_start_incoming_migration(p, errp);
@@ -1758,8 +1758,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
             fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
         }
 #ifdef CONFIG_RDMA
-    } else if (strstart(uri, "rdma:", &p)) {
-        rdma_start_outgoing_migration(s, p, &local_err);
+    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
+        rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
 #endif
     } else if (strstart(uri, "exec:", &p)) {
         exec_start_outgoing_migration(s, p, &local_err);
diff --git a/migration/rdma.c b/migration/rdma.c
index 7d2726d5b6..8bbf0b4c67 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -319,7 +319,6 @@ typedef struct RDMALocalBlocks {
 typedef struct RDMAContext {
     char *host;
     int port;
-    char *host_port;
 
     RDMAWorkRequestData wr_data[RDMA_WRID_MAX];
 
@@ -2475,9 +2474,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
         rdma->channel = NULL;
     }
     g_free(rdma->host);
-    g_free(rdma->host_port);
     rdma->host = NULL;
-    rdma->host_port = NULL;
 }
 
 
@@ -2759,28 +2756,17 @@ static void qemu_rdma_return_path_dest_init(RDMAContext *rdma_return_path,
     rdma_return_path->is_return_path = true;
 }
 
-static void *qemu_rdma_data_init(const char *host_port, Error **errp)
+static void *qemu_rdma_data_init(InetSocketAddress *saddr, Error **errp)
 {
     RDMAContext *rdma = NULL;
-    InetSocketAddress *addr;
 
-    if (host_port) {
+    if (saddr) {
         rdma = g_new0(RDMAContext, 1);
         rdma->current_index = -1;
         rdma->current_chunk = -1;
 
-        addr = g_new(InetSocketAddress, 1);
-        if (!inet_parse(addr, host_port, NULL)) {
-            rdma->port = atoi(addr->port);
-            rdma->host = g_strdup(addr->host);
-            rdma->host_port = g_strdup(host_port);
-        } else {
-            ERROR(errp, "bad RDMA migration address '%s'", host_port);
-            g_free(rdma);
-            rdma = NULL;
-        }
-
-        qapi_free_InetSocketAddress(addr);
+        rdma->host = g_strdup(saddr->host);
+        rdma->port = atoi(saddr->port);
     }
 
     return rdma;
@@ -3368,6 +3354,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
                                             .private_data_len = sizeof(cap),
                                          };
     RDMAContext *rdma_return_path = NULL;
+    g_autoptr(InetSocketAddress) isock = g_new0(InetSocketAddress, 1);
     struct rdma_cm_event *cm_event;
     struct ibv_context *verbs;
     int ret = -EINVAL;
@@ -3383,13 +3370,16 @@ static int qemu_rdma_accept(RDMAContext *rdma)
         goto err_rdma_dest_wait;
     }
 
+    isock->host = rdma->host;
+    isock->port = g_strdup_printf("%d", rdma->port);
+
     /*
      * initialize the RDMAContext for return path for postcopy after first
      * connection request reached.
      */
     if ((migrate_postcopy() || migrate_return_path())
         && !rdma->is_return_path) {
-        rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
+        rdma_return_path = qemu_rdma_data_init(isock, NULL);
         if (rdma_return_path == NULL) {
             rdma_ack_cm_event(cm_event);
             goto err_rdma_dest_wait;
@@ -4121,7 +4111,8 @@ static void rdma_accept_incoming_migration(void *opaque)
     }
 }
 
-void rdma_start_incoming_migration(const char *host_port, Error **errp)
+void rdma_start_incoming_migration(InetSocketAddress *host_port,
+                                   Error **errp)
 {
     int ret;
     RDMAContext *rdma;
@@ -4167,13 +4158,12 @@ err:
     error_propagate(errp, local_err);
     if (rdma) {
         g_free(rdma->host);
-        g_free(rdma->host_port);
     }
     g_free(rdma);
 }
 
 void rdma_start_outgoing_migration(void *opaque,
-                            const char *host_port, Error **errp)
+                            InetSocketAddress *host_port, Error **errp)
 {
     MigrationState *s = opaque;
     RDMAContext *rdma_return_path = NULL;
diff --git a/migration/rdma.h b/migration/rdma.h
index de2ba09dc5..ee89296555 100644
--- a/migration/rdma.h
+++ b/migration/rdma.h
@@ -14,12 +14,14 @@
  *
  */
 
+#include "qemu/sockets.h"
+
 #ifndef QEMU_MIGRATION_RDMA_H
 #define QEMU_MIGRATION_RDMA_H
 
-void rdma_start_outgoing_migration(void *opaque, const char *host_port,
+void rdma_start_outgoing_migration(void *opaque, InetSocketAddress *host_port,
                                    Error **errp);
 
-void rdma_start_incoming_migration(const char *host_port, Error **errp);
+void rdma_start_incoming_migration(InetSocketAddress *host_port, Error **errp);
 
 #endif
-- 
2.22.3

[PATCH v11 05/10] migration: convert exec backend to accept MigrateAddress.

[Het Gala]

Exec transport backend for 'migrate'/'migrate-incoming' QAPIs accept
new wire protocol of MigrateAddress struct.

It is achived by parsing 'uri' string and storing migration parameters
required for exec connection into strList struct.

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/exec.c      | 71 +++++++++++++++++++++++++++++++------------
 migration/exec.h      |  4 +--
 migration/migration.c | 10 +++---
 3 files changed, 57 insertions(+), 28 deletions(-)

diff --git a/migration/exec.c b/migration/exec.c
index 32f5143dfd..8bc321c66b 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -39,20 +39,50 @@ const char *exec_get_cmd_path(void)
 }
 #endif
 
-void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
+/* provides the length of strList */
+static int
+str_list_length(strList *list)
+{
+    int len = 0;
+    strList *elem;
+
+    for (elem = list; elem != NULL; elem = elem->next) {
+        len++;
+    }
+
+    return len;
+}
+
+static void
+init_exec_array(strList *command, char **argv, Error **errp)
+{
+    int i = 0;
+    strList *lst;
+
+    for (lst = command; lst; lst = lst->next) {
+        argv[i++] = lst->value;
+    }
+
+    argv[i] = NULL;
+    return;
+}
+
+void exec_start_outgoing_migration(MigrationState *s, strList *command,
+                                   Error **errp)
 {
     QIOChannel *ioc;
 
-#ifdef WIN32
-    const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
-#else
-    const char *argv[] = { "/bin/sh", "-c", command, NULL };
-#endif
+    int length = str_list_length(command);
+    g_auto(GStrv) argv = (char **) g_new0(const char *, length);
 
-    trace_migration_exec_outgoing(command);
-    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
-                                                    O_RDWR,
-                                                    errp));
+    init_exec_array(command, argv, errp);
+    g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
+
+    trace_migration_exec_outgoing(new_command);
+    ioc = QIO_CHANNEL(
+        qio_channel_command_new_spawn((const char * const *) argv,
+                                      O_RDWR,
+                                      errp));
     if (!ioc) {
         return;
     }
@@ -71,20 +101,21 @@ static gboolean exec_accept_incoming_migration(QIOChannel *ioc,
     return G_SOURCE_REMOVE;
 }
 
-void exec_start_incoming_migration(const char *command, Error **errp)
+void exec_start_incoming_migration(strList *command, Error **errp)
 {
     QIOChannel *ioc;
 
-#ifdef WIN32
-    const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
-#else
-    const char *argv[] = { "/bin/sh", "-c", command, NULL };
-#endif
+    int length = str_list_length(command);
+    g_auto(GStrv) argv = (char **) g_new0(const char *, length);
+
+    init_exec_array(command, argv, errp);
+    g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
 
-    trace_migration_exec_incoming(command);
-    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
-                                                    O_RDWR,
-                                                    errp));
+    trace_migration_exec_incoming(new_command);
+    ioc = QIO_CHANNEL(
+        qio_channel_command_new_spawn((const char * const *) argv,
+                                      O_RDWR,
+                                      errp));
     if (!ioc) {
         return;
     }
diff --git a/migration/exec.h b/migration/exec.h
index 736cd71028..3107f205e3 100644
--- a/migration/exec.h
+++ b/migration/exec.h
@@ -23,8 +23,8 @@
 #ifdef WIN32
 const char *exec_get_cmd_path(void);
 #endif
-void exec_start_incoming_migration(const char *host_port, Error **errp);
+void exec_start_incoming_migration(strList *host_port, Error **errp);
 
-void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
+void exec_start_outgoing_migration(MigrationState *s, strList *host_port,
                                    Error **errp);
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index b41fda6f80..ebe14b9c38 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -474,7 +474,6 @@ static bool migrate_uri_parse(const char *uri,
 
 static void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
-    const char *p = NULL;
     g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
 
     /* URI is not suitable for migration? */
@@ -500,8 +499,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
     } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
         rdma_start_incoming_migration(&channel->u.rdma, errp);
 #endif
-    } else if (strstart(uri, "exec:", &p)) {
-        exec_start_incoming_migration(p, errp);
+    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
+        exec_start_incoming_migration(channel->u.exec.args, errp);
     } else {
         error_setg(errp, "unknown migration protocol: %s", uri);
     }
@@ -1723,7 +1722,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     bool resume_requested;
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    const char *p = NULL;
     g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
 
     /* URI is not suitable for migration? */
@@ -1761,8 +1759,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
         rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
 #endif
-    } else if (strstart(uri, "exec:", &p)) {
-        exec_start_outgoing_migration(s, p, &local_err);
+    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
+        exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
     } else {
         error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
                    "a valid migration protocol");
-- 
2.22.3

[PATCH v11 06/10] migration: New migrate and migrate-incoming argument 'channels'

[Het Gala]

MigrateChannelList allows to connect accross multiple interfaces.
Add MigrateChannelList struct as argument to migration QAPIs.

We plan to include multiple channels in future, to connnect
multiple interfaces. Hence, we choose 'MigrateChannelList'
as the new argument over 'MigrateChannel' to make migration
QAPIs future proof.

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
Acked-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 migration/migration-hmp-cmds.c |   6 +-
 migration/migration.c          |  56 +++++++++++++++--
 qapi/migration.json            | 109 ++++++++++++++++++++++++++++++++-
 softmmu/vl.c                   |   2 +-
 4 files changed, 161 insertions(+), 12 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index c115ef2d23..a2e6a5c51e 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -442,7 +442,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
     const char *uri = qdict_get_str(qdict, "uri");
 
-    qmp_migrate_incoming(uri, &err);
+    qmp_migrate_incoming(uri, false, NULL, &err);
 
     hmp_handle_error(mon, err);
 }
@@ -731,8 +731,8 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
     const char *uri = qdict_get_str(qdict, "uri");
     Error *err = NULL;
 
-    qmp_migrate(uri, !!blk, blk, !!inc, inc,
-                false, false, true, resume, &err);
+    qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc,
+                 false, false, true, resume, &err);
     if (hmp_handle_error(mon, err)) {
         return;
     }
diff --git a/migration/migration.c b/migration/migration.c
index ebe14b9c38..825bf70e7a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -472,10 +472,33 @@ static bool migrate_uri_parse(const char *uri,
     return true;
 }
 
-static void qemu_start_incoming_migration(const char *uri, Error **errp)
+static void qemu_start_incoming_migration(const char *uri, bool has_channels,
+                                          MigrationChannelList *channels,
+                                          Error **errp)
 {
     g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
 
+    /*
+     * Having preliminary checks for uri and channel
+     */
+    if (has_channels) {
+        error_setg(errp, "'channels' argument should not be set yet.");
+        return;
+    }
+
+    if (uri && has_channels) {
+        error_setg(errp, "'uri' and 'channels' arguments are mutually "
+                   "exclusive; exactly one of the two should be present in "
+                   "'migrate-incoming' qmp command ");
+        return;
+    }
+
+    if (!uri && !has_channels) {
+        error_setg(errp, "neither 'uri' or 'channels' argument are "
+                   "specified in 'migrate-incoming' qmp command ");
+        return;
+    }
+
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
         return;
@@ -1530,7 +1553,8 @@ void migrate_del_blocker(Error *reason)
     migration_blockers = g_slist_remove(migration_blockers, reason);
 }
 
-void qmp_migrate_incoming(const char *uri, Error **errp)
+void qmp_migrate_incoming(const char *uri, bool has_channels,
+                          MigrationChannelList *channels, Error **errp)
 {
     Error *local_err = NULL;
     static bool once = true;
@@ -1548,7 +1572,7 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
         return;
     }
 
-    qemu_start_incoming_migration(uri, &local_err);
+    qemu_start_incoming_migration(uri, has_channels, channels, &local_err);
 
     if (local_err) {
         yank_unregister_instance(MIGRATION_YANK_INSTANCE);
@@ -1584,7 +1608,7 @@ void qmp_migrate_recover(const char *uri, Error **errp)
      * only re-setup the migration stream and poke existing migration
      * to continue using that newly established channel.
      */
-    qemu_start_incoming_migration(uri, errp);
+    qemu_start_incoming_migration(uri, false, NULL, errp);
 }
 
 void qmp_migrate_pause(Error **errp)
@@ -1715,7 +1739,8 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
     return true;
 }
 
-void qmp_migrate(const char *uri, bool has_blk, bool blk,
+void qmp_migrate(const char *uri, bool has_channels,
+                 MigrationChannelList *channels, bool has_blk, bool blk,
                  bool has_inc, bool inc, bool has_detach, bool detach,
                  bool has_resume, bool resume, Error **errp)
 {
@@ -1724,6 +1749,27 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     MigrationState *s = migrate_get_current();
     g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
 
+    /*
+     * Having preliminary checks for uri and channel
+     */
+    if (has_channels) {
+        error_setg(errp, "'channels' argument should not be set yet.");
+        return;
+    }
+
+    if (uri && has_channels) {
+        error_setg(errp, "'uri' and 'channels' arguments are mutually "
+                   "exclusive; exactly one of the two should be present in "
+                   "'migrate' qmp command ");
+        return;
+    }
+
+    if (!uri && !has_channels) {
+        error_setg(errp, "neither 'uri' or 'channels' argument are "
+                   "specified in 'migrate' qmp command ");
+        return;
+    }
+
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
         return;
diff --git a/qapi/migration.json b/qapi/migration.json
index 4e4c39a9bd..7b84c04617 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1519,6 +1519,34 @@
     'exec': 'MigrationExecCommand',
     'rdma': 'InetSocketAddress' } }
 
+##
+# @MigrationChannelType:
+#
+# The migration channel-type request options.
+#
+# @main: Main outbound migration channel.
+#
+# Since 8.1
+##
+{ 'enum': 'MigrationChannelType',
+  'data': [ 'main' ] }
+
+##
+# @MigrationChannel:
+#
+# Migration stream channel parameters.
+#
+# @channel-type: Channel type for transfering packet information.
+#
+# @addr: Migration endpoint configuration on destination interface.
+#
+# Since 8.1
+##
+{ 'struct': 'MigrationChannel',
+  'data': {
+      'channel-type': 'MigrationChannelType',
+      'addr': 'MigrationAddress' } }
+
 ##
 # @migrate:
 #
@@ -1526,6 +1554,9 @@
 #
 # @uri: the Uniform Resource Identifier of the destination VM
 #
+# @channels: list of migration stream channels with each stream in the
+#     list connected to a destination interface endpoint.
+#
 # @blk: do block migration (full disk copy)
 #
 # @inc: incremental disk copy migration
@@ -1550,14 +1581,50 @@
 # 3. The user Monitor's "detach" argument is invalid in QMP and should
 #    not be used
 #
+# 4. The uri argument should have the Uniform Resource Identifier of
+#    default destination VM. This connection will be bound to default
+#    network.
+#
+# 5. For now, number of migration streams is restricted to one, i.e
+#    number of items in 'channels' list is just 1.
+#
+# 6. The 'uri' and 'channels' arguments are mutually exclusive;
+#    exactly one of the two should be present.
+#
 # Example:
 #
 # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
 # <- { "return": {} }
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "socket",
+#                                    "type": "inet",
+#                                    "host": "10.12.34.9",
+#                                    "port": "1050" } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "exec",
+#                                    "args": [ "/bin/nc", "-p", "6000",
+#                                              "/some/sock" ] } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "rdma",
+#                                    "host": "10.12.34.9",
+#                                    "port": "1050" } } ] } }
+# <- { "return": {} }
+#
 ##
 { 'command': 'migrate',
-  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
-           '*detach': 'bool', '*resume': 'bool' } }
+  'data': {'*uri': 'str', '*channels': [ 'MigrationChannel' ],
+           '*blk': 'bool', '*inc': 'bool', '*detach': 'bool',
+           '*resume': 'bool' } }
 
 ##
 # @migrate-incoming:
@@ -1568,6 +1635,9 @@
 # @uri: The Uniform Resource Identifier identifying the source or
 #     address to listen on
 #
+# @channels: list of migration stream channels with each stream in the
+#     list connected to a destination interface endpoint.
+#
 # Returns: nothing on success
 #
 # Since: 2.3
@@ -1583,13 +1653,46 @@
 #
 # 3. The uri format is the same as for -incoming
 #
+# 5. For now, number of migration streams is restricted to one, i.e
+#    number of items in 'channels' list is just 1.
+#
+# 4. The 'uri' and 'channels' arguments are mutually exclusive;
+#    exactly one of the two should be present.
+#
 # Example:
 #
 # -> { "execute": "migrate-incoming",
 #      "arguments": { "uri": "tcp::4446" } }
 # <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "socket",
+#                                    "type": "inet",
+#                                    "host": "10.12.34.9",
+#                                    "port": "1050" } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "exec",
+#                                    "args": [ "/bin/nc", "-p", "6000",
+#                                              "/some/sock" ] } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "rdma",
+#                                    "host": "10.12.34.9",
+#                                    "port": "1050" } } ] } }
+# <- { "return": {} }
 ##
-{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
+{ 'command': 'migrate-incoming',
+             'data': {'*uri': 'str',
+                      '*channels': [ 'MigrationChannel' ] } }
 
 ##
 # @xen-save-devices-state:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 98e071e63b..99ab0d3a51 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2690,7 +2690,7 @@ void qmp_x_exit_preconfig(Error **errp)
     if (incoming) {
         Error *local_err = NULL;
         if (strcmp(incoming, "defer") != 0) {
-            qmp_migrate_incoming(incoming, &local_err);
+            qmp_migrate_incoming(incoming, false, NULL, &local_err);
             if (local_err) {
                 error_reportf_err(local_err, "-incoming %s: ", incoming);
                 exit(1);
-- 
2.22.3

[PATCH v11 07/10] migration: modify migration_channels_and_uri_compatible() for new QAPI syntax

[Het Gala]

migration_channels_and_uri_compatible() check for transport mechanism
suitable for multifd migration gets executed when the caller calls old
uri syntax. It needs it to be run when using the modern MigrateChannel
QAPI syntax too.

After URI -> 'MigrateChannel' :
migration_channels_and_uri_compatible() ->
migration_channels_and_transport_compatible() passes object as argument
and check for valid transport mechanism.

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.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 825bf70e7a..6f948988ec 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -106,17 +106,20 @@ static bool migration_needs_multiple_sockets(void)
     return migrate_multifd() || migrate_postcopy_preempt();
 }
 
-static bool uri_supports_multi_channels(const char *uri)
+static bool transport_supports_multi_channels(SocketAddress *saddr)
 {
-    return strstart(uri, "tcp:", NULL) || strstart(uri, "unix:", NULL) ||
-           strstart(uri, "vsock:", NULL);
+    return saddr->type == SOCKET_ADDRESS_TYPE_INET ||
+           saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+           saddr->type == SOCKET_ADDRESS_TYPE_VSOCK;
 }
 
 static bool
-migration_channels_and_uri_compatible(const char *uri, Error **errp)
+migration_channels_and_transport_compatible(MigrationAddress *addr,
+                                            Error **errp)
 {
     if (migration_needs_multiple_sockets() &&
-        !uri_supports_multi_channels(uri)) {
+        (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) &&
+        !transport_supports_multi_channels(&addr->u.socket)) {
         error_setg(errp, "Migration requires multi-channel URIs (e.g. tcp)");
         return false;
     }
@@ -499,12 +502,12 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
         return;
     }
 
-    /* URI is not suitable for migration? */
-    if (!migration_channels_and_uri_compatible(uri, errp)) {
+    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
         return;
     }
 
-    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
+    /* transport mechanism not suitable for migration? */
+    if (!migration_channels_and_transport_compatible(channel, errp)) {
         return;
     }
 
@@ -1770,12 +1773,12 @@ void qmp_migrate(const char *uri, bool has_channels,
         return;
     }
 
-    /* URI is not suitable for migration? */
-    if (!migration_channels_and_uri_compatible(uri, errp)) {
+    if (!migrate_uri_parse(uri, &channel, errp)) {
         return;
     }
 
-    if (!migrate_uri_parse(uri, &channel, errp)) {
+    /* transport mechanism not suitable for migration? */
+    if (!migration_channels_and_transport_compatible(channel, errp)) {
         return;
     }
 
-- 
2.22.3

[PATCH v11 08/10] migration: Implement MigrateChannelList to qmp migration flow.

[Het Gala]

Integrate MigrateChannelList with all transport backends
(socket, exec and rdma) for both src and dest migration
endpoints for qmp migration.

For current series, limit the size of MigrateChannelList
to single element (single interface) as runtime check.

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.c | 95 +++++++++++++++++++++++--------------------
 1 file changed, 52 insertions(+), 43 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 6f948988ec..3eae32e616 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -432,9 +432,10 @@ void migrate_add_address(SocketAddress *address)
 }
 
 static bool migrate_uri_parse(const char *uri,
-                              MigrationAddress **channel,
+                              MigrationChannel **channel,
                               Error **errp)
 {
+    g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1);
     g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
     SocketAddress *saddr = &addr->u.socket;
     InetSocketAddress *isock = &addr->u.rdma;
@@ -471,7 +472,9 @@ static bool migrate_uri_parse(const char *uri,
         return false;
     }
 
-    *channel = addr;
+    val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN;
+    val->addr = addr;
+    *channel = val;
     return true;
 }
 
@@ -479,41 +482,44 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
                                           MigrationChannelList *channels,
                                           Error **errp)
 {
-    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
+    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
+    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
 
     /*
      * Having preliminary checks for uri and channel
      */
-    if (has_channels) {
-        error_setg(errp, "'channels' argument should not be set yet.");
-        return;
-    }
-
     if (uri && has_channels) {
         error_setg(errp, "'uri' and 'channels' arguments are mutually "
                    "exclusive; exactly one of the two should be present in "
                    "'migrate-incoming' qmp command ");
         return;
-    }
-
-    if (!uri && !has_channels) {
+    } else if (channels) {
+        /* To verify that Migrate channel list has only item */
+        if (channels->next) {
+            error_setg(errp, "Channel list has more than one entries");
+            return;
+        }
+        channel = channels->value;
+    } else if (uri) {
+        /* caller uses the old URI syntax */
+        if (!migrate_uri_parse(uri, &channel, errp)) {
+            return;
+        }
+    } else {
         error_setg(errp, "neither 'uri' or 'channels' argument are "
                    "specified in 'migrate-incoming' qmp command ");
         return;
     }
-
-    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
-        return;
-    }
+    addr = channel->addr;
 
     /* transport mechanism not suitable for migration? */
-    if (!migration_channels_and_transport_compatible(channel, errp)) {
+    if (!migration_channels_and_transport_compatible(addr, errp)) {
         return;
     }
 
     qapi_event_send_migration(MIGRATION_STATUS_SETUP);
-    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
-        SocketAddress *saddr = &channel->u.socket;
+    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
+        SocketAddress *saddr = &addr->u.socket;
         if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
             saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
             saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
@@ -522,11 +528,11 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
             fd_start_incoming_migration(saddr->u.fd.str, errp);
         }
 #ifdef CONFIG_RDMA
-    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
-        rdma_start_incoming_migration(&channel->u.rdma, errp);
-#endif
-    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
-        exec_start_incoming_migration(channel->u.exec.args, errp);
+    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
+        rdma_start_incoming_migration(&addr->u.rdma, errp);
+ #endif
+    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
+        exec_start_incoming_migration(addr->u.exec.args, errp);
     } else {
         error_setg(errp, "unknown migration protocol: %s", uri);
     }
@@ -1750,35 +1756,38 @@ void qmp_migrate(const char *uri, bool has_channels,
     bool resume_requested;
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
+    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
+    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
 
     /*
      * Having preliminary checks for uri and channel
      */
-    if (has_channels) {
-        error_setg(errp, "'channels' argument should not be set yet.");
-        return;
-    }
-
     if (uri && has_channels) {
         error_setg(errp, "'uri' and 'channels' arguments are mutually "
                    "exclusive; exactly one of the two should be present in "
                    "'migrate' qmp command ");
         return;
-    }
-
-    if (!uri && !has_channels) {
+    } else if (channels) {
+        /* To verify that Migrate channel list has only item */
+        if (channels->next) {
+            error_setg(errp, "Channel list has more than one entries");
+            return;
+        }
+        channel = channels->value;
+    } else if (uri) {
+        /* caller uses the old URI syntax */
+        if (!migrate_uri_parse(uri, &channel, errp)) {
+            return;
+        }
+    } else {
         error_setg(errp, "neither 'uri' or 'channels' argument are "
                    "specified in 'migrate' qmp command ");
         return;
     }
-
-    if (!migrate_uri_parse(uri, &channel, errp)) {
-        return;
-    }
+    addr = channel->addr;
 
     /* transport mechanism not suitable for migration? */
-    if (!migration_channels_and_transport_compatible(channel, errp)) {
+    if (!migration_channels_and_transport_compatible(addr, errp)) {
         return;
     }
 
@@ -1795,8 +1804,8 @@ void qmp_migrate(const char *uri, bool has_channels,
         }
     }
 
-    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
-        SocketAddress *saddr = &channel->u.socket;
+    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
+        SocketAddress *saddr = &addr->u.socket;
         if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
             saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
             saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
@@ -1805,11 +1814,11 @@ void qmp_migrate(const char *uri, bool has_channels,
             fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
         }
 #ifdef CONFIG_RDMA
-    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
-        rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
+    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
+        rdma_start_outgoing_migration(s, &addr->u.rdma, &local_err);
 #endif
-    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
-        exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
+    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
+        exec_start_outgoing_migration(s, addr->u.exec.args, &local_err);
     } else {
         error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
                    "a valid migration protocol");
-- 
2.22.3

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

[Het Gala]

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

[PATCH v11 10/10] migration: modify test_multifd_tcp_none() to use new QAPI syntax.

[Het Gala]

modify multifd tcp common test to incorporate the new QAPI
syntax defined.

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>
---
 tests/qtest/migration-test.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 46f1c275a2..246cab6451 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2205,7 +2205,12 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState *from,
 
     /* Start incoming migration from the 1st socket */
     qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
-                             "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+                             "  'arguments': { "
+                             "      'channels': [ { 'channeltype': 'main',"
+                             "      'addr': { 'transport': 'socket',"
+                             "                'type': 'inet',"
+                             "                'host': '127.0.0.1',"
+                             "                'port': '0' } } ] } }");
 
     return NULL;
 }
-- 
2.22.3

Re: [PATCH v11 02/10] migration: convert migration 'uri' into 'MigrateAddress'

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri'
> string containing migration connection related information
> and stores them inside well defined 'MigrateAddress' struct.
>
> 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>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH v11 02/10] migration: convert migration 'uri' into 'MigrateAddress'

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri'
> string containing migration connection related information
> and stores them inside well defined 'MigrateAddress' struct.
>
> 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>

Reviewed-by: Juan Quintela <quintela@redhat.com>

queued.

Re: [PATCH v11 00/10] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> This is v11 patchset of modified 'migrate' and 'migrate-incoming' QAPI design
> for upstream review.
>
> Update: Daniel has reviewed all patches and is okay with them. Markus has also
>         given Acked-by tag for patches related to QAPI syntax change.
> Fabiano, Juan and other migration maintainers, let me know if there are still
> improvements to be made in this patchset series.
>
> Link to previous upstream community patchset links:
> v1: https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg04339.html
> v2: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg02106.html
> v3: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg02473.html
> v4: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03064.html
> v5: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04845.html
> v6: https://lists.gnu.org/archive/html/qemu-devel/2023-06/msg01251.html
> v7: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg02027.html
> v8: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg02770.html
> v9: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg04216.html
> v10: https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg05022.html
>
> v10 -> v11 changelog:
> -------------------
> - Resolved make check errors as its been almost two months since v10
>   version of this patchset series went out. Till date migration workflow
>   might have changed which caused make check errors.

Sorry, there must be a misunderstanding here. This series still has
problems. Just look at patch 6 that adds the "channel-type" parameter and
patch 10 that uses "channeltype" in the test (without hyphen). This
cannot work.

There's also several instances of g_autoptr being used incorrectly. I
could comment on every patch individually, but this series cannot have
passed make check.

Please resend this with the issues fixed and drop the Reviewed-bys from
the affected patches.

Summary of Failures:

  1/418 qemu:qtest+qtest-x86_64 / qtest-x86_64/migration-test         ERROR           0.44s   killed by signal 6 SIGABRT
  7/418 qemu:qtest+qtest-i386 / qtest-i386/migration-test             ERROR           0.47s   killed by signal 6 SIGABRT
121/418 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-swtpm-test     ERROR           0.55s   killed by signal 6 SIGABRT
128/418 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-tis-swtpm-test     ERROR           0.72s   killed by signal 6 SIGABRT
131/418 qemu:qtest+qtest-i386 / qtest-i386/ahci-test                  ERROR          12.53s   killed by signal 6 SIGABRT
134/418 qemu:qtest+qtest-x86_64 / qtest-x86_64/ahci-test              ERROR          13.04s   killed by signal 6 SIGABRT
143/418 qemu:qtest+qtest-x86_64 / qtest-x86_64/virtio-net-failover    ERROR           2.95s   killed by signal 6 SIGABRT
147/418 qemu:qtest+qtest-i386 / qtest-i386/qos-test                   ERROR          16.12s   killed by signal 6 SIGABRT
148/418 qemu:qtest+qtest-x86_64 / qtest-x86_64/qos-test               ERROR          16.15s   killed by signal 6 SIGABRT
177/418 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-swtpm-test         ERROR           0.55s   killed by signal 6 SIGABRT
180/418 qemu:qtest+qtest-i386 / qtest-i386/tpm-tis-swtpm-test         ERROR           0.59s   killed by signal 6 SIGABRT
197/418 qemu:qtest+qtest-i386 / qtest-i386/virtio-net-failover        ERROR           2.38s   killed by signal 6 SIGABRT
305/418 qemu:block / io-qcow2-181                                     ERROR           0.52s   exit status 1
312/418 qemu:block / io-qcow2-060                                     ERROR           9.89s   exit status 1
316/418 qemu:block / io-qcow2-203                                     ERROR           0.84s   exit status 1

Re: [PATCH v11 00/10] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration

[Het Gala]

On 04/10/23 7:03 pm, Fabiano Rosas wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> This is v11 patchset of modified 'migrate' and 'migrate-incoming' QAPI design
>> for upstream review.
>>
>> Update: Daniel has reviewed all patches and is okay with them. Markus has also
>>          given Acked-by tag for patches related to QAPI syntax change.
>> Fabiano, Juan and other migration maintainers, let me know if there are still
>> improvements to be made in this patchset series.
>>
>> Link to previous upstream community patchset links:
>> v1: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2022-2D12_msg04339.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=jsRvKRy1JOiy05KX1CtLqWN1su5XNmKPKuJTSx5sZpU&e=
>> v2: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D02_msg02106.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=mzt3n5PD1QclHfpZEh-VMoLkkwT8xqjPYN-1r7MOly0&e=
>> v3: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D02_msg02473.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=fa9W71JU6-3xZrjLH7AmElgqwJGUkPeQv3P7n6EXxOM&e=
>> v4: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D05_msg03064.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=Xr1y3EvBzEtWT9O1fVNapCb3WnD-aWR8UeXv6J6gZQM&e=
>> v5: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D05_msg04845.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=OtK10W2Z0DobrktRfTCMYPxbcMaaZ6f6qoA65D4RG_A&e=
>> v6: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D06_msg01251.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=XH-4qFQgdkAKmRsa9DuqaZgJMvGUi1p4-s05AsAEYRo&e=
>> v7: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg02027.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=RwvfliI4wLm7S0TKl5RMku-gSSE-5fZPYH0MkzJdoPw&e=
>> v8: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg02770.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=BZsKBJGVPDWXwGgb2-fAnS9pWzTYuLzI92TmuWBcB3k&e=
>> v9: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg04216.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=YcWFU9I2u-R6QbVjweZ3lFvJlllm-i9o5_jtLBxC_oc&e=
>> v10: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg05022.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=JQt63Ikbz21vmsLmSensQu8zknGuS9bls-IFpndor78&e=
>>
>> v10 -> v11 changelog:
>> -------------------
>> - Resolved make check errors as its been almost two months since v10
>>    version of this patchset series went out. Till date migration workflow
>>    might have changed which caused make check errors.
> Sorry, there must be a misunderstanding here. This series still has
> problems. Just look at patch 6 that adds the "channel-type" parameter and
> patch 10 that uses "channeltype" in the test (without hyphen). This
> cannot work.
Ack. I will change that.
> There's also several instances of g_autoptr being used incorrectly. I
> could comment on every patch individually, but this series cannot have
> passed make check.
Are we allowed to run the make checks ? I am not aware from where these 
failures are arising. It would be helpful if you could point out to me 
where g_autoptr is incorrectly used ?
> Please resend this with the issues fixed and drop the Reviewed-bys from
> the affected patches.
How to verify which are the affected patches here ?
> Summary of Failures:
>
>    1/418 qemu:qtest+qtest-x86_64 / qtest-x86_64/migration-test         ERROR           0.44s   killed by signal 6 SIGABRT
>    7/418 qemu:qtest+qtest-i386 / qtest-i386/migration-test             ERROR           0.47s   killed by signal 6 SIGABRT
> 121/418 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-swtpm-test     ERROR           0.55s   killed by signal 6 SIGABRT
> 128/418 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-tis-swtpm-test     ERROR           0.72s   killed by signal 6 SIGABRT
> 131/418 qemu:qtest+qtest-i386 / qtest-i386/ahci-test                  ERROR          12.53s   killed by signal 6 SIGABRT
> 134/418 qemu:qtest+qtest-x86_64 / qtest-x86_64/ahci-test              ERROR          13.04s   killed by signal 6 SIGABRT
> 143/418 qemu:qtest+qtest-x86_64 / qtest-x86_64/virtio-net-failover    ERROR           2.95s   killed by signal 6 SIGABRT
> 147/418 qemu:qtest+qtest-i386 / qtest-i386/qos-test                   ERROR          16.12s   killed by signal 6 SIGABRT
> 148/418 qemu:qtest+qtest-x86_64 / qtest-x86_64/qos-test               ERROR          16.15s   killed by signal 6 SIGABRT
> 177/418 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-swtpm-test         ERROR           0.55s   killed by signal 6 SIGABRT
> 180/418 qemu:qtest+qtest-i386 / qtest-i386/tpm-tis-swtpm-test         ERROR           0.59s   killed by signal 6 SIGABRT
> 197/418 qemu:qtest+qtest-i386 / qtest-i386/virtio-net-failover        ERROR           2.38s   killed by signal 6 SIGABRT
> 305/418 qemu:block / io-qcow2-181                                     ERROR           0.52s   exit status 1
> 312/418 qemu:block / io-qcow2-060                                     ERROR           9.89s   exit status 1
> 316/418 qemu:block / io-qcow2-203                                     ERROR           0.84s   exit status 1
>
Regards,
Het Gala

Re: [PATCH v11 00/10] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> On 04/10/23 7:03 pm, Fabiano Rosas wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>>
>>> This is v11 patchset of modified 'migrate' and 'migrate-incoming' QAPI design
>>> for upstream review.
>>>
>>> Update: Daniel has reviewed all patches and is okay with them. Markus has also
>>>          given Acked-by tag for patches related to QAPI syntax change.
>>> Fabiano, Juan and other migration maintainers, let me know if there are still
>>> improvements to be made in this patchset series.
>>>
>>> Link to previous upstream community patchset links:
>>> v1: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2022-2D12_msg04339.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=jsRvKRy1JOiy05KX1CtLqWN1su5XNmKPKuJTSx5sZpU&e=
>>> v2: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D02_msg02106.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=mzt3n5PD1QclHfpZEh-VMoLkkwT8xqjPYN-1r7MOly0&e=
>>> v3: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D02_msg02473.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=fa9W71JU6-3xZrjLH7AmElgqwJGUkPeQv3P7n6EXxOM&e=
>>> v4: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D05_msg03064.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=Xr1y3EvBzEtWT9O1fVNapCb3WnD-aWR8UeXv6J6gZQM&e=
>>> v5: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D05_msg04845.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=OtK10W2Z0DobrktRfTCMYPxbcMaaZ6f6qoA65D4RG_A&e=
>>> v6: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D06_msg01251.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=XH-4qFQgdkAKmRsa9DuqaZgJMvGUi1p4-s05AsAEYRo&e=
>>> v7: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg02027.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=RwvfliI4wLm7S0TKl5RMku-gSSE-5fZPYH0MkzJdoPw&e=
>>> v8: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg02770.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=BZsKBJGVPDWXwGgb2-fAnS9pWzTYuLzI92TmuWBcB3k&e=
>>> v9: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg04216.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=YcWFU9I2u-R6QbVjweZ3lFvJlllm-i9o5_jtLBxC_oc&e=
>>> v10: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg05022.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=JQt63Ikbz21vmsLmSensQu8zknGuS9bls-IFpndor78&e=
>>>
>>> v10 -> v11 changelog:
>>> -------------------
>>> - Resolved make check errors as its been almost two months since v10
>>>    version of this patchset series went out. Till date migration workflow
>>>    might have changed which caused make check errors.
>> Sorry, there must be a misunderstanding here. This series still has
>> problems. Just look at patch 6 that adds the "channel-type" parameter and
>> patch 10 that uses "channeltype" in the test (without hyphen). This
>> cannot work.
> Ack. I will change that.
>> There's also several instances of g_autoptr being used incorrectly. I
>> could comment on every patch individually, but this series cannot have
>> passed make check.
> Are we allowed to run the make checks ? I am not aware from where these 
> failures are arising. It would be helpful if you could point out to me 
> where g_autoptr is incorrectly used ?

I mean just the project's make check command:

cd build/
../configure
make -j$(nproc)
make -j$(nproc) check

>> Please resend this with the issues fixed and drop the Reviewed-bys from
>> the affected patches.
> How to verify which are the affected patches here ?

I'll comment in each patch individually.

We'll also have to add compatibility with the new file: URI that's
included in the latest migration pull request. I'll add comments on
where I think we'll need to add code to support that feature.

Re: [PATCH v11 02/10] migration: convert migration 'uri' into 'MigrateAddress'

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri'
> string containing migration connection related information
> and stores them inside well defined 'MigrateAddress' struct.
>
> 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/exec.c      |  1 -
>  migration/exec.h      |  4 ++++
>  migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index 2bf882bbe1..32f5143dfd 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -27,7 +27,6 @@
>  #include "qemu/cutils.h"
>  
>  #ifdef WIN32
> -const char *exec_get_cmd_path(void);
>  const char *exec_get_cmd_path(void)
>  {
>      g_autofree char *detected_path = g_new(char, MAX_PATH);
> diff --git a/migration/exec.h b/migration/exec.h
> index b210ffde7a..736cd71028 100644
> --- a/migration/exec.h
> +++ b/migration/exec.h
> @@ -19,6 +19,10 @@
>  
>  #ifndef QEMU_MIGRATION_EXEC_H
>  #define QEMU_MIGRATION_EXEC_H
> +
> +#ifdef WIN32
> +const char *exec_get_cmd_path(void);
> +#endif
>  void exec_start_incoming_migration(const char *host_port, Error **errp);
>  
>  void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
> diff --git a/migration/migration.c b/migration/migration.c
> index 6d3cf5d5cd..dcbd509d56 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -65,6 +65,7 @@
>  #include "sysemu/qtest.h"
>  #include "options.h"
>  #include "sysemu/dirtylimit.h"
> +#include "qemu/sockets.h"
>  
>  static NotifierList migration_state_notifiers =
>      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address)
>                        QAPI_CLONE(SocketAddress, address));
>  }
>  
> +static bool migrate_uri_parse(const char *uri,
> +                              MigrationAddress **channel,
> +                              Error **errp)
> +{
> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);

This cannot be g_autoptr because you're passing it out of scope at the
end of the function.

> +    SocketAddress *saddr = &addr->u.socket;

This attribution is useless. Down below you overwrite it with the result
of socket_parse.

> +    InetSocketAddress *isock = &addr->u.rdma;
> +    strList **tail = &addr->u.exec.args;
> +
> +    if (strstart(uri, "exec:", NULL)) {
> +        addr->transport = MIGRATION_ADDRESS_TYPE_EXEC;
> +#ifdef WIN32
> +        QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
> +        QAPI_LIST_APPEND(tail, g_strdup("/c"));
> +#else
> +        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> +        QAPI_LIST_APPEND(tail, g_strdup("-c"));
> +#endif
> +        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> +    } else if (strstart(uri, "rdma:", NULL)) {
> +        if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
> +            qapi_free_InetSocketAddress(isock);
> +            return false;
> +        }
> +        addr->transport = MIGRATION_ADDRESS_TYPE_RDMA;
> +    } else if (strstart(uri, "tcp:", NULL) ||
> +                strstart(uri, "unix:", NULL) ||
> +                strstart(uri, "vsock:", NULL) ||
> +                strstart(uri, "fd:", NULL)) {
> +        addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET;
> +        saddr = socket_parse(uri, errp);
> +        if (!saddr) {
> +            qapi_free_SocketAddress(saddr);

Shouldn't free here. socket_parse() already does so on failure.

> +            return false;
> +        }

Then here you can set the values you intended to set.

addr->u.socket.type = saddr->type;
addr->u.socket.u = saddr->u;

> +    } else {
> +        error_setg(errp, "unknown migration protocol: %s", uri);
> +        return false;
> +    }
> +
> +    *channel = addr;
> +    return true;
> +}
> +
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p = NULL;
> +    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);

The memory is leaked here because the pointer is overwritten below. Just
set it to NULL. You can keep the g_autoptr.

>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
>          return;
>      }
>  
> +    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> +        return;
> +    }
> +
>      qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>      if (strstart(uri, "tcp:", &p) ||
>          strstart(uri, "unix:", NULL) ||
> @@ -1671,12 +1721,17 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
>      const char *p = NULL;
> +    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);

Same here.

>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
>          return;
>      }
>  
> +    if (!migrate_uri_parse(uri, &channel, errp)) {
> +        return;
> +    }
> +
>      resume_requested = has_resume && resume;
>      if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
>                           resume_requested, errp)) {

Re: [PATCH v11 05/10] migration: convert exec backend to accept MigrateAddress.

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> Exec transport backend for 'migrate'/'migrate-incoming' QAPIs accept
> new wire protocol of MigrateAddress struct.
>
> It is achived by parsing 'uri' string and storing migration parameters
> required for exec connection into strList struct.
>
> 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/exec.c      | 71 +++++++++++++++++++++++++++++++------------
>  migration/exec.h      |  4 +--
>  migration/migration.c | 10 +++---
>  3 files changed, 57 insertions(+), 28 deletions(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index 32f5143dfd..8bc321c66b 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -39,20 +39,50 @@ const char *exec_get_cmd_path(void)
>  }
>  #endif
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
> +/* provides the length of strList */
> +static int
> +str_list_length(strList *list)
> +{
> +    int len = 0;
> +    strList *elem;
> +
> +    for (elem = list; elem != NULL; elem = elem->next) {
> +        len++;
> +    }
> +
> +    return len;
> +}
> +
> +static void
> +init_exec_array(strList *command, char **argv, Error **errp)
> +{
> +    int i = 0;
> +    strList *lst;
> +
> +    for (lst = command; lst; lst = lst->next) {
> +        argv[i++] = lst->value;
> +    }
> +
> +    argv[i] = NULL;

This will write out of bounds. 

> +    return;
> +}
> +
> +void exec_start_outgoing_migration(MigrationState *s, strList *command,
> +                                   Error **errp)
>  {
>      QIOChannel *ioc;
>  
> -#ifdef WIN32
> -    const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
> -#else
> -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> -#endif
> +    int length = str_list_length(command);
> +    g_auto(GStrv) argv = (char **) g_new0(const char *, length);

This allocation does not leave space for the NULL byte.

>  
> -    trace_migration_exec_outgoing(command);
> -    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
> -                                                    O_RDWR,
> -                                                    errp));
> +    init_exec_array(command, argv, errp);
> +    g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
> +
> +    trace_migration_exec_outgoing(new_command);
> +    ioc = QIO_CHANNEL(
> +        qio_channel_command_new_spawn((const char * const *) argv,
> +                                      O_RDWR,
> +                                      errp));
>      if (!ioc) {
>          return;
>      }
> @@ -71,20 +101,21 @@ static gboolean exec_accept_incoming_migration(QIOChannel *ioc,
>      return G_SOURCE_REMOVE;
>  }
>  
> -void exec_start_incoming_migration(const char *command, Error **errp)
> +void exec_start_incoming_migration(strList *command, Error **errp)
>  {
>      QIOChannel *ioc;
>  
> -#ifdef WIN32
> -    const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
> -#else
> -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> -#endif
> +    int length = str_list_length(command);
> +    g_auto(GStrv) argv = (char **) g_new0(const char *, length);

Here as well.

> +
> +    init_exec_array(command, argv, errp);
> +    g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
>  
> -    trace_migration_exec_incoming(command);
> -    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
> -                                                    O_RDWR,
> -                                                    errp));
> +    trace_migration_exec_incoming(new_command);
> +    ioc = QIO_CHANNEL(
> +        qio_channel_command_new_spawn((const char * const *) argv,
> +                                      O_RDWR,
> +                                      errp));
>      if (!ioc) {
>          return;
>      }
> diff --git a/migration/exec.h b/migration/exec.h
> index 736cd71028..3107f205e3 100644
> --- a/migration/exec.h
> +++ b/migration/exec.h
> @@ -23,8 +23,8 @@
>  #ifdef WIN32
>  const char *exec_get_cmd_path(void);
>  #endif
> -void exec_start_incoming_migration(const char *host_port, Error **errp);
> +void exec_start_incoming_migration(strList *host_port, Error **errp);
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
> +void exec_start_outgoing_migration(MigrationState *s, strList *host_port,
>                                     Error **errp);
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index b41fda6f80..ebe14b9c38 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -474,7 +474,6 @@ static bool migrate_uri_parse(const char *uri,
>  
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
> -    const char *p = NULL;
>      g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>  
>      /* URI is not suitable for migration? */
> @@ -500,8 +499,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>      } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>          rdma_start_incoming_migration(&channel->u.rdma, errp);
>  #endif
> -    } else if (strstart(uri, "exec:", &p)) {
> -        exec_start_incoming_migration(p, errp);
> +    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> +        exec_start_incoming_migration(channel->u.exec.args, errp);
>      } else {
>          error_setg(errp, "unknown migration protocol: %s", uri);
>      }
> @@ -1723,7 +1722,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      bool resume_requested;
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
> -    const char *p = NULL;
>      g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>  
>      /* URI is not suitable for migration? */
> @@ -1761,8 +1759,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>          rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
>  #endif
> -    } else if (strstart(uri, "exec:", &p)) {
> -        exec_start_outgoing_migration(s, p, &local_err);
> +    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> +        exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
>      } else {
>          error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
>                     "a valid migration protocol");

Re: [PATCH v11 08/10] migration: Implement MigrateChannelList to qmp migration flow.

[Fabiano Rosas]

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 qmp migration.
>
> For current series, limit the size of MigrateChannelList
> to single element (single interface) as runtime check.
>
> 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.c | 95 +++++++++++++++++++++++--------------------
>  1 file changed, 52 insertions(+), 43 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 6f948988ec..3eae32e616 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -432,9 +432,10 @@ void migrate_add_address(SocketAddress *address)
>  }
>  
>  static bool migrate_uri_parse(const char *uri,
> -                              MigrationAddress **channel,
> +                              MigrationChannel **channel,
>                                Error **errp)
>  {
> +    g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1);

Here val is passed out of scope so it shouldn't be g_autoptr.

>      g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>      SocketAddress *saddr = &addr->u.socket;
>      InetSocketAddress *isock = &addr->u.rdma;
> @@ -471,7 +472,9 @@ static bool migrate_uri_parse(const char *uri,
>          return false;
>      }
>  
> -    *channel = addr;
> +    val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN;
> +    val->addr = addr;
> +    *channel = val;
>      return true;
>  }
>  
> @@ -479,41 +482,44 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>                                            MigrationChannelList *channels,
>                                            Error **errp)
>  {
> -    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
> +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);

Here we want just the pointer, no allocation, no freeing. For both
channel and addr.

>  
>      /*
>       * Having preliminary checks for uri and channel
>       */
> -    if (has_channels) {
> -        error_setg(errp, "'channels' argument should not be set yet.");
> -        return;
> -    }
> -
>      if (uri && has_channels) {
>          error_setg(errp, "'uri' and 'channels' arguments are mutually "
>                     "exclusive; exactly one of the two should be present in "
>                     "'migrate-incoming' qmp command ");
>          return;
> -    }
> -
> -    if (!uri && !has_channels) {
> +    } else if (channels) {
> +        /* To verify that Migrate channel list has only item */
> +        if (channels->next) {
> +            error_setg(errp, "Channel list has more than one entries");
> +            return;
> +        }
> +        channel = channels->value;
> +    } else if (uri) {
> +        /* caller uses the old URI syntax */
> +        if (!migrate_uri_parse(uri, &channel, errp)) {
> +            return;
> +        }
> +    } else {
>          error_setg(errp, "neither 'uri' or 'channels' argument are "
>                     "specified in 'migrate-incoming' qmp command ");
>          return;
>      }
> -
> -    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> -        return;
> -    }
> +    addr = channel->addr;
>  
>      /* transport mechanism not suitable for migration? */
> -    if (!migration_channels_and_transport_compatible(channel, errp)) {
> +    if (!migration_channels_and_transport_compatible(addr, errp)) {
>          return;
>      }
>  
>      qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> -    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> -        SocketAddress *saddr = &channel->u.socket;
> +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> +        SocketAddress *saddr = &addr->u.socket;
>          if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>              saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>              saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> @@ -522,11 +528,11 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>              fd_start_incoming_migration(saddr->u.fd.str, errp);
>          }
>  #ifdef CONFIG_RDMA
> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
> -        rdma_start_incoming_migration(&channel->u.rdma, errp);
> -#endif
> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> -        exec_start_incoming_migration(channel->u.exec.args, errp);
> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
> +        rdma_start_incoming_migration(&addr->u.rdma, errp);
> + #endif
> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> +        exec_start_incoming_migration(addr->u.exec.args, errp);
>      } else {
>          error_setg(errp, "unknown migration protocol: %s", uri);
>      }
> @@ -1750,35 +1756,38 @@ void qmp_migrate(const char *uri, bool has_channels,
>      bool resume_requested;
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
> -    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
> +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);

Again just the pointers.

We'll have to make another pass and check whether we're missing freeing
something. But for now let's first clear all the errors then we can look
at the code working and do the necessary changes.

>  
>      /*
>       * Having preliminary checks for uri and channel
>       */
> -    if (has_channels) {
> -        error_setg(errp, "'channels' argument should not be set yet.");
> -        return;
> -    }
> -
>      if (uri && has_channels) {
>          error_setg(errp, "'uri' and 'channels' arguments are mutually "
>                     "exclusive; exactly one of the two should be present in "
>                     "'migrate' qmp command ");
>          return;
> -    }
> -
> -    if (!uri && !has_channels) {
> +    } else if (channels) {
> +        /* To verify that Migrate channel list has only item */
> +        if (channels->next) {
> +            error_setg(errp, "Channel list has more than one entries");
> +            return;
> +        }
> +        channel = channels->value;
> +    } else if (uri) {
> +        /* caller uses the old URI syntax */
> +        if (!migrate_uri_parse(uri, &channel, errp)) {
> +            return;
> +        }
> +    } else {
>          error_setg(errp, "neither 'uri' or 'channels' argument are "
>                     "specified in 'migrate' qmp command ");
>          return;
>      }
> -
> -    if (!migrate_uri_parse(uri, &channel, errp)) {
> -        return;
> -    }
> +    addr = channel->addr;
>  
>      /* transport mechanism not suitable for migration? */
> -    if (!migration_channels_and_transport_compatible(channel, errp)) {
> +    if (!migration_channels_and_transport_compatible(addr, errp)) {
>          return;
>      }
>  
> @@ -1795,8 +1804,8 @@ void qmp_migrate(const char *uri, bool has_channels,
>          }
>      }
>  
> -    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> -        SocketAddress *saddr = &channel->u.socket;
> +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> +        SocketAddress *saddr = &addr->u.socket;
>          if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>              saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>              saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> @@ -1805,11 +1814,11 @@ void qmp_migrate(const char *uri, bool has_channels,
>              fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
>          }
>  #ifdef CONFIG_RDMA
> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
> -        rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
> +        rdma_start_outgoing_migration(s, &addr->u.rdma, &local_err);
>  #endif
> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> -        exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> +        exec_start_outgoing_migration(s, addr->u.exec.args, &local_err);
>      } else {
>          error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
>                     "a valid migration protocol");

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

[Fabiano Rosas]

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 10/10] migration: modify test_multifd_tcp_none() to use new QAPI syntax.

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> modify multifd tcp common test to incorporate the new QAPI
> syntax defined.
>
> 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>
> ---
>  tests/qtest/migration-test.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 46f1c275a2..246cab6451 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -2205,7 +2205,12 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState *from,
>  
>      /* Start incoming migration from the 1st socket */
>      qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
> -                             "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
> +                             "  'arguments': { "
> +                             "      'channels': [ { 'channeltype': 'main',"

channel-type

> +                             "      'addr': { 'transport': 'socket',"
> +                             "                'type': 'inet',"
> +                             "                'host': '127.0.0.1',"
> +                             "                'port': '0' } } ] } }");
>  
>      return NULL;
>  }

Re: [PATCH v11 00/10] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Het Gala <het.gala@nutanix.com> writes:
>
>> On 04/10/23 7:03 pm, Fabiano Rosas wrote:
>>> Het Gala <het.gala@nutanix.com> writes:
>>>
>>>> This is v11 patchset of modified 'migrate' and 'migrate-incoming' QAPI design
>>>> for upstream review.
>>>>
>>>> Update: Daniel has reviewed all patches and is okay with them. Markus has also
>>>>          given Acked-by tag for patches related to QAPI syntax change.
>>>> Fabiano, Juan and other migration maintainers, let me know if there are still
>>>> improvements to be made in this patchset series.
>>>>
>>>> Link to previous upstream community patchset links:
>>>> v1: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2022-2D12_msg04339.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=jsRvKRy1JOiy05KX1CtLqWN1su5XNmKPKuJTSx5sZpU&e=
>>>> v2: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D02_msg02106.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=mzt3n5PD1QclHfpZEh-VMoLkkwT8xqjPYN-1r7MOly0&e=
>>>> v3: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D02_msg02473.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=fa9W71JU6-3xZrjLH7AmElgqwJGUkPeQv3P7n6EXxOM&e=
>>>> v4: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D05_msg03064.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=Xr1y3EvBzEtWT9O1fVNapCb3WnD-aWR8UeXv6J6gZQM&e=
>>>> v5: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D05_msg04845.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=OtK10W2Z0DobrktRfTCMYPxbcMaaZ6f6qoA65D4RG_A&e=
>>>> v6: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D06_msg01251.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=XH-4qFQgdkAKmRsa9DuqaZgJMvGUi1p4-s05AsAEYRo&e=
>>>> v7: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg02027.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=RwvfliI4wLm7S0TKl5RMku-gSSE-5fZPYH0MkzJdoPw&e=
>>>> v8: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg02770.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=BZsKBJGVPDWXwGgb2-fAnS9pWzTYuLzI92TmuWBcB3k&e=
>>>> v9: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg04216.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=YcWFU9I2u-R6QbVjweZ3lFvJlllm-i9o5_jtLBxC_oc&e=
>>>> v10: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg05022.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=JQt63Ikbz21vmsLmSensQu8zknGuS9bls-IFpndor78&e=
>>>>
>>>> v10 -> v11 changelog:
>>>> -------------------
>>>> - Resolved make check errors as its been almost two months since v10
>>>>    version of this patchset series went out. Till date migration workflow
>>>>    might have changed which caused make check errors.
>>> Sorry, there must be a misunderstanding here. This series still has
>>> problems. Just look at patch 6 that adds the "channel-type" parameter and
>>> patch 10 that uses "channeltype" in the test (without hyphen). This
>>> cannot work.
>> Ack. I will change that.
>>> There's also several instances of g_autoptr being used incorrectly. I
>>> could comment on every patch individually, but this series cannot have
>>> passed make check.
>> Are we allowed to run the make checks ? I am not aware from where these 
>> failures are arising. It would be helpful if you could point out to me 
>> where g_autoptr is incorrectly used ?
>
> I mean just the project's make check command:
>
> cd build/
> ../configure
> make -j$(nproc)
> make -j$(nproc) check
>
>>> Please resend this with the issues fixed and drop the Reviewed-bys from
>>> the affected patches.
>> How to verify which are the affected patches here ?
>
> I'll comment in each patch individually.

Done.

We had some double-frees when using g_autoptr in structures that are
nested into another. The qapi code already descends and frees the
children.

There were also issues with allocating memory and later overwriting the
pointers.

This might still not put us in the most correct situation regarding
memory, but I think it will at least get make check passing. Feel free
to investigate the errors with make check and propose alternative
solutions. It has been a while since I looked at this series, I might
have missed something further.

> We'll also have to add compatibility with the new file: URI that's
> included in the latest migration pull request. I'll add comments on
> where I think we'll need to add code to support that feature.

I'll actually defer here until you post your series with the
fixes. It'll probably be easier if I just send individual additions to
your patches.

Re: [PATCH v11 02/10] migration: convert migration 'uri' into 'MigrateAddress'

[Daniel P. Berrangé]

On Wed, Oct 04, 2023 at 11:43:12AM -0300, Fabiano Rosas wrote:
> Het Gala <het.gala@nutanix.com> writes:
> 
> > This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri'
> > string containing migration connection related information
> > and stores them inside well defined 'MigrateAddress' struct.
> >
> > 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/exec.c      |  1 -
> >  migration/exec.h      |  4 ++++
> >  migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 59 insertions(+), 1 deletion(-)
> >
> > diff --git a/migration/exec.c b/migration/exec.c
> > index 2bf882bbe1..32f5143dfd 100644
> > --- a/migration/exec.c
> > +++ b/migration/exec.c
> > @@ -27,7 +27,6 @@
> >  #include "qemu/cutils.h"
> >  
> >  #ifdef WIN32
> > -const char *exec_get_cmd_path(void);
> >  const char *exec_get_cmd_path(void)
> >  {
> >      g_autofree char *detected_path = g_new(char, MAX_PATH);
> > diff --git a/migration/exec.h b/migration/exec.h
> > index b210ffde7a..736cd71028 100644
> > --- a/migration/exec.h
> > +++ b/migration/exec.h
> > @@ -19,6 +19,10 @@
> >  
> >  #ifndef QEMU_MIGRATION_EXEC_H
> >  #define QEMU_MIGRATION_EXEC_H
> > +
> > +#ifdef WIN32
> > +const char *exec_get_cmd_path(void);
> > +#endif
> >  void exec_start_incoming_migration(const char *host_port, Error **errp);
> >  
> >  void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 6d3cf5d5cd..dcbd509d56 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -65,6 +65,7 @@
> >  #include "sysemu/qtest.h"
> >  #include "options.h"
> >  #include "sysemu/dirtylimit.h"
> > +#include "qemu/sockets.h"
> >  
> >  static NotifierList migration_state_notifiers =
> >      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> > @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address)
> >                        QAPI_CLONE(SocketAddress, address));
> >  }
> >  
> > +static bool migrate_uri_parse(const char *uri,
> > +                              MigrationAddress **channel,
> > +                              Error **errp)
> > +{
> > +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
> 
> This cannot be g_autoptr because you're passing it out of scope at the
> end of the function.

It is still good to use g_autoptr to deal with the error paths.

On the success path though you need   g_steal_pointer(&addr) to
prevent the autofree cleanup running.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v11 02/10] migration: convert migration 'uri' into 'MigrateAddress'

[Fabiano Rosas]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Oct 04, 2023 at 11:43:12AM -0300, Fabiano Rosas wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>> 
>> > This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri'
>> > string containing migration connection related information
>> > and stores them inside well defined 'MigrateAddress' struct.
>> >
>> > 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/exec.c      |  1 -
>> >  migration/exec.h      |  4 ++++
>> >  migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 59 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/migration/exec.c b/migration/exec.c
>> > index 2bf882bbe1..32f5143dfd 100644
>> > --- a/migration/exec.c
>> > +++ b/migration/exec.c
>> > @@ -27,7 +27,6 @@
>> >  #include "qemu/cutils.h"
>> >  
>> >  #ifdef WIN32
>> > -const char *exec_get_cmd_path(void);
>> >  const char *exec_get_cmd_path(void)
>> >  {
>> >      g_autofree char *detected_path = g_new(char, MAX_PATH);
>> > diff --git a/migration/exec.h b/migration/exec.h
>> > index b210ffde7a..736cd71028 100644
>> > --- a/migration/exec.h
>> > +++ b/migration/exec.h
>> > @@ -19,6 +19,10 @@
>> >  
>> >  #ifndef QEMU_MIGRATION_EXEC_H
>> >  #define QEMU_MIGRATION_EXEC_H
>> > +
>> > +#ifdef WIN32
>> > +const char *exec_get_cmd_path(void);
>> > +#endif
>> >  void exec_start_incoming_migration(const char *host_port, Error **errp);
>> >  
>> >  void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index 6d3cf5d5cd..dcbd509d56 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -65,6 +65,7 @@
>> >  #include "sysemu/qtest.h"
>> >  #include "options.h"
>> >  #include "sysemu/dirtylimit.h"
>> > +#include "qemu/sockets.h"
>> >  
>> >  static NotifierList migration_state_notifiers =
>> >      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
>> > @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address)
>> >                        QAPI_CLONE(SocketAddress, address));
>> >  }
>> >  
>> > +static bool migrate_uri_parse(const char *uri,
>> > +                              MigrationAddress **channel,
>> > +                              Error **errp)
>> > +{
>> > +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>> 
>> This cannot be g_autoptr because you're passing it out of scope at the
>> end of the function.
>
> It is still good to use g_autoptr to deal with the error paths.
>
> On the success path though you need   g_steal_pointer(&addr) to
> prevent the autofree cleanup running.

Ah good point, this has been suggested in an earlier version already, I
forgot to mention. We should definitely use g_steal_pointer() whenever
the variable goes out of scope.

Re: [PATCH v11 00/10] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 5534 bytes --]


On 10/4/2023 7:33 PM, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> On 04/10/23 7:03 pm, Fabiano Rosas wrote:
>>> Het Gala<het.gala@nutanix.com>  writes:
>>>
>>>> This is v11 patchset of modified 'migrate' and 'migrate-incoming' QAPI design
>>>> for upstream review.
>>>>
>>>> Update: Daniel has reviewed all patches and is okay with them. Markus has also
>>>>           given Acked-by tag for patches related to QAPI syntax change.
>>>> Fabiano, Juan and other migration maintainers, let me know if there are still
>>>> improvements to be made in this patchset series.
>>>>
>>>> Link to previous upstream community patchset links:
>>>> v1:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2022-2D12_msg04339.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=jsRvKRy1JOiy05KX1CtLqWN1su5XNmKPKuJTSx5sZpU&e=
>>>> v2:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D02_msg02106.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=mzt3n5PD1QclHfpZEh-VMoLkkwT8xqjPYN-1r7MOly0&e=
>>>> v3:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D02_msg02473.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=fa9W71JU6-3xZrjLH7AmElgqwJGUkPeQv3P7n6EXxOM&e=
>>>> v4:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D05_msg03064.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=Xr1y3EvBzEtWT9O1fVNapCb3WnD-aWR8UeXv6J6gZQM&e=
>>>> v5:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D05_msg04845.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=OtK10W2Z0DobrktRfTCMYPxbcMaaZ6f6qoA65D4RG_A&e=
>>>> v6:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D06_msg01251.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=XH-4qFQgdkAKmRsa9DuqaZgJMvGUi1p4-s05AsAEYRo&e=
>>>> v7:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg02027.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=RwvfliI4wLm7S0TKl5RMku-gSSE-5fZPYH0MkzJdoPw&e=
>>>> v8:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg02770.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=BZsKBJGVPDWXwGgb2-fAnS9pWzTYuLzI92TmuWBcB3k&e=
>>>> v9:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg04216.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=YcWFU9I2u-R6QbVjweZ3lFvJlllm-i9o5_jtLBxC_oc&e=
>>>> v10:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg05022.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=JQt63Ikbz21vmsLmSensQu8zknGuS9bls-IFpndor78&e=
>>>>
>>>> v10 -> v11 changelog:
>>>> -------------------
>>>> - Resolved make check errors as its been almost two months since v10
>>>>     version of this patchset series went out. Till date migration workflow
>>>>     might have changed which caused make check errors.
>>> Sorry, there must be a misunderstanding here. This series still has
>>> problems. Just look at patch 6 that adds the "channel-type" parameter and
>>> patch 10 that uses "channeltype" in the test (without hyphen). This
>>> cannot work.
>> Ack. I will change that.
>>> There's also several instances of g_autoptr being used incorrectly. I
>>> could comment on every patch individually, but this series cannot have
>>> passed make check.
>> Are we allowed to run the make checks ? I am not aware from where these
>> failures are arising. It would be helpful if you could point out to me
>> where g_autoptr is incorrectly used ?
> I mean just the project's make check command:
>
> cd build/
> ../configure
> make -j$(nproc)
> make -j$(nproc) check
Okay, now I got it. I was not aware of make -j check to pass the qemu 
tests. Will make sure that it passes the make check.
>>> Please resend this with the issues fixed and drop the Reviewed-bys from
>>> the affected patches.
>> How to verify which are the affected patches here ?
> I'll comment in each patch individually.
Thankyou for commenting on the patches. Will try to make all the changes 
in the coming version.
> We'll also have to add compatibility with the new file: URI that's
> included in the latest migration pull request. I'll add comments on
> where I think we'll need to add code to support that feature.

Ack.

Regards,
Het Gala.

[-- Attachment #2: Type: text/html, Size: 11252 bytes --]

Re: [PATCH v11 02/10] migration: convert migration 'uri' into 'MigrateAddress'

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 5854 bytes --]


On 10/4/2023 8:13 PM, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri'
>> string containing migration connection related information
>> and stores them inside well defined 'MigrateAddress' struct.
>>
>> 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/exec.c      |  1 -
>>   migration/exec.h      |  4 ++++
>>   migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/exec.c b/migration/exec.c
>> index 2bf882bbe1..32f5143dfd 100644
>> --- a/migration/exec.c
>> +++ b/migration/exec.c
>> @@ -27,7 +27,6 @@
>>   #include "qemu/cutils.h"
>>   
>>   #ifdef WIN32
>> -const char *exec_get_cmd_path(void);
>>   const char *exec_get_cmd_path(void)
>>   {
>>       g_autofree char *detected_path = g_new(char, MAX_PATH);
>> diff --git a/migration/exec.h b/migration/exec.h
>> index b210ffde7a..736cd71028 100644
>> --- a/migration/exec.h
>> +++ b/migration/exec.h
>> @@ -19,6 +19,10 @@
>>   
>>   #ifndef QEMU_MIGRATION_EXEC_H
>>   #define QEMU_MIGRATION_EXEC_H
>> +
>> +#ifdef WIN32
>> +const char *exec_get_cmd_path(void);
>> +#endif
>>   void exec_start_incoming_migration(const char *host_port, Error **errp);
>>   
>>   void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 6d3cf5d5cd..dcbd509d56 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -65,6 +65,7 @@
>>   #include "sysemu/qtest.h"
>>   #include "options.h"
>>   #include "sysemu/dirtylimit.h"
>> +#include "qemu/sockets.h"
>>   
>>   static NotifierList migration_state_notifiers =
>>       NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
>> @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address)
>>                         QAPI_CLONE(SocketAddress, address));
>>   }
>>   
>> +static bool migrate_uri_parse(const char *uri,
>> +                              MigrationAddress **channel,
>> +                              Error **errp)
>> +{
>> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
> This cannot be g_autoptr because you're passing it out of scope at the
> end of the function.
>> +    SocketAddress *saddr = &addr->u.socket;
> This attribution is useless. Down below you overwrite it with the result
> of socket_parse.
Ack. Will assign saddr to NULL here.
>> +    InetSocketAddress *isock = &addr->u.rdma;
>> +    strList **tail = &addr->u.exec.args;
>> +
>> +    if (strstart(uri, "exec:", NULL)) {
>> +        addr->transport = MIGRATION_ADDRESS_TYPE_EXEC;
>> +#ifdef WIN32
>> +        QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
>> +        QAPI_LIST_APPEND(tail, g_strdup("/c"));
>> +#else
>> +        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
>> +        QAPI_LIST_APPEND(tail, g_strdup("-c"));
>> +#endif
>> +        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
>> +    } else if (strstart(uri, "rdma:", NULL)) {
>> +        if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
>> +            qapi_free_InetSocketAddress(isock);
>> +            return false;
>> +        }
>> +        addr->transport = MIGRATION_ADDRESS_TYPE_RDMA;
>> +    } else if (strstart(uri, "tcp:", NULL) ||
>> +                strstart(uri, "unix:", NULL) ||
>> +                strstart(uri, "vsock:", NULL) ||
>> +                strstart(uri, "fd:", NULL)) {
>> +        addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET;
>> +        saddr = socket_parse(uri, errp);
>> +        if (!saddr) {
>> +            qapi_free_SocketAddress(saddr);
> Shouldn't free here. socket_parse() already does so on failure.
Okay, understood. So should just return false, nothing else here.
>> +            return false;
>> +        }
> Then here you can set the values you intended to set.
>
> addr->u.socket.type = saddr->type;
> addr->u.socket.u = saddr->u;
Ack. Will do that change.
>> +    } else {
>> +        error_setg(errp, "unknown migration protocol: %s", uri);
>> +        return false;
>> +    }
>> +
>> +    *channel = addr;
>> +    return true;
>> +}
>> +
>>   static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>   {
>>       const char *p = NULL;
>> +    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
> The memory is leaked here because the pointer is overwritten below. Just
> set it to NULL. You can keep the g_autoptr.
Ack. You mean it is overwritten by migrate_parse_uri function.
>>   
>>       /* URI is not suitable for migration? */
>>       if (!migration_channels_and_uri_compatible(uri, errp)) {
>>           return;
>>       }
>>   
>> +    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
>> +        return;
>> +    }
>> +
>>       qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>>       if (strstart(uri, "tcp:", &p) ||
>>           strstart(uri, "unix:", NULL) ||
>> @@ -1671,12 +1721,17 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>       Error *local_err = NULL;
>>       MigrationState *s = migrate_get_current();
>>       const char *p = NULL;
>> +    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
> Same here.
Ack. Will change that.
>>   
>>       /* URI is not suitable for migration? */
>>       if (!migration_channels_and_uri_compatible(uri, errp)) {
>>           return;
>>       }
>>   
>> +    if (!migrate_uri_parse(uri, &channel, errp)) {
>> +        return;
>> +    }
>> +
>>       resume_requested = has_resume && resume;
>>       if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
>>                            resume_requested, errp)) {
Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 8092 bytes --]

Re: [PATCH v11 02/10] migration: convert migration 'uri' into 'MigrateAddress'

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 3795 bytes --]


On 10/4/2023 11:42 PM, Fabiano Rosas wrote:
> Daniel P. Berrangé<berrange@redhat.com>  writes:
>
>> On Wed, Oct 04, 2023 at 11:43:12AM -0300, Fabiano Rosas wrote:
>>> Het Gala<het.gala@nutanix.com>  writes:
>>>
>>>> This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri'
>>>> string containing migration connection related information
>>>> and stores them inside well defined 'MigrateAddress' struct.
>>>>
>>>> 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/exec.c      |  1 -
>>>>   migration/exec.h      |  4 ++++
>>>>   migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 59 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/migration/exec.c b/migration/exec.c
>>>> index 2bf882bbe1..32f5143dfd 100644
>>>> --- a/migration/exec.c
>>>> +++ b/migration/exec.c
>>>> @@ -27,7 +27,6 @@
>>>>   #include "qemu/cutils.h"
>>>>   
>>>>   #ifdef WIN32
>>>> -const char *exec_get_cmd_path(void);
>>>>   const char *exec_get_cmd_path(void)
>>>>   {
>>>>       g_autofree char *detected_path = g_new(char, MAX_PATH);
>>>> diff --git a/migration/exec.h b/migration/exec.h
>>>> index b210ffde7a..736cd71028 100644
>>>> --- a/migration/exec.h
>>>> +++ b/migration/exec.h
>>>> @@ -19,6 +19,10 @@
>>>>   
>>>>   #ifndef QEMU_MIGRATION_EXEC_H
>>>>   #define QEMU_MIGRATION_EXEC_H
>>>> +
>>>> +#ifdef WIN32
>>>> +const char *exec_get_cmd_path(void);
>>>> +#endif
>>>>   void exec_start_incoming_migration(const char *host_port, Error **errp);
>>>>   
>>>>   void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index 6d3cf5d5cd..dcbd509d56 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -65,6 +65,7 @@
>>>>   #include "sysemu/qtest.h"
>>>>   #include "options.h"
>>>>   #include "sysemu/dirtylimit.h"
>>>> +#include "qemu/sockets.h"
>>>>   
>>>>   static NotifierList migration_state_notifiers =
>>>>       NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
>>>> @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address)
>>>>                         QAPI_CLONE(SocketAddress, address));
>>>>   }
>>>>   
>>>> +static bool migrate_uri_parse(const char *uri,
>>>> +                              MigrationAddress **channel,
>>>> +                              Error **errp)
>>>> +{
>>>> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>>> This cannot be g_autoptr because you're passing it out of scope at the
>>> end of the function.
>> It is still good to use g_autoptr to deal with the error paths.
>>
>> On the success path though you need   g_steal_pointer(&addr) to
>> prevent the autofree cleanup running.
> Ah good point, this has been suggested in an earlier version already, I
> forgot to mention. We should definitely use g_steal_pointer() whenever
> the variable goes out of scope.
Okay. I get the point that g_autoptr helps to deal with freeing of 
pointer in case error occurs inside the function.
But I am still trying to figure out why we need g_steal_pointer() ? If 
you could please explain once again.
My understanding till now was that if we want to return 'addr' variable 
as return type, then we would want to make use of g_steal_pointer(&addr) 
so instead of addr, we pass a temp ptr refrencing to the same location 
as addr, and then assign addr = NULL. But we are already assigning the 
memory location where addr was refrencing to 'channel'. Let me know if I 
am missing something ?
I think the syntax follows as the second example given here: 
https://docs.gtk.org/glib/func.steal_pointer.html ?


Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 4923 bytes --]

Re: [PATCH v11 05/10] migration: convert exec backend to accept MigrateAddress.

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 3636 bytes --]

On 10/4/2023 8:25 PM, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> Exec transport backend for 'migrate'/'migrate-incoming' QAPIs accept
>> new wire protocol of MigrateAddress struct.
>>
>> It is achived by parsing 'uri' string and storing migration parameters
>> required for exec connection into strList struct.
>>
>> 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/exec.c      | 71 +++++++++++++++++++++++++++++++------------
>>   migration/exec.h      |  4 +--
>>   migration/migration.c | 10 +++---
>>   3 files changed, 57 insertions(+), 28 deletions(-)
>>
>> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
>> +/* provides the length of strList */
>> +static int
>> +str_list_length(strList *list)
>> +{
>> +    int len = 0;
>> +    strList *elem;
>> +
>> +    for (elem = list; elem != NULL; elem = elem->next) {
>> +        len++;
>> +    }
>> +
>> +    return len;
>> +}
>> +
>> +static void
>> +init_exec_array(strList *command, char **argv, Error **errp)
>> +{
>> +    int i = 0;
>> +    strList *lst;
>> +
>> +    for (lst = command; lst; lst = lst->next) {
>> +        argv[i++] = lst->value;
>> +    }
>> +
>> +    argv[i] = NULL;
> This will write out of bounds.
Yes, will increase the length of argv in exec_start_outgoing_migration() 
by one, that would solve the issue right.
>> +    return;
>> +}
>> +
>> +void exec_start_outgoing_migration(MigrationState *s, strList *command,
>> +                                   Error **errp)
>>   {
>>       QIOChannel *ioc;
>>   
>> -#ifdef WIN32
>> -    const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
>> -#else
>> -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
>> -#endif
>> +    int length = str_list_length(command);
>> +    g_auto(GStrv) argv = (char **) g_new0(const char *, length);
> This allocation does not leave space for the NULL byte.

Yes you are rigght. I assumed the user will itself have to end the list 
of argument with a 'NULL', but that's not correct. Thanks for pointing 
it out. Will make the length of argv from length -> length+1.

>>   
>> -    trace_migration_exec_outgoing(command);
>> -    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
>> -                                                    O_RDWR,
>> -                                                    errp));
>> +    init_exec_array(command, argv, errp);
>> +    g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
>> +
>> +    trace_migration_exec_outgoing(new_command);
>> +    ioc = QIO_CHANNEL(
>> +        qio_channel_command_new_spawn((const char * const *) argv,
>> +                                      O_RDWR,
>> +                                      errp));
>>       if (!ioc) {
>>           return;
>>       }
>> @@ -71,20 +101,21 @@ static gboolean exec_accept_incoming_migration(QIOChannel *ioc,
>>       return G_SOURCE_REMOVE;
>>   }
>>   
>> -void exec_start_incoming_migration(const char *command, Error **errp)
>> +void exec_start_incoming_migration(strList *command, Error **errp)
>>   {
>>       QIOChannel *ioc;
>>   
>> -#ifdef WIN32
>> -    const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
>> -#else
>> -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
>> -#endif
>> +    int length = str_list_length(command);
>> +    g_auto(GStrv) argv = (char **) g_new0(const char *, length);
> Here as well.
Ack, will increase length of argv by one while initalization.
Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 5128 bytes --]

Re: [PATCH v11 08/10] migration: Implement MigrateChannelList to qmp migration flow.

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 6286 bytes --]

On 10/4/2023 8:51 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 qmp migration.
>>
>> For current series, limit the size of MigrateChannelList
>> to single element (single interface) as runtime check.
>>
>> 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.c | 95 +++++++++++++++++++++++--------------------
>>   1 file changed, 52 insertions(+), 43 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 6f948988ec..3eae32e616 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -432,9 +432,10 @@ void migrate_add_address(SocketAddress *address)
>>   }
>>   
>>   static bool migrate_uri_parse(const char *uri,
>> -                              MigrationAddress **channel,
>> +                              MigrationChannel **channel,
>>                                 Error **errp)
>>   {
>> +    g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1);
> Here val is passed out of scope so it shouldn't be g_autoptr.
I guess, same as for 'addr' we need to go with adding 
g_steal_pointer(&val) here too ?
>>       g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>>       SocketAddress *saddr = &addr->u.socket;
>>       InetSocketAddress *isock = &addr->u.rdma;
>> @@ -471,7 +472,9 @@ static bool migrate_uri_parse(const char *uri,
>>           return false;
>>       }
>>   
>> -    *channel = addr;
>> +    val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN;
>> +    val->addr = addr;
>> +    *channel = val;
>>       return true;
>>   }
>>   
>> @@ -479,41 +482,44 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>>                                             MigrationChannelList *channels,
>>                                             Error **errp)
>>   {
>> -    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>> +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
>> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
> Here we want just the pointer, no allocation, no freeing. For both
> channel and addr.
Ack, same as channel in patch 2.
>>   
>>       /*
>>        * Having preliminary checks for uri and channel
>>        */
>> -    if (has_channels) {
>> -        error_setg(errp, "'channels' argument should not be set yet.");
>> -        return;
>> -    }
>> -
>>       if (uri && has_channels) {
>>           error_setg(errp, "'uri' and 'channels' arguments are mutually "
>>                      "exclusive; exactly one of the two should be present in "
>>                      "'migrate-incoming' qmp command ");
>>           return;
>> -    }
>> -
>> -    if (!uri && !has_channels) {
>> +    } else if (channels) {
>> +        /* To verify that Migrate channel list has only item */
>> +        if (channels->next) {
>> +            error_setg(errp, "Channel list has more than one entries");
>> +            return;
>> +        }
>> +        channel = channels->value;
>> +    } else if (uri) {
>> +        /* caller uses the old URI syntax */
>> +        if (!migrate_uri_parse(uri, &channel, errp)) {
>> +            return;
>> +        }
>> +    } else {
>>           error_setg(errp, "neither 'uri' or 'channels' argument are "
>>                      "specified in 'migrate-incoming' qmp command ");
>>           return;
>>       }
>> -
>> -    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
>> -        return;
>> -    }
>> +    addr = channel->addr;
>>   
>>       /* transport mechanism not suitable for migration? */
>> -    if (!migration_channels_and_transport_compatible(channel, errp)) {
>> +    if (!migration_channels_and_transport_compatible(addr, errp)) {
>>           return;
>>       }
>>   
>>       qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>> -    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>> -        SocketAddress *saddr = &channel->u.socket;
>> +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>> +        SocketAddress *saddr = &addr->u.socket;
>>           if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>>               saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>>               saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>> @@ -522,11 +528,11 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>>               fd_start_incoming_migration(saddr->u.fd.str, errp);
>>           }
>>   #ifdef CONFIG_RDMA
>> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>> -        rdma_start_incoming_migration(&channel->u.rdma, errp);
>> -#endif
>> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>> -        exec_start_incoming_migration(channel->u.exec.args, errp);
>> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>> +        rdma_start_incoming_migration(&addr->u.rdma, errp);
>> + #endif
>> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>> +        exec_start_incoming_migration(addr->u.exec.args, errp);
>>       } else {
>>           error_setg(errp, "unknown migration protocol: %s", uri);
>>       }
>> @@ -1750,35 +1756,38 @@ void qmp_migrate(const char *uri, bool has_channels,
>>       bool resume_requested;
>>       Error *local_err = NULL;
>>       MigrationState *s = migrate_get_current();
>> -    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>> +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
>> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
> Again just the pointers.
>
> We'll have to make another pass and check whether we're missing freeing
> something. But for now let's first clear all the errors then we can look
> at the code working and do the necessary changes.
Ack.
>>   
>>       /*
>>        * Having preliminary checks for uri and channel
>>        */
>> -    if (has_channels) {
>> -        error_setg(errp, "'channels' argument should not be set yet.");
>> -        return;
>> -    }
>>    
Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 7666 bytes --]

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

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 3991 bytes --]

Re: [PATCH v11 10/10] migration: modify test_multifd_tcp_none() to use new QAPI syntax.

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]


On 10/4/2023 8:55 PM, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> modify multifd tcp common test to incorporate the new QAPI
>> syntax defined.
>>
>> 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>
>> ---
>>   tests/qtest/migration-test.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index 46f1c275a2..246cab6451 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -2205,7 +2205,12 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState *from,
>>   
>>       /* Start incoming migration from the 1st socket */
>>       qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
>> -                             "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
>> +                             "  'arguments': { "
>> +                             "      'channels': [ { 'channeltype': 'main',"
> channel-type
Ack.
>
>> +                             "      'addr': { 'transport': 'socket',"
>> +                             "                'type': 'inet',"
>> +                             "                'host': '127.0.0.1',"
>> +                             "                'port': '0' } } ] } }");
>>   
>>       return NULL;
>>   }
Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 2539 bytes --]

Re: [PATCH v11 00/10] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 6621 bytes --]


On 10/4/2023 9:02 PM, Fabiano Rosas wrote:
> Fabiano Rosas<farosas@suse.de>  writes:
>
>> Het Gala<het.gala@nutanix.com>  writes:
>>
>>> On 04/10/23 7:03 pm, Fabiano Rosas wrote:
>>>> Het Gala<het.gala@nutanix.com>  writes:
>>>>
>>>>> This is v11 patchset of modified 'migrate' and 'migrate-incoming' QAPI design
>>>>> for upstream review.
>>>>>
>>>>> Update: Daniel has reviewed all patches and is okay with them. Markus has also
>>>>>           given Acked-by tag for patches related to QAPI syntax change.
>>>>> Fabiano, Juan and other migration maintainers, let me know if there are still
>>>>> improvements to be made in this patchset series.
>>>>>
>>>>> Link to previous upstream community patchset links:
>>>>> v1:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2022-2D12_msg04339.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=jsRvKRy1JOiy05KX1CtLqWN1su5XNmKPKuJTSx5sZpU&e=
>>>>> v2:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D02_msg02106.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=mzt3n5PD1QclHfpZEh-VMoLkkwT8xqjPYN-1r7MOly0&e=
>>>>> v3:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D02_msg02473.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=fa9W71JU6-3xZrjLH7AmElgqwJGUkPeQv3P7n6EXxOM&e=
>>>>> v4:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D05_msg03064.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=Xr1y3EvBzEtWT9O1fVNapCb3WnD-aWR8UeXv6J6gZQM&e=
>>>>> v5:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D05_msg04845.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=OtK10W2Z0DobrktRfTCMYPxbcMaaZ6f6qoA65D4RG_A&e=
>>>>> v6:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D06_msg01251.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=XH-4qFQgdkAKmRsa9DuqaZgJMvGUi1p4-s05AsAEYRo&e=
>>>>> v7:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg02027.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=RwvfliI4wLm7S0TKl5RMku-gSSE-5fZPYH0MkzJdoPw&e=
>>>>> v8:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg02770.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=BZsKBJGVPDWXwGgb2-fAnS9pWzTYuLzI92TmuWBcB3k&e=
>>>>> v9:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg04216.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=YcWFU9I2u-R6QbVjweZ3lFvJlllm-i9o5_jtLBxC_oc&e=
>>>>> v10:https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2023-2D07_msg05022.html&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=xuVA--dLVo9lijpitqSt7EOEzBGpEvigXGCb9p_MIk0xmhQZ8bPasLgZ2aOlEBcz&s=JQt63Ikbz21vmsLmSensQu8zknGuS9bls-IFpndor78&e=
>>>>>
>>>>> v10 -> v11 changelog:
>>>>> -------------------
>>>>> - Resolved make check errors as its been almost two months since v10
>>>>>     version of this patchset series went out. Till date migration workflow
>>>>>     might have changed which caused make check errors.
>>>> Sorry, there must be a misunderstanding here. This series still has
>>>> problems. Just look at patch 6 that adds the "channel-type" parameter and
>>>> patch 10 that uses "channeltype" in the test (without hyphen). This
>>>> cannot work.
>>> Ack. I will change that.
>>>> There's also several instances of g_autoptr being used incorrectly. I
>>>> could comment on every patch individually, but this series cannot have
>>>> passed make check.
>>> Are we allowed to run the make checks ? I am not aware from where these
>>> failures are arising. It would be helpful if you could point out to me
>>> where g_autoptr is incorrectly used ?
>> I mean just the project's make check command:
>>
>> cd build/
>> ../configure
>> make -j$(nproc)
>> make -j$(nproc) check
Yes, I got it now. Thanks
>>>> Please resend this with the issues fixed and drop the Reviewed-bys from
>>>> the affected patches.
>>> How to verify which are the affected patches here ?
>> I'll comment in each patch individually.
> Done.
>
> We had some double-frees when using g_autoptr in structures that are
> nested into another. The qapi code already descends and frees the
> children.
>
> There were also issues with allocating memory and later overwriting the
> pointers.
>
> This might still not put us in the most correct situation regarding
> memory, but I think it will at least get make check passing. Feel free
> to investigate the errors with make check and propose alternative
> solutions. It has been a while since I looked at this series, I might
> have missed something further.
Yes, for now I have tried to address all the comments made in the 
individual patches and tried to fix issue of pointer overwriting 
wherever I could spot, also double frees in the hmp code workflow. By 
doing this, I have passed all the make checks, but if there are places 
which needs to be re-looked for the above issues you mentioned, please 
let me know.
>> We'll also have to add compatibility with the new file: URI that's
>> included in the latest migration pull request. I'll add comments on
>> where I think we'll need to add code to support that feature.
> I'll actually defer here until you post your series with the
> fixes. It'll probably be easier if I just send individual additions to
> your patches.

Ack, thanks for reviewing patches and giving valuable feedback. Sending 
new patchset series within sometime.

Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 12698 bytes --]

Re: [PATCH v11 02/10] migration: convert migration 'uri' into 'MigrateAddress'

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> On 10/4/2023 11:42 PM, Fabiano Rosas wrote:
>> Daniel P. Berrangé<berrange@redhat.com>  writes:
>>
>>> On Wed, Oct 04, 2023 at 11:43:12AM -0300, Fabiano Rosas wrote:
>>>> Het Gala<het.gala@nutanix.com>  writes:
>>>>
>>>>> This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri'
>>>>> string containing migration connection related information
>>>>> and stores them inside well defined 'MigrateAddress' struct.
>>>>>
>>>>> 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/exec.c      |  1 -
>>>>>   migration/exec.h      |  4 ++++
>>>>>   migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++
>>>>>   3 files changed, 59 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/migration/exec.c b/migration/exec.c
>>>>> index 2bf882bbe1..32f5143dfd 100644
>>>>> --- a/migration/exec.c
>>>>> +++ b/migration/exec.c
>>>>> @@ -27,7 +27,6 @@
>>>>>   #include "qemu/cutils.h"
>>>>>   
>>>>>   #ifdef WIN32
>>>>> -const char *exec_get_cmd_path(void);
>>>>>   const char *exec_get_cmd_path(void)
>>>>>   {
>>>>>       g_autofree char *detected_path = g_new(char, MAX_PATH);
>>>>> diff --git a/migration/exec.h b/migration/exec.h
>>>>> index b210ffde7a..736cd71028 100644
>>>>> --- a/migration/exec.h
>>>>> +++ b/migration/exec.h
>>>>> @@ -19,6 +19,10 @@
>>>>>   
>>>>>   #ifndef QEMU_MIGRATION_EXEC_H
>>>>>   #define QEMU_MIGRATION_EXEC_H
>>>>> +
>>>>> +#ifdef WIN32
>>>>> +const char *exec_get_cmd_path(void);
>>>>> +#endif
>>>>>   void exec_start_incoming_migration(const char *host_port, Error **errp);
>>>>>   
>>>>>   void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
>>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>>> index 6d3cf5d5cd..dcbd509d56 100644
>>>>> --- a/migration/migration.c
>>>>> +++ b/migration/migration.c
>>>>> @@ -65,6 +65,7 @@
>>>>>   #include "sysemu/qtest.h"
>>>>>   #include "options.h"
>>>>>   #include "sysemu/dirtylimit.h"
>>>>> +#include "qemu/sockets.h"
>>>>>   
>>>>>   static NotifierList migration_state_notifiers =
>>>>>       NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
>>>>> @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address)
>>>>>                         QAPI_CLONE(SocketAddress, address));
>>>>>   }
>>>>>   
>>>>> +static bool migrate_uri_parse(const char *uri,
>>>>> +                              MigrationAddress **channel,
>>>>> +                              Error **errp)
>>>>> +{
>>>>> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>>>> This cannot be g_autoptr because you're passing it out of scope at the
>>>> end of the function.
>>> It is still good to use g_autoptr to deal with the error paths.
>>>
>>> On the success path though you need   g_steal_pointer(&addr) to
>>> prevent the autofree cleanup running.
>> Ah good point, this has been suggested in an earlier version already, I
>> forgot to mention. We should definitely use g_steal_pointer() whenever
>> the variable goes out of scope.
> Okay. I get the point that g_autoptr helps to deal with freeing of 
> pointer in case error occurs inside the function.

Yes, but note g_autoptr will free the memory any time the variable goes
out of scope, i.e. any time we return from the function. Even when
there's no error and we actually want that memory to still be around for
the callers to use.

> But I am still trying to figure out why we need g_steal_pointer() ? If 
> you could please explain once again.
> My understanding till now was that if we want to return 'addr' variable 
> as return type, then we would want to make use of g_steal_pointer(&addr) 
> so instead of addr, we pass a temp ptr refrencing to the same location 
> as addr, and then assign addr = NULL. But we are already assigning the 
> memory location where addr was refrencing to 'channel'. Let me know if I 
> am missing something ?

So now 'channel' points to the memory you allocated at the start of the
function with g_new0. When the function returns, g_autoptr has no idea
that someone is still using that memory, so it will just free it.

Whenever you want a chunk of memory to be accessed across function calls
like that you need to steal the reference. After stealing, the pointer
that was registered with g_autoptr (in this case 'addr') now points to
nothing and the pointer that was returned/assigned is a different one
that isn't known by any cleanup functions.

Note that after g_steal_pointer, that memory now becomes responsibility
of whatever piece of code owns that pointer. In this case,
qemu_start_incoming_migration() and qmp_migrate(). Those two functions
will have to free the memory once they are done with it. Or do the
g_autoptr/g_steal_pointer trick once more.

> I think the syntax follows as the second example given here: 
> https://docs.gtk.org/glib/func.steal_pointer.html ?

Yep, that's it.

Re: [PATCH v11 08/10] migration: Implement MigrateChannelList to qmp migration flow.

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

> On 10/4/2023 8:51 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 qmp migration.
>>>
>>> For current series, limit the size of MigrateChannelList
>>> to single element (single interface) as runtime check.
>>>
>>> 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.c | 95 +++++++++++++++++++++++--------------------
>>>   1 file changed, 52 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 6f948988ec..3eae32e616 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -432,9 +432,10 @@ void migrate_add_address(SocketAddress *address)
>>>   }
>>>   
>>>   static bool migrate_uri_parse(const char *uri,
>>> -                              MigrationAddress **channel,
>>> +                              MigrationChannel **channel,
>>>                                 Error **errp)
>>>   {
>>> +    g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1);
>> Here val is passed out of scope so it shouldn't be g_autoptr.
> I guess, same as for 'addr' we need to go with adding 
> g_steal_pointer(&val) here too ?

Yes, you cannot give the same pointer to *channel because this one is
already being tracked by g_autoptr and it will free the memory when it
gets the chance. So we need to steal it from g_autoptr, so to speak.

>>>       g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>>>       SocketAddress *saddr = &addr->u.socket;
>>>       InetSocketAddress *isock = &addr->u.rdma;
>>> @@ -471,7 +472,9 @@ static bool migrate_uri_parse(const char *uri,
>>>           return false;
>>>       }
>>>   
>>> -    *channel = addr;
>>> +    val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN;
>>> +    val->addr = addr;
>>> +    *channel = val;
>>>       return true;
>>>   }
>>>   
>>> @@ -479,41 +482,44 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>>>                                             MigrationChannelList *channels,
>>>                                             Error **errp)
>>>   {
>>> -    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>>> +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
>>> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>> Here we want just the pointer, no allocation, no freeing. For both
>> channel and addr.
> Ack, same as channel in patch 2.

This is actually one of the cases where we need to think about how we
are going to free that memory. You need to make sure no one is using the
'addr' after you call into the *_incoming_migration functions. All users
should either use the value and return or make a copy if they intend to
pass it forward. If you determine that no one is using 'addr' and
'channel', then we could bring the channel g_autoptr back.

>>>   
>>>       /*
>>>        * Having preliminary checks for uri and channel
>>>        */
>>> -    if (has_channels) {
>>> -        error_setg(errp, "'channels' argument should not be set yet.");
>>> -        return;
>>> -    }
>>> -
>>>       if (uri && has_channels) {
>>>           error_setg(errp, "'uri' and 'channels' arguments are mutually "
>>>                      "exclusive; exactly one of the two should be present in "
>>>                      "'migrate-incoming' qmp command ");
>>>           return;
>>> -    }
>>> -
>>> -    if (!uri && !has_channels) {
>>> +    } else if (channels) {
>>> +        /* To verify that Migrate channel list has only item */
>>> +        if (channels->next) {
>>> +            error_setg(errp, "Channel list has more than one entries");
>>> +            return;
>>> +        }
>>> +        channel = channels->value;
>>> +    } else if (uri) {
>>> +        /* caller uses the old URI syntax */
>>> +        if (!migrate_uri_parse(uri, &channel, errp)) {
>>> +            return;
>>> +        }
>>> +    } else {
>>>           error_setg(errp, "neither 'uri' or 'channels' argument are "
>>>                      "specified in 'migrate-incoming' qmp command ");
>>>           return;
>>>       }
>>> -
>>> -    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
>>> -        return;
>>> -    }
>>> +    addr = channel->addr;
>>>   
>>>       /* transport mechanism not suitable for migration? */
>>> -    if (!migration_channels_and_transport_compatible(channel, errp)) {
>>> +    if (!migration_channels_and_transport_compatible(addr, errp)) {
>>>           return;
>>>       }
>>>   
>>>       qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>>> -    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>>> -        SocketAddress *saddr = &channel->u.socket;
>>> +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>>> +        SocketAddress *saddr = &addr->u.socket;
>>>           if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>>>               saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>>>               saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>>> @@ -522,11 +528,11 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>>>               fd_start_incoming_migration(saddr->u.fd.str, errp);
>>>           }
>>>   #ifdef CONFIG_RDMA
>>> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>>> -        rdma_start_incoming_migration(&channel->u.rdma, errp);
>>> -#endif
>>> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>>> -        exec_start_incoming_migration(channel->u.exec.args, errp);
>>> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>>> +        rdma_start_incoming_migration(&addr->u.rdma, errp);
>>> + #endif
>>> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>>> +        exec_start_incoming_migration(addr->u.exec.args, errp);
>>>       } else {
>>>           error_setg(errp, "unknown migration protocol: %s", uri);
>>>       }
>>> @@ -1750,35 +1756,38 @@ void qmp_migrate(const char *uri, bool has_channels,
>>>       bool resume_requested;
>>>       Error *local_err = NULL;
>>>       MigrationState *s = migrate_get_current();
>>> -    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>>> +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
>>> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>> Again just the pointers.
>>
>> We'll have to make another pass and check whether we're missing freeing
>> something. But for now let's first clear all the errors then we can look
>> at the code working and do the necessary changes.
> Ack.
>>>   
>>>       /*
>>>        * Having preliminary checks for uri and channel
>>>        */
>>> -    if (has_channels) {
>>> -        error_setg(errp, "'channels' argument should not be set yet.");
>>> -        return;
>>> -    }
>>>    
> Regards,
> Het Gala

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

[Fabiano Rosas]

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'

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 4692 bytes --]


On 10/9/2023 7:43 PM, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> On 10/4/2023 11:42 PM, Fabiano Rosas wrote:
>>> Daniel P. Berrangé<berrange@redhat.com>   writes:
>>>
>>>> On Wed, Oct 04, 2023 at 11:43:12AM -0300, Fabiano Rosas wrote:
>>>>> Het Gala<het.gala@nutanix.com>   writes:
>>>>>
>>>>>> This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri'
>>>>>> string containing migration connection related information
>>>>>> and stores them inside well defined 'MigrateAddress' struct.
>>>>>>
>>>>>> 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/exec.c      |  1 -
>>>>>>    migration/exec.h      |  4 ++++
>>>>>>    migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++
>>>>>>    3 files changed, 59 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/migration/exec.c b/migration/exec.c
>>>>>> index 2bf882bbe1..32f5143dfd 100644
>>>>>> --- a/migration/exec.c
>>>>>> +++ b/migration/exec.c
>>>>>> @@ -27,7 +27,6 @@
>>>>>>    #include "qemu/cutils.h"
>>>>>>    
>>>>>>    #ifdef WIN32
>>>>>> -const char *exec_get_cmd_path(void);
>>>>>>    const char *exec_get_cmd_path(void)
>>>>>>    {
>>>>>>        g_autofree char *detected_path = g_new(char, MAX_PATH);
>>>>>> diff --git a/migration/exec.h b/migration/exec.h
>>>>>> index b210ffde7a..736cd71028 100644
>>>>>> --- a/migration/exec.h
>>>>>> +++ b/migration/exec.h
>>>>>> @@ -19,6 +19,10 @@
>>>>>>    
>>>>>>    #ifndef QEMU_MIGRATION_EXEC_H
>>>>>> rate_add_address(SocketAddress *address)
>>>>>> ng it out of scope at the
>>>>>> end of the function.
>>>> It is still good to use g_autoptr to deal with the error paths.
>>>>
>>>> On the success path though you need   g_steal_pointer(&addr) to
>>>> prevent the autofree cleanup running.
>>> Ah good point, this has been suggested in an earlier version already, I
>>> forgot to mention. We should definitely use g_steal_pointer() whenever
>>> the variable goes out of scope.
>> Okay. I get the point that g_autoptr helps to deal with freeing of
>> pointer in case error occurs inside the function.
> Yes, but note g_autoptr will free the memory any time the variable goes
> out of scope, i.e. any time we return from the function. Even when
> there's no error and we actually want that memory to still be around for
> the callers to use.
>
>> But I am still trying to figure out why we need g_steal_pointer() ? If
>> you could please explain once again.
>> My understanding till now was that if we want to return 'addr' variable
>> as return type, then we would want to make use of g_steal_pointer(&addr)
>> so instead of addr, we pass a temp ptr refrencing to the same location
>> as addr, and then assign addr = NULL. But we are already assigning the
>> memory location where addr was refrencing to 'channel'. Let me know if I
>> am missing something ?
> So now 'channel' points to the memory you allocated at the start of the
> function with g_new0. When the function returns, g_autoptr has no idea
> that someone is still using that memory, so it will just free it.
>
> Whenever you want a chunk of memory to be accessed across function calls
> like that you need to steal the reference. After stealing, the pointer
> that was registered with g_autoptr (in this case 'addr') now points to
> nothing and the pointer that was returned/assigned is a different one
> that isn't known by any cleanup functions.
>
> Note that after g_steal_pointer, that memory now becomes responsibility
> of whatever piece of code owns that pointer. In this case,
> qemu_start_incoming_migration() and qmp_migrate(). Those two functions
> will have to free the memory once they are done with it. Or do the
> g_autoptr/g_steal_pointer trick once more.

Got your point perfectly now. Thanks a lot. Just so that I understood 
right, summarising it in short:

g_autoptr --> helps in error paths so we use it, but for non-error paths 
if it goes out of scope, it will free it so to prevent that we have to 
use g_steal_pointer(). Either use g_autoptr/g_steal_pointer pair or do 
it simple but then will have to take care of error paths as well as 
non-error paths to free the pointers.

>> I think the syntax follows as the second example given here:
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.gtk.org_glib_func.steal-5Fpointer.html&d=DwIFaQ&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=Fn2ZhrrsSvcV_ZUX3PUnI1zMMD7JM5o1X9LbURlU1B7O06RNnzmOS3hKI4HBe6fB&s=iJFFbZnxzfqd1xu3gRVfvdjjE_X3XT3t0aRRAIeaUUc&e=   ?
> Yep, that's it.

Ack.

Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 6856 bytes --]

Re: [PATCH v11 08/10] migration: Implement MigrateChannelList to qmp migration flow.

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 7680 bytes --]


On 10/9/2023 7:59 PM, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> On 10/4/2023 8:51 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 qmp migration.
>>>>
>>>> For current series, limit the size of MigrateChannelList
>>>> to single element (single interface) as runtime check.
>>>>
>>>> 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.c | 95 +++++++++++++++++++++++--------------------
>>>>    1 file changed, 52 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index 6f948988ec..3eae32e616 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -432,9 +432,10 @@ void migrate_add_address(SocketAddress *address)
>>>>    }
>>>>    
>>>>    static bool migrate_uri_parse(const char *uri,
>>>> -                              MigrationAddress **channel,
>>>> +                              MigrationChannel **channel,
>>>>                                  Error **errp)
>>>>    {
>>>> +    g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1);
>>> Here val is passed out of scope so it shouldn't be g_autoptr.
>> I guess, same as for 'addr' we need to go with adding
>> g_steal_pointer(&val) here too ?
> Yes, you cannot give the same pointer to *channel because this one is
> already being tracked by g_autoptr and it will free the memory when it
> gets the chance. So we need to steal it from g_autoptr, so to speak.
Yes true. Ack.
>>>>        g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>>>>        SocketAddress *saddr = &addr->u.socket;
>>>>        InetSocketAddress *isock = &addr->u.rdma;
>>>> @@ -471,7 +472,9 @@ static bool migrate_uri_parse(const char *uri,
>>>>            return false;
>>>>        }
>>>>    
>>>> -    *channel = addr;
>>>> +    val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN;
>>>> +    val->addr = addr;
>>>> +    *channel = val;
>>>>        return true;
>>>>    }
>>>>    
>>>> @@ -479,41 +482,44 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>>>>                                              MigrationChannelList *channels,
>>>>                                              Error **errp)
>>>>    {
>>>> -    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>>>> +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
>>>> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>>> Here we want just the pointer, no allocation, no freeing. For both
>>> channel and addr.
>> Ack, same as channel in patch 2.
> This is actually one of the cases where we need to think about how we
> are going to free that memory. You need to make sure no one is using the
> 'addr' after you call into the *_incoming_migration functions. All users
> should either use the value and return or make a copy if they intend to
> pass it forward. If you determine that no one is using 'addr' and
> 'channel', then we could bring the channel g_autoptr back.
Honestly, I think g_autoptr would work here because no one uses addr or 
channel after *_incoming_migration or *_outgoing_migration functions. 
But not sure. But that does not make the checks pass, so I have gone 
forward with simple pointers to pass make checks.
>
>>>>    
>>>>        /*
>>>>         * Having preliminary checks for uri and channel
>>>>         */
>>>> -    if (has_channels) {
>>>> -        error_setg(errp, "'channels' argument should not be set yet.");
>>>> -        return;
>>>> -    }
>>>> -
>>>>        if (uri && has_channels) {
>>>>            error_setg(errp, "'uri' and 'channels' arguments are mutually "
>>>>                       "exclusive; exactly one of the two should be present in "
>>>>                       "'migrate-incoming' qmp command ");
>>>>            return;
>>>> -    }
>>>> -
>>>> -    if (!uri && !has_channels) {
>>>> +    } else if (channels) {
>>>> +        /* To verify that Migrate channel list has only item */
>>>> +        if (channels->next) {
>>>> +            error_setg(errp, "Channel list has more than one entries");
>>>> +            return;
>>>> +        }
>>>> +        channel = channels->value;
>>>> +    } else if (uri) {
>>>> +        /* caller uses the old URI syntax */
>>>> +        if (!migrate_uri_parse(uri, &channel, errp)) {
>>>> +            return;
>>>> +        }
>>>> +    } else {
>>>>            error_setg(errp, "neither 'uri' or 'channels' argument are "
>>>>                       "specified in 'migrate-incoming' qmp command ");
>>>>            return;
>>>>        }
>>>> -
>>>> -    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
>>>> -        return;
>>>> -    }
>>>> +    addr = channel->addr;
>>>>    
>>>>        /* transport mechanism not suitable for migration? */
>>>> -    if (!migration_channels_and_transport_compatible(channel, errp)) {
>>>> +    if (!migration_channels_and_transport_compatible(addr, errp)) {
>>>>            return;
>>>>        }
>>>>    
>>>>        qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>>>> -    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>>>> -        SocketAddress *saddr = &channel->u.socket;
>>>> +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>>>> +        SocketAddress *saddr = &addr->u.socket;
>>>>            if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>>>>                saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>>>>                saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>>>> @@ -522,11 +528,11 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>>>>                fd_start_incoming_migration(saddr->u.fd.str, errp);
>>>>            }
>>>>    #ifdef CONFIG_RDMA
>>>> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>>>> -        rdma_start_incoming_migration(&channel->u.rdma, errp);
>>>> -#endif
>>>> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>>>> -        exec_start_incoming_migration(channel->u.exec.args, errp);
>>>> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>>>> +        rdma_start_incoming_migration(&addr->u.rdma, errp);
>>>> + #endif
>>>> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>>>> +        exec_start_incoming_migration(addr->u.exec.args, errp);
>>>>        } else {
>>>>            error_setg(errp, "unknown migration protocol: %s", uri);
>>>>        }
>>>> @@ -1750,35 +1756,38 @@ void qmp_migrate(const char *uri, bool has_channels,
>>>>        bool resume_requested;
>>>>        Error *local_err = NULL;
>>>>        MigrationState *s = migrate_get_current();
>>>> -    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>>>> +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
>>>> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>>> Again just the pointers.
>>>
>>> We'll have to make another pass and check whether we're missing freeing
>>> something. But for now let's first clear all the errors then we can look
>>> at the code working and do the necessary changes.
>> Ack.
>>>>    
>>>>        /*
>>>>         * Having preliminary checks for uri and channel
>>>>         */
>>>> -    if (has_channels) {
>>>> -        error_setg(errp, "'channels' argument should not be set yet.");
>>>> -        return;
>>>> -    }
>>>>     
>> Regards,
>> Het Gala
Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 9611 bytes --]

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

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 3651 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 5467 bytes --]