qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration
@ 2023-10-19 19:23 Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 01/14] migration: New QAPI type 'MigrateAddress' Fabiano Rosas
                   ` (13 more replies)
  0 siblings, 14 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

Hi,

I had to make this a new version because the file: tests are already
merged and Het's patches break them unless we also convert the file
transport to the new API.

I did the conversion and added separate patches as fixups so we can
review my additions separately.

Het's series untouched aside from conflict resolution.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/1043006299

v13 by Het Gala:
https://lore.kernel.org/r/20231012151052.154106-1-het.gala@nutanix.com

Fabiano Rosas (4):
  fixup! migration: New QAPI type 'MigrateAddress'
  fixup! migration: convert migration 'uri' into 'MigrateAddress'
  migration: Convert the file backend the new QAPI syntax
  fixup! migration: New migrate and migrate-incoming argument 'channels'

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

 migration/exec.c               |  74 ++++++++----
 migration/exec.h               |   8 +-
 migration/file.c               |  24 ++--
 migration/file.h               |  10 +-
 migration/migration-hmp-cmds.c |  27 ++++-
 migration/migration.c          | 203 ++++++++++++++++++++++++++-------
 migration/migration.h          |   3 +-
 migration/rdma.c               |  33 ++----
 migration/rdma.h               |   6 +-
 migration/socket.c             |  39 ++-----
 migration/socket.h             |   7 +-
 qapi/migration.json            | 174 +++++++++++++++++++++++++++-
 system/vl.c                    |   2 +-
 tests/qtest/migration-test.c   |   7 +-
 14 files changed, 467 insertions(+), 150 deletions(-)

-- 
2.35.3



^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v14 01/14] migration: New QAPI type 'MigrateAddress'
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 02/14] fixup! " Fabiano Rosas
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

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

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

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

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

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

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

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



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress'
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 01/14] migration: New QAPI type 'MigrateAddress' Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-20  5:06   ` Markus Armbruster
  2023-10-23 11:56   ` [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress' Daniel P. Berrangé
  2023-10-19 19:23 ` [PATCH v14 03/14] migration: convert migration 'uri' into 'MigrateAddress' Fabiano Rosas
                   ` (11 subsequent siblings)
  13 siblings, 2 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 qapi/migration.json | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index c352c7ac52..602cb706e3 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1519,10 +1519,25 @@
 #
 # @rdma: Migrate via RDMA.
 #
+# @file: Direct the migration stream to a file.
+#
 # Since 8.2
 ##
 { 'enum': 'MigrationAddressType',
-  'data': ['socket', 'exec', 'rdma'] }
+  'data': ['socket', 'exec', 'rdma', 'file'] }
+
+##
+# @FileMigrationArgs:
+#
+# @path: file path
+#
+# @offset: initial offset for the file
+#
+# Since 8.2
+##
+{ 'struct': 'FileMigrationArgs',
+  'data': {'path': 'str',
+           'offset': 'uint64' } }
 
 ##
 # @MigrationExecCommand:
@@ -1547,7 +1562,8 @@
   'data': {
     'socket': 'SocketAddress',
     'exec': 'MigrationExecCommand',
-    'rdma': 'InetSocketAddress' } }
+    'rdma': 'InetSocketAddress',
+    'file': 'FileMigrationArgs' } }
 
 ##
 # @migrate:
-- 
2.35.3



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 03/14] migration: convert migration 'uri' into 'MigrateAddress'
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 01/14] migration: New QAPI type 'MigrateAddress' Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 02/14] fixup! " Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 04/14] fixup! " Fabiano Rosas
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

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

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

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/exec.c      |  1 -
 migration/exec.h      |  4 ++++
 migration/migration.c | 56 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 1 deletion(-)

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



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 04/14] fixup! migration: convert migration 'uri' into 'MigrateAddress'
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
                   ` (2 preceding siblings ...)
  2023-10-19 19:23 ` [PATCH v14 03/14] migration: convert migration 'uri' into 'MigrateAddress' Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-23 11:55   ` Daniel P. Berrangé
  2023-10-19 19:23 ` [PATCH v14 05/14] migration: convert socket backend to accept MigrateAddress Fabiano Rosas
                   ` (9 subsequent siblings)
  13 siblings, 1 reply; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/file.c      | 2 +-
 migration/file.h      | 1 +
 migration/migration.c | 6 ++++++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/migration/file.c b/migration/file.c
index cf5b1bf365..ec069ef329 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -19,7 +19,7 @@
 
 /* Remove the offset option from @filespec and return it in @offsetp. */
 
-static int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
+int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
 {
     char *option = strstr(filespec, OFFSET_OPTION);
     int ret;
diff --git a/migration/file.h b/migration/file.h
index 90fa4849e0..3888a57105 100644
--- a/migration/file.h
+++ b/migration/file.h
@@ -11,4 +11,5 @@ void file_start_incoming_migration(const char *filename, Error **errp);
 
 void file_start_outgoing_migration(MigrationState *s, const char *filename,
                                    Error **errp);
+int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp);
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index c1108f409c..e3608d7f60 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -465,6 +465,12 @@ static bool migrate_uri_parse(const char *uri,
         }
         addr->u.socket.type = saddr->type;
         addr->u.socket.u = saddr->u;
+    } else if (strstart(uri, "file:", NULL)) {
+        addr->transport = MIGRATION_ADDRESS_TYPE_FILE;
+        addr->u.file.path = g_strdup(uri + strlen("file:"));
+        if (file_parse_offset(addr->u.file.path, &addr->u.file.offset, errp)) {
+            return false;
+        }
     } else {
         error_setg(errp, "unknown migration protocol: %s", uri);
         return false;
-- 
2.35.3



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 05/14] migration: convert socket backend to accept MigrateAddress
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
                   ` (3 preceding siblings ...)
  2023-10-19 19:23 ` [PATCH v14 04/14] fixup! " Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 06/14] migration: convert rdma " Fabiano Rosas
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

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

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

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

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 30 ++++++++++++++++++------------
 migration/socket.c    | 39 +++++++++------------------------------
 migration/socket.h    |  7 ++++---
 3 files changed, 31 insertions(+), 45 deletions(-)

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



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 06/14] migration: convert rdma backend to accept MigrateAddress
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
                   ` (4 preceding siblings ...)
  2023-10-19 19:23 ` [PATCH v14 05/14] migration: convert socket backend to accept MigrateAddress Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 07/14] migration: convert exec " Fabiano Rosas
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras, Li Zhijian

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

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

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

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c |  8 ++++----
 migration/rdma.c      | 33 +++++++++++----------------------
 migration/rdma.h      |  6 ++++--
 3 files changed, 19 insertions(+), 28 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0827dd1116..75a275569b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -508,8 +508,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
             fd_start_incoming_migration(saddr->u.fd.str, errp);
         }
 #ifdef CONFIG_RDMA
-    } else if (strstart(uri, "rdma:", &p)) {
-        rdma_start_incoming_migration(p, errp);
+    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
+        rdma_start_incoming_migration(&channel->u.rdma, errp);
 #endif
     } else if (strstart(uri, "exec:", &p)) {
         exec_start_incoming_migration(p, errp);
@@ -1782,8 +1782,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
             fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
         }
 #ifdef CONFIG_RDMA
-    } else if (strstart(uri, "rdma:", &p)) {
-        rdma_start_outgoing_migration(s, p, &local_err);
+    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
+        rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
 #endif
     } else if (strstart(uri, "exec:", &p)) {
         exec_start_outgoing_migration(s, p, &local_err);
diff --git a/migration/rdma.c b/migration/rdma.c
index 2a1852ec7f..7a9ce5230e 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -289,7 +289,6 @@ typedef struct RDMALocalBlocks {
 typedef struct RDMAContext {
     char *host;
     int port;
-    char *host_port;
 
     RDMAWorkRequestData wr_data[RDMA_WRID_MAX];
 
@@ -2431,9 +2430,7 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
         rdma->channel = NULL;
     }
     g_free(rdma->host);
-    g_free(rdma->host_port);
     rdma->host = NULL;
-    rdma->host_port = NULL;
 }
 
 
@@ -2723,28 +2720,16 @@ static void qemu_rdma_return_path_dest_init(RDMAContext *rdma_return_path,
     rdma_return_path->is_return_path = true;
 }
 
-static RDMAContext *qemu_rdma_data_init(const char *host_port, Error **errp)
+static RDMAContext *qemu_rdma_data_init(InetSocketAddress *saddr, Error **errp)
 {
     RDMAContext *rdma = NULL;
-    InetSocketAddress *addr;
 
     rdma = g_new0(RDMAContext, 1);
     rdma->current_index = -1;
     rdma->current_chunk = -1;
 
-    addr = g_new(InetSocketAddress, 1);
-    if (!inet_parse(addr, host_port, NULL)) {
-        rdma->port = atoi(addr->port);
-        rdma->host = g_strdup(addr->host);
-        rdma->host_port = g_strdup(host_port);
-    } else {
-        error_setg(errp, "RDMA ERROR: bad RDMA migration address '%s'",
-                   host_port);
-        g_free(rdma);
-        rdma = NULL;
-    }
-
-    qapi_free_InetSocketAddress(addr);
+    rdma->host = g_strdup(saddr->host);
+    rdma->port = atoi(saddr->port);
     return rdma;
 }
 
@@ -3353,6 +3338,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
                                             .private_data_len = sizeof(cap),
                                          };
     RDMAContext *rdma_return_path = NULL;
+    g_autoptr(InetSocketAddress) isock = g_new0(InetSocketAddress, 1);
     struct rdma_cm_event *cm_event;
     struct ibv_context *verbs;
     int ret;
@@ -3367,13 +3353,16 @@ static int qemu_rdma_accept(RDMAContext *rdma)
         goto err_rdma_dest_wait;
     }
 
+    isock->host = rdma->host;
+    isock->port = g_strdup_printf("%d", rdma->port);
+
     /*
      * initialize the RDMAContext for return path for postcopy after first
      * connection request reached.
      */
     if ((migrate_postcopy() || migrate_return_path())
         && !rdma->is_return_path) {
-        rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
+        rdma_return_path = qemu_rdma_data_init(isock, NULL);
         if (rdma_return_path == NULL) {
             rdma_ack_cm_event(cm_event);
             goto err_rdma_dest_wait;
@@ -4072,7 +4061,8 @@ static void rdma_accept_incoming_migration(void *opaque)
     }
 }
 
-void rdma_start_incoming_migration(const char *host_port, Error **errp)
+void rdma_start_incoming_migration(InetSocketAddress *host_port,
+                                   Error **errp)
 {
     MigrationState *s = migrate_get_current();
     int ret;
@@ -4116,13 +4106,12 @@ cleanup_rdma:
 err:
     if (rdma) {
         g_free(rdma->host);
-        g_free(rdma->host_port);
     }
     g_free(rdma);
 }
 
 void rdma_start_outgoing_migration(void *opaque,
-                            const char *host_port, Error **errp)
+                            InetSocketAddress *host_port, Error **errp)
 {
     MigrationState *s = opaque;
     RDMAContext *rdma_return_path = NULL;
diff --git a/migration/rdma.h b/migration/rdma.h
index 30b15b4466..a8d27f33b8 100644
--- a/migration/rdma.h
+++ b/migration/rdma.h
@@ -14,15 +14,17 @@
  *
  */
 
+#include "qemu/sockets.h"
+
 #ifndef QEMU_MIGRATION_RDMA_H
 #define QEMU_MIGRATION_RDMA_H
 
 #include "exec/memory.h"
 
-void rdma_start_outgoing_migration(void *opaque, const char *host_port,
+void rdma_start_outgoing_migration(void *opaque, InetSocketAddress *host_port,
                                    Error **errp);
 
-void rdma_start_incoming_migration(const char *host_port, Error **errp);
+void rdma_start_incoming_migration(InetSocketAddress *host_port, Error **errp);
 
 /*
  * Constants used by rdma return codes
-- 
2.35.3



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 07/14] migration: convert exec backend to accept MigrateAddress.
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
                   ` (5 preceding siblings ...)
  2023-10-19 19:23 ` [PATCH v14 06/14] migration: convert rdma " Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 08/14] migration: Convert the file backend the new QAPI syntax Fabiano Rosas
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

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

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

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

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/exec.c      | 73 +++++++++++++++++++++++++++++++------------
 migration/exec.h      |  4 +--
 migration/migration.c |  8 ++---
 3 files changed, 59 insertions(+), 26 deletions(-)

diff --git a/migration/exec.c b/migration/exec.c
index 32f5143dfd..47d2f3b8fb 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -39,20 +39,51 @@ const char *exec_get_cmd_path(void)
 }
 #endif
 
-void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
+/* provides the length of strList */
+static int
+str_list_length(strList *list)
+{
+    int len = 0;
+    strList *elem;
+
+    for (elem = list; elem != NULL; elem = elem->next) {
+        len++;
+    }
+
+    return len;
+}
+
+static void
+init_exec_array(strList *command, char **argv, Error **errp)
+{
+    int i = 0;
+    strList *lst;
+
+    for (lst = command; lst; lst = lst->next) {
+        argv[i++] = lst->value;
+    }
+
+    argv[i] = NULL;
+    return;
+}
+
+void exec_start_outgoing_migration(MigrationState *s, strList *command,
+                                   Error **errp)
 {
     QIOChannel *ioc;
 
-#ifdef WIN32
-    const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
-#else
-    const char *argv[] = { "/bin/sh", "-c", command, NULL };
-#endif
+    int length = str_list_length(command);
+    g_auto(GStrv) argv = (char **) g_new0(const char *, length + 1);
 
-    trace_migration_exec_outgoing(command);
-    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
-                                                    O_RDWR,
-                                                    errp));
+    init_exec_array(command, argv, errp);
+    g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
+
+    trace_migration_exec_outgoing(new_command);
+    ioc = QIO_CHANNEL(
+        qio_channel_command_new_spawn(
+                            (const char * const *) g_steal_pointer(&argv),
+                            O_RDWR,
+                            errp));
     if (!ioc) {
         return;
     }
@@ -71,20 +102,22 @@ static gboolean exec_accept_incoming_migration(QIOChannel *ioc,
     return G_SOURCE_REMOVE;
 }
 
-void exec_start_incoming_migration(const char *command, Error **errp)
+void exec_start_incoming_migration(strList *command, Error **errp)
 {
     QIOChannel *ioc;
 
-#ifdef WIN32
-    const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
-#else
-    const char *argv[] = { "/bin/sh", "-c", command, NULL };
-#endif
+    int length = str_list_length(command);
+    g_auto(GStrv) argv = (char **) g_new0(const char *, length + 1);
 
-    trace_migration_exec_incoming(command);
-    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
-                                                    O_RDWR,
-                                                    errp));
+    init_exec_array(command, argv, errp);
+    g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
+
+    trace_migration_exec_incoming(new_command);
+    ioc = QIO_CHANNEL(
+        qio_channel_command_new_spawn(
+                            (const char * const *) g_steal_pointer(&argv),
+                            O_RDWR,
+                            errp));
     if (!ioc) {
         return;
     }
diff --git a/migration/exec.h b/migration/exec.h
index 736cd71028..3107f205e3 100644
--- a/migration/exec.h
+++ b/migration/exec.h
@@ -23,8 +23,8 @@
 #ifdef WIN32
 const char *exec_get_cmd_path(void);
 #endif
-void exec_start_incoming_migration(const char *host_port, Error **errp);
+void exec_start_incoming_migration(strList *host_port, Error **errp);
 
-void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
+void exec_start_outgoing_migration(MigrationState *s, strList *host_port,
                                    Error **errp);
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index 75a275569b..0f16910156 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -511,8 +511,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
     } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
         rdma_start_incoming_migration(&channel->u.rdma, errp);
 #endif
-    } else if (strstart(uri, "exec:", &p)) {
-        exec_start_incoming_migration(p, errp);
+    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
+        exec_start_incoming_migration(channel->u.exec.args, errp);
     } else if (strstart(uri, "file:", &p)) {
         file_start_incoming_migration(p, errp);
     } else {
@@ -1785,8 +1785,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
         rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
 #endif
-    } else if (strstart(uri, "exec:", &p)) {
-        exec_start_outgoing_migration(s, p, &local_err);
+    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
+        exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
     } else if (strstart(uri, "file:", &p)) {
         file_start_outgoing_migration(s, p, &local_err);
     } else {
-- 
2.35.3



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 08/14] migration: Convert the file backend the new QAPI syntax
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
                   ` (6 preceding siblings ...)
  2023-10-19 19:23 ` [PATCH v14 07/14] migration: convert exec " Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 09/14] migration: New migrate and migrate-incoming argument 'channels' Fabiano Rosas
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

Convert the file: URI to accept a FileMigrationArgs to be compatible
with the new migration QAPI.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/file.c      | 22 +++++++---------------
 migration/file.h      |  9 ++++++---
 migration/migration.c | 10 ++++------
 3 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/migration/file.c b/migration/file.c
index ec069ef329..b36abc050e 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -36,20 +36,16 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
     return 0;
 }
 
-void file_start_outgoing_migration(MigrationState *s, const char *filespec,
-                                   Error **errp)
+void file_start_outgoing_migration(MigrationState *s,
+                                   FileMigrationArgs *file_args, Error **errp)
 {
-    g_autofree char *filename = g_strdup(filespec);
     g_autoptr(QIOChannelFile) fioc = NULL;
-    uint64_t offset = 0;
+    g_autofree char *filename = g_strdup(file_args->path);
+    uint64_t offset = file_args->offset;
     QIOChannel *ioc;
 
     trace_migration_file_outgoing(filename);
 
-    if (file_parse_offset(filename, &offset, errp)) {
-        return;
-    }
-
     fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC,
                                      0600, errp);
     if (!fioc) {
@@ -73,19 +69,15 @@ static gboolean file_accept_incoming_migration(QIOChannel *ioc,
     return G_SOURCE_REMOVE;
 }
 
-void file_start_incoming_migration(const char *filespec, Error **errp)
+void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp)
 {
-    g_autofree char *filename = g_strdup(filespec);
+    g_autofree char *filename = g_strdup(file_args->path);
     QIOChannelFile *fioc = NULL;
-    uint64_t offset = 0;
+    uint64_t offset = file_args->offset;
     QIOChannel *ioc;
 
     trace_migration_file_incoming(filename);
 
-    if (file_parse_offset(filename, &offset, errp)) {
-        return;
-    }
-
     fioc = qio_channel_file_new_path(filename, O_RDONLY, 0, errp);
     if (!fioc) {
         return;
diff --git a/migration/file.h b/migration/file.h
index 3888a57105..37d6a08bfc 100644
--- a/migration/file.h
+++ b/migration/file.h
@@ -7,9 +7,12 @@
 
 #ifndef QEMU_MIGRATION_FILE_H
 #define QEMU_MIGRATION_FILE_H
-void file_start_incoming_migration(const char *filename, Error **errp);
 
-void file_start_outgoing_migration(MigrationState *s, const char *filename,
-                                   Error **errp);
+#include "qapi/qapi-types-migration.h"
+
+void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp);
+
+void file_start_outgoing_migration(MigrationState *s,
+                                   FileMigrationArgs *file_args, Error **errp);
 int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp);
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index 0f16910156..9531aec8d3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -482,7 +482,6 @@ static bool migrate_uri_parse(const char *uri,
 
 static void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
-    const char *p = NULL;
     g_autoptr(MigrationAddress) channel = NULL;
     MigrationIncomingState *mis = migration_incoming_get_current();
 
@@ -513,8 +512,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
 #endif
     } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
         exec_start_incoming_migration(channel->u.exec.args, errp);
-    } else if (strstart(uri, "file:", &p)) {
-        file_start_incoming_migration(p, errp);
+    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_FILE) {
+        file_start_incoming_migration(&channel->u.file, errp);
     } else {
         error_setg(errp, "unknown migration protocol: %s", uri);
     }
@@ -1747,7 +1746,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     bool resume_requested;
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    const char *p = NULL;
     g_autoptr(MigrationAddress) channel = NULL;
 
     /* URI is not suitable for migration? */
@@ -1787,8 +1785,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 #endif
     } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
         exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
-    } else if (strstart(uri, "file:", &p)) {
-        file_start_outgoing_migration(s, p, &local_err);
+    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_FILE) {
+        file_start_outgoing_migration(s, &channel->u.file, &local_err);
     } else {
         error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
                    "a valid migration protocol");
-- 
2.35.3



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 09/14] migration: New migrate and migrate-incoming argument 'channels'
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
                   ` (7 preceding siblings ...)
  2023-10-19 19:23 ` [PATCH v14 08/14] migration: Convert the file backend the new QAPI syntax Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 10/14] fixup! " Fabiano Rosas
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

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

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

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

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

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index d206700a43..efd2c268b2 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -446,7 +446,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
     const char *uri = qdict_get_str(qdict, "uri");
 
-    qmp_migrate_incoming(uri, &err);
+    qmp_migrate_incoming(uri, false, NULL, &err);
 
     hmp_handle_error(mon, err);
 }
@@ -745,8 +745,8 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
     const char *uri = qdict_get_str(qdict, "uri");
     Error *err = NULL;
 
-    qmp_migrate(uri, !!blk, blk, !!inc, inc,
-                false, false, true, resume, &err);
+    qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc,
+                 false, false, true, resume, &err);
     if (hmp_handle_error(mon, err)) {
         return;
     }
diff --git a/migration/migration.c b/migration/migration.c
index 9531aec8d3..f5bad11777 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -480,11 +480,34 @@ static bool migrate_uri_parse(const char *uri,
     return true;
 }
 
-static void qemu_start_incoming_migration(const char *uri, Error **errp)
+static void qemu_start_incoming_migration(const char *uri, bool has_channels,
+                                          MigrationChannelList *channels,
+                                          Error **errp)
 {
     g_autoptr(MigrationAddress) channel = NULL;
     MigrationIncomingState *mis = migration_incoming_get_current();
 
+    /*
+     * Having preliminary checks for uri and channel
+     */
+    if (has_channels) {
+        error_setg(errp, "'channels' argument should not be set yet.");
+        return;
+    }
+
+    if (uri && has_channels) {
+        error_setg(errp, "'uri' and 'channels' arguments are mutually "
+                   "exclusive; exactly one of the two should be present in "
+                   "'migrate-incoming' qmp command ");
+        return;
+    }
+
+    if (!uri && !has_channels) {
+        error_setg(errp, "neither 'uri' or 'channels' argument are "
+                   "specified in 'migrate-incoming' qmp command ");
+        return;
+    }
+
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
         return;
@@ -1554,7 +1577,8 @@ void migrate_del_blocker(Error *reason)
     migration_blockers = g_slist_remove(migration_blockers, reason);
 }
 
-void qmp_migrate_incoming(const char *uri, Error **errp)
+void qmp_migrate_incoming(const char *uri, bool has_channels,
+                          MigrationChannelList *channels, Error **errp)
 {
     Error *local_err = NULL;
     static bool once = true;
@@ -1572,7 +1596,7 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
         return;
     }
 
-    qemu_start_incoming_migration(uri, &local_err);
+    qemu_start_incoming_migration(uri, has_channels, channels, &local_err);
 
     if (local_err) {
         yank_unregister_instance(MIGRATION_YANK_INSTANCE);
@@ -1608,7 +1632,7 @@ void qmp_migrate_recover(const char *uri, Error **errp)
      * only re-setup the migration stream and poke existing migration
      * to continue using that newly established channel.
      */
-    qemu_start_incoming_migration(uri, errp);
+    qemu_start_incoming_migration(uri, false, NULL, errp);
 }
 
 void qmp_migrate_pause(Error **errp)
@@ -1739,7 +1763,8 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
     return true;
 }
 
-void qmp_migrate(const char *uri, bool has_blk, bool blk,
+void qmp_migrate(const char *uri, bool has_channels,
+                 MigrationChannelList *channels, bool has_blk, bool blk,
                  bool has_inc, bool inc, bool has_detach, bool detach,
                  bool has_resume, bool resume, Error **errp)
 {
@@ -1748,6 +1773,27 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     MigrationState *s = migrate_get_current();
     g_autoptr(MigrationAddress) channel = NULL;
 
+    /*
+     * Having preliminary checks for uri and channel
+     */
+    if (has_channels) {
+        error_setg(errp, "'channels' argument should not be set yet.");
+        return;
+    }
+
+    if (uri && has_channels) {
+        error_setg(errp, "'uri' and 'channels' arguments are mutually "
+                   "exclusive; exactly one of the two should be present in "
+                   "'migrate' qmp command ");
+        return;
+    }
+
+    if (!uri && !has_channels) {
+        error_setg(errp, "neither 'uri' or 'channels' argument are "
+                   "specified in 'migrate' qmp command ");
+        return;
+    }
+
     /* URI is not suitable for migration? */
     if (!migration_channels_and_uri_compatible(uri, errp)) {
         return;
diff --git a/qapi/migration.json b/qapi/migration.json
index 602cb706e3..85ad5f2601 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1565,6 +1565,34 @@
     'rdma': 'InetSocketAddress',
     'file': 'FileMigrationArgs' } }
 
+##
+# @MigrationChannelType:
+#
+# The migration channel-type request options.
+#
+# @main: Main outbound migration channel.
+#
+# Since 8.1
+##
+{ 'enum': 'MigrationChannelType',
+  'data': [ 'main' ] }
+
+##
+# @MigrationChannel:
+#
+# Migration stream channel parameters.
+#
+# @channel-type: Channel type for transfering packet information.
+#
+# @addr: Migration endpoint configuration on destination interface.
+#
+# Since 8.1
+##
+{ 'struct': 'MigrationChannel',
+  'data': {
+      'channel-type': 'MigrationChannelType',
+      'addr': 'MigrationAddress' } }
+
 ##
 # @migrate:
 #
@@ -1572,6 +1600,9 @@
 #
 # @uri: the Uniform Resource Identifier of the destination VM
 #
+# @channels: list of migration stream channels with each stream in the
+#     list connected to a destination interface endpoint.
+#
 # @blk: do block migration (full disk copy)
 #
 # @inc: incremental disk copy migration
@@ -1596,14 +1627,50 @@
 # 3. The user Monitor's "detach" argument is invalid in QMP and should
 #    not be used
 #
+# 4. The uri argument should have the Uniform Resource Identifier of
+#    default destination VM. This connection will be bound to default
+#    network.
+#
+# 5. For now, number of migration streams is restricted to one, i.e
+#    number of items in 'channels' list is just 1.
+#
+# 6. The 'uri' and 'channels' arguments are mutually exclusive;
+#    exactly one of the two should be present.
+#
 # Example:
 #
 # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
 # <- { "return": {} }
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "socket",
+#                                    "type": "inet",
+#                                    "host": "10.12.34.9",
+#                                    "port": "1050" } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "exec",
+#                                    "args": [ "/bin/nc", "-p", "6000",
+#                                              "/some/sock" ] } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "rdma",
+#                                    "host": "10.12.34.9",
+#                                    "port": "1050" } } ] } }
+# <- { "return": {} }
+#
 ##
 { 'command': 'migrate',
-  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
-           '*detach': 'bool', '*resume': 'bool' } }
+  'data': {'*uri': 'str', '*channels': [ 'MigrationChannel' ],
+           '*blk': 'bool', '*inc': 'bool', '*detach': 'bool',
+           '*resume': 'bool' } }
 
 ##
 # @migrate-incoming:
@@ -1614,6 +1681,9 @@
 # @uri: The Uniform Resource Identifier identifying the source or
 #     address to listen on
 #
+# @channels: list of migration stream channels with each stream in the
+#     list connected to a destination interface endpoint.
+#
 # Returns: nothing on success
 #
 # Since: 2.3
@@ -1629,13 +1699,46 @@
 #
 # 3. The uri format is the same as for -incoming
 #
+# 5. For now, number of migration streams is restricted to one, i.e
+#    number of items in 'channels' list is just 1.
+#
+# 4. The 'uri' and 'channels' arguments are mutually exclusive;
+#    exactly one of the two should be present.
+#
 # Example:
 #
 # -> { "execute": "migrate-incoming",
 #      "arguments": { "uri": "tcp::4446" } }
 # <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "socket",
+#                                    "type": "inet",
+#                                    "host": "10.12.34.9",
+#                                    "port": "1050" } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "exec",
+#                                    "args": [ "/bin/nc", "-p", "6000",
+#                                              "/some/sock" ] } } ] } }
+# <- { "return": {} }
+#
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "rdma",
+#                                    "host": "10.12.34.9",
+#                                    "port": "1050" } } ] } }
+# <- { "return": {} }
 ##
-{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
+{ 'command': 'migrate-incoming',
+             'data': {'*uri': 'str',
+                      '*channels': [ 'MigrationChannel' ] } }
 
 ##
 # @xen-save-devices-state:
diff --git a/system/vl.c b/system/vl.c
index 3100ac01ed..69a8cffe81 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2696,7 +2696,7 @@ void qmp_x_exit_preconfig(Error **errp)
     if (incoming) {
         Error *local_err = NULL;
         if (strcmp(incoming, "defer") != 0) {
-            qmp_migrate_incoming(incoming, &local_err);
+            qmp_migrate_incoming(incoming, false, NULL, &local_err);
             if (local_err) {
                 error_reportf_err(local_err, "-incoming %s: ", incoming);
                 exit(1);
-- 
2.35.3



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 10/14] fixup! migration: New migrate and migrate-incoming argument 'channels'
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
                   ` (8 preceding siblings ...)
  2023-10-19 19:23 ` [PATCH v14 09/14] migration: New migrate and migrate-incoming argument 'channels' Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-23 11:54   ` Daniel P. Berrangé
  2023-10-19 19:23 ` [PATCH v14 11/14] migration: modify migration_channels_and_uri_compatible() for new QAPI syntax Fabiano Rosas
                   ` (3 subsequent siblings)
  13 siblings, 1 reply; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 qapi/migration.json | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/qapi/migration.json b/qapi/migration.json
index 85ad5f2601..f51e6663bb 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1666,6 +1666,14 @@
 #                                    "port": "1050" } } ] } }
 # <- { "return": {} }
 #
+# -> { "execute": "migrate",
+#      "arguments": {
+#          "channels": [ { "channel-type": "main",
+#                          "addr": { "transport": "file",
+#                                    "path": "/tmp/migfile",
+#                                    "offset": "0x1000" } } ] } }
+# <- { "return": {} }
+#
 ##
 { 'command': 'migrate',
   'data': {'*uri': 'str', '*channels': [ 'MigrationChannel' ],
-- 
2.35.3



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 11/14] migration: modify migration_channels_and_uri_compatible() for new QAPI syntax
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
                   ` (9 preceding siblings ...)
  2023-10-19 19:23 ` [PATCH v14 10/14] fixup! " Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 12/14] migration: Implement MigrateChannelList to qmp migration flow Fabiano Rosas
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

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

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

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

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index f5bad11777..5a721893f3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -107,17 +107,20 @@ static bool migration_needs_multiple_sockets(void)
     return migrate_multifd() || migrate_postcopy_preempt();
 }
 
-static bool uri_supports_multi_channels(const char *uri)
+static bool transport_supports_multi_channels(SocketAddress *saddr)
 {
-    return strstart(uri, "tcp:", NULL) || strstart(uri, "unix:", NULL) ||
-           strstart(uri, "vsock:", NULL);
+    return saddr->type == SOCKET_ADDRESS_TYPE_INET ||
+           saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+           saddr->type == SOCKET_ADDRESS_TYPE_VSOCK;
 }
 
 static bool
-migration_channels_and_uri_compatible(const char *uri, Error **errp)
+migration_channels_and_transport_compatible(MigrationAddress *addr,
+                                            Error **errp)
 {
     if (migration_needs_multiple_sockets() &&
-        !uri_supports_multi_channels(uri)) {
+        (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) &&
+        !transport_supports_multi_channels(&addr->u.socket)) {
         error_setg(errp, "Migration requires multi-channel URIs (e.g. tcp)");
         return false;
     }
@@ -508,15 +511,15 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
         return;
     }
 
-    /* URI is not suitable for migration? */
-    if (!migration_channels_and_uri_compatible(uri, errp)) {
-        return;
-    }
-
     if (uri && !migrate_uri_parse(uri, &channel, errp)) {
         return;
     }
 
+    /* transport mechanism not suitable for migration? */
+    if (!migration_channels_and_transport_compatible(channel, errp)) {
+        return;
+    }
+
     migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
                       MIGRATION_STATUS_SETUP);
 
@@ -1794,15 +1797,15 @@ void qmp_migrate(const char *uri, bool has_channels,
         return;
     }
 
-    /* URI is not suitable for migration? */
-    if (!migration_channels_and_uri_compatible(uri, errp)) {
-        return;
-    }
-
     if (!migrate_uri_parse(uri, &channel, errp)) {
         return;
     }
 
+    /* transport mechanism not suitable for migration? */
+    if (!migration_channels_and_transport_compatible(channel, errp)) {
+        return;
+    }
+
     resume_requested = has_resume && resume;
     if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
                          resume_requested, errp)) {
-- 
2.35.3



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 12/14] migration: Implement MigrateChannelList to qmp migration flow.
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
                   ` (10 preceding siblings ...)
  2023-10-19 19:23 ` [PATCH v14 11/14] migration: modify migration_channels_and_uri_compatible() for new QAPI syntax Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 13/14] migration: Implement MigrateChannelList to hmp " Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 14/14] migration: modify test_multifd_tcp_none() to use new QAPI syntax Fabiano Rosas
  13 siblings, 0 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

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

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

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

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 101 +++++++++++++++++++++++-------------------
 1 file changed, 55 insertions(+), 46 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 5a721893f3..6a6267248a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -433,9 +433,10 @@ void migrate_add_address(SocketAddress *address)
 }
 
 static bool migrate_uri_parse(const char *uri,
-                              MigrationAddress **channel,
+                              MigrationChannel **channel,
                               Error **errp)
 {
+    g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1);
     g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
     SocketAddress *saddr = NULL;
     InetSocketAddress *isock = &addr->u.rdma;
@@ -479,7 +480,9 @@ static bool migrate_uri_parse(const char *uri,
         return false;
     }
 
-    *channel = g_steal_pointer(&addr);
+    val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN;
+    val->addr = g_steal_pointer(&addr);
+    *channel = g_steal_pointer(&val);
     return true;
 }
 
@@ -487,44 +490,47 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
                                           MigrationChannelList *channels,
                                           Error **errp)
 {
-    g_autoptr(MigrationAddress) channel = NULL;
+    MigrationChannel *channel = NULL;
+    MigrationAddress *addr = NULL;
     MigrationIncomingState *mis = migration_incoming_get_current();
 
     /*
      * Having preliminary checks for uri and channel
      */
-    if (has_channels) {
-        error_setg(errp, "'channels' argument should not be set yet.");
-        return;
-    }
-
     if (uri && has_channels) {
         error_setg(errp, "'uri' and 'channels' arguments are mutually "
                    "exclusive; exactly one of the two should be present in "
                    "'migrate-incoming' qmp command ");
         return;
-    }
-
-    if (!uri && !has_channels) {
+    } else if (channels) {
+        /* To verify that Migrate channel list has only item */
+        if (channels->next) {
+            error_setg(errp, "Channel list has more than one entries");
+            return;
+        }
+        channel = channels->value;
+    } else if (uri) {
+        /* caller uses the old URI syntax */
+        if (!migrate_uri_parse(uri, &channel, errp)) {
+            return;
+        }
+    } else {
         error_setg(errp, "neither 'uri' or 'channels' argument are "
                    "specified in 'migrate-incoming' qmp command ");
         return;
     }
-
-    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
-        return;
-    }
+    addr = channel->addr;
 
     /* transport mechanism not suitable for migration? */
-    if (!migration_channels_and_transport_compatible(channel, errp)) {
+    if (!migration_channels_and_transport_compatible(addr, errp)) {
         return;
     }
 
     migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
                       MIGRATION_STATUS_SETUP);
 
-    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
-        SocketAddress *saddr = &channel->u.socket;
+    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
+        SocketAddress *saddr = &addr->u.socket;
         if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
             saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
             saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
@@ -533,13 +539,13 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
             fd_start_incoming_migration(saddr->u.fd.str, errp);
         }
 #ifdef CONFIG_RDMA
-    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
-        rdma_start_incoming_migration(&channel->u.rdma, errp);
+    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
+        rdma_start_incoming_migration(&addr->u.rdma, errp);
 #endif
-    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
-        exec_start_incoming_migration(channel->u.exec.args, errp);
-    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_FILE) {
-        file_start_incoming_migration(&channel->u.file, errp);
+    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
+        exec_start_incoming_migration(addr->u.exec.args, errp);
+    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) {
+        file_start_incoming_migration(&addr->u.file, errp);
     } else {
         error_setg(errp, "unknown migration protocol: %s", uri);
     }
@@ -1774,35 +1780,38 @@ void qmp_migrate(const char *uri, bool has_channels,
     bool resume_requested;
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    g_autoptr(MigrationAddress) channel = NULL;
+    MigrationChannel *channel = NULL;
+    MigrationAddress *addr = NULL;
 
     /*
      * Having preliminary checks for uri and channel
      */
-    if (has_channels) {
-        error_setg(errp, "'channels' argument should not be set yet.");
-        return;
-    }
-
     if (uri && has_channels) {
         error_setg(errp, "'uri' and 'channels' arguments are mutually "
                    "exclusive; exactly one of the two should be present in "
                    "'migrate' qmp command ");
         return;
-    }
-
-    if (!uri && !has_channels) {
+    } else if (channels) {
+        /* To verify that Migrate channel list has only item */
+        if (channels->next) {
+            error_setg(errp, "Channel list has more than one entries");
+            return;
+        }
+        channel = channels->value;
+    } else if (uri) {
+        /* caller uses the old URI syntax */
+        if (!migrate_uri_parse(uri, &channel, errp)) {
+            return;
+        }
+    } else {
         error_setg(errp, "neither 'uri' or 'channels' argument are "
                    "specified in 'migrate' qmp command ");
         return;
     }
-
-    if (!migrate_uri_parse(uri, &channel, errp)) {
-        return;
-    }
+    addr = channel->addr;
 
     /* transport mechanism not suitable for migration? */
-    if (!migration_channels_and_transport_compatible(channel, errp)) {
+    if (!migration_channels_and_transport_compatible(addr, errp)) {
         return;
     }
 
@@ -1819,8 +1828,8 @@ void qmp_migrate(const char *uri, bool has_channels,
         }
     }
 
-    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
-        SocketAddress *saddr = &channel->u.socket;
+    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
+        SocketAddress *saddr = &addr->u.socket;
         if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
             saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
             saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
@@ -1829,13 +1838,13 @@ void qmp_migrate(const char *uri, bool has_channels,
             fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
         }
 #ifdef CONFIG_RDMA
-    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
-        rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
+    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
+        rdma_start_outgoing_migration(s, &addr->u.rdma, &local_err);
 #endif
-    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
-        exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
-    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_FILE) {
-        file_start_outgoing_migration(s, &channel->u.file, &local_err);
+    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
+        exec_start_outgoing_migration(s, addr->u.exec.args, &local_err);
+    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) {
+        file_start_outgoing_migration(s, &addr->u.file, &local_err);
     } else {
         error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
                    "a valid migration protocol");
-- 
2.35.3



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 13/14] migration: Implement MigrateChannelList to hmp migration flow.
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
                   ` (11 preceding siblings ...)
  2023-10-19 19:23 ` [PATCH v14 12/14] migration: Implement MigrateChannelList to qmp migration flow Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  2023-10-19 19:23 ` [PATCH v14 14/14] migration: modify test_multifd_tcp_none() to use new QAPI syntax Fabiano Rosas
  13 siblings, 0 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

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

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

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration-hmp-cmds.c | 27 ++++++++++++++++++++++-----
 migration/migration.c          |  5 ++---
 migration/migration.h          |  3 ++-
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index efd2c268b2..e796aea8b4 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -445,9 +445,18 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
     const char *uri = qdict_get_str(qdict, "uri");
+    MigrationChannelList *caps = NULL;
+    g_autoptr(MigrationChannel) channel = NULL;
 
-    qmp_migrate_incoming(uri, false, NULL, &err);
+    if (!migrate_uri_parse(uri, &channel, &err)) {
+        goto end;
+    }
+    QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
 
+    qmp_migrate_incoming(NULL, true, caps, &err);
+    qapi_free_MigrationChannelList(caps);
+
+end:
     hmp_handle_error(mon, err);
 }
 
@@ -744,12 +753,17 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
     bool resume = qdict_get_try_bool(qdict, "resume", false);
     const char *uri = qdict_get_str(qdict, "uri");
     Error *err = NULL;
+    MigrationChannelList *caps = NULL;
+    g_autoptr(MigrationChannel) channel = NULL;
 
-    qmp_migrate(uri, false, NULL, !!blk, blk, !!inc, inc,
+    if (!migrate_uri_parse(uri, &channel, &err)) {
+        goto end;
+    }
+    QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
+
+    qmp_migrate(NULL, true, caps, !!blk, blk, !!inc, inc,
                  false, false, true, resume, &err);
-    if (hmp_handle_error(mon, err)) {
-        return;
-    }
+    qapi_free_MigrationChannelList(caps);
 
     if (!detach) {
         HMPMigrationStatus *status;
@@ -767,6 +781,9 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
                                           status);
         timer_mod(status->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
     }
+
+end:
+    hmp_handle_error(mon, err);
 }
 
 void migrate_set_capability_completion(ReadLineState *rs, int nb_args,
diff --git a/migration/migration.c b/migration/migration.c
index 6a6267248a..9c1ee74dbd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -432,9 +432,8 @@ void migrate_add_address(SocketAddress *address)
                       QAPI_CLONE(SocketAddress, address));
 }
 
-static bool migrate_uri_parse(const char *uri,
-                              MigrationChannel **channel,
-                              Error **errp)
+bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
+                       Error **errp)
 {
     g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1);
     g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
diff --git a/migration/migration.h b/migration/migration.h
index ae82004892..c2a5333e0e 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -520,7 +520,8 @@ bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
                                       Error **errp);
 
 void migrate_add_address(SocketAddress *address);
-
+bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
+                       Error **errp);
 int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
 
 #define qemu_ram_foreach_block \
-- 
2.35.3



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v14 14/14] migration: modify test_multifd_tcp_none() to use new QAPI syntax.
  2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
                   ` (12 preceding siblings ...)
  2023-10-19 19:23 ` [PATCH v14 13/14] migration: Implement MigrateChannelList to hmp " Fabiano Rosas
@ 2023-10-19 19:23 ` Fabiano Rosas
  13 siblings, 0 replies; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-19 19:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: prerna.saxena, dgilbert, pbonzini, berrange, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras, Thomas Huth, Laurent Vivier

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

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

Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index e1c110537b..76b3a472ed 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1284,7 +1284,12 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
 
     migrate_prepare_for_dirty_mem(from);
     qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
-                             "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+                             "  'arguments': { "
+                             "      'channels': [ { 'channel-type': 'main',"
+                             "      'addr': { 'transport': 'socket',"
+                             "                'type': 'inet',"
+                             "                'host': '127.0.0.1',"
+                             "                'port': '0' } } ] } }");
 
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
-- 
2.35.3



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress'
  2023-10-19 19:23 ` [PATCH v14 02/14] fixup! " Fabiano Rosas
@ 2023-10-20  5:06   ` Markus Armbruster
  2023-10-20 12:07     ` Fabiano Rosas
  2023-10-23 11:56   ` [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress' Daniel P. Berrangé
  1 sibling, 1 reply; 25+ messages in thread
From: Markus Armbruster @ 2023-10-20  5:06 UTC (permalink / raw)
  To: Fabiano Rosas
  Cc: qemu-devel, prerna.saxena, dgilbert, pbonzini, berrange, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

Fabiano Rosas <farosas@suse.de> writes:

> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  qapi/migration.json | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c352c7ac52..602cb706e3 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1519,10 +1519,25 @@
>  #
>  # @rdma: Migrate via RDMA.
>  #
> +# @file: Direct the migration stream to a file.
> +#
>  # Since 8.2
>  ##
>  { 'enum': 'MigrationAddressType',
> -  'data': ['socket', 'exec', 'rdma'] }
> +  'data': ['socket', 'exec', 'rdma', 'file'] }

I don't like our use of spaces around parenthesis in the QAPI schema,
but I like inconsistency even less: please insert a space after '['.

> +
> +##
> +# @FileMigrationArgs:
> +#
> +# @path: file path

Name this @filename for local consistency.  We use both @filename and
@path for filenames in the schema, which is sad.  However,
migration.json uses only @filename so far.  Let's keep it that way.

"file path" is awfully terse.  Maybe "file to receive the migration
stream"?

> +#
> +# @offset: initial offset for the file

Again, too terse.  How is @offset to be used?  Start writing at this
file offset?

> +#
> +# Since 8.2
> +##
> +{ 'struct': 'FileMigrationArgs',
> +  'data': {'path': 'str',

Please insert a space after '{', and reindent the next line.

> +           'offset': 'uint64' } }
>  
>  ##
>  # @MigrationExecCommand:
> @@ -1547,7 +1562,8 @@
>    'data': {
>      'socket': 'SocketAddress',
>      'exec': 'MigrationExecCommand',
> -    'rdma': 'InetSocketAddress' } }
> +    'rdma': 'InetSocketAddress',
> +    'file': 'FileMigrationArgs' } }
>  
>  ##
>  # @migrate:



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress'
  2023-10-20  5:06   ` Markus Armbruster
@ 2023-10-20 12:07     ` Fabiano Rosas
  2023-10-20 12:37       ` Markus Armbruster
  0 siblings, 1 reply; 25+ messages in thread
From: Fabiano Rosas @ 2023-10-20 12:07 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: qemu-devel, prerna.saxena, dgilbert, pbonzini, berrange, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

Markus Armbruster <armbru@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  qapi/migration.json | 20 ++++++++++++++++++--
>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index c352c7ac52..602cb706e3 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1519,10 +1519,25 @@
>>  #
>>  # @rdma: Migrate via RDMA.
>>  #
>> +# @file: Direct the migration stream to a file.
>> +#
>>  # Since 8.2
>>  ##
>>  { 'enum': 'MigrationAddressType',
>> -  'data': ['socket', 'exec', 'rdma'] }
>> +  'data': ['socket', 'exec', 'rdma', 'file'] }
>
> I don't like our use of spaces around parenthesis in the QAPI schema,
> but I like inconsistency even less: please insert a space after '['.
>

Yes. But,

a contributor today has to guess what is the preferred syntax. Could we
have a checkpatch rule for this? Or should I send a patch to make the
whole file consistent at once?

Side question: are we using valid JSON at all? I threw this in a random
online linter and it complains about the single quotes. We could have a
proper tool doing the validation in CI.


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress'
  2023-10-20 12:07     ` Fabiano Rosas
@ 2023-10-20 12:37       ` Markus Armbruster
  2023-10-20 13:31         ` Daniel P. Berrangé
  0 siblings, 1 reply; 25+ messages in thread
From: Markus Armbruster @ 2023-10-20 12:37 UTC (permalink / raw)
  To: Fabiano Rosas
  Cc: qemu-devel, prerna.saxena, dgilbert, pbonzini, berrange, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>  qapi/migration.json | 20 ++++++++++++++++++--
>>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index c352c7ac52..602cb706e3 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -1519,10 +1519,25 @@
>>>  #
>>>  # @rdma: Migrate via RDMA.
>>>  #
>>> +# @file: Direct the migration stream to a file.
>>> +#
>>>  # Since 8.2
>>>  ##
>>>  { 'enum': 'MigrationAddressType',
>>> -  'data': ['socket', 'exec', 'rdma'] }
>>> +  'data': ['socket', 'exec', 'rdma', 'file'] }
>>
>> I don't like our use of spaces around parenthesis in the QAPI schema,
>> but I like inconsistency even less: please insert a space after '['.
>>
>
> Yes. But,
>
> a contributor today has to guess what is the preferred syntax. Could we
> have a checkpatch rule for this? Or should I send a patch to make the
> whole file consistent at once?
>
> Side question: are we using valid JSON at all? I threw this in a random
> online linter and it complains about the single quotes. We could have a
> proper tool doing the validation in CI.

You've come a sad, sad place.

docs/devel/qapi-code-gen.rst:

    Schema syntax
    -------------

    Syntax is loosely based on `JSON <http://www.ietf.org/rfc/rfc8259.txt>`_.
    Differences:

    * Comments: start with a hash character (``#``) that is not part of a
      string, and extend to the end of the line.

    * Strings are enclosed in ``'single quotes'``, not ``"double quotes"``.

    * Strings are restricted to printable ASCII, and escape sequences to
      just ``\\``.

    * Numbers and ``null`` are not supported.

If your reaction to item 2 is "this is stupid", you'd be exactly right.

Here's the conclusion of a discussion on possible improvements we had in
2020:
https://lore.kernel.org/qemu-devel/877dt5ofoi.fsf@dusky.pond.sub.org/



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress'
  2023-10-20 12:37       ` Markus Armbruster
@ 2023-10-20 13:31         ` Daniel P. Berrangé
  2023-10-23  9:01           ` Should we replace QAPI? (was: [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress') Markus Armbruster
  0 siblings, 1 reply; 25+ messages in thread
From: Daniel P. Berrangé @ 2023-10-20 13:31 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Fabiano Rosas, qemu-devel, prerna.saxena, dgilbert, pbonzini,
	eblake, manish.mishra, aravind.retnakaran, Het Gala,
	Juan Quintela, Peter Xu, Leonardo Bras

On Fri, Oct 20, 2023 at 02:37:16PM +0200, Markus Armbruster wrote:
> Fabiano Rosas <farosas@suse.de> writes:
> 
> > Markus Armbruster <armbru@redhat.com> writes:
> >
> >> Fabiano Rosas <farosas@suse.de> writes:
> >>
> >>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >
> > Side question: are we using valid JSON at all? I threw this in a random
> > online linter and it complains about the single quotes. We could have a
> > proper tool doing the validation in CI.
> 
> You've come a sad, sad place.
> 
> docs/devel/qapi-code-gen.rst:
> 
>     Schema syntax
>     -------------
> 
>     Syntax is loosely based on `JSON <http://www.ietf.org/rfc/rfc8259.txt>`_.
>     Differences:
> 
>     * Comments: start with a hash character (``#``) that is not part of a
>       string, and extend to the end of the line.
> 
>     * Strings are enclosed in ``'single quotes'``, not ``"double quotes"``.
> 
>     * Strings are restricted to printable ASCII, and escape sequences to
>       just ``\\``.
> 
>     * Numbers and ``null`` are not supported.
> 
> If your reaction to item 2 is "this is stupid", you'd be exactly right.
> 
> Here's the conclusion of a discussion on possible improvements we had in
> 2020:
> https://lore.kernel.org/qemu-devel/877dt5ofoi.fsf@dusky.pond.sub.org/

Looking at those options again I so strongly want to be able to
say "none of the above".

We have a need to describe data structures, and generate code for
serializing/deserializing them on the wire. We created a language
for this and wrote our own C code generator, our own docs generator,
own our json parser & formatter, and now are also writing our own
Go code generator (Python, Rust too ?).

IMHO this is way too much NiH for my likely, and creates a maint
burden for ourselves that we could do without.

<open-can-of-worms>
At the point in time we invented QAPI this was perhaps justifiable,
though even back then I thought we should have done a binary format
and used XDR to describe it, as a standard pre-existing language and
toolset.

Today I wouldn't suggest XDR, but would be inclined to suggest
Protobuf. This already has code & docs generators for every
programming language we would ever need.

Protobuf has a defined serialization format too though which is
binary based IIUC, so distinct from our JSON wire format.

The interesting question though is whether it would be feasible to
transition to this today, despite our historical baggage ? With
recent interest in accessing QAPI for many more languages, it
is timely to consider if we can adopt a standardized toolset to
reduce this burden of talking to QEMU from other languages.

A transition would need several big ticket items

 * A QAPI visitor impl that can spit out the protobuf document.
   ie to translate all our existing QAPI formats into protobuf,
   as doing this manually would be madness. This is probably
   the easy bit.

 * A custom protobuf visitor impl that can spit out C code
   that has the same API as our QAPI C generator. ie so we can
   avoid a big bang to convert all of QEMU internals. I suspect
   this is quite alot of work.

 * A custom protobuf serializer compatible with our current
   JSON format. ie so that QEMU can continue to expose the
   current QMP protocol to apps for legacy compat for some
   period of time, while also exposing a new native binary
   protobuf protocol for future usage. Also probably quite
   alot of work.

That's all certainly quite alot of work and probably things I have
forgotten. Above all it does assume it is possible to define a
loss-less mapping from QAPI -> Protobuf language. I've not proved
this is possible, but my inclination is that it probably should be,
and if not, we could likely enable it by strategically deprecated
and then deleting troublesome bits prior to a conversion.
</open-can-of-worms>

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 :|



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Should we replace QAPI? (was: [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress')
  2023-10-20 13:31         ` Daniel P. Berrangé
@ 2023-10-23  9:01           ` Markus Armbruster
  2023-10-23 12:20             ` Daniel P. Berrangé
  0 siblings, 1 reply; 25+ messages in thread
From: Markus Armbruster @ 2023-10-23  9:01 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: Fabiano Rosas, qemu-devel, prerna.saxena, dgilbert, pbonzini,
	eblake, manish.mishra, aravind.retnakaran, Het Gala,
	Juan Quintela, Peter Xu, Leonardo Bras

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

> On Fri, Oct 20, 2023 at 02:37:16PM +0200, Markus Armbruster wrote:
>> Fabiano Rosas <farosas@suse.de> writes:
>> 
>> > Markus Armbruster <armbru@redhat.com> writes:
>> >
>> >> Fabiano Rosas <farosas@suse.de> writes:
>> >>
>> >>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> >
>> > Side question: are we using valid JSON at all? I threw this in a random
>> > online linter and it complains about the single quotes. We could have a
>> > proper tool doing the validation in CI.
>> 
>> You've come a sad, sad place.
>> 
>> docs/devel/qapi-code-gen.rst:
>> 
>>     Schema syntax
>>     -------------
>> 
>>     Syntax is loosely based on `JSON <http://www.ietf.org/rfc/rfc8259.txt>`_.
>>     Differences:
>> 
>>     * Comments: start with a hash character (``#``) that is not part of a
>>       string, and extend to the end of the line.
>> 
>>     * Strings are enclosed in ``'single quotes'``, not ``"double quotes"``.
>> 
>>     * Strings are restricted to printable ASCII, and escape sequences to
>>       just ``\\``.
>> 
>>     * Numbers and ``null`` are not supported.
>> 
>> If your reaction to item 2 is "this is stupid", you'd be exactly right.
>> 
>> Here's the conclusion of a discussion on possible improvements we had in
>> 2020:
>> https://lore.kernel.org/qemu-devel/877dt5ofoi.fsf@dusky.pond.sub.org/
>
> Looking at those options again I so strongly want to be able to
> say "none of the above".
>
> We have a need to describe data structures, and generate code for
> serializing/deserializing them on the wire. We created a language
> for this and wrote our own C code generator, our own docs generator,
> own our json parser & formatter, and now are also writing our own
> Go code generator (Python, Rust too ?).
>
> IMHO this is way too much NiH for my likely, and creates a maint
> burden for ourselves that we could do without.
>
> <open-can-of-worms>
> At the point in time we invented QAPI this was perhaps justifiable,
> though even back then I thought we should have done a binary format
> and used XDR to describe it, as a standard pre-existing language and
> toolset.

Path dependence...

I wasn't involved in the decisions that led to QAPI, nor its initial
design and implementation.

Its design was substantially constrained by QMP, which predates QAPI by
almost two years.  I was involved in QMP's design for a bit.  We argued
back and forth, and I eventually stepped aside to let the guys doing the
actual work make the decisions.

QAPI was further constrained by the desire to use it with QOM.  The two
are joined at the hip by visitors.  Nevertheless, QOM and QAPI are
integrated poorly.

My general attitude towards generating source code is "don't; use a more
powerful programming language".  Instead of creating yet another pillar
supporting Greenspun's Tenth Rule, I feel we should have embedded a
sufficiently powerful programming language, and used it for the control
plane.  My choice would've been Lisp.

The maintenance burden is modest, but real.  The QAPI generator has
gotten ~1.6 patches per week for the last five years, trending down.  In
the last year, it's been 68 patches, 437 insertions, 338 deletions
total, much of it in docs/devel/qapi-code-gen.rst.  Meanwhile, the
entire project has had 130 times as many patches, 620 times as many
insertions, and 400 times as many deletions.

QAPI infrastructure maintenance is dwarved several times over by QAPI
schema maintenance.  Chiefly patch review.

To be fair, adding language bindings will take a non-trivial one time
investment plus ongoing maintenance for each language.

> Today I wouldn't suggest XDR, but would be inclined to suggest
> Protobuf. This already has code & docs generators for every
> programming language we would ever need.
>
> Protobuf has a defined serialization format too though which is
> binary based IIUC, so distinct from our JSON wire format.
>
> The interesting question though is whether it would be feasible to
> transition to this today, despite our historical baggage ? With
> recent interest in accessing QAPI for many more languages, it

Go, Rust, and what else?

> is timely to consider if we can adopt a standardized toolset to
> reduce this burden of talking to QEMU from other languages.
>
> A transition would need several big ticket items
>
>  * A QAPI visitor impl that can spit out the protobuf document.
>    ie to translate all our existing QAPI formats into protobuf,
>    as doing this manually would be madness. This is probably
>    the easy bit.

This is about machine-assisted translation of the QAPI schema to
protobuf, isn't it?

>  * A custom protobuf visitor impl that can spit out C code
>    that has the same API as our QAPI C generator. ie so we can
>    avoid a big bang to convert all of QEMU internals. I suspect
>    this is quite alot of work.

The alternative is a big bang to convert, which could be less work or
more.

>  * A custom protobuf serializer compatible with our current
>    JSON format. ie so that QEMU can continue to expose the
>    current QMP protocol to apps for legacy compat for some
>    period of time, while also exposing a new native binary
>    protobuf protocol for future usage. Also probably quite
>    alot of work.

No alternative.

> That's all certainly quite alot of work and probably things I have
> forgotten.

QOM's use of QAPI might add problems.

>            Above all it does assume it is possible to define a
> loss-less mapping from QAPI -> Protobuf language. I've not proved
> this is possible, but my inclination is that it probably should be,
> and if not, we could likely enable it by strategically deprecated
> and then deleting troublesome bits prior to a conversion.
> </open-can-of-worms>

While I share your distaste for the massive NIH QAPI has become, I'm not
sure replacing it now is economical.  It requires a massive one-time
investment, offset by saving us one-time investments into QAPI bindings
for other languages of interest.  Whether it can actually reduce ongoing
maintenance appreciably after the replacement is unclear.

Pretty much the same for QOM.



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v14 10/14] fixup! migration: New migrate and migrate-incoming argument 'channels'
  2023-10-19 19:23 ` [PATCH v14 10/14] fixup! " Fabiano Rosas
@ 2023-10-23 11:54   ` Daniel P. Berrangé
  0 siblings, 0 replies; 25+ messages in thread
From: Daniel P. Berrangé @ 2023-10-23 11:54 UTC (permalink / raw)
  To: Fabiano Rosas
  Cc: qemu-devel, prerna.saxena, dgilbert, pbonzini, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

On Thu, Oct 19, 2023 at 04:23:49PM -0300, Fabiano Rosas wrote:
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  qapi/migration.json | 8 ++++++++
>  1 file changed, 8 insertions(+)

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

assuming you'll just squash into the previous patch

> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 85ad5f2601..f51e6663bb 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1666,6 +1666,14 @@
>  #                                    "port": "1050" } } ] } }
>  # <- { "return": {} }
>  #
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channel-type": "main",
> +#                          "addr": { "transport": "file",
> +#                                    "path": "/tmp/migfile",
> +#                                    "offset": "0x1000" } } ] } }
> +# <- { "return": {} }
> +#
>  ##
>  { 'command': 'migrate',
>    'data': {'*uri': 'str', '*channels': [ 'MigrationChannel' ],
> -- 
> 2.35.3
> 

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



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v14 04/14] fixup! migration: convert migration 'uri' into 'MigrateAddress'
  2023-10-19 19:23 ` [PATCH v14 04/14] fixup! " Fabiano Rosas
@ 2023-10-23 11:55   ` Daniel P. Berrangé
  2023-10-31 15:10     ` Juan Quintela
  0 siblings, 1 reply; 25+ messages in thread
From: Daniel P. Berrangé @ 2023-10-23 11:55 UTC (permalink / raw)
  To: Fabiano Rosas
  Cc: qemu-devel, prerna.saxena, dgilbert, pbonzini, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

On Thu, Oct 19, 2023 at 04:23:43PM -0300, Fabiano Rosas wrote:
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/file.c      | 2 +-
>  migration/file.h      | 1 +
>  migration/migration.c | 6 ++++++
>  3 files changed, 8 insertions(+), 1 deletion(-)

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

assuming it'll be squashed into previous


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 :|



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress'
  2023-10-19 19:23 ` [PATCH v14 02/14] fixup! " Fabiano Rosas
  2023-10-20  5:06   ` Markus Armbruster
@ 2023-10-23 11:56   ` Daniel P. Berrangé
  1 sibling, 0 replies; 25+ messages in thread
From: Daniel P. Berrangé @ 2023-10-23 11:56 UTC (permalink / raw)
  To: Fabiano Rosas
  Cc: qemu-devel, prerna.saxena, dgilbert, pbonzini, armbru, eblake,
	manish.mishra, aravind.retnakaran, Het Gala, Juan Quintela,
	Peter Xu, Leonardo Bras

On Thu, Oct 19, 2023 at 04:23:41PM -0300, Fabiano Rosas wrote:
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  qapi/migration.json | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)

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

from a functional POV, and assuming Markus' feedback
will be included.

> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c352c7ac52..602cb706e3 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1519,10 +1519,25 @@
>  #
>  # @rdma: Migrate via RDMA.
>  #
> +# @file: Direct the migration stream to a file.
> +#
>  # Since 8.2
>  ##
>  { 'enum': 'MigrationAddressType',
> -  'data': ['socket', 'exec', 'rdma'] }
> +  'data': ['socket', 'exec', 'rdma', 'file'] }
> +
> +##
> +# @FileMigrationArgs:
> +#
> +# @path: file path
> +#
> +# @offset: initial offset for the file
> +#
> +# Since 8.2
> +##
> +{ 'struct': 'FileMigrationArgs',
> +  'data': {'path': 'str',
> +           'offset': 'uint64' } }
>  
>  ##
>  # @MigrationExecCommand:
> @@ -1547,7 +1562,8 @@
>    'data': {
>      'socket': 'SocketAddress',
>      'exec': 'MigrationExecCommand',
> -    'rdma': 'InetSocketAddress' } }
> +    'rdma': 'InetSocketAddress',
> +    'file': 'FileMigrationArgs' } }
>  
>  ##
>  # @migrate:
> -- 
> 2.35.3
> 

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



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: Should we replace QAPI? (was: [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress')
  2023-10-23  9:01           ` Should we replace QAPI? (was: [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress') Markus Armbruster
@ 2023-10-23 12:20             ` Daniel P. Berrangé
  0 siblings, 0 replies; 25+ messages in thread
From: Daniel P. Berrangé @ 2023-10-23 12:20 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Fabiano Rosas, qemu-devel, prerna.saxena, dgilbert, pbonzini,
	eblake, manish.mishra, aravind.retnakaran, Het Gala,
	Juan Quintela, Peter Xu, Leonardo Bras

On Mon, Oct 23, 2023 at 11:01:43AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Oct 20, 2023 at 02:37:16PM +0200, Markus Armbruster wrote:
> >> Fabiano Rosas <farosas@suse.de> writes:
> >> 
> >> > Markus Armbruster <armbru@redhat.com> writes:
> >> >
> >> >> Fabiano Rosas <farosas@suse.de> writes:
> >> >>
> >> >>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> >
> >> > Side question: are we using valid JSON at all? I threw this in a random
> >> > online linter and it complains about the single quotes. We could have a
> >> > proper tool doing the validation in CI.
> >> 
> >> You've come a sad, sad place.
> >> 
> >> docs/devel/qapi-code-gen.rst:
> >> 
> >>     Schema syntax
> >>     -------------
> >> 
> >>     Syntax is loosely based on `JSON <http://www.ietf.org/rfc/rfc8259.txt>`_.
> >>     Differences:
> >> 
> >>     * Comments: start with a hash character (``#``) that is not part of a
> >>       string, and extend to the end of the line.
> >> 
> >>     * Strings are enclosed in ``'single quotes'``, not ``"double quotes"``.
> >> 
> >>     * Strings are restricted to printable ASCII, and escape sequences to
> >>       just ``\\``.
> >> 
> >>     * Numbers and ``null`` are not supported.
> >> 
> >> If your reaction to item 2 is "this is stupid", you'd be exactly right.
> >> 
> >> Here's the conclusion of a discussion on possible improvements we had in
> >> 2020:
> >> https://lore.kernel.org/qemu-devel/877dt5ofoi.fsf@dusky.pond.sub.org/
> >
> > Looking at those options again I so strongly want to be able to
> > say "none of the above".
> >
> > We have a need to describe data structures, and generate code for
> > serializing/deserializing them on the wire. We created a language
> > for this and wrote our own C code generator, our own docs generator,
> > own our json parser & formatter, and now are also writing our own
> > Go code generator (Python, Rust too ?).
> >
> > IMHO this is way too much NiH for my likely, and creates a maint
> > burden for ourselves that we could do without.
> >
> > <open-can-of-worms>
> > At the point in time we invented QAPI this was perhaps justifiable,
> > though even back then I thought we should have done a binary format
> > and used XDR to describe it, as a standard pre-existing language and
> > toolset.
> 
> Path dependence...
> 
> I wasn't involved in the decisions that led to QAPI, nor its initial
> design and implementation.
> 
> Its design was substantially constrained by QMP, which predates QAPI by
> almost two years.  I was involved in QMP's design for a bit.  We argued
> back and forth, and I eventually stepped aside to let the guys doing the
> actual work make the decisions.

I'm somewhat conflating QMP & QAPI in my previous message too.

> The maintenance burden is modest, but real.  The QAPI generator has
> gotten ~1.6 patches per week for the last five years, trending down.  In
> the last year, it's been 68 patches, 437 insertions, 338 deletions
> total, much of it in docs/devel/qapi-code-gen.rst.  Meanwhile, the
> entire project has had 130 times as many patches, 620 times as many
> insertions, and 400 times as many deletions.
> 
> QAPI infrastructure maintenance is dwarved several times over by QAPI
> schema maintenance.  Chiefly patch review.

Yeah, patch review on new additions is primary amount and work
and indeed the bit we actually do want to be investing most
time in, as API modelling is a hard problem that needs careful
review.

> To be fair, adding language bindings will take a non-trivial one time
> investment plus ongoing maintenance for each language.

As well as the developer & reviewer man hours cost from creatnig
such language bindings, there's also the negative impact of
potential developers being discouraged from interacting with QEMU
precisely to avoid taking on this man hours cost. Impossible to
quantify, as we can't measure how many projects haven't even been
started, due to such avoidance. This hidden cost of discouragement
is where I think the value of using standard protocols wins out
the most.

> 
> > Today I wouldn't suggest XDR, but would be inclined to suggest
> > Protobuf. This already has code & docs generators for every
> > programming language we would ever need.
> >
> > Protobuf has a defined serialization format too though which is
> > binary based IIUC, so distinct from our JSON wire format.
> >
> > The interesting question though is whether it would be feasible to
> > transition to this today, despite our historical baggage ? With
> > recent interest in accessing QAPI for many more languages, it
> 
> Go, Rust, and what else?

The big one is Python, which we should have done years ago, since
we use it widely in our own test suites.

Over time QEMU is widely used enough though, that it would be
conceptually useful for many other non-niche languages to have
formal APIs for talking to QEMU, but not compelling enough to
justify manually writing such APIs or code generators ourselves.

> > is timely to consider if we can adopt a standardized toolset to
> > reduce this burden of talking to QEMU from other languages.
> >
> > A transition would need several big ticket items
> >
> >  * A QAPI visitor impl that can spit out the protobuf document.
> >    ie to translate all our existing QAPI formats into protobuf,
> >    as doing this manually would be madness. This is probably
> >    the easy bit.
> 
> This is about machine-assisted translation of the QAPI schema to
> protobuf, isn't it?

Yeah, schema conversion.


> >            Above all it does assume it is possible to define a
> > loss-less mapping from QAPI -> Protobuf language. I've not proved
> > this is possible, but my inclination is that it probably should be,
> > and if not, we could likely enable it by strategically deprecated
> > and then deleting troublesome bits prior to a conversion.
> > </open-can-of-worms>
> 
> While I share your distaste for the massive NIH QAPI has become, I'm not
> sure replacing it now is economical.  It requires a massive one-time
> investment, offset by saving us one-time investments into QAPI bindings
> for other languages of interest.  Whether it can actually reduce ongoing
> maintenance appreciably after the replacement is unclear.

This is the really hard to answer question, as the calculating value of
a conversion relies on predicting an unknowable future. It is right to
be sceptical about the value of changing, but also worth thinking about
it none the less.

Its probably the kind of project that someone who doesn't have to justify
their time would do as an experiment in a fork of QEMU for a while to
see how it actually works out.

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 :|



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v14 04/14] fixup! migration: convert migration 'uri' into 'MigrateAddress'
  2023-10-23 11:55   ` Daniel P. Berrangé
@ 2023-10-31 15:10     ` Juan Quintela
  0 siblings, 0 replies; 25+ messages in thread
From: Juan Quintela @ 2023-10-31 15:10 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: Fabiano Rosas, qemu-devel, prerna.saxena, dgilbert, pbonzini,
	armbru, eblake, manish.mishra, aravind.retnakaran, Het Gala,
	Peter Xu, Leonardo Bras

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, Oct 19, 2023 at 04:23:43PM -0300, Fabiano Rosas wrote:
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/file.c      | 2 +-
>>  migration/file.h      | 1 +
>>  migration/migration.c | 6 ++++++
>>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

I am assuming this reviewed-by applies to this and previous patches, as
I am squashing them, I can't do the "partial"e reviewed-by.

> assuming it'll be squashed into previous
>
>
> With regards,
> Daniel



^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2023-10-31 15:11 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-19 19:23 [PATCH v14 00/14] migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration Fabiano Rosas
2023-10-19 19:23 ` [PATCH v14 01/14] migration: New QAPI type 'MigrateAddress' Fabiano Rosas
2023-10-19 19:23 ` [PATCH v14 02/14] fixup! " Fabiano Rosas
2023-10-20  5:06   ` Markus Armbruster
2023-10-20 12:07     ` Fabiano Rosas
2023-10-20 12:37       ` Markus Armbruster
2023-10-20 13:31         ` Daniel P. Berrangé
2023-10-23  9:01           ` Should we replace QAPI? (was: [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress') Markus Armbruster
2023-10-23 12:20             ` Daniel P. Berrangé
2023-10-23 11:56   ` [PATCH v14 02/14] fixup! migration: New QAPI type 'MigrateAddress' Daniel P. Berrangé
2023-10-19 19:23 ` [PATCH v14 03/14] migration: convert migration 'uri' into 'MigrateAddress' Fabiano Rosas
2023-10-19 19:23 ` [PATCH v14 04/14] fixup! " Fabiano Rosas
2023-10-23 11:55   ` Daniel P. Berrangé
2023-10-31 15:10     ` Juan Quintela
2023-10-19 19:23 ` [PATCH v14 05/14] migration: convert socket backend to accept MigrateAddress Fabiano Rosas
2023-10-19 19:23 ` [PATCH v14 06/14] migration: convert rdma " Fabiano Rosas
2023-10-19 19:23 ` [PATCH v14 07/14] migration: convert exec " Fabiano Rosas
2023-10-19 19:23 ` [PATCH v14 08/14] migration: Convert the file backend the new QAPI syntax Fabiano Rosas
2023-10-19 19:23 ` [PATCH v14 09/14] migration: New migrate and migrate-incoming argument 'channels' Fabiano Rosas
2023-10-19 19:23 ` [PATCH v14 10/14] fixup! " Fabiano Rosas
2023-10-23 11:54   ` Daniel P. Berrangé
2023-10-19 19:23 ` [PATCH v14 11/14] migration: modify migration_channels_and_uri_compatible() for new QAPI syntax Fabiano Rosas
2023-10-19 19:23 ` [PATCH v14 12/14] migration: Implement MigrateChannelList to qmp migration flow Fabiano Rosas
2023-10-19 19:23 ` [PATCH v14 13/14] migration: Implement MigrateChannelList to hmp " Fabiano Rosas
2023-10-19 19:23 ` [PATCH v14 14/14] migration: modify test_multifd_tcp_none() to use new QAPI syntax Fabiano Rosas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).