[PATCH 0/5] migration: Modified 'migrate' QAPI command for migration

[PATCH 0/5] migration: Modified 'migrate' QAPI command for migration

[Het Gala]

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

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

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

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

-----------------------------------------------------------------------------

Author Het Gala (5):
  migration: Updated QAPI format for 'migrate' qemu monitor command
  migration: HMP side changes for modified 'migrate' QAPI design
  migration: Avoid multiple parsing of uri in migration code flow
  migration: Modified 'migrate-incoming' QAPI and HMP side changes on
    the destination interface.
  migration: Established connection for listener sockets on the dest
    interface

 migration/migration.c | 133 +++++++++++++++++++++++++++++----------
 migration/socket.c    |  31 +--------
 migration/socket.h    |   5 +-
 monitor/hmp-cmds.c    | 101 ++++++++++++++++++++++++++++-
 qapi/migration.json   | 143 ++++++++++++++++++++++++++++++++++++++++--
 softmmu/vl.c          |   2 +-
 6 files changed, 344 insertions(+), 71 deletions(-)

-- 
2.22.3

[PATCH 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

From: Author Het Gala <het.gala@nutanix.com>

Existing 'migrate' QAPI design enforces transport mechanism, ip address
of destination interface and corresponding port number in the form
of a unified string 'uri' parameter. This scheme does seem to have an issue
in it, i.e. double-level encoding of URIs.

The current patch maps existing QAPI design into a well-defined data
structure - 'MigrateChannel' only from the design perspective. Please note that
the existing 'uri' parameter is kept untouched for backward compatibility.

Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 qapi/migration.json | 121 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 119 insertions(+), 2 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 88ecf86ac8..753e187ce2 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1449,12 +1449,108 @@
 ##
 { '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'] }
+
+##
+# @MigrateSocketAddr:
+#
+# To support different type of socket.
+#
+# @socket-type: Different type of socket connections.
+#
+# Since 8.0
+##
+{ 'struct': 'MigrateSocketAddr',
+  'data': {'socket-type': 'SocketAddress' } }
+
+##
+# @MigrateExecAddr:
+ #
+ # Since 8.0
+ ##
+{ 'struct': 'MigrateExecAddr',
+   'data' : {'exec-str': 'str' } }
+
+##
+# @MigrateRdmaAddr:
+#
+# Since 8.0
+##
+{ 'struct': 'MigrateRdmaAddr',
+   'data' : {'rdma-str': 'str' } }
+
+##
+# @MigrateAddress:
+#
+# The options available for communication transport mechanisms for migration
+#
+# Since 8.0
+##
+{ 'union' : 'MigrateAddress',
+  'base' : { 'transport' : 'MigrateTransport'},
+  'discriminator' : 'transport',
+  'data' : {
+    'socket' : 'MigrateSocketAddr',
+    'exec' : 'MigrateExecAddr',
+    'rdma': 'MigrateRdmaAddr' } }
+
+##
+# @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: SocketAddress 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
+#
+# @channel: Struct containing migration channel type, along with all
+#           the details of destination interface required for initiating
+#           a migration stream.
 #
 # @blk: do block migration (full disk copy)
 #
@@ -1479,15 +1575,36 @@
 # 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. Both 'uri' and 'channel' arguments, are mututally exclusive but, atleast
+#    one of the two arguments should be present.
+#
 # Example:
 #
 # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
 # <- { "return": {} }
 #
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channel": { "channeltype": "main",
+#                        "addr": { "transport": "socket",
+#                                  "socket-type": { "type': "inet',
+#                                                   "host": "10.12.34.9",
+#                                                   "port": "1050" } } } } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": { "channel": { "channeltype": "main",
+#                                  "addr": { "transport": "exec",
+#                                            "exec-str": "/tmp/exec" } } } }
+# <- { "return": {} }
+#
 ##
 { 'command': 'migrate',
-  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
-           '*detach': 'bool', '*resume': 'bool' } }
+  'data': {'*uri': 'str', '*channel': 'MigrateChannel', '*blk': 'bool',
+           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
 
 ##
 # @migrate-incoming:
-- 
2.22.3

[PATCH 2/5] migration: HMP side changes for modified 'migrate' QAPI design

[Het Gala]

From: Author Het Gala <het.gala@nutanix.com>

hmp_migrate() accepts uri (backward compatibility) and a
well-defined struct for migration parameters, and with help of
migrate_channel_from_qdict() maps QDict's 'key':'value' pair
required for migration stream into MigrateChannel struct.

Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 migration/migration.c |  6 +--
 monitor/hmp-cmds.c    | 91 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 52b5d39244..1b6e62612a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2391,9 +2391,9 @@ 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, MigrateChannel *channel, 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();
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index ed78a87ddd..e44d96f5dc 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -999,6 +999,91 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict)
     qapi_free_AnnounceParameters(params);
 }
 
+static void
+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_str = qdict_get_try_str(qdict, "str");
+    const char *exec_str = qdict_get_try_str(qdict, "exec-str");
+    const char *rdma_str = qdict_get_try_str(qdict, "rdma-str");
+    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:
+        switch (type) {
+        case SOCKET_ADDRESS_TYPE_INET:
+            saddr->type = 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->type = SOCKET_ADDRESS_TYPE_UNIX;
+            saddr->u.q_unix.path = (char *)unix_path;
+            break;
+        case SOCKET_ADDRESS_TYPE_VSOCK:
+            saddr->type = 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->type = SOCKET_ADDRESS_TYPE_FD;
+            saddr->u.fd.str = (char *)fd_str;
+            break;
+        default:
+            break;
+        }
+        addr->u.socket.socket_type = saddr;
+        break;
+    case MIGRATE_TRANSPORT_EXEC:
+        addr->u.exec.exec_str = (char *)exec_str;
+        break;
+    case MIGRATE_TRANSPORT_RDMA:
+        addr->u.rdma.rdma_str = (char *)rdma_str;
+        break;
+    default:
+        break;
+    }
+
+    val->channeltype = channel_type;
+    val->addr = addr;
+    *channel = val;
+end:
+    error_propagate(errp, err);
+}
+
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
 {
     qmp_migrate_cancel(NULL);
@@ -1441,8 +1526,12 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
     const char *uri = qdict_get_str(qdict, "uri");
     Error *err = NULL;
 
-    qmp_migrate(uri, !!blk, blk, !!inc, inc,
+    MigrateChannel *channel = g_new0(MigrateChannel, 1);
+    migrate_channel_from_qdict(&channel, qdict, &err);
+
+    qmp_migrate(uri, channel, !!blk, blk, !!inc, inc,
                 false, false, true, resume, &err);
+    qapi_free_MigrateChannel(channel);
     if (hmp_handle_error(mon, err)) {
         return;
     }
-- 
2.22.3

[PATCH 3/5] migration: Avoid multiple parsing of uri in migration code flow

[Het Gala]

From: Author Het Gala <het.gala@nutanix.com>

Existing uri is encoded at multiple levels to extract the relevant
migration information.

The modified QAPI design maps migration parameters into MigrateChannel
struct before, thus avoiding double-level uri encoding.

socket_outgoing_migration() has been depricated as It's only purpose was
uri parsing.
Renamed socket_outgoing_migration_internal() as socket_outgoing_migration().
qemu_uri_parsing() has been introduced to parse uri string (backward
compatibility) and populate the MigrateChannel struct parameters. Note that
the function will no longer be needed once the 'uri' parameter is depricated.

Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 migration/migration.c | 78 +++++++++++++++++++++++++++++++++++--------
 migration/socket.c    | 15 +--------
 migration/socket.h    |  3 +-
 3 files changed, 67 insertions(+), 29 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 1b6e62612a..36de9f6a6b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -61,6 +61,7 @@
 #include "sysemu/cpus.h"
 #include "yank_functions.h"
 #include "sysemu/qtest.h"
+#include "qemu/sockets.h"
 
 #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed throttling */
 
@@ -486,6 +487,39 @@ void migrate_add_address(SocketAddress *address)
                       QAPI_CLONE(SocketAddress, address));
 }
 
+static void qemu_uri_parsing(const char *uri,
+                             MigrateChannel **channel,
+                             Error **errp)
+{
+    Error *local_err = NULL;
+    const char *p = NULL;
+    MigrateChannel *val = g_new0(MigrateChannel, 1);
+    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
+    SocketAddress *saddr = g_new0(SocketAddress, 1);
+
+    if (strstart(uri, "exec:", &p)) {
+        addrs->transport = MIGRATE_TRANSPORT_EXEC;
+        addrs->u.exec.exec_str = g_strdup(p + strlen("exec:"));
+    } else if (strstart(uri, "rdma:", NULL)) {
+        addrs->transport = MIGRATE_TRANSPORT_RDMA;
+        addrs->u.rdma.rdma_str = g_strdup(p + strlen("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.socket_type = saddr;
+    }
+    val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
+    val->addr = addrs;
+    *channel = val;
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
+}
+
 static void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p = NULL;
@@ -2397,7 +2431,8 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
 {
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    const char *p = NULL;
+    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
+    SocketAddress *saddr = g_new0(SocketAddress, 1);
 
     if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
                          has_resume && resume, errp)) {
@@ -2411,20 +2446,35 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
         }
     }
 
+    /*
+     * motive here is just to have checks and convert uri into
+     * socketaddress struct
+     */
+    if (uri && channel) {
+        error_setg(errp, "uri and channels options should be"
+                          "mutually exclusive");
+    } else if (uri) {
+        qemu_uri_parsing(uri, &channel, &local_err);
+    }
+
     migrate_protocol_allow_multi_channels(false);
-    if (strstart(uri, "tcp:", &p) ||
-        strstart(uri, "unix:", NULL) ||
-        strstart(uri, "vsock:", NULL)) {
-        migrate_protocol_allow_multi_channels(true);
-        socket_start_outgoing_migration(s, p ? p : uri, &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);
+    addrs = channel->addr;
+    saddr = channel->addr->u.socket.socket_type;
+    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) {
+            migrate_protocol_allow_multi_channels(true);
+            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 (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
+        rdma_start_outgoing_migration(s, addrs->u.rdma.rdma_str, &local_err);
+    #endif
+    } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
+        exec_start_outgoing_migration(s, addrs->u.exec.exec_str, &local_err);
     } else {
         if (!(has_resume && resume)) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
diff --git a/migration/socket.c b/migration/socket.c
index e6fdf3c5e1..ecf98b7e6b 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -107,8 +107,7 @@ out:
     object_unref(OBJECT(sioc));
 }
 
-static void
-socket_start_outgoing_migration_internal(MigrationState *s,
+void socket_start_outgoing_migration(MigrationState *s,
                                          SocketAddress *saddr,
                                          Error **errp)
 {
@@ -134,18 +133,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)
diff --git a/migration/socket.h b/migration/socket.h
index dc54df4e6c..95c9c166ec 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -19,6 +19,7 @@
 
 #include "io/channel.h"
 #include "io/task.h"
+#include "io/channel-socket.h"
 
 void socket_send_channel_create(QIOTaskFunc f, void *data);
 QIOChannel *socket_send_channel_create_sync(Error **errp);
@@ -26,6 +27,6 @@ int socket_send_channel_destroy(QIOChannel *send);
 
 void socket_start_incoming_migration(const char *str, Error **errp);
 
-void socket_start_outgoing_migration(MigrationState *s, const char *str,
+void socket_start_outgoing_migration(MigrationState *s, SocketAddress *saddr,
                                      Error **errp);
 #endif
-- 
2.22.3

[PATCH 4/5] migration: Modified 'migrate-incoming' QAPI and HMP side changes on the destination interface.

[Het Gala]

From: Author Het Gala <het.gala@nutanix.com>

'migrate-incoming' QAPI design have been modified into a well-defined struct
'MigrateChannel'. Similarly like the source side, modified design was
introduced on destination side mainly to prevent multiple-level encoding of
uri string.

The struct contains various fields for type of migration channel, type of
transport backends and various associated migration parameters with each
backends.

Please note that the 'uri' parameter is kept for backward compatibility.
HMP side changes similar to source interface , on dest interface uses
migrate_channel_from_qdict() to redirect migration parameters from QDict
into 'MigrateChannel' struct.

Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 migration/migration.c |  3 ++-
 monitor/hmp-cmds.c    | 10 ++++++++--
 qapi/migration.json   | 22 ++++++++++++++++++++--
 softmmu/vl.c          |  2 +-
 4 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 36de9f6a6b..838940fd55 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2237,7 +2237,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, MigrateChannel *channel,
+                          Error **errp)
 {
     Error *local_err = NULL;
     static bool once = true;
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index e44d96f5dc..7f45624c41 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1106,9 +1106,15 @@ 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);
+    migrate_channel_from_qdict(&channel, qdict, &err);
+    if (err) {
+        error_setg(&err, "error in retrieving channel from qdict");
+        return;
+    }
 
-    qmp_migrate_incoming(uri, &err);
-
+    qmp_migrate_incoming(uri, channel, &err);
+    qapi_free_MigrateChannel(channel);
     hmp_handle_error(mon, err);
 }
 
diff --git a/qapi/migration.json b/qapi/migration.json
index 753e187ce2..201b085715 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1613,7 +1613,11 @@
 # with -incoming defer
 #
 # @uri: The Uniform Resource Identifier identifying the source or
-#       address to listen on
+#       the address to listen on
+#
+# @channel: Struct containing migration channel type, along with
+#           all the details of the destination interface required
+#           for the address to listen on for migration stream.
 #
 # Returns: nothing on success
 #
@@ -1630,14 +1634,28 @@
 #
 # 3. The uri format is the same as for -incoming
 #
+# 4. The 'uri' and 'channel' arguments are mutually exclusive but, atleast
+#    one of the two arguments should be present.
+#
 # Example:
 #
 # -> { "execute": "migrate-incoming",
 #      "arguments": { "uri": "tcp::4446" } }
 # <- { "return": {} }
 #
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": { 'channeltype': 'main',
+#                        'addr': { 'transport': 'socket',
+#                                  'socket-type': {'type': 'inet',
+#                                  'host': '10.12.34.9',
+#                                  'port': '1050' } } } } }
+# <- { "return": {} }
+#
 ##
-{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
+{ 'command': 'migrate-incoming',
+             'data': {'*uri': 'str',
+                      '*channel': 'MigrateChannel'} }
 
 ##
 # @xen-save-devices-state:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 798e1dc933..7005be978a 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2614,7 +2614,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, NULL, &local_err);
             if (local_err) {
                 error_reportf_err(local_err, "-incoming %s: ", incoming);
                 exit(1);
-- 
2.22.3

[PATCH 5/5] migration: Established connection for listener sockets on the dest interface

[Het Gala]

From: Author Het Gala <het.gala@nutanix.com>

Modified 'migrate-incoming' QAPI supports MigrateChannel parameters to prevent
multi-level encodings of uri on the destination side.

socket_start_incoming_migration() has been depricated as it's only purpose was
uri parsing.
Renamed socket_outgoing_migration_internal() as socket_start_incoming_migration().
qemu_uri_parsing() is used to populate the new struct from 'uri' string
needed for migration connection. The function will no longer be used once the
'uri' parameter is depricated, as the parameters will already be mapped into
new struct.

Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 migration/migration.c | 48 ++++++++++++++++++++++++++++---------------
 migration/socket.c    | 16 ++-------------
 migration/socket.h    |  2 +-
 3 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 838940fd55..c70fd0ab4f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -520,27 +520,43 @@ static void qemu_uri_parsing(const char *uri,
     }
 }
 
-static void qemu_start_incoming_migration(const char *uri, Error **errp)
+static void qemu_start_incoming_migration(const char *uri,
+                                          MigrateChannel *channel,
+                                          Error **errp)
 {
-    const char *p = NULL;
+    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
+    SocketAddress *saddr = g_new0(SocketAddress, 1);
+
+    /*
+     * motive here is just to have checks and convert uri into
+     * socketaddress struct
+     */
+    if (uri && channel) {
+        error_setg(errp, "uri and channels options should be used "
+                          "mutually exclusive");
+    } else if (uri) {
+        qemu_uri_parsing(uri, &channel, errp);
+    }
 
     migrate_protocol_allow_multi_channels(false); /* reset it anyway */
     qapi_event_send_migration(MIGRATION_STATUS_SETUP);
-    if (strstart(uri, "tcp:", &p) ||
-        strstart(uri, "unix:", NULL) ||
-        strstart(uri, "vsock:", NULL)) {
-        migrate_protocol_allow_multi_channels(true);
-        socket_start_incoming_migration(p ? p : uri, errp);
+    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) {
+            migrate_protocol_allow_multi_channels(true);
+            socket_start_incoming_migration(saddr, errp);
+        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
+            fd_start_incoming_migration(saddr->u.fd.str, errp);
+        }
 #ifdef CONFIG_RDMA
-    } else if (strstart(uri, "rdma:", &p)) {
-        rdma_start_incoming_migration(p, errp);
+    } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
+        rdma_start_incomng_migration(addrs->u.rdma.rdma_str, 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 if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
+        exec_start_incoming_migration(addrs->u.exec.exec_str, errp);
     } else {
-        error_setg(errp, "unknown migration protocol: %s", uri);
+        error_setg(errp, "unknown migration protocol: %i", addrs->transport);
     }
 }
 
@@ -2256,7 +2272,7 @@ void qmp_migrate_incoming(const char *uri, MigrateChannel *channel,
         return;
     }
 
-    qemu_start_incoming_migration(uri, &local_err);
+    qemu_start_incoming_migration(uri, channel, &local_err);
 
     if (local_err) {
         yank_unregister_instance(MIGRATION_YANK_INSTANCE);
@@ -2292,7 +2308,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, NULL, errp);
 }
 
 void qmp_migrate_pause(Error **errp)
diff --git a/migration/socket.c b/migration/socket.c
index ecf98b7e6b..3558821298 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -158,9 +158,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();
@@ -198,14 +197,3 @@ socket_start_incoming_migration_internal(SocketAddress *saddr,
         qapi_free_SocketAddress(address);
     }
 }
-
-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 95c9c166ec..4769a2bdf9 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -25,7 +25,7 @@ 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, SocketAddress *saddr,
                                      Error **errp);
-- 
2.22.3

Re: [PATCH 0/5] migration: Modified 'migrate' QAPI command for migration

[Het Gala]

On 26/12/22 11:03 am, Het Gala wrote:
> Current QAPI 'migrate' command design (for initiating a migration
> stream) contains information regarding different migrate transport mechanism
> (tcp / unix / exec), dest-host IP address, and binding port number in form of
> a string. Thus the design does seem to have some design issues. Some of the
> issues, stated below are:
>
> 1. Use of string URIs is a data encoding scheme within a data encoding scheme.
>     QEMU code should directly be able to work with the results from QAPI,
>     without resorting to do a second level of parsing (eg. socket_parse()).
> 2. For features / parameters related to migration, the migration tunables needs
>     to be defined and updated upfront. For example, 'migrate-set-capability'
>     and 'migrate-set-parameter' is required to enable multifd capability and
>     multifd-number of channels respectively. Instead, 'Multifd-channels' can
>     directly be represented as a single additional parameter to 'migrate'
>     QAPI. 'migrate-set-capability' and 'migrate-set-parameter' commands could
>     be used for runtime tunables that need setting after migration has already
>     started.
>
> The current patchset focuses on solving the first problem of multi-level
> encoding of URIs. The patch defines 'migrate' command as a QAPI discriminated
> union for the various transport backends (like socket, exec and rdma), and on
> basis of transport backends, different migration parameters are defined.
>
> (uri) string -->  (channel) Channel-type
>                              Transport-type
>                              Migration parameters based on transport type

I hope all of you had nice a lovely christmas :) and wish you all a 
beautiful new year!!

Just a gentle reminder for patch review.
This patchset series focuses on the idea of modifying qemu 'migration' 
QAPI syntax into a well defined data structure.
Hoping for suggestions and active discussions on the patchset series 
from all the maintainers.


Regards,
Het Gala

Re: [PATCH 0/5] migration: Modified 'migrate' QAPI command for migration

[Het Gala]

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


On 02/01/23 12:48 pm, Het Gala wrote:
>
> On 26/12/22 11:03 am, Het Gala wrote:
>> Current QAPI 'migrate' command design (for initiating a migration
>> stream) contains information regarding different migrate transport 
>> mechanism
>> (tcp / unix / exec), dest-host IP address, and binding port number in 
>> form of
>> a string. Thus the design does seem to have some design issues. Some 
>> of the
>> issues, stated below are:
>>
>> 1. Use of string URIs is a data encoding scheme within a data 
>> encoding scheme.
>>     QEMU code should directly be able to work with the results from 
>> QAPI,
>>     without resorting to do a second level of parsing (eg. 
>> socket_parse()).
>> 2. For features / parameters related to migration, the migration 
>> tunables needs
>>     to be defined and updated upfront. For example, 
>> 'migrate-set-capability'
>>     and 'migrate-set-parameter' is required to enable multifd 
>> capability and
>>     multifd-number of channels respectively. Instead, 
>> 'Multifd-channels' can
>>     directly be represented as a single additional parameter to 
>> 'migrate'
>>     QAPI. 'migrate-set-capability' and 'migrate-set-parameter' 
>> commands could
>>     be used for runtime tunables that need setting after migration 
>> has already
>>     started.
>>
>> The current patchset focuses on solving the first problem of multi-level
>> encoding of URIs. The patch defines 'migrate' command as a QAPI 
>> discriminated
>> union for the various transport backends (like socket, exec and 
>> rdma), and on
>> basis of transport backends, different migration parameters are defined.
>>
>> (uri) string -->  (channel) Channel-type
>>                              Transport-type
>>                              Migration parameters based on transport 
>> type
>
> I hope all of you had nice a lovely christmas :) and wish you all a 
> beautiful new year!!
>
> Just a gentle reminder for patch review.
> This patchset series focuses on the idea of modifying qemu 'migration' 
> QAPI syntax into a well defined data structure.
> Hoping for suggestions and active discussions on the patchset series 
> from all the maintainers.
A gentle reminder once again :)
This patchset could prove to the base of changing wire protocol around 
migration QAPIs. It could help in taking other decisions regarding 
restructuring around live migration code base in future.
Would be glad to have some feedback on the patches from the maintainers. 
Waiting for a positive response from the upstream community.

Regards, Het Gala

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

Re: [PATCH 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Daniel P. Berrangé]

On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote:
> From: Author Het Gala <het.gala@nutanix.com>
> 
> Existing 'migrate' QAPI design enforces transport mechanism, ip address
> of destination interface and corresponding port number in the form
> of a unified string 'uri' parameter. This scheme does seem to have an issue
> in it, i.e. double-level encoding of URIs.
> 
> The current patch maps existing QAPI design into a well-defined data
> structure - 'MigrateChannel' only from the design perspective. Please note that
> the existing 'uri' parameter is kept untouched for backward compatibility.
> 
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  qapi/migration.json | 121 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 119 insertions(+), 2 deletions(-)
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 88ecf86ac8..753e187ce2 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1449,12 +1449,108 @@
>  ##
>  { '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'] }
> +
> +##
> +# @MigrateSocketAddr:
> +#
> +# To support different type of socket.
> +#
> +# @socket-type: Different type of socket connections.
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateSocketAddr',
> +  'data': {'socket-type': 'SocketAddress' } }
> +
> +##
> +# @MigrateExecAddr:
> + #
> + # Since 8.0
> + ##
> +{ 'struct': 'MigrateExecAddr',
> +   'data' : {'exec-str': 'str' } }

Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command
that is passed

  const char *argv[] = { "/bin/sh", "-c", command, NULL };

I have a strong preference for making it possible to avoid use
of shell when spawning commands, since much of thue time it is
not required and has the potential to open up vulnerabilities.
It would be nice to be able to just take the full argv directly
IOW

 { 'struct': 'MigrateExecAddr',
    'data' : {'argv': ['str'] } }

If the caller wants to keep life safe and simple now they can
use
   ["/bin/nc", "-U", "/some/sock"]

but if they still want to send it via shell, they can also do
so

   ["/bin/sh", "-c", "...arbitrary shell script code...."]

> +
> +##
> +# @MigrateRdmaAddr:
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateRdmaAddr',
> +   'data' : {'rdma-str': 'str' } }

Loooking at the RDMA code it takes the str, and treats it
as an IPv4 address:


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

so we really ought to accept an InetSocketAddress struct
directly 

 { 'struct': 'MigrateRdmaAddr',
    'data' : {'rdma-str': 'InetSocketAddress' } }



> +
> +##
> +# @MigrateAddress:
> +#
> +# The options available for communication transport mechanisms for migration
> +#
> +# Since 8.0
> +##
> +{ 'union' : 'MigrateAddress',
> +  'base' : { 'transport' : 'MigrateTransport'},
> +  'discriminator' : 'transport',
> +  'data' : {
> +    'socket' : 'MigrateSocketAddr',
> +    'exec' : 'MigrateExecAddr',
> +    'rdma': 'MigrateRdmaAddr' } }
> +
> +##
> +# @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: SocketAddress 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
> +#
> +# @channel: Struct containing migration channel type, along with all
> +#           the details of destination interface required for initiating
> +#           a migration stream.
>  #
>  # @blk: do block migration (full disk copy)
>  #
> @@ -1479,15 +1575,36 @@
>  # 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. Both 'uri' and 'channel' arguments, are mututally exclusive but, atleast


Minor typos                                   "mutually"                "at least"

> +#    one of the two arguments should be present.
> +#
>  # Example:
>  #
>  # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>  # <- { "return": {} }
>  #
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channel": { "channeltype": "main",
> +#                        "addr": { "transport": "socket",
> +#                                  "socket-type": { "type': "inet',
> +#                                                   "host": "10.12.34.9",
> +#                                                   "port": "1050" } } } } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": { "channel": { "channeltype": "main",
> +#                                  "addr": { "transport": "exec",
> +#                                            "exec-str": "/tmp/exec" } } } }

Will need updating given my feedback above

> +# <- { "return": {} }
> +#
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> -           '*detach': 'bool', '*resume': 'bool' } }
> +  'data': {'*uri': 'str', '*channel': 'MigrateChannel', '*blk': 'bool',
> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }

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 3/5] migration: Avoid multiple parsing of uri in migration code flow

[Daniel P. Berrangé]

On Mon, Dec 26, 2022 at 05:33:27AM +0000, Het Gala wrote:
> From: Author Het Gala <het.gala@nutanix.com>
> 
> Existing uri is encoded at multiple levels to extract the relevant
> migration information.
> 
> The modified QAPI design maps migration parameters into MigrateChannel
> struct before, thus avoiding double-level uri encoding.
> 
> socket_outgoing_migration() has been depricated as It's only purpose was
> uri parsing.
> Renamed socket_outgoing_migration_internal() as socket_outgoing_migration().
> qemu_uri_parsing() has been introduced to parse uri string (backward
> compatibility) and populate the MigrateChannel struct parameters. Note that
> the function will no longer be needed once the 'uri' parameter is depricated.
> 
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/migration.c | 78 +++++++++++++++++++++++++++++++++++--------
>  migration/socket.c    | 15 +--------
>  migration/socket.h    |  3 +-
>  3 files changed, 67 insertions(+), 29 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 1b6e62612a..36de9f6a6b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -61,6 +61,7 @@
>  #include "sysemu/cpus.h"
>  #include "yank_functions.h"
>  #include "sysemu/qtest.h"
> +#include "qemu/sockets.h"
>  
>  #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed throttling */
>  
> @@ -486,6 +487,39 @@ void migrate_add_address(SocketAddress *address)
>                        QAPI_CLONE(SocketAddress, address));
>  }
>  
> +static void qemu_uri_parsing(const char *uri,
> +                             MigrateChannel **channel,
> +                             Error **errp)

Coding style would prefer 'bool' instad of 'void'...

Also lets call this 'migrate_uri_parse'

> +{
> +    Error *local_err = NULL;
> +    const char *p = NULL;
> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr = g_new0(SocketAddress, 1);
> +
> +    if (strstart(uri, "exec:", &p)) {
> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
> +        addrs->u.exec.exec_str = g_strdup(p + strlen("exec:"));
> +    } else if (strstart(uri, "rdma:", NULL)) {
> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
> +        addrs->u.rdma.rdma_str = g_strdup(p + strlen("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.socket_type = saddr;
> +    }
> +    val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
> +    val->addr = addrs;
> +    *channel = val;
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);

     ...   'return false';
> +    }
  ...  'return true;'

> +}
> +
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p = NULL;
> @@ -2397,7 +2431,8 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
>  {
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
> -    const char *p = NULL;
> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr = g_new0(SocketAddress, 1);
>  
>      if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
>                           has_resume && resume, errp)) {
> @@ -2411,20 +2446,35 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
>          }
>      }
>  
> +    /*
> +     * motive here is just to have checks and convert uri into
> +     * socketaddress struct
> +     */
> +    if (uri && channel) {
> +        error_setg(errp, "uri and channels options should be"
> +                          "mutually exclusive");

Needs a 'return' statement after reporting the error. ALso, this
check should be moved to the earlier patch that introduced the
'channel' field.

> +    } else if (uri) {
> +        qemu_uri_parsing(uri, &channel, &local_err);

Needs to 'return' on error, eg

  } else if (uri && !qemu_uri_parsing(...))
      return;

> +    }
> +

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 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

On 09/01/23 7:37 pm, Daniel P. Berrangé wrote:
> On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote:
>> From: Author Het Gala <het.gala@nutanix.com>
>>
>> Existing 'migrate' QAPI design enforces transport mechanism, ip address
>> of destination interface and corresponding port number in the form
>> of a unified string 'uri' parameter. This scheme does seem to have an issue
>> in it, i.e. double-level encoding of URIs.
>>
>> The current patch maps existing QAPI design into a well-defined data
>> structure - 'MigrateChannel' only from the design perspective. Please note that
>> the existing 'uri' parameter is kept untouched for backward compatibility.
>>
>> +##
>> +# @MigrateExecAddr:
>> + #
>> + # Since 8.0
>> + ##
>> +{ 'struct': 'MigrateExecAddr',
>> +   'data' : {'exec-str': 'str' } }
> Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command
> that is passed
>
>    const char *argv[] = { "/bin/sh", "-c", command, NULL };
>
> I have a strong preference for making it possible to avoid use
> of shell when spawning commands, since much of thue time it is
> not required and has the potential to open up vulnerabilities.
> It would be nice to be able to just take the full argv directly
> IOW
>
>   { 'struct': 'MigrateExecAddr',
>      'data' : {'argv': ['str'] } }
>
> If the caller wants to keep life safe and simple now they can
> use
>     ["/bin/nc", "-U", "/some/sock"]
>
> but if they still want to send it via shell, they can also do
> so
>
>     ["/bin/sh", "-c", "...arbitrary shell script code...."]
Okay Daniel. I get your point and it makes sense too. This will also 
have some code changes in exec.c file I assume.
>> +
>> +##
>> +# @MigrateRdmaAddr:
>> +#
>> +# Since 8.0
>> +##
>> +{ 'struct': 'MigrateRdmaAddr',
>> +   'data' : {'rdma-str': 'str' } }
> Loooking at the RDMA code it takes the str, and treats it
> as an IPv4 address:
>
>
>          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);
>          }
>
> so we really ought to accept an InetSocketAddress struct
> directly
>
>   { 'struct': 'MigrateRdmaAddr',
>      'data' : {'rdma-str': 'InetSocketAddress' } }
>
Yes, It resembles to InetSocketAddress. Will make the relevant changes 
in rdma.c file.

With this, I had a small question in mind, do qemu need to develop / 
leverage some functionality to check the correctness for host or port.
So that if the user enters an invalid host address, they get an error 
message to enter correct address, if trying to migrate via qmp command 
line interface.
Please correct me if I am wrong here.
>   along with all
> +#           the details of destination interface required for initiating
> +#           a migration stream.
>   #
>   # @blk: do block migration (full disk copy)
>   #
> @@ -1479,15 +1575,36 @@
>   # 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. Both 'uri' and 'channel' arguments, are mututally exclusive but, atleast
>
> Minor typos                                   "mutually"                "at least"
Thanks for pointing that out Daniel. Ack.
>   # Example:
>   #
>   # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>   # <- { "return": {} }
>   #
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channel": { "channeltype": "main",
> +#                        "addr": { "transport": "socket",
> +#                                  "socket-type": { "type': "inet',
> +#                                                   "host": "10.12.34.9",
> +#                                                   "port": "1050" } } } } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": { "channel": { "channeltype": "main",
> +#                                  "addr": { "transport": "exec",
> +#                                            "exec-str": "/tmp/exec" } } } }
> Will need updating given my feedback above
Yeah sure.Thanks for the feedback above.
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH 3/5] migration: Avoid multiple parsing of uri in migration code flow

[Het Gala]

On 09/01/23 7:44 pm, Daniel P. Berrangé wrote:
> On Mon, Dec 26, 2022 at 05:33:27AM +0000, Het Gala wrote:
>> From: Author Het Gala <het.gala@nutanix.com>
>>
>> Existing uri is encoded at multiple levels to extract the relevant
>> migration information.
>>
>> The modified QAPI design maps migration parameters into MigrateChannel
>> struct before, thus avoiding double-level uri encoding.
>>
>> socket_outgoing_migration() has been depricated as It's only purpose was
>> uri parsing.
>> Renamed socket_outgoing_migration_internal() as socket_outgoing_migration().
>> qemu_uri_parsing() has been introduced to parse uri string (backward
>> compatibility) and populate the MigrateChannel struct parameters. Note that
>> the function will no longer be needed once the 'uri' parameter is depricated.
>>
>>   
>>   
>> @@ -486,6 +487,39 @@ void migrate_add_address(SocketAddress *address)
>>                         QAPI_CLONE(SocketAddress, address));
>>   }
>>   
>> +static void qemu_uri_parsing(const char *uri,
>> +                             MigrateChannel **channel,
>> +                             Error **errp)
> Coding style would prefer 'bool' instad of 'void'...
>
> Also lets call this 'migrate_uri_parse'
Sure, Ack. Will change it in the upcoming patch.
>> +    if (strstart(uri, "exec:", &p)) {
>> +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
>> +        addrs->u.exec.exec_str = g_strdup(p + strlen("exec:"));
>> +    } else if (strstart(uri, "rdma:", NULL)) {
>> +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
>> +        addrs->u.rdma.rdma_str = g_strdup(p + strlen("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.socket_type = saddr;
>> +    }
>> +    val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
>> +    val->addr = addrs;
>> +    *channel = val;
>> +
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>       ...   'return false';
>> +    }
>    ...  'return true;'
Ack.
>> +}
>> +
>>   static void qemu_start_incoming_migration(const char *uri, Error **errp)
>>   {
>>       const char *p = NULL;
>> @@ -2397,7 +2431,8 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
>>   {
>>       Error *local_err = NULL;
>>       MigrationState *s = migrate_get_current();
>> -    const char *p = NULL;
>> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
>> +    SocketAddress *saddr = g_new0(SocketAddress, 1);
>>   
>>       if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
>>                            has_resume && resume, errp)) {
>> @@ -2411,20 +2446,35 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
>>           }
>>       }
>>   
>> +    /*
>> +     * motive here is just to have checks and convert uri into
>> +     * socketaddress struct
>> +     */
>> +    if (uri && channel) {
>> +        error_setg(errp, "uri and channels options should be"
>> +                          "mutually exclusive");
> Needs a 'return' statement after reporting the error. ALso, this
> check should be moved to the earlier patch that introduced the
> 'channel' field.
Okay sure. Will introduce the check in 2nd patch itself.
>> +    } else if (uri) {
>> +        qemu_uri_parsing(uri, &channel, &local_err);
> Needs to 'return' on error, eg
>
>    } else if (uri && !qemu_uri_parsing(...))
>        return;
Ack.
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Daniel P. Berrangé]

On Tue, Jan 10, 2023 at 01:09:35PM +0530, Het Gala wrote:
> 
> On 09/01/23 7:37 pm, Daniel P. Berrangé wrote:
> > On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote:
> > > From: Author Het Gala <het.gala@nutanix.com>
> > > 
> > > Existing 'migrate' QAPI design enforces transport mechanism, ip address
> > > of destination interface and corresponding port number in the form
> > > of a unified string 'uri' parameter. This scheme does seem to have an issue
> > > in it, i.e. double-level encoding of URIs.
> > > 
> > > The current patch maps existing QAPI design into a well-defined data
> > > structure - 'MigrateChannel' only from the design perspective. Please note that
> > > the existing 'uri' parameter is kept untouched for backward compatibility.
> > > 
> > > +##
> > > +# @MigrateExecAddr:
> > > + #
> > > + # Since 8.0
> > > + ##
> > > +{ 'struct': 'MigrateExecAddr',
> > > +   'data' : {'exec-str': 'str' } }
> > Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command
> > that is passed
> > 
> >    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> > 
> > I have a strong preference for making it possible to avoid use
> > of shell when spawning commands, since much of thue time it is
> > not required and has the potential to open up vulnerabilities.
> > It would be nice to be able to just take the full argv directly
> > IOW
> > 
> >   { 'struct': 'MigrateExecAddr',
> >      'data' : {'argv': ['str'] } }
> > 
> > If the caller wants to keep life safe and simple now they can
> > use
> >     ["/bin/nc", "-U", "/some/sock"]
> > 
> > but if they still want to send it via shell, they can also do
> > so
> > 
> >     ["/bin/sh", "-c", "...arbitrary shell script code...."]
> Okay Daniel. I get your point and it makes sense too. This will also have
> some code changes in exec.c file I assume.
> > > +
> > > +##
> > > +# @MigrateRdmaAddr:
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'struct': 'MigrateRdmaAddr',
> > > +   'data' : {'rdma-str': 'str' } }
> > Loooking at the RDMA code it takes the str, and treats it
> > as an IPv4 address:
> > 
> > 
> >          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);
> >          }
> > 
> > so we really ought to accept an InetSocketAddress struct
> > directly
> > 
> >   { 'struct': 'MigrateRdmaAddr',
> >      'data' : {'rdma-str': 'InetSocketAddress' } }
> > 
> Yes, It resembles to InetSocketAddress. Will make the relevant changes in
> rdma.c file.
> 
> With this, I had a small question in mind, do qemu need to develop /
> leverage some functionality to check the correctness for host or port.
> So that if the user enters an invalid host address, they get an error
> message to enter correct address, if trying to migrate via qmp command line
> interface.

When the RDMA code uses the host address to resolve the RDMA
endpoint, it will fail and report an error back.


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 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

On 10/01/23 3:02 pm, Daniel P. Berrangé wrote:
> On Tue, Jan 10, 2023 at 01:09:35PM +0530, Het Gala wrote:
>> On 09/01/23 7:37 pm, Daniel P. Berrangé wrote:
>>
>>> Loooking at the RDMA code it takes the str, and treats it
>>> as an IPv4 address:
>>>
>>>
>>>           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);
>>>           }
>>>
>>> so we really ought to accept an InetSocketAddress struct
>>> directly
>>>
>>>    { 'struct': 'MigrateRdmaAddr',
>>>       'data' : {'rdma-str': 'InetSocketAddress' } }
>>>
>> Yes, It resembles to InetSocketAddress. Will make the relevant changes in
>> rdma.c file.
>>
>> With this, I had a small question in mind, do qemu need to develop /
>> leverage some functionality to check the correctness for host or port.
>> So that if the user enters an invalid host address, they get an error
>> message to enter correct address, if trying to migrate via qmp command line
>> interface.
> When the RDMA code uses the host address to resolve the RDMA
> endpoint, it will fail and report an error back.
>
>
> With regards,
> Daniel

Okay, understood. Thanks for the explanation Daniel

Regards,
Het Gala

Re: [PATCH 5/5] migration: Established connection for listener sockets on the dest interface

[Markus Armbruster]

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

> From: Author Het Gala <het.gala@nutanix.com>
>
> Modified 'migrate-incoming' QAPI supports MigrateChannel parameters to prevent
> multi-level encodings of uri on the destination side.
>
> socket_start_incoming_migration() has been depricated as it's only purpose was
> uri parsing.
> Renamed socket_outgoing_migration_internal() as socket_start_incoming_migration().
> qemu_uri_parsing() is used to populate the new struct from 'uri' string
> needed for migration connection. The function will no longer be used once the
> 'uri' parameter is depricated, as the parameters will already be mapped into
> new struct.
>
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/migration.c | 48 ++++++++++++++++++++++++++++---------------
>  migration/socket.c    | 16 ++-------------
>  migration/socket.h    |  2 +-
>  3 files changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 838940fd55..c70fd0ab4f 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -520,27 +520,43 @@ static void qemu_uri_parsing(const char *uri,
>      }
>  }
>  
> -static void qemu_start_incoming_migration(const char *uri, Error **errp)
> +static void qemu_start_incoming_migration(const char *uri,
> +                                          MigrateChannel *channel,
> +                                          Error **errp)
>  {
> -    const char *p = NULL;
> +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr = g_new0(SocketAddress, 1);
> +
> +    /*
> +     * motive here is just to have checks and convert uri into
> +     * socketaddress struct
> +     */
> +    if (uri && channel) {
> +        error_setg(errp, "uri and channels options should be used "
> +                          "mutually exclusive");
> +    } else if (uri) {
> +        qemu_uri_parsing(uri, &channel, errp);
> +    }
>  
>      migrate_protocol_allow_multi_channels(false); /* reset it anyway */
>      qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> -    if (strstart(uri, "tcp:", &p) ||
> -        strstart(uri, "unix:", NULL) ||
> -        strstart(uri, "vsock:", NULL)) {
> -        migrate_protocol_allow_multi_channels(true);
> -        socket_start_incoming_migration(p ? p : uri, errp);
> +    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) {
> +            migrate_protocol_allow_multi_channels(true);
> +            socket_start_incoming_migration(saddr, errp);
> +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> +            fd_start_incoming_migration(saddr->u.fd.str, errp);
> +        }
>  #ifdef CONFIG_RDMA
> -    } else if (strstart(uri, "rdma:", &p)) {
> -        rdma_start_incoming_migration(p, errp);
> +    } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) {
> +        rdma_start_incomng_migration(addrs->u.rdma.rdma_str, errp);


Fails to compile:

    ../migration/migration.c: In function ‘qemu_start_incoming_migration’:
    ../migration/migration.c:554:9: error: implicit declaration of function ‘rdma_start_incomng_migration’; did you mean ‘rdma_start_incoming_migration’? [-Werror=implicit-function-declaration]
      554 |         rdma_start_incomng_migration(addrs->u.rdma.rdma_str, errp);
          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
          |         rdma_start_incoming_migration
    ../migration/migration.c:554:9: error: nested extern declaration of ‘rdma_start_incomng_migration’ [-Werror=nested-externs]

Please fix that, and also test RDMA.

>  #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 if (addrs->transport == MIGRATE_TRANSPORT_EXEC) {
> +        exec_start_incoming_migration(addrs->u.exec.exec_str, errp);
>      } else {
> -        error_setg(errp, "unknown migration protocol: %s", uri);
> +        error_setg(errp, "unknown migration protocol: %i", addrs->transport);
>      }
>  }
>  

[...]

Re: [PATCH 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

On 09/01/23 7:37 pm, Daniel P. Berrangé wrote:
> On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote:
>> From: Author Het Gala <het.gala@nutanix.com>
>>
>> Existing 'migrate' QAPI design enforces transport mechanism, ip address
>> of destination interface and corresponding port number in the form
>> of a unified string 'uri' parameter. This scheme does seem to have an issue
>> in it, i.e. double-level encoding of URIs.
>>
>> The current patch maps existing QAPI design into a well-defined data
>> structure - 'MigrateChannel' only from the design perspective. Please note that
>> the existing 'uri' parameter is kept untouched for backward compatibility.
>>
>> +##
>> +# @MigrateRdmaAddr:
>> +#
>> +# Since 8.0
>> +##
>> +{ 'struct': 'MigrateRdmaAddr',
>> +   'data' : {'rdma-str': 'str' } }
> Loooking at the RDMA code it takes the str, and treats it
> as an IPv4 address:
>
>
>          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);
>          }
>
> so we really ought to accept an InetSocketAddress struct
> directly
>
>   { 'struct': 'MigrateRdmaAddr',
>      'data' : {'rdma-str': 'InetSocketAddress' } }
>
Hi Daniel. I was going through the rdma code, and I think we also need 
'host_port' for rdma_return_path context 
https://github.com/qemu/qemu/commit/44bcfd45e9806c78d9d526d69b0590227d215a78. 
I dont have much understanding but If you have any suggestion or a 
workaround for this, please suggest :)
>
>> +
>> +##
>> +# @MigrateAddress:
>> +#
>> +# The options available for communication transport mechanisms for migration
>> +#
>> +# Since 8.0
>> +##
>>
> With regards,
> Daniel
Regards,
Het Gala

Re: [PATCH 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Daniel P. Berrangé]

On Fri, Jan 13, 2023 at 01:37:26PM +0530, Het Gala wrote:
> 
> On 09/01/23 7:37 pm, Daniel P. Berrangé wrote:
> > On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote:
> > > From: Author Het Gala <het.gala@nutanix.com>
> > > 
> > > Existing 'migrate' QAPI design enforces transport mechanism, ip address
> > > of destination interface and corresponding port number in the form
> > > of a unified string 'uri' parameter. This scheme does seem to have an issue
> > > in it, i.e. double-level encoding of URIs.
> > > 
> > > The current patch maps existing QAPI design into a well-defined data
> > > structure - 'MigrateChannel' only from the design perspective. Please note that
> > > the existing 'uri' parameter is kept untouched for backward compatibility.
> > > 
> > > +##
> > > +# @MigrateRdmaAddr:
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'struct': 'MigrateRdmaAddr',
> > > +   'data' : {'rdma-str': 'str' } }
> > Loooking at the RDMA code it takes the str, and treats it
> > as an IPv4 address:
> > 
> > 
> >          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);
> >          }
> > 
> > so we really ought to accept an InetSocketAddress struct
> > directly
> > 
> >   { 'struct': 'MigrateRdmaAddr',
> >      'data' : {'rdma-str': 'InetSocketAddress' } }
> > 
> Hi Daniel. I was going through the rdma code, and I think we also need
> 'host_port' for rdma_return_path context
> https://github.com/qemu/qemu/commit/44bcfd45e9806c78d9d526d69b0590227d215a78.
> I dont have much understanding but If you have any suggestion or a
> workaround for this, please suggest :)

The return path calls back into qemu_rdma_data_init() which takes
host_port, but you'll already be changing that to take InetSocketAddress,
so that'll be fine.

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 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote:
> > From: Author Het Gala <het.gala@nutanix.com>
> > 
> > Existing 'migrate' QAPI design enforces transport mechanism, ip address
> > of destination interface and corresponding port number in the form
> > of a unified string 'uri' parameter. This scheme does seem to have an issue
> > in it, i.e. double-level encoding of URIs.
> > 
> > The current patch maps existing QAPI design into a well-defined data
> > structure - 'MigrateChannel' only from the design perspective. Please note that
> > the existing 'uri' parameter is kept untouched for backward compatibility.
> > 
> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> > Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > ---
> >  qapi/migration.json | 121 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 119 insertions(+), 2 deletions(-)
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 88ecf86ac8..753e187ce2 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -1449,12 +1449,108 @@
> >  ##
> >  { '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'] }
> > +
> > +##
> > +# @MigrateSocketAddr:
> > +#
> > +# To support different type of socket.
> > +#
> > +# @socket-type: Different type of socket connections.
> > +#
> > +# Since 8.0
> > +##
> > +{ 'struct': 'MigrateSocketAddr',
> > +  'data': {'socket-type': 'SocketAddress' } }
> > +
> > +##
> > +# @MigrateExecAddr:
> > + #
> > + # Since 8.0
> > + ##
> > +{ 'struct': 'MigrateExecAddr',
> > +   'data' : {'exec-str': 'str' } }
> 
> Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command
> that is passed
> 
>   const char *argv[] = { "/bin/sh", "-c", command, NULL };
> 
> I have a strong preference for making it possible to avoid use
> of shell when spawning commands, since much of thue time it is
> not required and has the potential to open up vulnerabilities.
> It would be nice to be able to just take the full argv directly
> IOW
> 
>  { 'struct': 'MigrateExecAddr',
>     'data' : {'argv': ['str'] } }
> 
> If the caller wants to keep life safe and simple now they can
> use
>    ["/bin/nc", "-U", "/some/sock"]
> 
> but if they still want to send it via shell, they can also do
> so
> 
>    ["/bin/sh", "-c", "...arbitrary shell script code...."]
> 
> > +
> > +##
> > +# @MigrateRdmaAddr:
> > +#
> > +# Since 8.0
> > +##
> > +{ 'struct': 'MigrateRdmaAddr',
> > +   'data' : {'rdma-str': 'str' } }
> 
> Loooking at the RDMA code it takes the str, and treats it
> as an IPv4 address:
> 
> 
>         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);
>         } 
> 
> so we really ought to accept an InetSocketAddress struct
> directly 
> 
>  { 'struct': 'MigrateRdmaAddr',
>     'data' : {'rdma-str': 'InetSocketAddress' } }

I think that's probably the right thing to do; there is a native RDMA
addressing scheme that people occasionally (once a decade or so)
ask about but I don't think we've ever supported it.

Dave

> 
> 
> > +
> > +##
> > +# @MigrateAddress:
> > +#
> > +# The options available for communication transport mechanisms for migration
> > +#
> > +# Since 8.0
> > +##
> > +{ 'union' : 'MigrateAddress',
> > +  'base' : { 'transport' : 'MigrateTransport'},
> > +  'discriminator' : 'transport',
> > +  'data' : {
> > +    'socket' : 'MigrateSocketAddr',
> > +    'exec' : 'MigrateExecAddr',
> > +    'rdma': 'MigrateRdmaAddr' } }
> > +
> > +##
> > +# @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: SocketAddress 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
> > +#
> > +# @channel: Struct containing migration channel type, along with all
> > +#           the details of destination interface required for initiating
> > +#           a migration stream.
> >  #
> >  # @blk: do block migration (full disk copy)
> >  #
> > @@ -1479,15 +1575,36 @@
> >  # 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. Both 'uri' and 'channel' arguments, are mututally exclusive but, atleast
> 
> 
> Minor typos                                   "mutually"                "at least"
> 
> > +#    one of the two arguments should be present.
> > +#
> >  # Example:
> >  #
> >  # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
> >  # <- { "return": {} }
> >  #
> > +# -> { "execute": "migrate",
> > +#      "arguments": {
> > +#          "channel": { "channeltype": "main",
> > +#                        "addr": { "transport": "socket",
> > +#                                  "socket-type": { "type': "inet',
> > +#                                                   "host": "10.12.34.9",
> > +#                                                   "port": "1050" } } } } }
> > +# <- { "return": {} }
> > +#
> > +# -> { "execute": "migrate",
> > +#      "arguments": { "channel": { "channeltype": "main",
> > +#                                  "addr": { "transport": "exec",
> > +#                                            "exec-str": "/tmp/exec" } } } }
> 
> Will need updating given my feedback above
> 
> > +# <- { "return": {} }
> > +#
> >  ##
> >  { 'command': 'migrate',
> > -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> > -           '*detach': 'bool', '*resume': 'bool' } }
> > +  'data': {'*uri': 'str', '*channel': 'MigrateChannel', '*blk': 'bool',
> > +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
> 
> 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 :|
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Dr. David Alan Gilbert]

* Het Gala (het.gala@nutanix.com) wrote:
> From: Author Het Gala <het.gala@nutanix.com>
> 
> Existing 'migrate' QAPI design enforces transport mechanism, ip address
> of destination interface and corresponding port number in the form
> of a unified string 'uri' parameter. This scheme does seem to have an issue
> in it, i.e. double-level encoding of URIs.
> 
> The current patch maps existing QAPI design into a well-defined data
> structure - 'MigrateChannel' only from the design perspective. Please note that
> the existing 'uri' parameter is kept untouched for backward compatibility.
> 
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  qapi/migration.json | 121 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 119 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 88ecf86ac8..753e187ce2 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1449,12 +1449,108 @@
>  ##
>  { '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'] }
> +
> +##
> +# @MigrateSocketAddr:
> +#
> +# To support different type of socket.
> +#
> +# @socket-type: Different type of socket connections.
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateSocketAddr',
> +  'data': {'socket-type': 'SocketAddress' } }
> +
> +##
> +# @MigrateExecAddr:
> + #
> + # Since 8.0
> + ##
> +{ 'struct': 'MigrateExecAddr',
> +   'data' : {'exec-str': 'str' } }
> +
> +##
> +# @MigrateRdmaAddr:
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateRdmaAddr',
> +   'data' : {'rdma-str': 'str' } }
> +
> +##
> +# @MigrateAddress:
> +#
> +# The options available for communication transport mechanisms for migration
> +#
> +# Since 8.0
> +##
> +{ 'union' : 'MigrateAddress',
> +  'base' : { 'transport' : 'MigrateTransport'},
> +  'discriminator' : 'transport',
> +  'data' : {
> +    'socket' : 'MigrateSocketAddr',
> +    'exec' : 'MigrateExecAddr',
> +    'rdma': 'MigrateRdmaAddr' } }
> +
> +##
> +# @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: SocketAddress of destination interface
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateChannel',
> +  'data' : {
> +	'channeltype' : 'MigrateChannelType',
> +	'addr' : 'MigrateAddress' } }
> +

The presence of the channeltype field suggests you're going to try to
support multiple addresses; that's OK, but can you show an example of
how that might look in the migrate command below?

Dave

>  ##
>  # @migrate:
>  #
>  # Migrates the current running guest to another Virtual Machine.
>  #
>  # @uri: the Uniform Resource Identifier of the destination VM
> +#       for migration thread
> +#
> +# @channel: Struct containing migration channel type, along with all
> +#           the details of destination interface required for initiating
> +#           a migration stream.
>  #
>  # @blk: do block migration (full disk copy)
>  #
> @@ -1479,15 +1575,36 @@
>  # 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. Both 'uri' and 'channel' arguments, are mututally exclusive but, atleast
> +#    one of the two arguments should be present.
> +#
>  # Example:
>  #
>  # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>  # <- { "return": {} }
>  #
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channel": { "channeltype": "main",
> +#                        "addr": { "transport": "socket",
> +#                                  "socket-type": { "type': "inet',
> +#                                                   "host": "10.12.34.9",
> +#                                                   "port": "1050" } } } } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": { "channel": { "channeltype": "main",
> +#                                  "addr": { "transport": "exec",
> +#                                            "exec-str": "/tmp/exec" } } } }
> +# <- { "return": {} }
> +#
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> -           '*detach': 'bool', '*resume': 'bool' } }
> +  'data': {'*uri': 'str', '*channel': 'MigrateChannel', '*blk': 'bool',
> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>  
>  ##
>  # @migrate-incoming:
> -- 
> 2.22.3
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/5] migration: Modified 'migrate' QAPI command for migration

[Claudio Fontana]

Hi,

On 12/26/22 06:33, Het Gala wrote:
> Current QAPI 'migrate' command design (for initiating a migration
> stream) contains information regarding different migrate transport mechanism
> (tcp / unix / exec), dest-host IP address, and binding port number in form of
> a string. Thus the design does seem to have some design issues. Some of the
> issues, stated below are:
> 
> 1. Use of string URIs is a data encoding scheme within a data encoding scheme.
>    QEMU code should directly be able to work with the results from QAPI,
>    without resorting to do a second level of parsing (eg. socket_parse()).
> 2. For features / parameters related to migration, the migration tunables needs
>    to be defined and updated upfront. For example, 'migrate-set-capability'
>    and 'migrate-set-parameter' is required to enable multifd capability and
>    multifd-number of channels respectively. Instead, 'Multifd-channels' can
>    directly be represented as a single additional parameter to 'migrate'
>    QAPI. 'migrate-set-capability' and 'migrate-set-parameter' commands could
>    be used for runtime tunables that need setting after migration has already
>    started.

Is efficient and parallel migration to file of large VMs in scope for this design?

Thanks,

Claudio

> 
> The current patchset focuses on solving the first problem of multi-level
> encoding of URIs. The patch defines 'migrate' command as a QAPI discriminated
> union for the various transport backends (like socket, exec and rdma), and on
> basis of transport backends, different migration parameters are defined.
> 
> (uri) string -->  (channel) Channel-type
>                             Transport-type
>                             Migration parameters based on transport type
> 
> -----------------------------------------------------------------------------
> 
> Author Het Gala (5):
>   migration: Updated QAPI format for 'migrate' qemu monitor command
>   migration: HMP side changes for modified 'migrate' QAPI design
>   migration: Avoid multiple parsing of uri in migration code flow
>   migration: Modified 'migrate-incoming' QAPI and HMP side changes on
>     the destination interface.
>   migration: Established connection for listener sockets on the dest
>     interface
> 
>  migration/migration.c | 133 +++++++++++++++++++++++++++++----------
>  migration/socket.c    |  31 +--------
>  migration/socket.h    |   5 +-
>  monitor/hmp-cmds.c    | 101 ++++++++++++++++++++++++++++-
>  qapi/migration.json   | 143 ++++++++++++++++++++++++++++++++++++++++--
>  softmmu/vl.c          |   2 +-
>  6 files changed, 344 insertions(+), 71 deletions(-)
> 

Re: [PATCH 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

On 17/01/23 4:13 pm, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>> On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote:
>>> From: Author Het Gala <het.gala@nutanix.com>
>>>
>>> Existing 'migrate' QAPI design enforces transport mechanism, ip address
>>> of destination interface and corresponding port number in the form
>>> of a unified string 'uri' parameter. This scheme does seem to have an issue
>>> in it, i.e. double-level encoding of URIs.
>>>
>>> The current patch maps existing QAPI design into a well-defined data
>>> structure - 'MigrateChannel' only from the design perspective. Please note that
>>> the existing 'uri' parameter is kept untouched for backward compatibility.
>>>
>>> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>>> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command
>> that is passed
>>
>>    const char *argv[] = { "/bin/sh", "-c", command, NULL };
>>
>> I have a strong preference for making it possible to avoid use
>> of shell when spawning commands, since much of thue time it is
>> not required and has the potential to open up vulnerabilities.
>> It would be nice to be able to just take the full argv directly
>> IOW
>>
>>   { 'struct': 'MigrateExecAddr',
>>      'data' : {'argv': ['str'] } }
>>
>> If the caller wants to keep life safe and simple now they can
>> use
>>     ["/bin/nc", "-U", "/some/sock"]
>>
>> but if they still want to send it via shell, they can also do
>> so
>>
>>     ["/bin/sh", "-c", "...arbitrary shell script code...."]
>>
>>> +
>>> +##
>>> +# @MigrateRdmaAddr:
>>> +#
>>> +# Since 8.0
>>> +##
>>> +{ 'struct': 'MigrateRdmaAddr',
>>> +   'data' : {'rdma-str': 'str' } }
>> Loooking at the RDMA code it takes the str, and treats it
>> as an IPv4 address:
>>
>>
>>          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);
>>          }
>>
>> so we really ought to accept an InetSocketAddress struct
>> directly
>>
>>   { 'struct': 'MigrateRdmaAddr',
>>      'data' : {'rdma-str': 'InetSocketAddress' } }
> I think that's probably the right thing to do; there is a native RDMA
> addressing scheme that people occasionally (once a decade or so)
> ask about but I don't think we've ever supported it.
>
> Dave
Yes Dave. I will be implementing Rdma in form of InetSocketAddress only 
in the upcoming patch.
>>> +
>>> +##
>>> +# @MigrateAddress:
>>> +#
>>> +# The options available for communication transport mechanisms for migration
>>> +#
>>> +# Since 8.0
>>> +##
>>> +{ 'union' : 'MigrateAddress',
>>> +  'base' : { 'transport' : 'MigrateTransport'},
>>> +  'discriminator' : 'transport',
>>> +  'data' : {
>>> +    'socket' : 'MigrateSocketAddr',
>>> +    'exec' : 'MigrateExecAddr',
>>> +    'rdma': 'MigrateRdmaAddr' } }
>>> +
>> With regards,
>> Daniel
>> -- 
>> |: https://urldefense.proofpoint.com/v2/url?u=https-3A__berrange.com&d=DwIDAw&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=p8peRp4ioaDxoipqUtW15yQdVtCPnDBQv-1wk3r3y41SXWuI5JUUPMATMyMNDI4q&s=hukETUEPKU_01AyhkaMiQFWSRE1kUv84DdpSGvVjr1Q&e=       -o-    https://urldefense.proofpoint.com/v2/url?u=https-3A__www.flickr.com_photos_dberrange&d=DwIDAw&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=p8peRp4ioaDxoipqUtW15yQdVtCPnDBQv-
>>
Regards,
Het Gala

Re: [PATCH 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

[Het Gala]

On 17/01/23 4:17 pm, Dr. David Alan Gilbert wrote:
> * Het Gala (het.gala@nutanix.com) wrote:
>> From: Author Het Gala <het.gala@nutanix.com>
>>
>> Existing 'migrate' QAPI design enforces transport mechanism, ip address
>> of destination interface and corresponding port number in the form
>> of a unified string 'uri' parameter. This scheme does seem to have an issue
>> in it, i.e. double-level encoding of URIs.
>>
>> The current patch maps existing QAPI design into a well-defined data
>> structure - 'MigrateChannel' only from the design perspective. Please note that
>> the existing 'uri' parameter is kept untouched for backward compatibility.
>>
>> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>>   qapi/migration.json | 121 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 119 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 88ecf86ac8..753e187ce2 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1449,12 +1449,108 @@
>>   ##
>>   { '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'] }
>> +
>> +##
>> +# @MigrateSocketAddr:
>> +#
>> +# To support different type of socket.
>> +#
>> +# @socket-type: Different type of socket connections.
>> +#
>> +# Since 8.0
>> +##
>> +{ 'struct': 'MigrateSocketAddr',
>> +  'data': {'socket-type': 'SocketAddress' } }
>> +
>> +##
>> +# @MigrateExecAddr:
>> + #
>> + # Since 8.0
>> + ##
>> +{ 'struct': 'MigrateExecAddr',
>> +   'data' : {'exec-str': 'str' } }
>> +
>> +##
>> +# @MigrateRdmaAddr:
>> +#
>> +# Since 8.0
>> +##
>> +{ 'struct': 'MigrateRdmaAddr',
>> +   'data' : {'rdma-str': 'str' } }
>> +
>> +##
>> +# @MigrateAddress:
>> +#
>> +# The options available for communication transport mechanisms for migration
>> +#
>> +# Since 8.0
>> +##
>> +{ 'union' : 'MigrateAddress',
>> +  'base' : { 'transport' : 'MigrateTransport'},
>> +  'discriminator' : 'transport',
>> +  'data' : {
>> +    'socket' : 'MigrateSocketAddr',
>> +    'exec' : 'MigrateExecAddr',
>> +    'rdma': 'MigrateRdmaAddr' } }
>> +
>> +##
>> +# @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: SocketAddress of destination interface
>> +#
>> +# Since 8.0
>> +##
>> +{ 'struct': 'MigrateChannel',
>> +  'data' : {
>> +	'channeltype' : 'MigrateChannelType',
>> +	'addr' : 'MigrateAddress' } }
>> +
> The presence of the channeltype field suggests you're going to try to
> support multiple addresses; that's OK, but can you show an example of
> how that might look in the migrate command below?
>
> Dave

Yes Dave. Other options will be "data" (for multifd) and "post-copy" 
(for post-copy migration).

I am not very sure how will we represent "post-copy" part, but I will 
give you an example of how "main" and "data" Channels will be 
represented by an example below:

-> { "execute": "migrate",
        "arguments": {
             "channels": [ { 'channeltype': 'main',
                                   'addr': {'transport': 'socket',
                                              'socket-type': { 'type': 
'inet',
'host': '10.12.34.9', 'port': '1050'} }
                                 { 'channeltype': 'data',
                                   'addr': {'transport': 'socket',
                                              'socket-type': { 'type': 
'inet',
'host': '10.12.34.92', 'port': '1234'},
                                   'multifd-count': 5 },
                                 { 'channeltype': 'data',
                                   'addr': { 'transport': 'socket',
                                               'socket-type': {'type': 
'inet',
'host': '0.0.0.4', 'port': '3000'},
                                   'multifd-count': 3 } ] } }
So, 'data' channels will be used for multi-fd conection while 'main' 
will be used for just normal migration connection.
This 'data' channel option has been planned with a whole different 
patchset series in future.
Let me know if you have any more doubts regarding this :)
>>   #
>>   # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>>   # <- { "return": {} }
>>   #
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channel": { "channeltype": "main",
>> +#                        "addr": { "transport": "socket",
>> +#                                  "socket-type": { "type': "inet',
>> +#                                                   "host": "10.12.34.9",
>> +#                                                   "port": "1050" } } } } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +#      "arguments": { "channel": { "channeltype": "main",
>> +#                                  "addr": { "transport": "exec",
>> +#                                            "exec-str": "/tmp/exec" } } } }
>> +# <- { "return": {} }
>> +#
>>   ##
>>   { 'command': 'migrate',
>> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>> -           '*detach': 'bool', '*resume': 'bool' } }
>> +  'data': {'*uri': 'str', '*channel': 'MigrateChannel', '*blk': 'bool',
>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>   
>>   ##
>>   # @migrate-incoming:
Regards,
Het Gala

Re: [PATCH 0/5] migration: Modified 'migrate' QAPI command for migration

[Het Gala]

On 17/01/23 4:22 pm, Claudio Fontana wrote:
> Hi,
>
> On 12/26/22 06:33, Het Gala wrote:
>> Current QAPI 'migrate' command design (for initiating a migration
>> stream) contains information regarding different migrate transport mechanism
>> (tcp / unix / exec), dest-host IP address, and binding port number in form of
>> a string. Thus the design does seem to have some design issues. Some of the
>> issues, stated below are:
>>
>> 1. Use of string URIs is a data encoding scheme within a data encoding scheme.
>>     QEMU code should directly be able to work with the results from QAPI,
>>     without resorting to do a second level of parsing (eg. socket_parse()).
>> 2. For features / parameters related to migration, the migration tunables needs
>>     to be defined and updated upfront. For example, 'migrate-set-capability'
>>     and 'migrate-set-parameter' is required to enable multifd capability and
>>     multifd-number of channels respectively. Instead, 'Multifd-channels' can
>>     directly be represented as a single additional parameter to 'migrate'
>>     QAPI. 'migrate-set-capability' and 'migrate-set-parameter' commands could
>>     be used for runtime tunables that need setting after migration has already
>>     started.
> Is efficient and parallel migration to file of large VMs in scope for this design?
>
> Thanks,
>
> Claudio

This patch's design right now mainly focuses on revamping the design for 
'migrate' command.

In the upcomig patchset series in future, it will try to accomodate 
multifd as a feature in the same QAPI command and try to build multiple 
interface support on top of multifd feature. Main aim is to increase 
network bandwidth for migration with help of multiple interface and multifd.

Regards,
Het Gala.

Re: [PATCH 0/5] migration: Modified 'migrate' QAPI command for migration

[Claudio Fontana]

On 1/18/23 06:52, Het Gala wrote:
> 
> On 17/01/23 4:22 pm, Claudio Fontana wrote:
>> Hi,
>>
>> On 12/26/22 06:33, Het Gala wrote:
>>> Current QAPI 'migrate' command design (for initiating a migration
>>> stream) contains information regarding different migrate transport mechanism
>>> (tcp / unix / exec), dest-host IP address, and binding port number in form of
>>> a string. Thus the design does seem to have some design issues. Some of the
>>> issues, stated below are:
>>>
>>> 1. Use of string URIs is a data encoding scheme within a data encoding scheme.
>>>     QEMU code should directly be able to work with the results from QAPI,
>>>     without resorting to do a second level of parsing (eg. socket_parse()).
>>> 2. For features / parameters related to migration, the migration tunables needs
>>>     to be defined and updated upfront. For example, 'migrate-set-capability'
>>>     and 'migrate-set-parameter' is required to enable multifd capability and
>>>     multifd-number of channels respectively. Instead, 'Multifd-channels' can
>>>     directly be represented as a single additional parameter to 'migrate'
>>>     QAPI. 'migrate-set-capability' and 'migrate-set-parameter' commands could
>>>     be used for runtime tunables that need setting after migration has already
>>>     started.
>> Is efficient and parallel migration to file of large VMs in scope for this design?
>>
>> Thanks,
>>
>> Claudio
> 
> This patch's design right now mainly focuses on revamping the design for 
> 'migrate' command.
> 
> In the upcomig patchset series in future, it will try to accomodate 
> multifd as a feature in the same QAPI command and try to build multiple 
> interface support on top of multifd feature. Main aim is to increase 
> network bandwidth for migration with help of multiple interface and multifd.
> 
> Regards,
> Het Gala.


Understand, hopefully we can make sure that we can have a design that allows also increasing disk bandwidth for direct migration to disk.

Currently upstream migration to fast disks of medium to large size VMs is badly bottlenecked by qemu/libvirt interfaces.

Just FYI for existing work if interested see:

https://www.mail-archive.com/libvir-list@redhat.com/msg230248.html (not upstreamable, but dramatically improves VM save/restore performance)

https://lists.gnu.org/archive/html/qemu-devel/2022-10/msg02870.html (attempt to address the issue in QEMU project itself via migrating to file:///).

Ciao,

C