[PATCH v4 0/8] migration: Modified 'migrate' and 'migrate-incoming' QAPI commands for migration

[PATCH v4 0/8] migration: Modified 'migrate' and 'migrate-incoming' QAPI commands for migration

[Het Gala]

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

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

Changelog:
----------
- Restructured the patchset into 8 different independently build patches.
- Patches [1-2] focused on introducing MigrateAddress struct to QAPIs
- Patches [3-5] focused on implementing modified QAPI design to transport
  backends like (socket, exec and rdma).
- Patches [6-8] focused on introducing MigrateChannelList struct on top of
  MigrateAddress QAPI as an additional argument to both migration QAPIs
  and also implemented in the migration code path.

v4 series look a longer time to post, as it was blocked because of improvment
change required on the qapi design level to allow 'unions inside another
union'.
For detailed discussion on this, please follow the upstream discussion here:
https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg06782.html
https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg06783.html

I would like to thank Markus and Daniel for merging patches to allow union
inside another union and also would like to thank Eric for his reviews
on the patches.

Het Gala (8):
  migration: introduced 'MigrateAddress' in QAPI for migration wire
    protocol.
  migration: Converts uri parameter into 'MigrateAddress' struct
  migration: converts socket backend to accept MigrateAddress struct
  migration: converts rdma backend to accept MigrateAddress struct
  migration: converts exec backend to accept MigrateAddress struct.
  migration: modified 'migrate' QAPI to accept 'channels' argument for
    migration
  migration: modified 'migrate-incoming' QAPI to accept 'channels'
    argument for migration.
  migration: Introduced MigrateChannelList struct to migration code
    flow.

 migration/exec.c               |  60 +++++++---
 migration/exec.h               |   8 +-
 migration/migration-hmp-cmds.c | 127 ++++++++++++++++++++-
 migration/migration.c          | 196 +++++++++++++++++++++++++++------
 migration/rdma.c               |  38 +++----
 migration/rdma.h               |   6 +-
 migration/socket.c             |  39 ++-----
 migration/socket.h             |   7 +-
 qapi/migration.json            | 145 +++++++++++++++++++++++-
 softmmu/vl.c                   |   2 +-
 10 files changed, 512 insertions(+), 116 deletions(-)

-- 
2.22.3

[PATCH v4 1/8] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.

[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
string type. The current migration flow follows double encoding scheme for
fetching migration parameters such as '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.

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/qapi/migration.json b/qapi/migration.json
index 82000adce4..bf90bd8fe2 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1423,6 +1423,47 @@
 ##
 { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
 
+##
+# @MigrateTransport:
+#
+# The supported communication transport mechanisms for migration
+#
+# @socket: Supported communication type between two devices for migration.
+#          Socket is able to cover all of 'tcp', 'unix', 'vsock' and
+#          'fd' already
+#
+# @exec: Supported communication type to redirect migration stream into file.
+#
+# @rdma: Supported communication type to redirect rdma type migration stream.
+#
+# Since 8.0
+##
+{ 'enum': 'MigrateTransport',
+  'data': ['socket', 'exec', 'rdma'] }
+
+##
+# @MigrateExecCommand:
+ #
+ # Since 8.0
+ ##
+{ 'struct': 'MigrateExecCommand',
+   'data': {'args': [ 'str' ] } }
+
+##
+# @MigrateAddress:
+#
+# The options available for communication transport mechanisms for migration
+#
+# Since 8.0
+##
+{ 'union': 'MigrateAddress',
+  'base': { 'transport' : 'MigrateTransport'},
+  'discriminator': 'transport',
+  'data': {
+    'socket': 'SocketAddress',
+    'exec': 'MigrateExecCommand',
+    'rdma': 'InetSocketAddress' } }
+
 ##
 # @migrate:
 #
-- 
2.22.3

[PATCH v4 2/8] migration: Converts uri parameter into 'MigrateAddress' struct

[Het Gala]

This patch introduces code that can parse 'uri' string parameter and
spit out 'MigrateAddress' struct. All the required migration parameters
are stored in the struct.

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 migration/migration.c | 63 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0ee07802a5..a7e4e286aa 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -64,6 +64,7 @@
 #include "yank_functions.h"
 #include "sysemu/qtest.h"
 #include "options.h"
+#include "qemu/sockets.h"
 
 static NotifierList migration_state_notifiers =
     NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
@@ -408,13 +409,58 @@ void migrate_add_address(SocketAddress *address)
                       QAPI_CLONE(SocketAddress, address));
 }
 
+static bool migrate_uri_parse(const char *uri,
+                              MigrateAddress **channel,
+                              Error **errp)
+{
+    Error *local_err = NULL;
+    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
+    SocketAddress *saddr;
+    InetSocketAddress *isock = &addrs->u.rdma;
+    strList **tail = &addrs->u.exec.args;
+
+    if (strstart(uri, "exec:", NULL)) {
+        addrs->transport = MIGRATE_TRANSPORT_EXEC;
+        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
+        QAPI_LIST_APPEND(tail, g_strdup("-c"));
+        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
+    } else if (strstart(uri, "rdma:", NULL) &&
+               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
+        addrs->transport = MIGRATE_TRANSPORT_RDMA;
+    } else if (strstart(uri, "tcp:", NULL) ||
+                strstart(uri, "unix:", NULL) ||
+                strstart(uri, "vsock:", NULL) ||
+                strstart(uri, "fd:", NULL)) {
+        addrs->transport = MIGRATE_TRANSPORT_SOCKET;
+        saddr = socket_parse(uri, &local_err);
+        addrs->u.socket = *saddr;
+    }
+
+    if (local_err) {
+        qapi_free_MigrateAddress(addrs);
+        qapi_free_SocketAddress(saddr);
+        qapi_free_InetSocketAddress(isock);
+        error_propagate(errp, local_err);
+        return false;
+    }
+
+    *channel = addrs;
+    return true;
+}
+
 static void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p = NULL;
+    MigrateAddress *channel = g_new0(MigrateAddress, 1);
 
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
-        return;
+        goto out;
+    }
+
+    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
+        error_setg(errp, "Error parsing uri");
+        goto out;
     }
 
     qapi_event_send_migration(MIGRATION_STATUS_SETUP);
@@ -433,6 +479,9 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
     } else {
         error_setg(errp, "unknown migration protocol: %s", uri);
     }
+
+out:
+    qapi_free_MigrateAddress(channel);
 }
 
 static void process_incoming_migration_bh(void *opaque)
@@ -1638,10 +1687,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
     const char *p = NULL;
+    MigrateAddress *channel = g_new0(MigrateAddress, 1);
 
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
-        return;
+        goto out;
+    }
+
+    if (!migrate_uri_parse(uri, &channel, &local_err)) {
+        error_setg(errp, "Error parsing uri");
+        goto out;
     }
 
     if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
@@ -1688,6 +1743,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         error_propagate(errp, local_err);
         return;
     }
+
+out:
+    qapi_free_MigrateAddress(channel);
+    return;
 }
 
 void qmp_migrate_cancel(Error **errp)
-- 
2.22.3

[PATCH v4 3/8] migration: converts socket backend to accept MigrateAddress struct

[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>
---
 migration/exec.c      |  4 ++--
 migration/exec.h      |  4 ++++
 migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
 migration/socket.c    | 34 +++++----------------------------
 migration/socket.h    |  7 ++++---
 5 files changed, 47 insertions(+), 46 deletions(-)

diff --git a/migration/exec.c b/migration/exec.c
index 2bf882bbe1..c4a3293246 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);
@@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
 }
 #endif
 
-void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
+void exec_start_outgoing_migration(MigrationState *s, const char *command,
+                                   Error **errp)
 {
     QIOChannel *ioc;
 
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 a7e4e286aa..61f52d2f90 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -421,7 +421,11 @@ static bool migrate_uri_parse(const char *uri,
 
     if (strstart(uri, "exec:", NULL)) {
         addrs->transport = MIGRATE_TRANSPORT_EXEC;
+#ifdef WIN32
+        QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
+#else
         QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
+#endif
         QAPI_LIST_APPEND(tail, g_strdup("-c"));
         QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
     } else if (strstart(uri, "rdma:", NULL) &&
@@ -450,8 +454,10 @@ static bool migrate_uri_parse(const char *uri,
 
 static void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
+    Error *local_err = NULL;
     const char *p = NULL;
     MigrateAddress *channel = g_new0(MigrateAddress, 1);
+    SocketAddress *saddr;
 
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
@@ -463,23 +469,32 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
         goto out;
     }
 
+    saddr = &channel->u.socket;
     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 == MIGRATE_TRANSPORT_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, &local_err);
+        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
+            fd_start_incoming_migration(saddr->u.fd.str, &local_err);
+        }
 #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);
     }
 
+    if (local_err) {
+        qapi_free_SocketAddress(saddr);
+        error_propagate(errp, local_err);
+        return;
+    }
+
 out:
     qapi_free_MigrateAddress(channel);
 }
@@ -1688,6 +1703,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     MigrationState *s = migrate_get_current();
     const char *p = NULL;
     MigrateAddress *channel = g_new0(MigrateAddress, 1);
+    SocketAddress *saddr;
 
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
@@ -1711,18 +1727,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);
+    saddr = &channel->u.socket;
+    if (channel->transport == MIGRATE_TRANSPORT_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 {
         if (!(has_resume && resume)) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
@@ -1739,6 +1758,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         if (!(has_resume && resume)) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         }
+        qapi_free_SocketAddress(saddr);
         migrate_fd_error(s, local_err);
         error_propagate(errp, local_err);
         return;
diff --git a/migration/socket.c b/migration/socket.c
index 1b6f5baefb..8e7430b266 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -108,10 +108,9 @@ 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);
@@ -135,18 +134,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 +159,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 +199,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 v4 4/8] migration: converts rdma backend to accept MigrateAddress struct

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

diff --git a/migration/migration.c b/migration/migration.c
index 61f52d2f90..b7c3b939d5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -480,8 +480,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
             fd_start_incoming_migration(saddr->u.fd.str, &local_err);
         }
 #ifdef CONFIG_RDMA
-    } else if (strstart(uri, "rdma:", &p)) {
-        rdma_start_incoming_migration(p, errp);
+    } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
+        rdma_start_incoming_migration(&channel->u.rdma, &local_err);
 #endif
     } else if (strstart(uri, "exec:", &p)) {
         exec_start_incoming_migration(p, errp);
@@ -1737,8 +1737,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 == MIGRATE_TRANSPORT_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 2cd8f1cc66..32b4b8099e 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];
 
@@ -2455,9 +2454,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;
 }
 
 
@@ -2739,28 +2736,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;
@@ -3360,10 +3346,12 @@ static int qemu_rdma_accept(RDMAContext *rdma)
                                             .private_data_len = sizeof(cap),
                                          };
     RDMAContext *rdma_return_path = NULL;
+    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
     struct rdma_cm_event *cm_event;
     struct ibv_context *verbs;
     int ret = -EINVAL;
     int idx;
+    char arr[8];
 
     ret = rdma_get_cm_event(rdma->channel, &cm_event);
     if (ret) {
@@ -3375,13 +3363,17 @@ static int qemu_rdma_accept(RDMAContext *rdma)
         goto err_rdma_dest_wait;
     }
 
+    isock->host = rdma->host;
+    sprintf(arr,"%d", rdma->port);
+    isock->port = arr;
+
     /*
      * 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;
@@ -3506,6 +3498,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
 err_rdma_dest_wait:
     rdma->error_state = ret;
     qemu_rdma_cleanup(rdma);
+    qapi_free_InetSocketAddress(isock);
+    g_free(arr);
     g_free(rdma_return_path);
     return ret;
 }
@@ -4114,7 +4108,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;
@@ -4160,13 +4155,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 v4 5/8] migration: converts exec backend to accept MigrateAddress struct.

[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>
---
 migration/exec.c      | 58 ++++++++++++++++++++++++++++++++-----------
 migration/exec.h      |  4 +--
 migration/migration.c | 10 +++-----
 3 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/migration/exec.c b/migration/exec.c
index c4a3293246..210f4e9400 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -39,22 +39,51 @@ const char *exec_get_cmd_path(void)
 }
 #endif
 
-void exec_start_outgoing_migration(MigrationState *s, const char *command,
+/* 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, const 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);
+    const char **argv = g_malloc0(length * sizeof(const char *));
 
-    trace_migration_exec_outgoing(command);
+    init_exec_array(command, argv, errp);
+    char *new_command = g_strjoinv(" ", (char **)argv);
+
+    trace_migration_exec_outgoing(new_command);
     ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
                                                     O_RDWR,
                                                     errp));
     if (!ioc) {
+        g_free(argv);
         return;
     }
 
@@ -72,21 +101,22 @@ 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);
+    const char **argv = g_malloc0(length * sizeof(const char *));
+
+    init_exec_array(command, argv, errp);
+    char *new_command = g_strjoinv(" ", (char **)argv);
 
-    trace_migration_exec_incoming(command);
+    trace_migration_exec_incoming(new_command);
     ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
                                                     O_RDWR,
                                                     errp));
     if (!ioc) {
+        g_free(argv);
         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 b7c3b939d5..6abd69df8d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -455,7 +455,6 @@ static bool migrate_uri_parse(const char *uri,
 static void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     Error *local_err = NULL;
-    const char *p = NULL;
     MigrateAddress *channel = g_new0(MigrateAddress, 1);
     SocketAddress *saddr;
 
@@ -483,8 +482,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
     } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
         rdma_start_incoming_migration(&channel->u.rdma, &local_err);
 #endif
-    } else if (strstart(uri, "exec:", &p)) {
-        exec_start_incoming_migration(p, errp);
+    } else if (channel->transport == MIGRATE_TRANSPORT_EXEC) {
+        exec_start_incoming_migration(channel->u.exec.args, &local_err);
     } else {
         error_setg(errp, "unknown migration protocol: %s", uri);
     }
@@ -1701,7 +1700,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 {
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    const char *p = NULL;
     MigrateAddress *channel = g_new0(MigrateAddress, 1);
     SocketAddress *saddr;
 
@@ -1740,8 +1738,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     } else if (channel->transport == MIGRATE_TRANSPORT_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 == MIGRATE_TRANSPORT_EXEC) {
+        exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
     } else {
         if (!(has_resume && resume)) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
-- 
2.22.3

[PATCH v4 6/8] migration: modified 'migrate' QAPI to accept 'channels' argument for migration

[Het Gala]

MigrateChannelList ideally allows to connect accross multiple interfaces.

Added MigrateChannelList struct as argument to 'migrate' qapi. Introduced
MigrateChannelList in hmp_migrate() and qmp_migrate() functions.

Future patchset series plans to include multiple MigrateChannels
for multiple interfaces to be connected. That is the reason, 'MigrateChannelList'
is the preferred choice of argument over 'MigrateChannel' and making 'migrate'
QAPI future proof.

For current patch, have just limit size of MigrateChannelList to 1 element as
a runtime check.

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 migration/migration-hmp-cmds.c | 113 ++++++++++++++++++++++++++++++++-
 migration/migration.c          |  17 ++++-
 qapi/migration.json            |  69 +++++++++++++++++++-
 3 files changed, 192 insertions(+), 7 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 4e9f00e7dc..f098d04542 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -51,6 +51,101 @@ static void migration_global_dump(Monitor *mon)
                    ms->clear_bitmap_shift);
 }
 
+static bool
+migrate_channel_from_qdict(MigrateChannel **channel,
+                           const QDict *qdict, Error **errp)
+{
+    Error *err = NULL;
+    const char *channeltype  = qdict_get_try_str(qdict, "channeltype");
+    const char *transport_str = qdict_get_try_str(qdict, "transport");
+    const char *socketaddr_type = qdict_get_try_str(qdict, "type");
+    const char *inet_host = qdict_get_try_str(qdict, "host");
+    const char *inet_port = qdict_get_try_str(qdict, "port");
+    const char *unix_path = qdict_get_try_str(qdict, "path");
+    const char *vsock_cid = qdict_get_try_str(qdict, "cid");
+    const char *vsock_port = qdict_get_try_str(qdict, "port");
+    const char *fd = qdict_get_try_str(qdict, "str");
+    QList *exec = qdict_get_qlist(qdict, "exec");
+    MigrateChannel *val = g_new0(MigrateChannel, 1);
+    MigrateChannelType channel_type;
+    MigrateTransport transport;
+    MigrateAddress *addr = g_new0(MigrateAddress, 1);
+    SocketAddress *saddr = g_new(SocketAddress, 1);
+    SocketAddressType type;
+
+    channel_type = qapi_enum_parse(&MigrateChannelType_lookup,
+                                   channeltype, -1, &err);
+    if (channel_type < 0) {
+        goto end;
+    }
+
+    transport = qapi_enum_parse(&MigrateTransport_lookup,
+                                transport_str, -1, &err);
+    if (transport < 0) {
+        goto end;
+    }
+
+    type = qapi_enum_parse(&SocketAddressType_lookup,
+                           socketaddr_type, -1, &err);
+    if (type < 0) {
+        goto end;
+    }
+
+    addr->transport = transport;
+
+    switch (transport) {
+    case MIGRATE_TRANSPORT_SOCKET:
+        saddr->type = type;
+
+        switch (type) {
+        case SOCKET_ADDRESS_TYPE_INET:
+            saddr->u.inet.host = (char *)inet_host;
+            saddr->u.inet.port = (char *)inet_port;
+            break;
+        case SOCKET_ADDRESS_TYPE_UNIX:
+            saddr->u.q_unix.path = (char *)unix_path;
+            break;
+        case SOCKET_ADDRESS_TYPE_VSOCK:
+            saddr->u.vsock.cid = (char *)vsock_cid;
+            saddr->u.vsock.port = (char *)vsock_port;
+            break;
+        case SOCKET_ADDRESS_TYPE_FD:
+            saddr->u.fd.str = (char *)fd;
+            break;
+        default:
+            error_setg(errp, "%s: Unknown socket type %d",
+                       __func__, saddr->type);
+            goto end;
+        }
+
+        addr->u.socket = *saddr;
+        break;
+    case MIGRATE_TRANSPORT_EXEC:
+        addr->u.exec.args = (strList *)exec;
+         break;
+    case MIGRATE_TRANSPORT_RDMA:
+        addr->u.rdma.host = (char *)inet_host;
+        addr->u.rdma.port = (char *)inet_port;
+        break;
+    default:
+        error_setg(errp, "%s: Unknown migrate transport type %d",
+                   __func__, addr->transport);
+        goto end;
+    }
+
+    val->channeltype = channel_type;
+    val->addr = addr;
+    *channel = val;
+    return true;
+
+end:
+    error_propagate(errp, err);
+    qapi_free_MigrateChannel(val);
+    qapi_free_MigrateAddress(addr);
+    qapi_free_SocketAddress(saddr);
+    return false;
+}
+
 void hmp_info_migrate(Monitor *mon, const QDict *qdict)
 {
     MigrationInfo *info;
@@ -702,9 +797,18 @@ 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;
+    MigrateChannel *channel = g_new0(MigrateChannel, 1);
+    MigrateChannelList *caps = NULL;
+
+    if (!migrate_channel_from_qdict(&channel, qdict, &err)) {
+        error_setg(&err, "error in retrieving channel from qdict");
+        goto end;
+    }
 
-    qmp_migrate(uri, !!blk, blk, !!inc, inc,
-                false, false, true, resume, &err);
+    QAPI_LIST_PREPEND(caps, channel);
+    qmp_migrate(uri, !!caps, caps, !!blk, blk, !!inc, inc,
+                 false, false, true, resume, &err);
+    qapi_free_MigrateChannelList(caps);
     if (hmp_handle_error(mon, err)) {
         return;
     }
@@ -725,6 +829,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
                                           status);
         timer_mod(status->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
     }
+
+end:
+    qapi_free_MigrateChannel(channel);
+    hmp_handle_error(mon, err);
+    return;
 }
 
 void migrate_set_capability_completion(ReadLineState *rs, int nb_args,
diff --git a/migration/migration.c b/migration/migration.c
index 6abd69df8d..78f16e5041 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1694,15 +1694,26 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
     return true;
 }
 
-void qmp_migrate(const char *uri, bool has_blk, bool blk,
-                 bool has_inc, bool inc, bool has_detach, bool detach,
-                 bool has_resume, bool resume, Error **errp)
+void qmp_migrate(const char *uri, bool has_channels,
+                 MigrateChannelList *channels, bool has_blk, bool blk,
+                 bool has_inc, bool inc, bool has_detach,
+                 bool detach, bool has_resume, bool resume, Error **errp)
 {
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
     MigrateAddress *channel = g_new0(MigrateAddress, 1);
     SocketAddress *saddr;
 
+    /*
+     * Having preliminary checks for uri and channel
+     */
+    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;
+    }
+
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
         goto out;
diff --git a/qapi/migration.json b/qapi/migration.json
index bf90bd8fe2..a71af87ffe 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1464,12 +1464,47 @@
     'exec': 'MigrateExecCommand',
     'rdma': 'InetSocketAddress' } }
 
+##
+# @MigrateChannelType:
+#
+# The supported options for migration channel type requests
+#
+# @main: Support request for main outbound migration control channel
+#
+# Since 8.0
+##
+{ 'enum': 'MigrateChannelType',
+  'data': [ 'main' ] }
+
+##
+# @MigrateChannel:
+#
+# Information regarding migration Channel-type for transferring packets,
+# source and corresponding destination interface for socket connection
+# and number of multifd channels over the interface.
+#
+# @channeltype: Name of Channel type for transfering packet information
+#
+# @addr: Information regarding migration parameters of destination interface
+#
+# Since 8.0
+##
+{ 'struct': 'MigrateChannel',
+  'data': {
+       'channeltype': 'MigrateChannelType',
+       'addr': 'MigrateAddress' } }
+
 ##
 # @migrate:
 #
 # Migrates the current running guest to another Virtual Machine.
 #
 # @uri: the Uniform Resource Identifier of the destination VM
+#       for migration thread
+#
+# @channels: Struct containing list of migration channel types, with all
+#            the information regarding destination interfaces required for
+#            initiating a migration stream.
 #
 # @blk: do block migration (full disk copy)
 #
@@ -1494,15 +1529,45 @@
 # 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. The 'uri' and 'channel' 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": [ { "channeltype": "main",
+#                          "addr": { "transport": "socket", "type": "inet",
+#                                    "host": "10.12.34.9",
+#                                    "port": "1050" } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channeltype": "main",
+#                          "addr": { "transport": "exec",
+#                                    "args": [ "/bin/nc", "-p", "6000",
+#                                              "/some/sock" ] } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channeltype": "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': [ 'MigrateChannel' ], '*blk': 'bool',
+           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
 
 ##
 # @migrate-incoming:
-- 
2.22.3

[PATCH v4 7/8] migration: modified 'migrate-incoming' QAPI to accept 'channels' argument for migration.

[Het Gala]

MigrateChannelList ideally allows to have multiple listener interfaces up for
connection.

Added MigrateChannelList struct as argument to 'migrate-incoming' qapi. Introduced
MigrateChannelList in hmp_migrate_incoming() and qmp_migrate_incoming() functions.

Future patchset series plans to include multiple MigrateChannels to have multiple
listening interfaces up. That is the reason, 'MigrateChannelList' is the preferred
choice of argument over 'MigrateChannel' and making 'migrate-incoming' QAPI future
proof.

For current patch, have just limit size of MigrateChannelList to 1 element as
a runtime check.

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 migration/migration-hmp-cmds.c | 14 +++++++++++++-
 migration/migration.c          | 21 ++++++++++++++++----
 qapi/migration.json            | 35 +++++++++++++++++++++++++++++++++-
 softmmu/vl.c                   |  2 +-
 4 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index f098d04542..8c11a8c83b 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -518,10 +518,22 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
     const char *uri = qdict_get_str(qdict, "uri");
+    MigrateChannel *channel = g_new0(MigrateChannel, 1);
+    MigrateChannelList *caps = NULL;
+
+    if (!migrate_channel_from_qdict(&channel, qdict, &err)) {
+        error_setg(&err, "error in retrieving channel from qdict");
+        goto end;
+    }
 
-    qmp_migrate_incoming(uri, &err);
+    QAPI_LIST_PREPEND(caps, channel);
+    qmp_migrate_incoming(uri, !!caps, caps, &err);
+    qapi_free_MigrateChannelList(caps);
 
+end:
+    qapi_free_MigrateChannel(channel);
     hmp_handle_error(mon, err);
+    return;
 }
 
 void hmp_migrate_recover(Monitor *mon, const QDict *qdict)
diff --git a/migration/migration.c b/migration/migration.c
index 78f16e5041..de058643a6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -452,12 +452,24 @@ 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,
+                                          MigrateChannelList *channels,
+                                          Error **errp)
 {
     Error *local_err = NULL;
     MigrateAddress *channel = g_new0(MigrateAddress, 1);
     SocketAddress *saddr;
 
+    /*
+     * Having preliminary checks for uri and channel
+     */
+    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;
+    }
+
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
         goto out;
@@ -1507,7 +1519,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,
+                          MigrateChannelList *channels, Error **errp)
 {
     Error *local_err = NULL;
     static bool once = true;
@@ -1525,7 +1538,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);
@@ -1561,7 +1574,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)
diff --git a/qapi/migration.json b/qapi/migration.json
index a71af87ffe..9faecdd048 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1578,6 +1578,10 @@
 # @uri: The Uniform Resource Identifier identifying the source or
 #       address to listen on
 #
+# @channels: Struct containing list of migration channel types, with all
+#            the information regarding destination interfaces required for
+#            initiating a migration stream.
+#
 # Returns: nothing on success
 #
 # Since: 2.3
@@ -1593,14 +1597,43 @@
 #
 # 3. The uri format is the same as for -incoming
 #
+# 4. The 'uri' and 'channel' 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": [ { "channeltype": "main",
+#                          "addr": { "transport": "socket", "type": "inet",
+#                                    "host": "10.12.34.9",
+#                                    "port": "1050" } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channeltype": "main",
+#                          "addr": { "transport": "exec",
+#                                    "args": [ "/bin/nc", "-p", "6000",
+#                                              "/some/sock" ] } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channeltype": "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': [ 'MigrateChannel' ] } }
 
 ##
 # @xen-save-devices-state:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 6c2427262b..ade411eb4f 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2633,7 +2633,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 v4 8/8] migration: Introduced MigrateChannelList struct to migration code flow.

[Het Gala]

Integrated MigrateChannelList with all transport backends (socket, exec
and rdma) for both source and destination migration code flow.

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 migration/migration.c | 95 +++++++++++++++++++++++++++----------------
 migration/socket.c    |  5 ++-
 2 files changed, 64 insertions(+), 36 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index de058643a6..a37eba29e3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -410,10 +410,11 @@ void migrate_add_address(SocketAddress *address)
 }
 
 static bool migrate_uri_parse(const char *uri,
-                              MigrateAddress **channel,
+                              MigrateChannel **channel,
                               Error **errp)
 {
     Error *local_err = NULL;
+    MigrateChannel *val = g_new0(MigrateChannel, 1);
     MigrateAddress *addrs = g_new0(MigrateAddress, 1);
     SocketAddress *saddr;
     InetSocketAddress *isock = &addrs->u.rdma;
@@ -441,6 +442,7 @@ static bool migrate_uri_parse(const char *uri,
     }
 
     if (local_err) {
+        qapi_free_MigrateChannel(val);
         qapi_free_MigrateAddress(addrs);
         qapi_free_SocketAddress(saddr);
         qapi_free_InetSocketAddress(isock);
@@ -448,7 +450,9 @@ static bool migrate_uri_parse(const char *uri,
         return false;
     }
 
-    *channel = addrs;
+    val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
+    val->addr = addrs;
+    *channel = val;
     return true;
 }
 
@@ -457,8 +461,9 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
                                           Error **errp)
 {
     Error *local_err = NULL;
-    MigrateAddress *channel = g_new0(MigrateAddress, 1);
+    MigrateAddress *addrs;
     SocketAddress *saddr;
+    MigrateChannel *channel = NULL;
 
     /*
      * Having preliminary checks for uri and channel
@@ -467,22 +472,30 @@ static void qemu_start_incoming_migration(const char *uri, bool 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;
-    }
-
-    /* URI is not suitable for migration? */
-    if (!migration_channels_and_uri_compatible(uri, errp)) {
         goto out;
-    }
+    } 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");
+            goto out;
+        }
+        channel = channels->value;
+    } else {
+        /* URI is not suitable for migration? */
+        if (!migration_channels_and_uri_compatible(uri, errp)) {
+            goto out;
+        }
 
-    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
-        error_setg(errp, "Error parsing uri");
-        goto out;
+        if (uri && !migrate_uri_parse(uri, &channel, errp)) {
+            error_setg(errp, "Error parsing uri");
+            goto out;
+        }
     }
 
-    saddr = &channel->u.socket;
+    addrs = channel->addr;
+    saddr = &channel->addr->u.socket;
     qapi_event_send_migration(MIGRATION_STATUS_SETUP);
-    if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
+    if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
         if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
             saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
             saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
@@ -491,23 +504,25 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
             fd_start_incoming_migration(saddr->u.fd.str, &local_err);
         }
 #ifdef CONFIG_RDMA
-    } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
-        rdma_start_incoming_migration(&channel->u.rdma, &local_err);
+    } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
+        rdma_start_incoming_migration(&addrs->u.rdma, &local_err);
 #endif
-    } else if (channel->transport == MIGRATE_TRANSPORT_EXEC) {
-        exec_start_incoming_migration(channel->u.exec.args, &local_err);
+    } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
+        exec_start_incoming_migration(addrs->u.exec.args, &local_err);
     } else {
         error_setg(errp, "unknown migration protocol: %s", uri);
     }
 
     if (local_err) {
+        qapi_free_MigrateAddress(addrs);
         qapi_free_SocketAddress(saddr);
         error_propagate(errp, local_err);
         return;
     }
 
 out:
-    qapi_free_MigrateAddress(channel);
+    qapi_free_MigrateChannel(channel);
+    return;
 }
 
 static void process_incoming_migration_bh(void *opaque)
@@ -1714,8 +1729,9 @@ void qmp_migrate(const char *uri, bool has_channels,
 {
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    MigrateAddress *channel = g_new0(MigrateAddress, 1);
+    MigrateAddress *addrs;
     SocketAddress *saddr;
+    MigrateChannel *channel = NULL;
 
     /*
      * Having preliminary checks for uri and channel
@@ -1724,17 +1740,24 @@ void qmp_migrate(const char *uri, bool 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;
-    }
-
-    /* URI is not suitable for migration? */
-    if (!migration_channels_and_uri_compatible(uri, errp)) {
         goto out;
-    }
+    } 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");
+            goto out;
+        }
+        channel = channels->value;
+    } else {
+        /* URI is not suitable for migration? */
+        if (!migration_channels_and_uri_compatible(uri, errp)) {
+            goto out;
+        }
 
-    if (!migrate_uri_parse(uri, &channel, &local_err)) {
-        error_setg(errp, "Error parsing uri");
-        goto out;
+        if (!migrate_uri_parse(uri, &channel, &local_err)) {
+            error_setg(errp, "Error parsing uri");
+            goto out;
+        }
     }
 
     if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
@@ -1749,8 +1772,9 @@ void qmp_migrate(const char *uri, bool has_channels,
         }
     }
 
-    saddr = &channel->u.socket;
-    if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
+    addrs = channel->addr;
+    saddr = &channel->addr->u.socket;
+    if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
         if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
             saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
             saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
@@ -1759,11 +1783,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 == MIGRATE_TRANSPORT_RDMA) {
-        rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
+    } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
+        rdma_start_outgoing_migration(s, &addrs->u.rdma, &local_err);
 #endif
-    } else if (channel->transport == MIGRATE_TRANSPORT_EXEC) {
-        exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
+    } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
+        exec_start_outgoing_migration(s, addrs->u.exec.args, &local_err);
     } else {
         if (!(has_resume && resume)) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
@@ -1780,6 +1804,7 @@ void qmp_migrate(const char *uri, bool has_channels,
         if (!(has_resume && resume)) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         }
+        qapi_free_MigrateAddress(addrs);
         qapi_free_SocketAddress(saddr);
         migrate_fd_error(s, local_err);
         error_propagate(errp, local_err);
diff --git a/migration/socket.c b/migration/socket.c
index 8e7430b266..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;
@@ -114,12 +116,13 @@ void socket_start_outgoing_migration(MigrationState *s,
 {
     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);
-- 
2.22.3

Re: [PATCH v4 1/8] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> This patch introduces well defined MigrateAddress struct and its related child
> objects.
>
> The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
> string type. The current migration flow follows double encoding scheme for
> fetching migration parameters such as '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.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 82000adce4..bf90bd8fe2 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1423,6 +1423,47 @@
>  ##
>  { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>  
> +##
> +# @MigrateTransport:
> +#
> +# The supported communication transport mechanisms for migration
> +#
> +# @socket: Supported communication type between two devices for migration.
> +#          Socket is able to cover all of 'tcp', 'unix', 'vsock' and
> +#          'fd' already
> +#
> +# @exec: Supported communication type to redirect migration stream into file.
> +#
> +# @rdma: Supported communication type to redirect rdma type migration stream.
> +#
> +# Since 8.0

8.1

I can fix that if it gets acked from someone from QAPI.

> +##
> +{ 'enum': 'MigrateTransport',
> +  'data': ['socket', 'exec', 'rdma'] }
> +
> +##
> +# @MigrateExecCommand:
> + #
> + # Since 8.0
> + ##
> +{ 'struct': 'MigrateExecCommand',
> +   'data': {'args': [ 'str' ] } }
> +
> +##
> +# @MigrateAddress:
> +#
> +# The options available for communication transport mechanisms for migration
> +#
> +# Since 8.0
> +##
> +{ 'union': 'MigrateAddress',
> +  'base': { 'transport' : 'MigrateTransport'},
> +  'discriminator': 'transport',
> +  'data': {
> +    'socket': 'SocketAddress',
> +    'exec': 'MigrateExecCommand',
> +    'rdma': 'InetSocketAddress' } }
> +
>  ##
>  # @migrate:
>  #

I will wait for a comment from QAPI people, from migration point of
view:

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

Re: [PATCH v4 2/8] migration: Converts uri parameter into 'MigrateAddress' struct

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> This patch introduces code that can parse 'uri' string parameter and
> spit out 'MigrateAddress' struct. All the required migration parameters
> are stored in the struct.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>

Markus, could you comment?

> ---
>  migration/migration.c | 63 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 0ee07802a5..a7e4e286aa 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -64,6 +64,7 @@
>  #include "yank_functions.h"
>  #include "sysemu/qtest.h"
>  #include "options.h"
> +#include "qemu/sockets.h"
>  
>  static NotifierList migration_state_notifiers =
>      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> @@ -408,13 +409,58 @@ void migrate_add_address(SocketAddress *address)
>                        QAPI_CLONE(SocketAddress, address));
>  }
>  
> +static bool migrate_uri_parse(const char *uri,
> +                              MigrateAddress **channel,
> +                              Error **errp)
> +{
> +    Error *local_err = NULL;

Remove this.

> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr;
> +    InetSocketAddress *isock = &addrs->u.rdma;
> +    strList **tail = &addrs->u.exec.args;
> +
> +    if (strstart(uri, "exec:", NULL)) {
> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
> +        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> +        QAPI_LIST_APPEND(tail, g_strdup("-c"));
> +        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> +    } else if (strstart(uri, "rdma:", NULL) &&
> +               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
> +    } else if (strstart(uri, "tcp:", NULL) ||
> +                strstart(uri, "unix:", NULL) ||
> +                strstart(uri, "vsock:", NULL) ||
> +                strstart(uri, "fd:", NULL)) {
> +        addrs->transport = MIGRATE_TRANSPORT_SOCKET;
> +        saddr = socket_parse(uri, &local_err);

           saddr = socket_parse(uri, &local_err);

           if (saddr == NULL) {
                   qapi_free_MigrateAddress(addrs);
                   return false;
           }

> +        addrs->u.socket = *saddr;
> +    }
> +
> +    if (local_err) {
> +        qapi_free_SocketAddress(saddr);
> +        qapi_free_InetSocketAddress(isock);
> +        error_propagate(errp, local_err);
> +        return false;
> +    }

And remove this last chunk?
Or I am missing something?

> +
> +    *channel = addrs;
> +    return true;
> +}
> +
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p = NULL;
> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);
>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
> -        return;
> +        goto out;
> +    }
> +
> +    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> +        error_setg(errp, "Error parsing uri");

Why are we doing this error_setg?  We are supposed to have a better
error assigned to errp already, no?

> +        goto out;
>      }
>  
>      qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> @@ -433,6 +479,9 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>      } else {
>          error_setg(errp, "unknown migration protocol: %s", uri);
>      }
> +
> +out:
> +    qapi_free_MigrateAddress(channel);

I wish, I really wish, that there was a way to free things on error.


>  }
>  
>  static void process_incoming_migration_bh(void *opaque)
> @@ -1638,10 +1687,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
>      const char *p = NULL;
> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);
>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
> -        return;
> +        goto out;
> +    }
> +
> +    if (!migrate_uri_parse(uri, &channel, &local_err)) {
> +        error_setg(errp, "Error parsing uri");

Same here about setting errp.

> +        goto out;
>      }
>  
>      if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
> @@ -1688,6 +1743,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          error_propagate(errp, local_err);
>          return;
>      }
> +
> +out:
> +    qapi_free_MigrateAddress(channel);
> +    return;
>  }
>  
>  void qmp_migrate_cancel(Error **errp)

Re: [PATCH v4 3/8] migration: converts socket backend to accept MigrateAddress struct

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> 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>
> ---
>  migration/exec.c      |  4 ++--
>  migration/exec.h      |  4 ++++
>  migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
>  migration/socket.c    | 34 +++++----------------------------
>  migration/socket.h    |  7 ++++---
>  5 files changed, 47 insertions(+), 46 deletions(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index 2bf882bbe1..c4a3293246 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);

Make this and related chunks it its own patch.


> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
>  }
>  #endif
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
> +void exec_start_outgoing_migration(MigrationState *s, const char *command,
> +                                   Error **errp)
>  {
>      QIOChannel *ioc;
>  

Drop this bit.  If you want to cleanup longer that 80 chars lines, do it
in a sperate patch.


> +    if (local_err) {
> +        qapi_free_SocketAddress(saddr);
> +        error_propagate(errp, local_err);
> +        return;
> +    }

Aha, I see what you want to do here, but I 

From include/qapi/error.h

 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.
 *


I think that what you have to do here is drop local_err and just change

socket_start_incoming_migration() and fd_start_incoming_migration() to
return a bool.

> -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);
> -}

And here we are.  This function got it wrong, and you copied from here.
My understanding is that this function should have been written as:

void socket_start_outgoing_migration(MigrationState *s,
                                     const char *str,
                                     Error **errp)
{
    SocketAddress *saddr = socket_parse(str, &err);
    if (!saddr) {
        socket_start_outgoing_migration_internal(s, saddr, &err);
    }
}


> -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);
> -}

Exactly the same here.

Not your fault, but can you do the changes, please?

Later, Juan.

Re: [PATCH v4 4/8] migration: converts rdma backend to accept MigrateAddress struct

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> 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: Juan Quintela <quintela@redhat.com>

Re: [PATCH v4 5/8] migration: converts exec backend to accept MigrateAddress struct.

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> 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>
> ---
>  migration/exec.c      | 58 ++++++++++++++++++++++++++++++++-----------
>  migration/exec.h      |  4 +--
>  migration/migration.c | 10 +++-----
>  3 files changed, 50 insertions(+), 22 deletions(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index c4a3293246..210f4e9400 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -39,22 +39,51 @@ const char *exec_get_cmd_path(void)
>  }
>  #endif
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *command,
> +/* 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;
> +}

I can't believe tat we have a list type and we don't have a length()
function for that type.

Sniff.

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

Re: [PATCH v4 7/8] migration: modified 'migrate-incoming' QAPI to accept 'channels' argument for migration.

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> MigrateChannelList ideally allows to have multiple listener interfaces up for
> connection.
>
> Added MigrateChannelList struct as argument to 'migrate-incoming' qapi. Introduced
> MigrateChannelList in hmp_migrate_incoming() and qmp_migrate_incoming() functions.
>
> Future patchset series plans to include multiple MigrateChannels to have multiple
> listening interfaces up. That is the reason, 'MigrateChannelList' is the preferred
> choice of argument over 'MigrateChannel' and making 'migrate-incoming' QAPI future
> proof.
>
> For current patch, have just limit size of MigrateChannelList to 1 element as
> a runtime check.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/migration-hmp-cmds.c | 14 +++++++++++++-
>  migration/migration.c          | 21 ++++++++++++++++----
>  qapi/migration.json            | 35 +++++++++++++++++++++++++++++++++-
>  softmmu/vl.c                   |  2 +-
>  4 files changed, 65 insertions(+), 7 deletions(-)
>
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index f098d04542..8c11a8c83b 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -518,10 +518,22 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
>      const char *uri = qdict_get_str(qdict, "uri");
> +    MigrateChannel *channel = g_new0(MigrateChannel, 1);
> +    MigrateChannelList *caps = NULL;
> +
> +    if (!migrate_channel_from_qdict(&channel, qdict, &err)) {
> +        error_setg(&err, "error in retrieving channel from qdict");
> +        goto end;
> +    }
>  
> -    qmp_migrate_incoming(uri, &err);
> +    QAPI_LIST_PREPEND(caps, channel);
> +    qmp_migrate_incoming(uri, !!caps, caps, &err);
> +    qapi_free_MigrateChannelList(caps);
>  
> +end:
> +    qapi_free_MigrateChannel(channel);
>      hmp_handle_error(mon, err);
> +    return;

Useless return?


The rest seeams ok.

Re: [PATCH v4 8/8] migration: Introduced MigrateChannelList struct to migration code flow.

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> Integrated MigrateChannelList with all transport backends (socket, exec
> and rdma) for both source and destination migration code flow.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>

Nothing especially wrong appears here, will wait for 2nd submission with
the previous fixes done (specially the local_error bits).

And in another order of events, you are not changing
tests/qtest/migration-test.c

My suggestion will be that for things that have more than one test
(i.e. tcp/unix/...) just change some of them to use the new syntax for
channels.

What do you think?

Later, Juan.

Re: [PATCH v4 1/8] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.

[Daniel P. Berrangé]

On Fri, May 12, 2023 at 02:32:33PM +0000, Het Gala wrote:
> This patch introduces well defined MigrateAddress struct and its related child
> objects.
> 
> The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
> string type. The current migration flow follows double encoding scheme for
> fetching migration parameters such as '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.
> 
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 v4 2/8] migration: Converts uri parameter into 'MigrateAddress' struct

[Daniel P. Berrangé]

On Fri, May 12, 2023 at 02:32:34PM +0000, Het Gala wrote:
> This patch introduces code that can parse 'uri' string parameter and
> spit out 'MigrateAddress' struct. All the required migration parameters
> are stored in the struct.
> 
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/migration.c | 63 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0ee07802a5..a7e4e286aa 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -64,6 +64,7 @@
>  #include "yank_functions.h"
>  #include "sysemu/qtest.h"
>  #include "options.h"
> +#include "qemu/sockets.h"
>  
>  static NotifierList migration_state_notifiers =
>      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> @@ -408,13 +409,58 @@ void migrate_add_address(SocketAddress *address)
>                        QAPI_CLONE(SocketAddress, address));
>  }
>  
> +static bool migrate_uri_parse(const char *uri,
> +                              MigrateAddress **channel,
> +                              Error **errp)
> +{
> +    Error *local_err = NULL;
> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr;
> +    InetSocketAddress *isock = &addrs->u.rdma;
> +    strList **tail = &addrs->u.exec.args;
> +
> +    if (strstart(uri, "exec:", NULL)) {
> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
> +        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> +        QAPI_LIST_APPEND(tail, g_strdup("-c"));
> +        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> +    } else if (strstart(uri, "rdma:", NULL) &&
> +               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;

I would have this as 

    } else if (strstart(uri, "rdma:", NULL)) {
        if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
            addrs->transport = MIGRATE_TRANSPORT_RDMA;
	}

as IMHO it is bad practice to have control pass to the next
else if clause when inet_parse() fails, as we know this is
only an RDMA addr

Also you need to use '&local_err' not 'errp' in the inet_parse
call, otherwise the later code block for cleanup won't run.


> +    } else if (strstart(uri, "tcp:", NULL) ||
> +                strstart(uri, "unix:", NULL) ||
> +                strstart(uri, "vsock:", NULL) ||
> +                strstart(uri, "fd:", NULL)) {
> +        addrs->transport = MIGRATE_TRANSPORT_SOCKET;
> +        saddr = socket_parse(uri, &local_err);
> +        addrs->u.socket = *saddr;

Protect with

   if (saddr != NULL) {
       addrs->u.socket = *saddr;
   }

> +    }
> +
> +    if (local_err) {
> +        qapi_free_MigrateAddress(addrs);
> +        qapi_free_SocketAddress(saddr);
> +        qapi_free_InetSocketAddress(isock);
> +        error_propagate(errp, local_err);
> +        return false;
> +    }
> +
> +    *channel = addrs;
> +    return true;
> +}
> +
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p = NULL;
> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);

Avoid the later 'out:' cleanup block by using:

  g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);

>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
> -        return;
> +        goto out;
> +    }
> +
> +    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> +        error_setg(errp, "Error parsing uri");

This error_setg() is overwriting the error migrate_uri_parse already
set, so just drop it.

> +        goto out;
>      }
>  
>      qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> @@ -433,6 +479,9 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>      } else {
>          error_setg(errp, "unknown migration protocol: %s", uri);
>      }
> +
> +out:
> +    qapi_free_MigrateAddress(channel);
>  }
>  
>  static void process_incoming_migration_bh(void *opaque)
> @@ -1638,10 +1687,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
>      const char *p = NULL;
> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);

Here too, use g_autoptr(MigrateAddress) and drop the  'out:' block

>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
> -        return;
> +        goto out;
> +    }
> +
> +    if (!migrate_uri_parse(uri, &channel, &local_err)) {
> +        error_setg(errp, "Error parsing uri");
> +        goto out;
>      }
>  
>      if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
> @@ -1688,6 +1743,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          error_propagate(errp, local_err);
>          return;
>      }
> +
> +out:
> +    qapi_free_MigrateAddress(channel);
> +    return;
>  }
>  
>  void qmp_migrate_cancel(Error **errp)
> -- 
> 2.22.3
> 

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 v4 3/8] migration: converts socket backend to accept MigrateAddress struct

[Daniel P. Berrangé]

On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
> 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>
> ---
>  migration/exec.c      |  4 ++--
>  migration/exec.h      |  4 ++++
>  migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
>  migration/socket.c    | 34 +++++----------------------------
>  migration/socket.h    |  7 ++++---
>  5 files changed, 47 insertions(+), 46 deletions(-)
> 
> diff --git a/migration/exec.c b/migration/exec.c
> index 2bf882bbe1..c4a3293246 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);
> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
>  }
>  #endif
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
> +void exec_start_outgoing_migration(MigrationState *s, const char *command,
> +                                   Error **errp)
>  {
>      QIOChannel *ioc;
>  
> 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 a7e4e286aa..61f52d2f90 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -421,7 +421,11 @@ static bool migrate_uri_parse(const char *uri,
>  
>      if (strstart(uri, "exec:", NULL)) {
>          addrs->transport = MIGRATE_TRANSPORT_EXEC;
> +#ifdef WIN32
> +        QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
> +#else
>          QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> +#endif

This windows portability code should have been in the previous patch
I think.

>          QAPI_LIST_APPEND(tail, g_strdup("-c"));
>          QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
>      } else if (strstart(uri, "rdma:", NULL) &&
> @@ -450,8 +454,10 @@ static bool migrate_uri_parse(const char *uri,
>  
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
> +    Error *local_err = NULL;
>      const char *p = NULL;
>      MigrateAddress *channel = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr;
>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
> @@ -463,23 +469,32 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>          goto out;
>      }
>  
> +    saddr = &channel->u.socket;

Accessing u.socket before checkout transport == SOCKET is bad
practice, even though this is technically safe.

>      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 == MIGRATE_TRANSPORT_SOCKET) {

THis should have

    SocketAddress *saddr = &channe->u.socket

so that 'saddr' is limited in scope to where we've validated
transport == 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, &local_err);
> +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> +            fd_start_incoming_migration(saddr->u.fd.str, &local_err);
> +        }
>  #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);
>      }
>  
> +    if (local_err) {
> +        qapi_free_SocketAddress(saddr);
> +        error_propagate(errp, local_err);
> +        return;

THis leaks 'channel', and free's 'saddr' which actually  belongs
to channel.

With my comments on the previous patch suggesting g_autoptr for
'channel', we don't need any free calls for 'saddr' or 'channel'.

> +    }
> +
>  out:
>      qapi_free_MigrateAddress(channel);
>  }
> @@ -1688,6 +1703,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      MigrationState *s = migrate_get_current();
>      const char *p = NULL;
>      MigrateAddress *channel = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr;
>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
> @@ -1711,18 +1727,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);
> +    saddr = &channel->u.socket;

Again, put this *after*  checking transport == SOCKET

> +    if (channel->transport == MIGRATE_TRANSPORT_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 {
>          if (!(has_resume && resume)) {
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> @@ -1739,6 +1758,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          if (!(has_resume && resume)) {
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          }
> +        qapi_free_SocketAddress(saddr);

This saddr pointer belongs to 'channel' which must be freed.

>          migrate_fd_error(s, local_err);
>          error_propagate(errp, local_err);
>          return;
> diff --git a/migration/socket.c b/migration/socket.c
> index 1b6f5baefb..8e7430b266 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -108,10 +108,9 @@ 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);
> @@ -135,18 +134,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 +159,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 +199,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
> 

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 v4 4/8] migration: converts rdma backend to accept MigrateAddress struct

[Daniel P. Berrangé]

On Fri, May 12, 2023 at 02:32:36PM +0000, Het Gala wrote:
> 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>
> ---
>  migration/migration.c |  8 ++++----
>  migration/rdma.c      | 38 ++++++++++++++++----------------------
>  migration/rdma.h      |  6 ++++--
>  3 files changed, 24 insertions(+), 28 deletions(-)
> 

> @@ -3360,10 +3346,12 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>                                              .private_data_len = sizeof(cap),
>                                           };
>      RDMAContext *rdma_return_path = NULL;
> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
>      struct rdma_cm_event *cm_event;
>      struct ibv_context *verbs;
>      int ret = -EINVAL;
>      int idx;
> +    char arr[8];
>  
>      ret = rdma_get_cm_event(rdma->channel, &cm_event);
>      if (ret) {
> @@ -3375,13 +3363,17 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>          goto err_rdma_dest_wait;
>      }
>  
> +    isock->host = rdma->host;
> +    sprintf(arr,"%d", rdma->port);
> +    isock->port = arr;

While Inet ports are 16-bit, and so 65535 fits in a char[8], nothing
at the QAPI parser level is enforcing this.

IOW, someone can pass QEMU a QAPI config with port = 235252353253253253232
and casue this sprintf to smash the stack.

Also this is assigning a stack variable to isock->port which
expects a heap variable. qapi_free_InetSocketAddress() will
call free(isock->port) which will again crash.

Just do

  g_autoptr(InetSocketAddress) isock = g_new0(InetSocketAddress, 1);

  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;
> @@ -3506,6 +3498,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>  err_rdma_dest_wait:
>      rdma->error_state = ret;
>      qemu_rdma_cleanup(rdma);
> +    qapi_free_InetSocketAddress(isock);
> +    g_free(arr);

Free'ing a stack variable 

>      g_free(rdma_return_path);
>      return ret;
>  }
> @@ -4114,7 +4108,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;
> @@ -4160,13 +4155,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
> 

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 v4 5/8] migration: converts exec backend to accept MigrateAddress struct.

[Daniel P. Berrangé]

On Fri, May 12, 2023 at 02:32:37PM +0000, Het Gala wrote:
> 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>
> ---
>  migration/exec.c      | 58 ++++++++++++++++++++++++++++++++-----------
>  migration/exec.h      |  4 +--
>  migration/migration.c | 10 +++-----
>  3 files changed, 50 insertions(+), 22 deletions(-)
> 
> diff --git a/migration/exec.c b/migration/exec.c
> index c4a3293246..210f4e9400 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -39,22 +39,51 @@ const char *exec_get_cmd_path(void)
>  }
>  #endif
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *command,
> +/* 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, const 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);
> +    const char **argv = g_malloc0(length * sizeof(const char *));

g_malloc0 is almost never desirable to use, instead:

        g_new0(const char *, length);

>  
> -    trace_migration_exec_outgoing(command);
> +    init_exec_array(command, argv, errp);
> +    char *new_command = g_strjoinv(" ", (char **)argv);

Never freed - use

   g_autofree char *new_command...
   
> +    trace_migration_exec_outgoing(new_command);


>      ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
>                                                      O_RDWR,
>                                                      errp));
>      if (!ioc) {
> +        g_free(argv);
>          return;
>      }

argv needs freeing in success too. Simpler to declare it
with

    g_auto(GStrv) argv = .....



>  
> @@ -72,21 +101,22 @@ 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);
> +    const char **argv = g_malloc0(length * sizeof(const char *));
> +
> +    init_exec_array(command, argv, errp);
> +    char *new_command = g_strjoinv(" ", (char **)argv);
>  
> -    trace_migration_exec_incoming(command);
> +    trace_migration_exec_incoming(new_command);
>      ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
>                                                      O_RDWR,
>                                                      errp));
>      if (!ioc) {
> +        g_free(argv);
>          return;
>      }

All the same comments as the outgoing case.

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 v4 6/8] migration: modified 'migrate' QAPI to accept 'channels' argument for migration

[Daniel P. Berrangé]

On Fri, May 12, 2023 at 02:32:38PM +0000, Het Gala wrote:
> MigrateChannelList ideally allows to connect accross multiple interfaces.
> 
> Added MigrateChannelList struct as argument to 'migrate' qapi. Introduced
> MigrateChannelList in hmp_migrate() and qmp_migrate() functions.
> 
> Future patchset series plans to include multiple MigrateChannels
> for multiple interfaces to be connected. That is the reason, 'MigrateChannelList'
> is the preferred choice of argument over 'MigrateChannel' and making 'migrate'
> QAPI future proof.
> 
> For current patch, have just limit size of MigrateChannelList to 1 element as
> a runtime check.
> 
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/migration-hmp-cmds.c | 113 ++++++++++++++++++++++++++++++++-
>  migration/migration.c          |  17 ++++-
>  qapi/migration.json            |  69 +++++++++++++++++++-
>  3 files changed, 192 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 4e9f00e7dc..f098d04542 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -51,6 +51,101 @@ static void migration_global_dump(Monitor *mon)
>                     ms->clear_bitmap_shift);
>  }
>  
> +static bool
> +migrate_channel_from_qdict(MigrateChannel **channel,
> +                           const QDict *qdict, Error **errp)
> +{
> +    Error *err = NULL;
> +    const char *channeltype  = qdict_get_try_str(qdict, "channeltype");
> +    const char *transport_str = qdict_get_try_str(qdict, "transport");
> +    const char *socketaddr_type = qdict_get_try_str(qdict, "type");
> +    const char *inet_host = qdict_get_try_str(qdict, "host");
> +    const char *inet_port = qdict_get_try_str(qdict, "port");
> +    const char *unix_path = qdict_get_try_str(qdict, "path");
> +    const char *vsock_cid = qdict_get_try_str(qdict, "cid");
> +    const char *vsock_port = qdict_get_try_str(qdict, "port");
> +    const char *fd = qdict_get_try_str(qdict, "str");
> +    QList *exec = qdict_get_qlist(qdict, "exec");

THis seems to be implicitly defining a huge set of extra parameters
for the migrate 'HMP' command, but none of it is actually defined
at the hmp-commands.hx

Do we even need todo this ?  For HMP it is not unreasonable to just
keep using the URI syntax forever?

> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
> +    MigrateChannelType channel_type;
> +    MigrateTransport transport;
> +    MigrateAddress *addr = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr = g_new(SocketAddress, 1);
> +    SocketAddressType type;
> +
> +    channel_type = qapi_enum_parse(&MigrateChannelType_lookup,
> +                                   channeltype, -1, &err);
> +    if (channel_type < 0) {
> +        goto end;
> +    }
> +
> +    transport = qapi_enum_parse(&MigrateTransport_lookup,
> +                                transport_str, -1, &err);
> +    if (transport < 0) {
> +        goto end;
> +    }
> +
> +    type = qapi_enum_parse(&SocketAddressType_lookup,
> +                           socketaddr_type, -1, &err);
> +    if (type < 0) {
> +        goto end;
> +    }
> +
> +    addr->transport = transport;
> +
> +    switch (transport) {
> +    case MIGRATE_TRANSPORT_SOCKET:
> +        saddr->type = type;
> +
> +        switch (type) {
> +        case SOCKET_ADDRESS_TYPE_INET:
> +            saddr->u.inet.host = (char *)inet_host;
> +            saddr->u.inet.port = (char *)inet_port;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_UNIX:
> +            saddr->u.q_unix.path = (char *)unix_path;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_VSOCK:
> +            saddr->u.vsock.cid = (char *)vsock_cid;
> +            saddr->u.vsock.port = (char *)vsock_port;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_FD:
> +            saddr->u.fd.str = (char *)fd;
> +            break;
> +        default:
> +            error_setg(errp, "%s: Unknown socket type %d",
> +                       __func__, saddr->type);
> +            goto end;
> +        }
> +
> +        addr->u.socket = *saddr;
> +        break;
> +    case MIGRATE_TRANSPORT_EXEC:
> +        addr->u.exec.args = (strList *)exec;
> +         break;
> +    case MIGRATE_TRANSPORT_RDMA:
> +        addr->u.rdma.host = (char *)inet_host;
> +        addr->u.rdma.port = (char *)inet_port;
> +        break;
> +    default:
> +        error_setg(errp, "%s: Unknown migrate transport type %d",
> +                   __func__, addr->transport);
> +        goto end;
> +    }
> +
> +    val->channeltype = channel_type;
> +    val->addr = addr;
> +    *channel = val;
> +    return true;
> +
> +end:
> +    error_propagate(errp, err);
> +    qapi_free_MigrateChannel(val);
> +    qapi_free_MigrateAddress(addr);
> +    qapi_free_SocketAddress(saddr);
> +    return false;
> +}
> +
>  void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>  {
>      MigrationInfo *info;
> @@ -702,9 +797,18 @@ 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;
> +    MigrateChannel *channel = g_new0(MigrateChannel, 1);
> +    MigrateChannelList *caps = NULL;
> +
> +    if (!migrate_channel_from_qdict(&channel, qdict, &err)) {
> +        error_setg(&err, "error in retrieving channel from qdict");
> +        goto end;
> +    }
>  
> -    qmp_migrate(uri, !!blk, blk, !!inc, inc,
> -                false, false, true, resume, &err);
> +    QAPI_LIST_PREPEND(caps, channel);
> +    qmp_migrate(uri, !!caps, caps, !!blk, blk, !!inc, inc,
> +                 false, false, true, resume, &err);
> +    qapi_free_MigrateChannelList(caps);
>      if (hmp_handle_error(mon, err)) {
>          return;
>      }
> @@ -725,6 +829,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>                                            status);
>          timer_mod(status->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
>      }
> +
> +end:
> +    qapi_free_MigrateChannel(channel);
> +    hmp_handle_error(mon, err);
> +    return;
>  }
>  
>  void migrate_set_capability_completion(ReadLineState *rs, int nb_args,
> diff --git a/migration/migration.c b/migration/migration.c
> index 6abd69df8d..78f16e5041 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1694,15 +1694,26 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>      return true;
>  }
>  
> -void qmp_migrate(const char *uri, bool has_blk, bool blk,
> -                 bool has_inc, bool inc, bool has_detach, bool detach,
> -                 bool has_resume, bool resume, Error **errp)
> +void qmp_migrate(const char *uri, bool has_channels,
> +                 MigrateChannelList *channels, bool has_blk, bool blk,
> +                 bool has_inc, bool inc, bool has_detach,
> +                 bool detach, bool has_resume, bool resume, Error **errp)
>  {
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
>      MigrateAddress *channel = g_new0(MigrateAddress, 1);
>      SocketAddress *saddr;
>  
> +    /*
> +     * Having preliminary checks for uri and channel
> +     */
> +    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;
> +    }
> +



>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
>          goto out;
> diff --git a/qapi/migration.json b/qapi/migration.json
> index bf90bd8fe2..a71af87ffe 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1464,12 +1464,47 @@
>      'exec': 'MigrateExecCommand',
>      'rdma': 'InetSocketAddress' } }
>  
> +##
> +# @MigrateChannelType:
> +#
> +# The supported options for migration channel type requests
> +#
> +# @main: Support request for main outbound migration control channel
> +#
> +# Since 8.0
> +##
> +{ 'enum': 'MigrateChannelType',
> +  'data': [ 'main' ] }
> +
> +##
> +# @MigrateChannel:
> +#
> +# Information regarding migration Channel-type for transferring packets,
> +# source and corresponding destination interface for socket connection
> +# and number of multifd channels over the interface.
> +#
> +# @channeltype: Name of Channel type for transfering packet information
> +#
> +# @addr: Information regarding migration parameters of destination interface
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateChannel',
> +  'data': {
> +       'channeltype': 'MigrateChannelType',
> +       'addr': 'MigrateAddress' } }
> +
>  ##
>  # @migrate:
>  #
>  # Migrates the current running guest to another Virtual Machine.
>  #
>  # @uri: the Uniform Resource Identifier of the destination VM
> +#       for migration thread
> +#
> +# @channels: Struct containing list of migration channel types, with all
> +#            the information regarding destination interfaces required for
> +#            initiating a migration stream.
>  #
>  # @blk: do block migration (full disk copy)
>  #
> @@ -1494,15 +1529,45 @@
>  # 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. The 'uri' and 'channel' 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": [ { "channeltype": "main",
> +#                          "addr": { "transport": "socket", "type": "inet",
> +#                                    "host": "10.12.34.9",
> +#                                    "port": "1050" } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channeltype": "main",
> +#                          "addr": { "transport": "exec",
> +#                                    "args": [ "/bin/nc", "-p", "6000",
> +#                                              "/some/sock" ] } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channeltype": "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': [ 'MigrateChannel' ], '*blk': 'bool',
> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>  
>  ##
>  # @migrate-incoming:
> -- 
> 2.22.3
> 

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 v4 7/8] migration: modified 'migrate-incoming' QAPI to accept 'channels' argument for migration.

[Daniel P. Berrangé]

On Fri, May 12, 2023 at 02:32:39PM +0000, Het Gala wrote:
> MigrateChannelList ideally allows to have multiple listener interfaces up for
> connection.
> 
> Added MigrateChannelList struct as argument to 'migrate-incoming' qapi. Introduced
> MigrateChannelList in hmp_migrate_incoming() and qmp_migrate_incoming() functions.
> 
> Future patchset series plans to include multiple MigrateChannels to have multiple
> listening interfaces up. That is the reason, 'MigrateChannelList' is the preferred
> choice of argument over 'MigrateChannel' and making 'migrate-incoming' QAPI future
> proof.
> 
> For current patch, have just limit size of MigrateChannelList to 1 element as
> a runtime check.
> 
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/migration-hmp-cmds.c | 14 +++++++++++++-
>  migration/migration.c          | 21 ++++++++++++++++----
>  qapi/migration.json            | 35 +++++++++++++++++++++++++++++++++-
>  softmmu/vl.c                   |  2 +-
>  4 files changed, 65 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index f098d04542..8c11a8c83b 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -518,10 +518,22 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
>      const char *uri = qdict_get_str(qdict, "uri");
> +    MigrateChannel *channel = g_new0(MigrateChannel, 1);
> +    MigrateChannelList *caps = NULL;
> +
> +    if (!migrate_channel_from_qdict(&channel, qdict, &err)) {
> +        error_setg(&err, "error in retrieving channel from qdict");
> +        goto end;
> +    }

Again adding a bunch more parameters to HMP but nothing is documented.

>  
> -    qmp_migrate_incoming(uri, &err);
> +    QAPI_LIST_PREPEND(caps, channel);
> +    qmp_migrate_incoming(uri, !!caps, caps, &err);
> +    qapi_free_MigrateChannelList(caps);
>  
> +end:
> +    qapi_free_MigrateChannel(channel);
>      hmp_handle_error(mon, err);
> +    return;
>  }
>  
>  void hmp_migrate_recover(Monitor *mon, const QDict *qdict)
> diff --git a/migration/migration.c b/migration/migration.c
> index 78f16e5041..de058643a6 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -452,12 +452,24 @@ 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,
> +                                          MigrateChannelList *channels,
> +                                          Error **errp)
>  {
>      Error *local_err = NULL;
>      MigrateAddress *channel = g_new0(MigrateAddress, 1);
>      SocketAddress *saddr;
>  
> +    /*
> +     * Having preliminary checks for uri and channel
> +     */
> +    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;
> +    }
> +
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
>          goto out;
> @@ -1507,7 +1519,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,
> +                          MigrateChannelList *channels, Error **errp)
>  {
>      Error *local_err = NULL;
>      static bool once = true;
> @@ -1525,7 +1538,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);
> @@ -1561,7 +1574,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)
> diff --git a/qapi/migration.json b/qapi/migration.json
> index a71af87ffe..9faecdd048 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1578,6 +1578,10 @@
>  # @uri: The Uniform Resource Identifier identifying the source or
>  #       address to listen on
>  #
> +# @channels: Struct containing list of migration channel types, with all
> +#            the information regarding destination interfaces required for
> +#            initiating a migration stream.
> +#
>  # Returns: nothing on success
>  #
>  # Since: 2.3
> @@ -1593,14 +1597,43 @@
>  #
>  # 3. The uri format is the same as for -incoming
>  #
> +# 4. The 'uri' and 'channel' 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": [ { "channeltype": "main",
> +#                          "addr": { "transport": "socket", "type": "inet",
> +#                                    "host": "10.12.34.9",
> +#                                    "port": "1050" } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channeltype": "main",
> +#                          "addr": { "transport": "exec",
> +#                                    "args": [ "/bin/nc", "-p", "6000",
> +#                                              "/some/sock" ] } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channeltype": "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': [ 'MigrateChannel' ] } }
>  
>  ##
>  # @xen-save-devices-state:
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 6c2427262b..ade411eb4f 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2633,7 +2633,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
> 

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 v4 8/8] migration: Introduced MigrateChannelList struct to migration code flow.

[Daniel P. Berrangé]

On Fri, May 12, 2023 at 02:32:40PM +0000, Het Gala wrote:
> Integrated MigrateChannelList with all transport backends (socket, exec
> and rdma) for both source and destination migration code flow.
> 
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/migration.c | 95 +++++++++++++++++++++++++++----------------
>  migration/socket.c    |  5 ++-
>  2 files changed, 64 insertions(+), 36 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index de058643a6..a37eba29e3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -410,10 +410,11 @@ void migrate_add_address(SocketAddress *address)
>  }
>  
>  static bool migrate_uri_parse(const char *uri,
> -                              MigrateAddress **channel,
> +                              MigrateChannel **channel,
>                                Error **errp)
>  {
>      Error *local_err = NULL;
> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
>      MigrateAddress *addrs = g_new0(MigrateAddress, 1);
>      SocketAddress *saddr;
>      InetSocketAddress *isock = &addrs->u.rdma;
> @@ -441,6 +442,7 @@ static bool migrate_uri_parse(const char *uri,
>      }
>  
>      if (local_err) {
> +        qapi_free_MigrateChannel(val);
>          qapi_free_MigrateAddress(addrs);
>          qapi_free_SocketAddress(saddr);
>          qapi_free_InetSocketAddress(isock);
> @@ -448,7 +450,9 @@ static bool migrate_uri_parse(const char *uri,
>          return false;
>      }
>  
> -    *channel = addrs;
> +    val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
> +    val->addr = addrs;
> +    *channel = val;
>      return true;
>  }
>  
> @@ -457,8 +461,9 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>                                            Error **errp)
>  {
>      Error *local_err = NULL;
> -    MigrateAddress *channel = g_new0(MigrateAddress, 1);
> +    MigrateAddress *addrs;
>      SocketAddress *saddr;
> +    MigrateChannel *channel = NULL;
>  
>      /*
>       * Having preliminary checks for uri and channel
> @@ -467,22 +472,30 @@ static void qemu_start_incoming_migration(const char *uri, bool 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;
> -    }
> -
> -    /* URI is not suitable for migration? */
> -    if (!migration_channels_and_uri_compatible(uri, errp)) {
>          goto out;
> -    }
> +    } 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");
> +            goto out;
> +        }
> +        channel = channels->value;
> +    } else {
> +        /* URI is not suitable for migration? */
> +        if (!migration_channels_and_uri_compatible(uri, errp)) {
> +            goto out;
> +        }

THis check only gets executed when the caller uses the old
URI syntax. We need to it be run when using the modern
MigrateChannel QAPI syntax too.

IOW, migration_channels_and_uri_compatible() needs converting
to take a 'MigrateChannel' object instead of URI, and then
the check can be run after the URI -> MigrateCHannel conversion

>  
> -    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> -        error_setg(errp, "Error parsing uri");
> -        goto out;
> +        if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> +            error_setg(errp, "Error parsing uri");
> +            goto out;
> +        }
>      }
>  
> -    saddr = &channel->u.socket;
> +    addrs = channel->addr;
> +    saddr = &channel->addr->u.socket;
>      qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> -    if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
> +    if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
>          if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>              saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>              saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> @@ -491,23 +504,25 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>              fd_start_incoming_migration(saddr->u.fd.str, &local_err);
>          }
>  #ifdef CONFIG_RDMA
> -    } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
> -        rdma_start_incoming_migration(&channel->u.rdma, &local_err);
> +    } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
> +        rdma_start_incoming_migration(&addrs->u.rdma, &local_err);
>  #endif
> -    } else if (channel->transport == MIGRATE_TRANSPORT_EXEC) {
> -        exec_start_incoming_migration(channel->u.exec.args, &local_err);
> +    } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
> +        exec_start_incoming_migration(addrs->u.exec.args, &local_err);
>      } else {
>          error_setg(errp, "unknown migration protocol: %s", uri);
>      }
>  
>      if (local_err) {
> +        qapi_free_MigrateAddress(addrs);
>          qapi_free_SocketAddress(saddr);
>          error_propagate(errp, local_err);
>          return;
>      }
>  
>  out:
> -    qapi_free_MigrateAddress(channel);
> +    qapi_free_MigrateChannel(channel);
> +    return;
>  }
>  
>  static void process_incoming_migration_bh(void *opaque)
> @@ -1714,8 +1729,9 @@ void qmp_migrate(const char *uri, bool has_channels,
>  {
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
> -    MigrateAddress *channel = g_new0(MigrateAddress, 1);
> +    MigrateAddress *addrs;
>      SocketAddress *saddr;
> +    MigrateChannel *channel = NULL;
>  
>      /*
>       * Having preliminary checks for uri and channel
> @@ -1724,17 +1740,24 @@ void qmp_migrate(const char *uri, bool 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;
> -    }
> -
> -    /* URI is not suitable for migration? */
> -    if (!migration_channels_and_uri_compatible(uri, errp)) {
>          goto out;
> -    }
> +    } 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");
> +            goto out;
> +        }
> +        channel = channels->value;
> +    } else {
> +        /* URI is not suitable for migration? */
> +        if (!migration_channels_and_uri_compatible(uri, errp)) {
> +            goto out;
> +        }
>  
> -    if (!migrate_uri_parse(uri, &channel, &local_err)) {
> -        error_setg(errp, "Error parsing uri");
> -        goto out;
> +        if (!migrate_uri_parse(uri, &channel, &local_err)) {
> +            error_setg(errp, "Error parsing uri");
> +            goto out;
> +        }
>      }
>  
>      if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
> @@ -1749,8 +1772,9 @@ void qmp_migrate(const char *uri, bool has_channels,
>          }
>      }
>  
> -    saddr = &channel->u.socket;
> -    if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
> +    addrs = channel->addr;
> +    saddr = &channel->addr->u.socket;
> +    if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
>          if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>              saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>              saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> @@ -1759,11 +1783,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 == MIGRATE_TRANSPORT_RDMA) {
> -        rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
> +    } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
> +        rdma_start_outgoing_migration(s, &addrs->u.rdma, &local_err);
>  #endif
> -    } else if (channel->transport == MIGRATE_TRANSPORT_EXEC) {
> -        exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
> +    } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
> +        exec_start_outgoing_migration(s, addrs->u.exec.args, &local_err);
>      } else {
>          if (!(has_resume && resume)) {
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> @@ -1780,6 +1804,7 @@ void qmp_migrate(const char *uri, bool has_channels,
>          if (!(has_resume && resume)) {
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          }
> +        qapi_free_MigrateAddress(addrs);
>          qapi_free_SocketAddress(saddr);
>          migrate_fd_error(s, local_err);
>          error_propagate(errp, local_err);
> diff --git a/migration/socket.c b/migration/socket.c
> index 8e7430b266..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;
> @@ -114,12 +116,13 @@ void socket_start_outgoing_migration(MigrationState *s,
>  {
>      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);
> -- 
> 2.22.3
> 

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 v4 2/8] migration: Converts uri parameter into 'MigrateAddress' struct

[Het Gala]

Just so that, there is a wider attention, I will try to address and 
discuss the comments from Daniel and Juan both here, as many of them 
seems to be overlapping. I hope that is fine with the maintainers.

On 15/05/23 3:42 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:34PM +0000, Het Gala wrote:
>> This patch introduces code that can parse 'uri' string parameter and
>> spit out 'MigrateAddress' struct. All the required migration parameters
>> are stored in the struct.
>>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>>   migration/migration.c | 63 +++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 61 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 0ee07802a5..a7e4e286aa 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -64,6 +64,7 @@
>>   #include "yank_functions.h"
>>   #include "sysemu/qtest.h"
>>   #include "options.h"
>> +#include "qemu/sockets.h"
>>   
>>   static NotifierList migration_state_notifiers =
>>       NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
>> @@ -408,13 +409,58 @@ void migrate_add_address(SocketAddress *address)
>>                         QAPI_CLONE(SocketAddress, address));
>>   }
>>   
>> +static bool migrate_uri_parse(const char *uri,
>> +                              MigrateAddress **channel,
>> +                              Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr;
>> +    InetSocketAddress *isock = &addrs->u.rdma;
>> +    strList **tail = &addrs->u.exec.args;
>> +
>> +    if (strstart(uri, "exec:", NULL)) {
>> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
>> +        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
>> +        QAPI_LIST_APPEND(tail, g_strdup("-c"));
>> +        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
>> +    } else if (strstart(uri, "rdma:", NULL) &&
>> +               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
>> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
> I would have this as
>
>      } else if (strstart(uri, "rdma:", NULL)) {
>          if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
>              addrs->transport = MIGRATE_TRANSPORT_RDMA;
> 	}
>
> as IMHO it is bad practice to have control pass to the next
> else if clause when inet_parse() fails, as we know this is
> only an RDMA addr
Ack. I will change in the next patch.
> Also you need to use '&local_err' not 'errp' in the inet_parse
> call, otherwise the later code block for cleanup won't run.

Yes, thanks for pointing it out Daniel. Will modify that.

Also, Juan is of the opinion that we could omit 'local_error' variable 
and try to address and free the memory there itself. For ex:

if (saddr == NULL) {
     qapi_free_MigrateAddress(addrs);
     return false;
}

Or, Daniel, can I also define here the variables like you suggested down 
in the patch ? or is it used in some special case or I am missing 
something ?

g_autoptr(MigrateAddress) addrs = g_new0(MigrateAddress, 1);

So we would not have to worry to free MigrateAddress struct.

>> +    } else if (strstart(uri, "tcp:", NULL) ||
>> +                strstart(uri, "unix:", NULL) ||
>> +                strstart(uri, "vsock:", NULL) ||
>> +                strstart(uri, "fd:", NULL)) {
>> +        addrs->transport = MIGRATE_TRANSPORT_SOCKET;
>> +        saddr = socket_parse(uri, &local_err);
>> +        addrs->u.socket = *saddr;
> Protect with
>
>     if (saddr != NULL) {
>         addrs->u.socket = *saddr;
>     }
>
>> +    }
>> +
>> +    if (local_err) {
>> +        qapi_free_MigrateAddress(addrs);
>> +        qapi_free_SocketAddress(saddr);
>> +        qapi_free_InetSocketAddress(isock);
>> +        error_propagate(errp, local_err);
>> +        return false;
>> +    }
>> +
>> +    *channel = addrs;
>> +    return true;
>> +}
>> +
>>   static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>   {
>>       const char *p = NULL;
>> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);
> Avoid the later 'out:' cleanup block by using:
>
>    g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
Ack. I think this also solves the doubt raised by Juan "I wish, I really 
wish, that there was a way to free things on error". Am I right ?
>>   
>>       /* URI is not suitable for migration? */
>>       if (!migration_channels_and_uri_compatible(uri, errp)) {
>> -        return;
>> +        goto out;
>> +    }
>> +
>> +    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
>> +        error_setg(errp, "Error parsing uri");
> This error_setg() is overwriting the error migrate_uri_parse already
> set, so just drop it.

Okay, I got the point. I thought error_setg() would add error statement 
on top of errp, but I was incorrect.

Juan also made the same point I believe. I will remove error_setg() in 
the next version of patchset series.

>> +        goto out;
>>       }
>>   
>>       qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>> @@ -433,6 +479,9 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>       } else {
>>           error_setg(errp, "unknown migration protocol: %s", uri);
>>       }
>> +
>> +out:
>> +    qapi_free_MigrateAddress(channel);
>>   }
>>   
>>   static void process_incoming_migration_bh(void *opaque)
>> @@ -1638,10 +1687,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>       Error *local_err = NULL;
>>       MigrationState *s = migrate_get_current();
>>       const char *p = NULL;
>> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);
> Here too, use g_autoptr(MigrateAddress) and drop the  'out:' block
Ack.
>>   
>>       /* URI is not suitable for migration? */
>>       if (!migration_channels_and_uri_compatible(uri, errp)) {
>> -        return;
>> +        goto out;
>> +    }
>> +
>> +    if (!migrate_uri_parse(uri, &channel, &local_err)) {
>> +        error_setg(errp, "Error parsing uri");
>> +        goto out;
>>       }
>>   
>>       if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
>> @@ -1688,6 +1743,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>           error_propagate(errp, local_err);
>>           return;
>>       }
>> +
>> +out:
>> +    qapi_free_MigrateAddress(channel);
>> +    return;
>>   }
>>   
>>   void qmp_migrate_cancel(Error **errp)
>> -- 
>> 2.22.3
>>
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v4 2/8] migration: Converts uri parameter into 'MigrateAddress' struct

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
v> Just so that, there is a wider attention, I will try to address and
> discuss the comments from Daniel and Juan both here, as many of them
> seems to be overlapping. I hope that is fine with the maintainers.
>
> On 15/05/23 3:42 pm, Daniel P. Berrangé wrote:
>> On Fri, May 12, 2023 at 02:32:34PM +0000, Het Gala wrote:
>>> This patch introduces code that can parse 'uri' string parameter and
>>> spit out 'MigrateAddress' struct. All the required migration parameters
>>> are stored in the struct.
>>>
>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>> ---
>>>   migration/migration.c | 63 +++++++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 61 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 0ee07802a5..a7e4e286aa 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -64,6 +64,7 @@
>>>   #include "yank_functions.h"
>>>   #include "sysemu/qtest.h"
>>>   #include "options.h"
>>> +#include "qemu/sockets.h"
>>>     static NotifierList migration_state_notifiers =
>>>       NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
>>> @@ -408,13 +409,58 @@ void migrate_add_address(SocketAddress *address)
>>>                         QAPI_CLONE(SocketAddress, address));
>>>   }
>>>   +static bool migrate_uri_parse(const char *uri,
>>> +                              MigrateAddress **channel,
>>> +                              Error **errp)
>>> +{
>>> +    Error *local_err = NULL;
>>> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
>>> +    SocketAddress *saddr;
>>> +    InetSocketAddress *isock = &addrs->u.rdma;
>>> +    strList **tail = &addrs->u.exec.args;
>>> +
>>> +    if (strstart(uri, "exec:", NULL)) {
>>> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
>>> +        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
>>> +        QAPI_LIST_APPEND(tail, g_strdup("-c"));
>>> +        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
>>> +    } else if (strstart(uri, "rdma:", NULL) &&
>>> +               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
>>> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
>> I would have this as
>>
>>      } else if (strstart(uri, "rdma:", NULL)) {
>>          if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
>>              addrs->transport = MIGRATE_TRANSPORT_RDMA;
>> 	}
>>
>> as IMHO it is bad practice to have control pass to the next
>> else if clause when inet_parse() fails, as we know this is
>> only an RDMA addr
> Ack. I will change in the next patch.
>> Also you need to use '&local_err' not 'errp' in the inet_parse
>> call, otherwise the later code block for cleanup won't run.
>
> Yes, thanks for pointing it out Daniel. Will modify that.
>
> Also, Juan is of the opinion that we could omit 'local_error' variable
> and try to address and free the memory there itself. For ex:
>
> if (saddr == NULL) {
>     qapi_free_MigrateAddress(addrs);
>     return false;
> }
>
> Or, Daniel, can I also define here the variables like you suggested
> down in the patch ? or is it used in some special case or I am missing
> something ?
>
> g_autoptr(MigrateAddress) addrs = g_new0(MigrateAddress, 1);
>
> So we would not have to worry to free MigrateAddress struct.

https://blogs.gnome.org/desrt/2015/01/30/g_autoptr/

Yes, but that only happens for the cases where you want to always remove
them.

>>> +    } else if (strstart(uri, "tcp:", NULL) ||
>>> +                strstart(uri, "unix:", NULL) ||
>>> +                strstart(uri, "vsock:", NULL) ||
>>> +                strstart(uri, "fd:", NULL)) {
>>> +        addrs->transport = MIGRATE_TRANSPORT_SOCKET;
>>> +        saddr = socket_parse(uri, &local_err);
>>> +        addrs->u.socket = *saddr;
>> Protect with
>>
>>     if (saddr != NULL) {
>>         addrs->u.socket = *saddr;
>>     }
>>
>>> +    }
>>> +
>>> +    if (local_err) {
>>> +        qapi_free_MigrateAddress(addrs);
>>> +        qapi_free_SocketAddress(saddr);
>>> +        qapi_free_InetSocketAddress(isock);
>>> +        error_propagate(errp, local_err);
>>> +        return false;
>>> +    }
>>> +
>>> +    *channel = addrs;
>>> +    return true;
>>> +}
>>> +
>>>   static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>>   {
>>>       const char *p = NULL;
>>> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);
>> Avoid the later 'out:' cleanup block by using:
>>
>>    g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
> Ack. I think this also solves the doubt raised by Juan "I wish, I
> really wish, that there was a way to free things on error". Am I right
> ?

No, that was the case where we have something like:

Thing *foo(void)
{
    OtherThing *bar = g_new0(OtherThing, 1)

    if (whatever) {
        goto error;
    }
    if (whatever_else) {
        goto error;
    }
    return bar;
error:
    g_free(bad);
    return NULL;
}

See, we have to put the goto because we have to free it in all error
paths.  Not in the non-error path.

If it is a pure local variable, i.e. never used after the function
finishes, then g_autoptr is the right thing to do.

Later, Juan.

Re: [PATCH v4 2/8] migration: Converts uri parameter into 'MigrateAddress' struct

[Daniel P. Berrangé]

On Mon, May 15, 2023 at 01:55:59PM +0200, Juan Quintela wrote:
> Het Gala <het.gala@nutanix.com> wrote:
> v> Just so that, there is a wider attention, I will try to address and
> > discuss the comments from Daniel and Juan both here, as many of them
> > seems to be overlapping. I hope that is fine with the maintainers.
> >
> > On 15/05/23 3:42 pm, Daniel P. Berrangé wrote:
> >> On Fri, May 12, 2023 at 02:32:34PM +0000, Het Gala wrote:
> >>> This patch introduces code that can parse 'uri' string parameter and
> >>> spit out 'MigrateAddress' struct. All the required migration parameters
> >>> are stored in the struct.
> >>>
> >>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> >>> Signed-off-by: Het Gala <het.gala@nutanix.com>
> >>> ---
> >>>   migration/migration.c | 63 +++++++++++++++++++++++++++++++++++++++++--
> >>>   1 file changed, 61 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/migration/migration.c b/migration/migration.c
> >>> index 0ee07802a5..a7e4e286aa 100644
> >>> --- a/migration/migration.c
> >>> +++ b/migration/migration.c
> >>> @@ -64,6 +64,7 @@
> >>>   #include "yank_functions.h"
> >>>   #include "sysemu/qtest.h"
> >>>   #include "options.h"
> >>> +#include "qemu/sockets.h"
> >>>     static NotifierList migration_state_notifiers =
> >>>       NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> >>> @@ -408,13 +409,58 @@ void migrate_add_address(SocketAddress *address)
> >>>                         QAPI_CLONE(SocketAddress, address));
> >>>   }
> >>>   +static bool migrate_uri_parse(const char *uri,
> >>> +                              MigrateAddress **channel,
> >>> +                              Error **errp)
> >>> +{
> >>> +    Error *local_err = NULL;
> >>> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> >>> +    SocketAddress *saddr;
> >>> +    InetSocketAddress *isock = &addrs->u.rdma;
> >>> +    strList **tail = &addrs->u.exec.args;
> >>> +
> >>> +    if (strstart(uri, "exec:", NULL)) {
> >>> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
> >>> +        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> >>> +        QAPI_LIST_APPEND(tail, g_strdup("-c"));
> >>> +        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> >>> +    } else if (strstart(uri, "rdma:", NULL) &&
> >>> +               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
> >>> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
> >> I would have this as
> >>
> >>      } else if (strstart(uri, "rdma:", NULL)) {
> >>          if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
> >>              addrs->transport = MIGRATE_TRANSPORT_RDMA;
> >> 	}
> >>
> >> as IMHO it is bad practice to have control pass to the next
> >> else if clause when inet_parse() fails, as we know this is
> >> only an RDMA addr
> > Ack. I will change in the next patch.
> >> Also you need to use '&local_err' not 'errp' in the inet_parse
> >> call, otherwise the later code block for cleanup won't run.
> >
> > Yes, thanks for pointing it out Daniel. Will modify that.
> >
> > Also, Juan is of the opinion that we could omit 'local_error' variable
> > and try to address and free the memory there itself. For ex:
> >
> > if (saddr == NULL) {
> >     qapi_free_MigrateAddress(addrs);
> >     return false;
> > }
> >
> > Or, Daniel, can I also define here the variables like you suggested
> > down in the patch ? or is it used in some special case or I am missing
> > something ?
> >
> > g_autoptr(MigrateAddress) addrs = g_new0(MigrateAddress, 1);
> >
> > So we would not have to worry to free MigrateAddress struct.
> 
> https://blogs.gnome.org/desrt/2015/01/30/g_autoptr/
> 
> Yes, but that only happens for the cases where you want to always remove
> them.
> 
> >>> +    } else if (strstart(uri, "tcp:", NULL) ||
> >>> +                strstart(uri, "unix:", NULL) ||
> >>> +                strstart(uri, "vsock:", NULL) ||
> >>> +                strstart(uri, "fd:", NULL)) {
> >>> +        addrs->transport = MIGRATE_TRANSPORT_SOCKET;
> >>> +        saddr = socket_parse(uri, &local_err);
> >>> +        addrs->u.socket = *saddr;
> >> Protect with
> >>
> >>     if (saddr != NULL) {
> >>         addrs->u.socket = *saddr;
> >>     }
> >>
> >>> +    }
> >>> +
> >>> +    if (local_err) {
> >>> +        qapi_free_MigrateAddress(addrs);
> >>> +        qapi_free_SocketAddress(saddr);
> >>> +        qapi_free_InetSocketAddress(isock);
> >>> +        error_propagate(errp, local_err);
> >>> +        return false;
> >>> +    }
> >>> +
> >>> +    *channel = addrs;
> >>> +    return true;
> >>> +}
> >>> +
> >>>   static void qemu_start_incoming_migration(const char *uri, Error **errp)
> >>>   {
> >>>       const char *p = NULL;
> >>> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);
> >> Avoid the later 'out:' cleanup block by using:
> >>
> >>    g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
> > Ack. I think this also solves the doubt raised by Juan "I wish, I
> > really wish, that there was a way to free things on error". Am I right
> > ?
> 
> No, that was the case where we have something like:
> 
> Thing *foo(void)
> {
>     OtherThing *bar = g_new0(OtherThing, 1)
> 
>     if (whatever) {
>         goto error;
>     }
>     if (whatever_else) {
>         goto error;
>     }
>     return bar;
> error:
>     g_free(bad);
>     return NULL;
> }
[> 
> See, we have to put the goto because we have to free it in all error
> paths.  Not in the non-error path.
> 
> If it is a pure local variable, i.e. never used after the function
> finishes, then g_autoptr is the right thing to do.

It is still better to use g_autoptr even in that case. You just need
to add in a call to g_steal_pointer in the success path. eg

 Thing *foo(void)
 {
     g_autoptr(OtherThing) bar = g_new0(OtherThing, 1)
 
     if (whatever) {
         return NULL;
     }
     if (whatever_else) {
         return NULL;
     }
     return g_steal_pointer(&bar);
 }


g_steal_pointer(&bar) is the equivalent of doing


    OtherThing *tmp = bar;
    bar = NULL;
    return tmp;

thus avoiding free'ing the pointer you're returning

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 v4 2/8] migration: Converts uri parameter into 'MigrateAddress' struct

[Juan Quintela]

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

Ok, thanks!

Didn't know that trick.

On Mon, May 15, 2023, 14:17 Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Mon, May 15, 2023 at 01:55:59PM +0200, Juan Quintela wrote:
> > Het Gala <het.gala@nutanix.com> wrote:
> > v> Just so that, there is a wider attention, I will try to address and
> > > discuss the comments from Daniel and Juan both here, as many of them
> > > seems to be overlapping. I hope that is fine with the maintainers.
> > >
> > > On 15/05/23 3:42 pm, Daniel P. Berrangé wrote:
> > >> On Fri, May 12, 2023 at 02:32:34PM +0000, Het Gala wrote:
> > >>> This patch introduces code that can parse 'uri' string parameter and
> > >>> spit out 'MigrateAddress' struct. All the required migration
> parameters
> > >>> are stored in the struct.
> > >>>
> > >>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > >>> Signed-off-by: Het Gala <het.gala@nutanix.com>
> > >>> ---
> > >>>   migration/migration.c | 63
> +++++++++++++++++++++++++++++++++++++++++--
> > >>>   1 file changed, 61 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/migration/migration.c b/migration/migration.c
> > >>> index 0ee07802a5..a7e4e286aa 100644
> > >>> --- a/migration/migration.c
> > >>> +++ b/migration/migration.c
> > >>> @@ -64,6 +64,7 @@
> > >>>   #include "yank_functions.h"
> > >>>   #include "sysemu/qtest.h"
> > >>>   #include "options.h"
> > >>> +#include "qemu/sockets.h"
> > >>>     static NotifierList migration_state_notifiers =
> > >>>       NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> > >>> @@ -408,13 +409,58 @@ void migrate_add_address(SocketAddress
> *address)
> > >>>                         QAPI_CLONE(SocketAddress, address));
> > >>>   }
> > >>>   +static bool migrate_uri_parse(const char *uri,
> > >>> +                              MigrateAddress **channel,
> > >>> +                              Error **errp)
> > >>> +{
> > >>> +    Error *local_err = NULL;
> > >>> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> > >>> +    SocketAddress *saddr;
> > >>> +    InetSocketAddress *isock = &addrs->u.rdma;
> > >>> +    strList **tail = &addrs->u.exec.args;
> > >>> +
> > >>> +    if (strstart(uri, "exec:", NULL)) {
> > >>> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
> > >>> +        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> > >>> +        QAPI_LIST_APPEND(tail, g_strdup("-c"));
> > >>> +        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> > >>> +    } else if (strstart(uri, "rdma:", NULL) &&
> > >>> +               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
> > >>> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
> > >> I would have this as
> > >>
> > >>      } else if (strstart(uri, "rdma:", NULL)) {
> > >>          if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
> > >>              addrs->transport = MIGRATE_TRANSPORT_RDMA;
> > >>    }
> > >>
> > >> as IMHO it is bad practice to have control pass to the next
> > >> else if clause when inet_parse() fails, as we know this is
> > >> only an RDMA addr
> > > Ack. I will change in the next patch.
> > >> Also you need to use '&local_err' not 'errp' in the inet_parse
> > >> call, otherwise the later code block for cleanup won't run.
> > >
> > > Yes, thanks for pointing it out Daniel. Will modify that.
> > >
> > > Also, Juan is of the opinion that we could omit 'local_error' variable
> > > and try to address and free the memory there itself. For ex:
> > >
> > > if (saddr == NULL) {
> > >     qapi_free_MigrateAddress(addrs);
> > >     return false;
> > > }
> > >
> > > Or, Daniel, can I also define here the variables like you suggested
> > > down in the patch ? or is it used in some special case or I am missing
> > > something ?
> > >
> > > g_autoptr(MigrateAddress) addrs = g_new0(MigrateAddress, 1);
> > >
> > > So we would not have to worry to free MigrateAddress struct.
> >
> > https://blogs.gnome.org/desrt/2015/01/30/g_autoptr/
> >
> > Yes, but that only happens for the cases where you want to always remove
> > them.
> >
> > >>> +    } else if (strstart(uri, "tcp:", NULL) ||
> > >>> +                strstart(uri, "unix:", NULL) ||
> > >>> +                strstart(uri, "vsock:", NULL) ||
> > >>> +                strstart(uri, "fd:", NULL)) {
> > >>> +        addrs->transport = MIGRATE_TRANSPORT_SOCKET;
> > >>> +        saddr = socket_parse(uri, &local_err);
> > >>> +        addrs->u.socket = *saddr;
> > >> Protect with
> > >>
> > >>     if (saddr != NULL) {
> > >>         addrs->u.socket = *saddr;
> > >>     }
> > >>
> > >>> +    }
> > >>> +
> > >>> +    if (local_err) {
> > >>> +        qapi_free_MigrateAddress(addrs);
> > >>> +        qapi_free_SocketAddress(saddr);
> > >>> +        qapi_free_InetSocketAddress(isock);
> > >>> +        error_propagate(errp, local_err);
> > >>> +        return false;
> > >>> +    }
> > >>> +
> > >>> +    *channel = addrs;
> > >>> +    return true;
> > >>> +}
> > >>> +
> > >>>   static void qemu_start_incoming_migration(const char *uri, Error
> **errp)
> > >>>   {
> > >>>       const char *p = NULL;
> > >>> +    MigrateAddress *channel = g_new0(MigrateAddress, 1);
> > >> Avoid the later 'out:' cleanup block by using:
> > >>
> > >>    g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
> > > Ack. I think this also solves the doubt raised by Juan "I wish, I
> > > really wish, that there was a way to free things on error". Am I right
> > > ?
> >
> > No, that was the case where we have something like:
> >
> > Thing *foo(void)
> > {
> >     OtherThing *bar = g_new0(OtherThing, 1)
> >
> >     if (whatever) {
> >         goto error;
> >     }
> >     if (whatever_else) {
> >         goto error;
> >     }
> >     return bar;
> > error:
> >     g_free(bad);
> >     return NULL;
> > }
> [>
> > See, we have to put the goto because we have to free it in all error
> > paths.  Not in the non-error path.
> >
> > If it is a pure local variable, i.e. never used after the function
> > finishes, then g_autoptr is the right thing to do.
>
> It is still better to use g_autoptr even in that case. You just need
> to add in a call to g_steal_pointer in the success path. eg
>
>  Thing *foo(void)
>  {
>      g_autoptr(OtherThing) bar = g_new0(OtherThing, 1)
>
>      if (whatever) {
>          return NULL;
>      }
>      if (whatever_else) {
>          return NULL;
>      }
>      return g_steal_pointer(&bar);
>  }
>
>
> g_steal_pointer(&bar) is the equivalent of doing
>
>
>     OtherThing *tmp = bar;
>     bar = NULL;
>     return tmp;
>
> thus avoiding free'ing the pointer you're returning
>
> 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 :|
>
>

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

Re: [PATCH v4 3/8] migration: converts socket backend to accept MigrateAddress struct

[Het Gala]

On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
>> 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>
>> ---
>>   migration/exec.c      |  4 ++--
>>   migration/exec.h      |  4 ++++
>>   migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
>>   migration/socket.c    | 34 +++++----------------------------
>>   migration/socket.h    |  7 ++++---
>>   5 files changed, 47 insertions(+), 46 deletions(-)
>>
>> diff --git a/migration/exec.c b/migration/exec.c
>> index 2bf882bbe1..c4a3293246 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)
Even this, I will shift it to the 2nd patch, where I need to move 
exec_get_cmd_path() out accross other file (migration.c).
>>   {
>>       g_autofree char *detected_path = g_new(char, MAX_PATH);
>> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
>>   }
>>   #endif
>>   
>> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
>> +void exec_start_outgoing_migration(MigrationState *s, const char *command,
>> +                                   Error **errp)
>>   {
>>       QIOChannel *ioc;
Sure, Juan. Will change this in the 2nd patch itself instead of here. I 
am not very convinved why should we have a different patch all together 
for this, because we are just using this code outside this file in my 
opinion? But if you still think so, I can make a different patch for that.
>>   
>> 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 a7e4e286aa..61f52d2f90 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -421,7 +421,11 @@ static bool migrate_uri_parse(const char *uri,
>>   
>>       if (strstart(uri, "exec:", NULL)) {
>>           addrs->transport = MIGRATE_TRANSPORT_EXEC;
>> +#ifdef WIN32
>> +        QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
>> +#else
>>           QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
>> +#endif
> This windows portability code should have been in the previous patch
> I think.
Ack, yes, it could have also been added in the 2nd patch. Will 
accomodate windows portability change there itself.
>>           QAPI_LIST_APPEND(tail, g_strdup("-c"));
>>           QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
>>       } else if (strstart(uri, "rdma:", NULL) &&
>> @@ -450,8 +454,10 @@ static bool migrate_uri_parse(const char *uri,
>>   
>>   static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>   {
>> +    Error *local_err = NULL;
>>       const char *p = NULL;
>>       MigrateAddress *channel = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr;
>>   
>>       /* URI is not suitable for migration? */
>>       if (!migration_channels_and_uri_compatible(uri, errp)) {
>> @@ -463,23 +469,32 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>           goto out;
>>       }
>>   
>> +    saddr = &channel->u.socket;
> Accessing u.socket before checkout transport == SOCKET is bad
> practice, even though this is technically safe.

Ok, Ack. I understand your point, before defining whether transport 
could be a socket or not, it is not a good way representing and defining 
it earlier.

Thanks for pointing it out Daniel. I will change this in all the places, 
where I have accessed it before confirming if the transport == socket.

>>       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 == MIGRATE_TRANSPORT_SOCKET) {
> THis should have
>
>      SocketAddress *saddr = &channe->u.socket
>
> so that 'saddr' is limited in scope to where we've validated
> transport == SOCKET
Yes, Ack. Makes sense.
>> +        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>> +            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>> +            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>> +            socket_start_incoming_migration(saddr, &local_err);
>> +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
>> +            fd_start_incoming_migration(saddr->u.fd.str, &local_err);
>> +        }
>>   #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);
>>       }
>>   
>> +    if (local_err) {
>> +        qapi_free_SocketAddress(saddr);
>> +        error_propagate(errp, local_err);
>> +        return;
Juan, I get your point. But I think, we won't be needing local_err at 
all, if I use g_autoptr for 'channel' and 'saddr' is a part of 
'channel'. Let me have a v2 patchset and if it is still not convinving, 
we can have a discussion on this.
> THis leaks 'channel', and free's 'saddr' which actually  belongs
> to channel.
>
> With my comments on the previous patch suggesting g_autoptr for
> 'channel', we don't need any free calls for 'saddr' or 'channel'.

Right. With g_autoptr used for freeing 'channel' in last patch, we wont 
have to worry about freeing 'saddr' at all. Thanks Daniel

if (local_err) {
     qapi_free_SocketAddress(saddr);
     error_propagate(errp, local_err);
     return;
}
And after changing the position for assigning 'saddr' and using 
g_autoptr for 'channel' I believe we can get rid of 'local_error' 
variable too and replace it with 'errp'. Please suggest if I am missing 
something here. TIA!

>> +    }
>> +
>>   out:
>>       qapi_free_MigrateAddress(channel);
>>   }
>> @@ -1688,6 +1703,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>       MigrationState *s = migrate_get_current();
>>       const char *p = NULL;
>>       MigrateAddress *channel = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr;
>>   
>>       /* URI is not suitable for migration? */
>>       if (!migration_channels_and_uri_compatible(uri, errp)) {
>> @@ -1711,18 +1727,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);
>> +    saddr = &channel->u.socket;
> Again, put this *after*  checking transport == SOCKET
Ack.
>> +    if (channel->transport == MIGRATE_TRANSPORT_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 {
>>           if (!(has_resume && resume)) {
>>               yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>> @@ -1739,6 +1758,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>           if (!(has_resume && resume)) {
>>               yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>>           }
>> +        qapi_free_SocketAddress(saddr);
> This saddr pointer belongs to 'channel' which must be freed.
Again here, with your comment in last patch to use g_autoptr, we need 
not to explicitly free 'channel' or 'saddr' right.
>>           migrate_fd_error(s, local_err);
>>           error_propagate(errp, local_err);
>>           return;
>> diff --git a/migration/socket.c b/migration/socket.c
>> index 1b6f5baefb..8e7430b266 100644
>> --- a/migration/socket.c
>> +++ b/migration/socket.c
>> @@ -108,10 +108,9 @@ 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);
>> @@ -135,18 +134,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);
>> -}
>> -
Actually Juan. I don't need this function at all, because parsing of uri 
into socketAddress using socket_parse is already done. So there is no 
use of having this function in the first place, so I decided to delete 
this fucntion all together. Same with incoming function.
>>   static void socket_accept_incoming_migration(QIONetListener *listener,
>>                                                QIOChannelSocket *cioc,
>>                                                gpointer opaque)
>> @@ -172,9 +159,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 +199,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
>>
> With regards,
> Daniel

Re: [PATCH v4 4/8] migration: converts rdma backend to accept MigrateAddress struct

[Het Gala]

On 15/05/23 3:54 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:36PM +0000, Het Gala wrote:
>> 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>
>> ---
>>   migration/migration.c |  8 ++++----
>>   migration/rdma.c      | 38 ++++++++++++++++----------------------
>>   migration/rdma.h      |  6 ++++--
>>   3 files changed, 24 insertions(+), 28 deletions(-)
>>
>> @@ -3360,10 +3346,12 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>>                                               .private_data_len = sizeof(cap),
>>                                            };
>>       RDMAContext *rdma_return_path = NULL;
>> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
>>       struct rdma_cm_event *cm_event;
>>       struct ibv_context *verbs;
>>       int ret = -EINVAL;
>>       int idx;
>> +    char arr[8];
>>   
>>       ret = rdma_get_cm_event(rdma->channel, &cm_event);
>>       if (ret) {
>> @@ -3375,13 +3363,17 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>>           goto err_rdma_dest_wait;
>>       }
>>   
>> +    isock->host = rdma->host;
>> +    sprintf(arr,"%d", rdma->port);
>> +    isock->port = arr;
> While Inet ports are 16-bit, and so 65535 fits in a char[8], nothing
> at the QAPI parser level is enforcing this.
>
> IOW, someone can pass QEMU a QAPI config with port = 235252353253253253232
> and casue this sprintf to smash the stack.
>
> Also this is assigning a stack variable to isock->port which
> expects a heap variable. qapi_free_InetSocketAddress() will
> call free(isock->port) which will again crash.
>
> Just do
>
>    g_autoptr(InetSocketAddress) isock = g_new0(InetSocketAddress, 1);
>
>    isock->port = g_strdup_printf("%d", rdma->port);
Thanks Daniel. Will change this in next version of patchset. Is a 
protection for isock->host and isock->port needed here ?
>> +
>>       /*
>>        * 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;
>> @@ -3506,6 +3498,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>>   err_rdma_dest_wait:
>>       rdma->error_state = ret;
>>       qemu_rdma_cleanup(rdma);
>> +    qapi_free_InetSocketAddress(isock);
>> +    g_free(arr);
> Free'ing a stack variable
Ack, will delete both statements from here.
>>       g_free(rdma_return_path);
>>       return ret;
>>   }
>> @@ -4114,7 +4108,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;
>> @@ -4160,13 +4155,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
>>
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v4 3/8] migration: converts socket backend to accept MigrateAddress struct

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
>> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
>>> 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>
>>> ---
>>>   migration/exec.c      |  4 ++--
>>>   migration/exec.h      |  4 ++++
>>>   migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
>>>   migration/socket.c    | 34 +++++----------------------------
>>>   migration/socket.h    |  7 ++++---
>>>   5 files changed, 47 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/migration/exec.c b/migration/exec.c
>>> index 2bf882bbe1..c4a3293246 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)
> Even this, I will shift it to the 2nd patch, where I need to move
> exec_get_cmd_path() out accross other file (migration.c).

great.

>>>   {
>>>       g_autofree char *detected_path = g_new(char, MAX_PATH);
>>> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
>>>   }
>>>   #endif
>>>   -void exec_start_outgoing_migration(MigrationState *s, const char
>>> *command, Error **errp)
>>> +void exec_start_outgoing_migration(MigrationState *s, const char *command,
>>> +                                   Error **errp)
>>>   {
>>>       QIOChannel *ioc;
> Sure, Juan. Will change this in the 2nd patch itself instead of
> here. I am not very convinved why should we have a different patch all
> together for this, because we are just using this code outside this
> file in my opinion? But if you still think so, I can make a different
> patch for that.

It is up to you.


> Juan, I get your point. But I think, we won't be needing local_err at
> all, if I use g_autoptr for 'channel' and 'saddr' is a part of
> 'channel'. Let me have a v2 patchset and if it is still not
> convinving, we can have a discussion on this.
>> THis leaks 'channel', and free's 'saddr' which actually  belongs
>> to channel.
>>
>> With my comments on the previous patch suggesting g_autoptr for
>> 'channel', we don't need any free calls for 'saddr' or 'channel'.
>
> Right. With g_autoptr used for freeing 'channel' in last patch, we
> wont have to worry about freeing 'saddr' at all. Thanks Daniel
>
> if (local_err) {
>     qapi_free_SocketAddress(saddr);
>     error_propagate(errp, local_err);
>     return;
> }
> And after changing the position for assigning 'saddr' and using
> g_autoptr for 'channel' I believe we can get rid of 'local_error'
> variable too and replace it with 'errp'. Please suggest if I am
> missing something here. TIA!

great.  That is much better.

>>>   -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);
>>> -}
>>> -
> Actually Juan. I don't need this function at all, because parsing of
> uri into socketAddress using socket_parse is already done. So there is
> no use of having this function in the first place, so I decided to
> delete this fucntion all together. Same with incoming function.

What I mean is that this code was already there.  And it was doing it
wrong.  The parts that I corrected you were using this pattern, chcking
that err was NULL, intsead of cheking that saddr is NULL.

Later, Juan.

Re: [PATCH v4 4/8] migration: converts rdma backend to accept MigrateAddress struct

[Daniel P. Berrangé]

On Mon, May 15, 2023 at 08:08:57PM +0530, Het Gala wrote:
> 
> On 15/05/23 3:54 pm, Daniel P. Berrangé wrote:
> > On Fri, May 12, 2023 at 02:32:36PM +0000, Het Gala wrote:
> > > 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>
> > > ---
> > >   migration/migration.c |  8 ++++----
> > >   migration/rdma.c      | 38 ++++++++++++++++----------------------
> > >   migration/rdma.h      |  6 ++++--
> > >   3 files changed, 24 insertions(+), 28 deletions(-)
> > > 
> > > @@ -3360,10 +3346,12 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> > >                                               .private_data_len = sizeof(cap),
> > >                                            };
> > >       RDMAContext *rdma_return_path = NULL;
> > > +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
> > >       struct rdma_cm_event *cm_event;
> > >       struct ibv_context *verbs;
> > >       int ret = -EINVAL;
> > >       int idx;
> > > +    char arr[8];
> > >       ret = rdma_get_cm_event(rdma->channel, &cm_event);
> > >       if (ret) {
> > > @@ -3375,13 +3363,17 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> > >           goto err_rdma_dest_wait;
> > >       }
> > > +    isock->host = rdma->host;
> > > +    sprintf(arr,"%d", rdma->port);
> > > +    isock->port = arr;
> > While Inet ports are 16-bit, and so 65535 fits in a char[8], nothing
> > at the QAPI parser level is enforcing this.
> > 
> > IOW, someone can pass QEMU a QAPI config with port = 235252353253253253232
> > and casue this sprintf to smash the stack.
> > 
> > Also this is assigning a stack variable to isock->port which
> > expects a heap variable. qapi_free_InetSocketAddress() will
> > call free(isock->port) which will again crash.
> > 
> > Just do
> > 
> >    g_autoptr(InetSocketAddress) isock = g_new0(InetSocketAddress, 1);
> > 
> >    isock->port = g_strdup_printf("%d", rdma->port);
> Thanks Daniel. Will change this in next version of patchset. Is a protection
> for isock->host and isock->port needed here ?

This will be validated later by getaddrinfo() so IMHO QEMU doesn't
need todo anythgin


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 v4 5/8] migration: converts exec backend to accept MigrateAddress struct.

[Het Gala]

On 15/05/23 3:59 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:37PM +0000, Het Gala wrote:
>> 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>
>> ---
>>   migration/exec.c      | 58 ++++++++++++++++++++++++++++++++-----------
>>   migration/exec.h      |  4 +--
>>   migration/migration.c | 10 +++-----
>>   3 files changed, 50 insertions(+), 22 deletions(-)
>>
>> diff --git a/migration/exec.c b/migration/exec.c
>> index c4a3293246..210f4e9400 100644
>> --- a/migration/exec.c
>> +++ b/migration/exec.c
>> @@ -39,22 +39,51 @@ const char *exec_get_cmd_path(void)
>>   }
>>   #endif
>>   
>> -void exec_start_outgoing_migration(MigrationState *s, const char *command,
>> +/* 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;
>> +}
>> +

Juan for your comment : "I can't believe tat we have a list type and we 
don't have a length() function for that type."

I had seen come patches regarding adding utility for finding length of a 
strList in util.h file as a MACRO, but I think the patches have still 
not gone in.

>> +static void
>> +init_exec_array(strList *command, const 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);
>> +    const char **argv = g_malloc0(length * sizeof(const char *));
> g_malloc0 is almost never desirable to use, instead:
>
>          g_new0(const char *, length);
Ack. Will change that.
>>   
>> -    trace_migration_exec_outgoing(command);
>> +    init_exec_array(command, argv, errp);
>> +    char *new_command = g_strjoinv(" ", (char **)argv);
> Never freed - use
>
>     g_autofree char *new_command...
Ack.
>> +    trace_migration_exec_outgoing(new_command);
>>
>>       ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
>>                                                       O_RDWR,
>>                                                       errp));
>>       if (!ioc) {
>> +        g_free(argv);
>>           return;
>>       }
> argv needs freeing in success too. Simpler to declare it
> with
>
>      g_auto(GStrv) argv = .....
Yes, Ack.
>>   
>> @@ -72,21 +101,22 @@ 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);
>> +    const char **argv = g_malloc0(length * sizeof(const char *));
>> +
>> +    init_exec_array(command, argv, errp);
>> +    char *new_command = g_strjoinv(" ", (char **)argv);
>>   
>> -    trace_migration_exec_incoming(command);
>> +    trace_migration_exec_incoming(new_command);
>>       ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
>>                                                       O_RDWR,
>>                                                       errp));
>>       if (!ioc) {
>> +        g_free(argv);
>>           return;
>>       }
> All the same comments as the outgoing case.
Ack. Thanks Daniel for the inputs.
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v4 3/8] migration: converts socket backend to accept MigrateAddress struct

[Het Gala]

On 15/05/23 8:16 pm, Juan Quintela wrote:
> Het Gala <het.gala@nutanix.com> wrote:
>> On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
>>> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
>>>> 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>
>>>> ---
>>>>    migration/exec.c      |  4 ++--
>>>>    migration/exec.h      |  4 ++++
>>>>    migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
>>>>    migration/socket.c    | 34 +++++----------------------------
>>>>    migration/socket.h    |  7 ++++---
>>>>    5 files changed, 47 insertions(+), 46 deletions(-)
>>>>
>>>> diff --git a/migration/exec.c b/migration/exec.c
>>>> index 2bf882bbe1..c4a3293246 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)
>> Even this, I will shift it to the 2nd patch, where I need to move
>> exec_get_cmd_path() out accross other file (migration.c).
> great.
>
>>>>    {
>>>>        g_autofree char *detected_path = g_new(char, MAX_PATH);
>>>> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
>>>>    }
>>>>    #endif
>>>>    -void exec_start_outgoing_migration(MigrationState *s, const char
>>>> *command, Error **errp)
>>>> +void exec_start_outgoing_migration(MigrationState *s, const char *command,
>>>> +                                   Error **errp)
>>>>    {
>>>>        QIOChannel *ioc;
>> Sure, Juan. Will change this in the 2nd patch itself instead of
>> here. I am not very convinved why should we have a different patch all
>> together for this, because we are just using this code outside this
>> file in my opinion? But if you still think so, I can make a different
>> patch for that.
> It is up to you.

For now, I will keep it with 2nd patch. If any other maintainer also 
thinks that it should be a separate patch all together of windows 
support for exec, I will make a new patch for that. But thankyou for 
your suggestion 😁

>> Juan, I get your point. But I think, we won't be needing local_err at
>> all, if I use g_autoptr for 'channel' and 'saddr' is a part of
>> 'channel'. Let me have a v2 patchset and if it is still not
>> convinving, we can have a discussion on this.
>>> THis leaks 'channel', and free's 'saddr' which actually  belongs
>>> to channel.
>>>
>>> With my comments on the previous patch suggesting g_autoptr for
>>> 'channel', we don't need any free calls for 'saddr' or 'channel'.
>> Right. With g_autoptr used for freeing 'channel' in last patch, we
>> wont have to worry about freeing 'saddr' at all. Thanks Daniel
>>
>> if (local_err) {
>>      qapi_free_SocketAddress(saddr);
>>      error_propagate(errp, local_err);
>>      return;
>> }
>> And after changing the position for assigning 'saddr' and using
>> g_autoptr for 'channel' I believe we can get rid of 'local_error'
>> variable too and replace it with 'errp'. Please suggest if I am
>> missing something here. TIA!
> great.  That is much better.
Ack.
>>>>    -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);
>>>> -}
>>>> -
>> Actually Juan. I don't need this function at all, because parsing of
>> uri into socketAddress using socket_parse is already done. So there is
>> no use of having this function in the first place, so I decided to
>> delete this fucntion all together. Same with incoming function.
> What I mean is that this code was already there.  And it was doing it
> wrong.  The parts that I corrected you were using this pattern, chcking
> that err was NULL, intsead of cheking that saddr is NULL.
Yes, I get your point. But that function is useless if socket_parse() 
function is not needed. So have omitted the function all together as 
socket parsing is already done in earlier patches.
> Later, Juan.
Regards,
Het Gala

Re: [PATCH v4 4/8] migration: converts rdma backend to accept MigrateAddress struct

[Het Gala]

On 15/05/23 8:28 pm, Daniel P. Berrangé wrote:
> On Mon, May 15, 2023 at 08:08:57PM +0530, Het Gala wrote:
>> On 15/05/23 3:54 pm, Daniel P. Berrangé wrote:
>>> On Fri, May 12, 2023 at 02:32:36PM +0000, Het Gala wrote:
>>>> 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>
>>>> ---
>>>>    migration/migration.c |  8 ++++----
>>>>    migration/rdma.c      | 38 ++++++++++++++++----------------------
>>>>    migration/rdma.h      |  6 ++++--
>>>>    3 files changed, 24 insertions(+), 28 deletions(-)
>>>>
>>>> @@ -3360,10 +3346,12 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>>>>                                                .private_data_len = sizeof(cap),
>>>>                                             };
>>>>        RDMAContext *rdma_return_path = NULL;
>>>> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
>>>>        struct rdma_cm_event *cm_event;
>>>>        struct ibv_context *verbs;
>>>>        int ret = -EINVAL;
>>>>        int idx;
>>>> +    char arr[8];
>>>>        ret = rdma_get_cm_event(rdma->channel, &cm_event);
>>>>        if (ret) {
>>>> @@ -3375,13 +3363,17 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>>>>            goto err_rdma_dest_wait;
>>>>        }
>>>> +    isock->host = rdma->host;
>>>> +    sprintf(arr,"%d", rdma->port);
>>>> +    isock->port = arr;
>>> While Inet ports are 16-bit, and so 65535 fits in a char[8], nothing
>>> at the QAPI parser level is enforcing this.
>>>
>>> IOW, someone can pass QEMU a QAPI config with port = 235252353253253253232
>>> and casue this sprintf to smash the stack.
>>>
>>> Also this is assigning a stack variable to isock->port which
>>> expects a heap variable. qapi_free_InetSocketAddress() will
>>> call free(isock->port) which will again crash.
>>>
>>> Just do
>>>
>>>     g_autoptr(InetSocketAddress) isock = g_new0(InetSocketAddress, 1);
>>>
>>>     isock->port = g_strdup_printf("%d", rdma->port);
>> Thanks Daniel. Will change this in next version of patchset. Is a protection
>> for isock->host and isock->port needed here ?
> This will be validated later by getaddrinfo() so IMHO QEMU doesn't
> need todo anythgin
Yes. I will keep it as it is for now. Thanks
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v4 3/8] migration: converts socket backend to accept MigrateAddress struct

[Het Gala]

On 15/05/23 8:46 pm, Het Gala wrote:
>
> On 15/05/23 8:16 pm, Juan Quintela wrote:
>> Het Gala <het.gala@nutanix.com> wrote:
>>> On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
>>>> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
>>>>> 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>
>>>>> ---
>>>>>    migration/exec.c      |  4 ++--
>>>>>    migration/exec.h      |  4 ++++
>>>>>    migration/migration.c | 44 
>>>>> +++++++++++++++++++++++++++++++------------
>>>>>    migration/socket.c    | 34 +++++----------------------------
>>>>>    migration/socket.h    |  7 ++++---
>>>>>    5 files changed, 47 insertions(+), 46 deletions(-)
>>>>>
>>>>> diff --git a/migration/exec.c b/migration/exec.c
>>>>> index 2bf882bbe1..c4a3293246 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)
Maintainers please advice. If I want to see thatthe build is proper, how 
to enable WIN32 support on a CentOS guest operating system (what all 
dependencies to install, what to configure for a build which supports 
WIN32) ? Any pointers ?
>>> Even this, I will shift it to the 2nd patch, where I need to move
>>> exec_get_cmd_path() out accross other file (migration.c).
>> great.
>>
>>>>>    {
>>>>>        g_autofree char *detected_path = g_new(char, MAX_PATH);
>>>>> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
>>>>>    }
>>>>>    #endif
>>>>>    -void exec_start_outgoing_migration(MigrationState *s, const char
>>>>> *command, Error **errp)
>>>>> +void exec_start_outgoing_migration(MigrationState *s, const char 
>>>>> *command,
>>>>> +                                   Error **errp)
>>>>>    {
>>>>>        QIOChannel *ioc;
>>> Sure, Juan. Will change this in the 2nd patch itself instead of
>>> here. I am not very convinved why should we have a different patch all
>>> together for this, because we are just using this code outside this
>>> file in my opinion? But if you still think so, I can make a different
>>> patch for that.
>> It is up to you.
>
> For now, I will keep it with 2nd patch. If any other maintainer also 
> thinks that it should be a separate patch all together of windows 
> support for exec, I will make a new patch for that. But thankyou for 
> your suggestion 😁
>
>>> Juan, I get your point. But I think, we won't be needing local_err at
>>> all, if I use g_autoptr for 'channel' and 'saddr' is a part of
>>> 'channel'. Let me have a v2 patchset and if it is still not
>>> convinving, we can have a discussion on this.
>>>> THis leaks 'channel', and free's 'saddr' which actually  belongs
>>>> to channel.
>>>>
>>>> With my comments on the previous patch suggesting g_autoptr for
>>>> 'channel', we don't need any free calls for 'saddr' or 'channel'.
>>> Right. With g_autoptr used for freeing 'channel' in last patch, we
>>> wont have to worry about freeing 'saddr' at all. Thanks Daniel
>>>
>>> if (local_err) {
>>>      qapi_free_SocketAddress(saddr);
>>>      error_propagate(errp, local_err);
>>>      return;
>>> }
>>> And after changing the position for assigning 'saddr' and using
>>> g_autoptr for 'channel' I believe we can get rid of 'local_error'
>>> variable too and replace it with 'errp'. Please suggest if I am
>>> missing something here. TIA!
>> great.  That is much better.
> Ack.
>>>>>    -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);
>>>>> -}
>>>>> -
>>> Actually Juan. I don't need this function at all, because parsing of
>>> uri into socketAddress using socket_parse is already done. So there is
>>> no use of having this function in the first place, so I decided to
>>> delete this fucntion all together. Same with incoming function.
>> What I mean is that this code was already there.  And it was doing it
>> wrong.  The parts that I corrected you were using this pattern, chcking
>> that err was NULL, intsead of cheking that saddr is NULL.
> Yes, I get your point. But that function is useless if socket_parse() 
> function is not needed. So have omitted the function all together as 
> socket parsing is already done in earlier patches.
>> Later, Juan.
> Regards,
> Het Gala

Re: [PATCH v4 3/8] migration: converts socket backend to accept MigrateAddress struct

[Daniel P. Berrangé]

On Mon, May 15, 2023 at 09:58:37PM +0530, Het Gala wrote:
> 
> On 15/05/23 8:46 pm, Het Gala wrote:
> > 
> > On 15/05/23 8:16 pm, Juan Quintela wrote:
> > > Het Gala <het.gala@nutanix.com> wrote:
> > > > On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
> > > > > On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
> > > > > > 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>
> > > > > > ---
> > > > > >    migration/exec.c      |  4 ++--
> > > > > >    migration/exec.h      |  4 ++++
> > > > > >    migration/migration.c | 44
> > > > > > +++++++++++++++++++++++++++++++------------
> > > > > >    migration/socket.c    | 34 +++++----------------------------
> > > > > >    migration/socket.h    |  7 ++++---
> > > > > >    5 files changed, 47 insertions(+), 46 deletions(-)
> > > > > > 
> > > > > > diff --git a/migration/exec.c b/migration/exec.c
> > > > > > index 2bf882bbe1..c4a3293246 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)
> Maintainers please advice. If I want to see thatthe build is proper, how to
> enable WIN32 support on a CentOS guest operating system (what all
> dependencies to install, what to configure for a build which supports WIN32)
> ? Any pointers ?

QEMU does Windows build testing using Fedora, which ships mingw
packages. If you only have centos, then use docker/podman to
get a Fedora environment in a container locally.

Alternatively push to gitlab and run a CI pipeline (see docs/devel/ci.rst
for more info)

> > > > Even this, I will shift it to the 2nd patch, where I need to move
> > > > exec_get_cmd_path() out accross other file (migration.c).
> > > great.
> > > 
> > > > > >    {
> > > > > >        g_autofree char *detected_path = g_new(char, MAX_PATH);
> > > > > > @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
> > > > > >    }
> > > > > >    #endif
> > > > > >    -void exec_start_outgoing_migration(MigrationState *s, const char
> > > > > > *command, Error **errp)
> > > > > > +void exec_start_outgoing_migration(MigrationState *s,
> > > > > > const char *command,
> > > > > > +                                   Error **errp)
> > > > > >    {
> > > > > >        QIOChannel *ioc;
> > > > Sure, Juan. Will change this in the 2nd patch itself instead of
> > > > here. I am not very convinved why should we have a different patch all
> > > > together for this, because we are just using this code outside this
> > > > file in my opinion? But if you still think so, I can make a different
> > > > patch for that.
> > > It is up to you.
> > 
> > For now, I will keep it with 2nd patch. If any other maintainer also
> > thinks that it should be a separate patch all together of windows
> > support for exec, I will make a new patch for that. But thankyou for
> > your suggestion 😁
> > 
> > > > Juan, I get your point. But I think, we won't be needing local_err at
> > > > all, if I use g_autoptr for 'channel' and 'saddr' is a part of
> > > > 'channel'. Let me have a v2 patchset and if it is still not
> > > > convinving, we can have a discussion on this.
> > > > > THis leaks 'channel', and free's 'saddr' which actually  belongs
> > > > > to channel.
> > > > > 
> > > > > With my comments on the previous patch suggesting g_autoptr for
> > > > > 'channel', we don't need any free calls for 'saddr' or 'channel'.
> > > > Right. With g_autoptr used for freeing 'channel' in last patch, we
> > > > wont have to worry about freeing 'saddr' at all. Thanks Daniel
> > > > 
> > > > if (local_err) {
> > > >      qapi_free_SocketAddress(saddr);
> > > >      error_propagate(errp, local_err);
> > > >      return;
> > > > }
> > > > And after changing the position for assigning 'saddr' and using
> > > > g_autoptr for 'channel' I believe we can get rid of 'local_error'
> > > > variable too and replace it with 'errp'. Please suggest if I am
> > > > missing something here. TIA!
> > > great.  That is much better.
> > Ack.
> > > > > >    -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);
> > > > > > -}
> > > > > > -
> > > > Actually Juan. I don't need this function at all, because parsing of
> > > > uri into socketAddress using socket_parse is already done. So there is
> > > > no use of having this function in the first place, so I decided to
> > > > delete this fucntion all together. Same with incoming function.
> > > What I mean is that this code was already there.  And it was doing it
> > > wrong.  The parts that I corrected you were using this pattern, chcking
> > > that err was NULL, intsead of cheking that saddr is NULL.
> > Yes, I get your point. But that function is useless if socket_parse()
> > function is not needed. So have omitted the function all together as
> > socket parsing is already done in earlier patches.
> > > Later, Juan.
> > Regards,
> > Het Gala
> 

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 v4 6/8] migration: modified 'migrate' QAPI to accept 'channels' argument for migration

[Het Gala]

On 15/05/23 4:06 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:38PM +0000, Het Gala wrote:
>> MigrateChannelList ideally allows to connect accross multiple interfaces.
>>
>> Added MigrateChannelList struct as argument to 'migrate' qapi. Introduced
>> MigrateChannelList in hmp_migrate() and qmp_migrate() functions.
>>
>> Future patchset series plans to include multiple MigrateChannels
>> for multiple interfaces to be connected. That is the reason, 'MigrateChannelList'
>> is the preferred choice of argument over 'MigrateChannel' and making 'migrate'
>> QAPI future proof.
>>
>> For current patch, have just limit size of MigrateChannelList to 1 element as
>> a runtime check.
>>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>>   migration/migration-hmp-cmds.c | 113 ++++++++++++++++++++++++++++++++-
>>   migration/migration.c          |  17 ++++-
>>   qapi/migration.json            |  69 +++++++++++++++++++-
>>   3 files changed, 192 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index 4e9f00e7dc..f098d04542 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -51,6 +51,101 @@ static void migration_global_dump(Monitor *mon)
>>                      ms->clear_bitmap_shift);
>>   }
>>   
>> +static bool
>> +migrate_channel_from_qdict(MigrateChannel **channel,
>> +                           const QDict *qdict, Error **errp)
>> +{
>> +    Error *err = NULL;
>> +    const char *channeltype  = qdict_get_try_str(qdict, "channeltype");
>> +    const char *transport_str = qdict_get_try_str(qdict, "transport");
>> +    const char *socketaddr_type = qdict_get_try_str(qdict, "type");
>> +    const char *inet_host = qdict_get_try_str(qdict, "host");
>> +    const char *inet_port = qdict_get_try_str(qdict, "port");
>> +    const char *unix_path = qdict_get_try_str(qdict, "path");
>> +    const char *vsock_cid = qdict_get_try_str(qdict, "cid");
>> +    const char *vsock_port = qdict_get_try_str(qdict, "port");
>> +    const char *fd = qdict_get_try_str(qdict, "str");
>> +    QList *exec = qdict_get_qlist(qdict, "exec");
> THis seems to be implicitly defining a huge set of extra parameters
> for the migrate 'HMP' command, but none of it is actually defined
> at the hmp-commands.hx
I don't have a lot of knowledge on HMP commands. I had code changes here 
in HMP merely to to keep it compatible with QMP command as it used to 
call qmp_migrate() function.
> Do we even need todo this ?  For HMP it is not unreasonable to just
> keep using the URI syntax forever?

Daniel, I didn't completely understand your ask here. Are you implying 
that for the HMP wrapper on top of QMP, we should pass it as a string 
only to qmp_migrate() function ?

In that case, we won't be needing migrate_channel_from_qdict() function 
at all right ? Please guide.

>> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
>> +    MigrateChannelType channel_type;
>> +    MigrateTransport transport;
>> +    MigrateAddress *addr = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr = g_new(SocketAddress, 1);
>> +    SocketAddressType type;
>> +
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v4 6/8] migration: modified 'migrate' QAPI to accept 'channels' argument for migration

[Daniel P. Berrangé]

On Tue, May 16, 2023 at 11:18:16AM +0530, Het Gala wrote:
> 
> On 15/05/23 4:06 pm, Daniel P. Berrangé wrote:
> > On Fri, May 12, 2023 at 02:32:38PM +0000, Het Gala wrote:
> > > MigrateChannelList ideally allows to connect accross multiple interfaces.
> > > 
> > > Added MigrateChannelList struct as argument to 'migrate' qapi. Introduced
> > > MigrateChannelList in hmp_migrate() and qmp_migrate() functions.
> > > 
> > > Future patchset series plans to include multiple MigrateChannels
> > > for multiple interfaces to be connected. That is the reason, 'MigrateChannelList'
> > > is the preferred choice of argument over 'MigrateChannel' and making 'migrate'
> > > QAPI future proof.
> > > 
> > > For current patch, have just limit size of MigrateChannelList to 1 element as
> > > a runtime check.
> > > 
> > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > > ---
> > >   migration/migration-hmp-cmds.c | 113 ++++++++++++++++++++++++++++++++-
> > >   migration/migration.c          |  17 ++++-
> > >   qapi/migration.json            |  69 +++++++++++++++++++-
> > >   3 files changed, 192 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> > > index 4e9f00e7dc..f098d04542 100644
> > > --- a/migration/migration-hmp-cmds.c
> > > +++ b/migration/migration-hmp-cmds.c
> > > @@ -51,6 +51,101 @@ static void migration_global_dump(Monitor *mon)
> > >                      ms->clear_bitmap_shift);
> > >   }
> > > +static bool
> > > +migrate_channel_from_qdict(MigrateChannel **channel,
> > > +                           const QDict *qdict, Error **errp)
> > > +{
> > > +    Error *err = NULL;
> > > +    const char *channeltype  = qdict_get_try_str(qdict, "channeltype");
> > > +    const char *transport_str = qdict_get_try_str(qdict, "transport");
> > > +    const char *socketaddr_type = qdict_get_try_str(qdict, "type");
> > > +    const char *inet_host = qdict_get_try_str(qdict, "host");
> > > +    const char *inet_port = qdict_get_try_str(qdict, "port");
> > > +    const char *unix_path = qdict_get_try_str(qdict, "path");
> > > +    const char *vsock_cid = qdict_get_try_str(qdict, "cid");
> > > +    const char *vsock_port = qdict_get_try_str(qdict, "port");
> > > +    const char *fd = qdict_get_try_str(qdict, "str");
> > > +    QList *exec = qdict_get_qlist(qdict, "exec");
> > THis seems to be implicitly defining a huge set of extra parameters
> > for the migrate 'HMP' command, but none of it is actually defined
> > at the hmp-commands.hx
> I don't have a lot of knowledge on HMP commands. I had code changes here in
> HMP merely to to keep it compatible with QMP command as it used to call
> qmp_migrate() function.
> > Do we even need todo this ?  For HMP it is not unreasonable to just
> > keep using the URI syntax forever?
> 
> Daniel, I didn't completely understand your ask here. Are you implying that
> for the HMP wrapper on top of QMP, we should pass it as a string only to
> qmp_migrate() function ?

Yeah, that's my thought. HMP is targetted at humans and aims for
user friendliness, and does not need to have 100% parity with QMP.
For the multi-channel setup, I think that's going to be something
only mgmt apps do, unlikely humans using HMP need this.

> 
> In that case, we won't be needing migrate_channel_from_qdict() function at
> all right ? Please guide.

Yeah, I think it is redundant.

An earlier patch already added an API to convert a "char *uri" into a
MigrateChannel object. So we can keep HMP using a URI, but pass it to
QMP using the MigrateChannel struct.


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 v4 6/8] migration: modified 'migrate' QAPI to accept 'channels' argument for migration

[Het Gala]

On 16/05/23 2:27 pm, Daniel P. Berrangé wrote:
> On Tue, May 16, 2023 at 11:18:16AM +0530, Het Gala wrote:
>> On 15/05/23 4:06 pm, Daniel P. Berrangé wrote:
>>> On Fri, May 12, 2023 at 02:32:38PM +0000, Het Gala wrote:
>>>> MigrateChannelList ideally allows to connect accross multiple interfaces.
>>>>
>>>> @@ -51,6 +51,101 @@ static void migration_global_dump(Monitor *mon)
>>>>                       ms->clear_bitmap_shift);
>>>>    }
>>>> +static bool
>>>> +migrate_channel_from_qdict(MigrateChannel **channel,
>>>> +                           const QDict *qdict, Error **errp)
>>>> +{
>>>> +    Error *err = NULL;
>>>> +    const char *channeltype  = qdict_get_try_str(qdict, "channeltype");
>>>> +    const char *transport_str = qdict_get_try_str(qdict, "transport");
>>>> +    const char *socketaddr_type = qdict_get_try_str(qdict, "type");
>>>> +    const char *inet_host = qdict_get_try_str(qdict, "host");
>>>> +    const char *inet_port = qdict_get_try_str(qdict, "port");
>>>> +    const char *unix_path = qdict_get_try_str(qdict, "path");
>>>> +    const char *vsock_cid = qdict_get_try_str(qdict, "cid");
>>>> +    const char *vsock_port = qdict_get_try_str(qdict, "port");
>>>> +    const char *fd = qdict_get_try_str(qdict, "str");
>>>> +    QList *exec = qdict_get_qlist(qdict, "exec");
>>> THis seems to be implicitly defining a huge set of extra parameters
>>> for the migrate 'HMP' command, but none of it is actually defined
>>> at the hmp-commands.hx
>> I don't have a lot of knowledge on HMP commands. I had code changes here in
>> HMP merely to to keep it compatible with QMP command as it used to call
>> qmp_migrate() function.
>>> Do we even need todo this ?  For HMP it is not unreasonable to just
>>> keep using the URI syntax forever?
>> Daniel, I didn't completely understand your ask here. Are you implying that
>> for the HMP wrapper on top of QMP, we should pass it as a string only to
>> qmp_migrate() function ?
> Yeah, that's my thought. HMP is targetted at humans and aims for
> user friendliness, and does not need to have 100% parity with QMP.
> For the multi-channel setup, I think that's going to be something
> only mgmt apps do, unlikely humans using HMP need this.
Yes, I can agree.
>> In that case, we won't be needing migrate_channel_from_qdict() function at
>> all right ? Please guide.
> Yeah, I think it is redundant.
>
> An earlier patch already added an API to convert a "char *uri" into a
> MigrateChannel object. So we can keep HMP using a URI, but pass it to
> QMP using the MigrateChannel struct.

Ack.

IOW, now hmp_migrate() will pass NULL object in place of 'channels', and 
char *uri to the qmp_migrate(), and later 'uri' will be converted into 
MigrateChannel object anyways.

This might also cut many changes in the next patch, and if possible I 
can club Patch 6 and 7 together.

> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v4 0/8] migration: Modified 'migrate' and 'migrate-incoming' QAPI commands for migration

[Markus Armbruster]

Doesn't apply to current master anymore, and patchew couldn't apply it,
either[*].  I'll look at v5.  Thanks!


[*] https://patchew.org/QEMU/20230512143240.192504-1-het.gala@nutanix.com/

Re: [PATCH v4 8/8] migration: Introduced MigrateChannelList struct to migration code flow.

[Het Gala]

On 15/05/23 4:12 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:40PM +0000, Het Gala wrote:
>> Integrated MigrateChannelList with all transport backends (socket, exec
>> and rdma) for both source and destination migration code flow.
>>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>>   migration/migration.c | 95 +++++++++++++++++++++++++++----------------
>>   migration/socket.c    |  5 ++-
>>   2 files changed, 64 insertions(+), 36 deletions(-)
>>
>>       Error *local_err = NULL;
>> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
>>       MigrateAddress *addrs = g_new0(MigrateAddress, 1);
>>       SocketAddress *saddr;
>>       InetSocketAddress *isock = &addrs->u.rdma;
>> @@ -441,6 +442,7 @@ static bool migrate_uri_parse(const char *uri,
>>       }
>>   
>>       if (local_err) {
>> +        qapi_free_MigrateChannel(val);
>>           qapi_free_MigrateAddress(addrs);
>>           qapi_free_SocketAddress(saddr);
>>           qapi_free_InetSocketAddress(isock);
>> @@ -448,7 +450,9 @@ static bool migrate_uri_parse(const char *uri,
>>           return false;
>>       }
>>   
>> -    *channel = addrs;
>> +    val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
>> +    val->addr = addrs;
>> +    *channel = val;
>>       return true;
>>   }
>>   
>> @@ -457,8 +461,9 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>>                                             Error **errp)
>>   {
>>       Error *local_err = NULL;
>> -    MigrateAddress *channel = g_new0(MigrateAddress, 1);
>> +    MigrateAddress *addrs;
>>       SocketAddress *saddr;
>> +    MigrateChannel *channel = NULL;
>>   
>>       /*
>>        * Having preliminary checks for uri and channel
>> @@ -467,22 +472,30 @@ static void qemu_start_incoming_migration(const char *uri, bool 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;
>> -    }
>> -
>> -    /* URI is not suitable for migration? */
>> -    if (!migration_channels_and_uri_compatible(uri, errp)) {
>>           goto out;
>> -    }
>> +    } 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");
>> +            goto out;
>> +        }
>> +        channel = channels->value;
>> +    } else {
>> +        /* URI is not suitable for migration? */
>> +        if (!migration_channels_and_uri_compatible(uri, errp)) {
>> +            goto out;
>> +        }
> THis check only gets executed when the caller uses the old
> URI syntax. We need to it be run when using the modern
> MigrateChannel QAPI syntax too.
>
> IOW, migration_channels_and_uri_compatible() needs converting
> to take a 'MigrateChannel' object instead of URI, and then
> the check can be run after the URI -> MigrateCHannel conversion

Yes, Daniel. Got your point. Will change it in the next version.

For Juan's comments, I have not explored the test side code still, so is 
the idea to have some similar test functions like 
test_precopy_tcp_plain, test_precopy_unix_plain but with the new syntax 
? Please confirm this, and any advice on how appraoch this?

>>   
>> -    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
>> -        error_setg(errp, "Error parsing uri");
>> -        goto out;
>> +        if (uri && !migrate_uri_parse(uri, &channel, errp)) {
>> +            error_setg(errp, "Error parsing uri");
>> +            goto out;
>> +        }
>>       }
>>   
>> -    saddr = &channel->u.socket;
>> +    addrs = channel->addr;
>> +    saddr = &channel->addr->u.socket;
>>       qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>> -    if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
>> +    if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
>>           if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>>               saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>>               saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>> @@ -491,23 +504,25 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>>               fd_start_incoming_migration(saddr->u.fd.str, &local_err);
>>           }
>>   #ifdef CONFIG_RDMA
>> -    } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) {
>> -        rdma_start_incoming_migration(&channel->u.rdma, &local_err);
>> +    } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
>> +        rdma_start_incoming_migration(&addrs->u.rdma, &local_err);
>>   #endif
>> -    } else if (channel->transport == MIGRATE_TRANSPORT_EXEC) {
>> -        exec_start_incoming_migration(channel->u.exec.args, &local_err);
>> +    } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
>> +        exec_start_incoming_migration(addrs->u.exec.args, &local_err);
>>       } else {
>>           error_setg(errp, "unknown migration protocol: %s", uri);
>>       }
>>   
>>       if (local_err) {
>> +        qapi_free_MigrateAddress(addrs);
>>           qapi_free_SocketAddress(saddr);
>>           error_propagate(errp, local_err);
>>           return;
>>       }
>>   
>>   out:
>> -    qapi_free_MigrateAddress(channel);
>> +    qapi_free_MigrateChannel(channel);
>> +    return;
>>   }
>>   
>>   static void process_incoming_migration_bh(void *opaque)
>> @@ -1714,8 +1729,9 @@ void qmp_migrate(const char *uri, bool has_channels,
>>   {
>>       Error *local_err = NULL;
>>       MigrationState *s = migrate_get_current();
>> -    MigrateAddress *channel = g_new0(MigrateAddress, 1);
>> +    MigrateAddress *addrs;
>>       SocketAddress *saddr;
>> +    MigrateChannel *channel = NULL;
>>   
>>   struct SocketOutgoingArgs {
>>       SocketAddress *saddr;
>> @@ -114,12 +116,13 @@ void socket_start_outgoing_migration(MigrationState *s,
>>   {
>>       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);
>> -- 
>> 2.22.3
>>
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH v4 8/8] migration: Introduced MigrateChannelList struct to migration code flow.

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> On 15/05/23 4:12 pm, Daniel P. Berrangé wrote:
>> On Fri, May 12, 2023 at 02:32:40PM +0000, Het Gala wrote:
>>> Integrated MigrateChannelList with all transport backends (socket, exec
>>> and rdma) for both source and destination migration code flow.
>>>
>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>> ---
>>>   migration/migration.c | 95 +++++++++++++++++++++++++++----------------
>>>   migration/socket.c    |  5 ++-
>>>   2 files changed, 64 insertions(+), 36 deletions(-)
>>>
>>>       Error *local_err = NULL;
>>> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
>>>       MigrateAddress *addrs = g_new0(MigrateAddress, 1);
>>>       SocketAddress *saddr;
>>>       InetSocketAddress *isock = &addrs->u.rdma;
>>> @@ -441,6 +442,7 @@ static bool migrate_uri_parse(const char *uri,
>>>       }
>>>         if (local_err) {
>>> +        qapi_free_MigrateChannel(val);
>>>           qapi_free_MigrateAddress(addrs);
>>>           qapi_free_SocketAddress(saddr);
>>>           qapi_free_InetSocketAddress(isock);
>>> @@ -448,7 +450,9 @@ static bool migrate_uri_parse(const char *uri,
>>>           return false;
>>>       }
>>>   -    *channel = addrs;
>>> +    val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
>>> +    val->addr = addrs;
>>> +    *channel = val;
>>>       return true;
>>>   }
>>>   @@ -457,8 +461,9 @@ static void
>>> qemu_start_incoming_migration(const char *uri, bool has_channels,
>>>                                             Error **errp)
>>>   {
>>>       Error *local_err = NULL;
>>> -    MigrateAddress *channel = g_new0(MigrateAddress, 1);
>>> +    MigrateAddress *addrs;
>>>       SocketAddress *saddr;
>>> +    MigrateChannel *channel = NULL;
>>>         /*
>>>        * Having preliminary checks for uri and channel
>>> @@ -467,22 +472,30 @@ static void qemu_start_incoming_migration(const char *uri, bool 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;
>>> -    }
>>> -
>>> -    /* URI is not suitable for migration? */
>>> -    if (!migration_channels_and_uri_compatible(uri, errp)) {
>>>           goto out;
>>> -    }
>>> +    } 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");
>>> +            goto out;
>>> +        }
>>> +        channel = channels->value;
>>> +    } else {
>>> +        /* URI is not suitable for migration? */
>>> +        if (!migration_channels_and_uri_compatible(uri, errp)) {
>>> +            goto out;
>>> +        }
>> THis check only gets executed when the caller uses the old
>> URI syntax. We need to it be run when using the modern
>> MigrateChannel QAPI syntax too.
>>
>> IOW, migration_channels_and_uri_compatible() needs converting
>> to take a 'MigrateChannel' object instead of URI, and then
>> the check can be run after the URI -> MigrateCHannel conversion
>
> Yes, Daniel. Got your point. Will change it in the next version.
>
> For Juan's comments, I have not explored the test side code still, so
> is the idea to have some similar test functions like
> test_precopy_tcp_plain, test_precopy_unix_plain but with the new
> syntax ? Please confirm this, and any advice on how appraoch this?

There are several places that use this syntax:

    rsp = wait_command(to, "{ 'execute': 'migrate-incoming',"
                           "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");

Just change a couple of them to the new syntax.

I guess you will want to do the multifd tests with the new syntax, not
sure if it makes sense to also test the old one.

I guess you will also want to check that your are catching errors (like
different number of channels on source/destination).

Later, Juan.