[PATCH v3 0/3] migration mapped-ram fixes

[PATCH v3 0/3] migration mapped-ram fixes

[Fabiano Rosas]

Hi,

In this v3:

patch 1 - The fd_is_socket() verification and an update to the comment
          in the code;

patch 2 - The fix for the fd-reuse bug in outgoing_args;

patch 3 - A proposal on how to fix the fd-socket vs. fd-file
          issue. I'm basically moving the fd_is_socket() call earlier
          to be able to do the checks properly.

based-on: https://gitlab.com/peterx/qemu/-/commits/migration-stable

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

Fabiano Rosas (3):
  migration/multifd: Ensure we're not given a socket for file migration
  migration/multifd: Duplicate the fd for the outgoing_args
  migration: Add fd to FileMigrationArgs

 migration/fd.c        |  20 ++++++---
 migration/file.c      |   9 ++++
 migration/migration.c | 100 ++++++++++++++++++++++++++++++++++++------
 migration/migration.h |   1 +
 qapi/migration.json   |  11 ++++-
 5 files changed, 119 insertions(+), 22 deletions(-)

-- 
2.35.3

[PATCH v3 1/3] migration/multifd: Ensure we're not given a socket for file migration

[Fabiano Rosas]

When doing migration using the fd: URI, QEMU will fetch the file
descriptor passed in via the monitor at
fd_start_outgoing|incoming_migration(), which means the checks at
migration_channels_and_transport_compatible() happen too soon and we
don't know at that point whether the FD refers to a plain file or a
socket.

For this reason, we've been allowing a migration channel of type
SOCKET_ADDRESS_TYPE_FD to pass the initial verifications in scenarios
where the socket migration is not supported, such as with fd + multifd.

The commit decdc76772 ("migration/multifd: Add mapped-ram support to
fd: URI") was supposed to add a second check prior to starting
migration to make sure a socket fd is not passed instead of a file fd,
but failed to do so.

Add the missing verification and update the comment explaining this
situation which is currently incorrect.

Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI")
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/fd.c        | 8 ++++++++
 migration/file.c      | 7 +++++++
 migration/migration.c | 6 +++---
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/migration/fd.c b/migration/fd.c
index 39a52e5c90..c07030f715 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -22,6 +22,7 @@
 #include "migration.h"
 #include "monitor/monitor.h"
 #include "io/channel-file.h"
+#include "io/channel-socket.h"
 #include "io/channel-util.h"
 #include "options.h"
 #include "trace.h"
@@ -95,6 +96,13 @@ void fd_start_incoming_migration(const char *fdname, Error **errp)
     }
 
     if (migrate_multifd()) {
+        if (fd_is_socket(fd)) {
+            error_setg(errp,
+                       "Multifd migration to a socket FD is not supported");
+            object_unref(ioc);
+            return;
+        }
+
         file_create_incoming_channels(ioc, errp);
     } else {
         qio_channel_set_name(ioc, "migration-fd-incoming");
diff --git a/migration/file.c b/migration/file.c
index ddde0ca818..b6e8ba13f2 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -15,6 +15,7 @@
 #include "file.h"
 #include "migration.h"
 #include "io/channel-file.h"
+#include "io/channel-socket.h"
 #include "io/channel-util.h"
 #include "options.h"
 #include "trace.h"
@@ -58,6 +59,12 @@ bool file_send_channel_create(gpointer opaque, Error **errp)
     int fd = fd_args_get_fd();
 
     if (fd && fd != -1) {
+        if (fd_is_socket(fd)) {
+            error_setg(errp,
+                       "Multifd migration to a socket FD is not supported");
+            goto out;
+        }
+
         ioc = qio_channel_file_new_dupfd(fd, errp);
     } else {
         ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp);
diff --git a/migration/migration.c b/migration/migration.c
index 644e073b7d..f60bd371e3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -166,9 +166,9 @@ static bool transport_supports_seeking(MigrationAddress *addr)
     }
 
     /*
-     * At this point, the user might not yet have passed the file
-     * descriptor to QEMU, so we cannot know for sure whether it
-     * refers to a plain file or a socket. Let it through anyway.
+     * At this point QEMU has not yet fetched the fd passed in by the
+     * user, so we cannot know for sure whether it refers to a plain
+     * file or a socket. Let it through anyway and check at fd.c.
      */
     if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
         return addr->u.socket.type == SOCKET_ADDRESS_TYPE_FD;
-- 
2.35.3

Re: [PATCH v3 1/3] migration/multifd: Ensure we're not given a socket for file migration

[Peter Xu]

On Fri, Mar 15, 2024 at 12:20:38AM -0300, Fabiano Rosas wrote:
> When doing migration using the fd: URI, QEMU will fetch the file
> descriptor passed in via the monitor at
> fd_start_outgoing|incoming_migration(), which means the checks at
> migration_channels_and_transport_compatible() happen too soon and we
> don't know at that point whether the FD refers to a plain file or a
> socket.
> 
> For this reason, we've been allowing a migration channel of type
> SOCKET_ADDRESS_TYPE_FD to pass the initial verifications in scenarios
> where the socket migration is not supported, such as with fd + multifd.
> 
> The commit decdc76772 ("migration/multifd: Add mapped-ram support to
> fd: URI") was supposed to add a second check prior to starting
> migration to make sure a socket fd is not passed instead of a file fd,
> but failed to do so.
> 
> Add the missing verification and update the comment explaining this
> situation which is currently incorrect.
> 
> Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI")
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v3 2/3] migration/multifd: Duplicate the fd for the outgoing_args

[Fabiano Rosas]

We currently store the file descriptor used during the main outgoing
channel creation to use it again when creating the multifd
channels.

Since this fd is used for the first iochannel, there's risk that the
QIOChannel gets freed and the fd closed while outgoing_args.fd still
has it available. This could lead to an fd-reuse bug.

Duplicate the outgoing_args fd to avoid this issue.

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/fd.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/migration/fd.c b/migration/fd.c
index c07030f715..fe0d096abd 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -49,8 +49,7 @@ void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **
 {
     QIOChannel *ioc;
     int fd = monitor_get_fd(monitor_cur(), fdname, errp);
-
-    outgoing_args.fd = -1;
+    int newfd;
 
     if (fd == -1) {
         return;
@@ -63,7 +62,17 @@ void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **
         return;
     }
 
-    outgoing_args.fd = fd;
+    /*
+     * This is dup()ed just to avoid referencing an fd that might
+     * be already closed by the iochannel.
+     */
+    newfd = dup(fd);
+    if (newfd == -1) {
+        error_setg_errno(errp, errno, "Could not dup FD %d", fd);
+        object_unref(ioc);
+        return;
+    }
+    outgoing_args.fd = newfd;
 
     qio_channel_set_name(ioc, "migration-fd-outgoing");
     migration_channel_connect(s, ioc, NULL, NULL);
-- 
2.35.3

Re: [PATCH v3 2/3] migration/multifd: Duplicate the fd for the outgoing_args

[Peter Xu]

On Fri, Mar 15, 2024 at 12:20:39AM -0300, Fabiano Rosas wrote:
> We currently store the file descriptor used during the main outgoing
> channel creation to use it again when creating the multifd
> channels.
> 
> Since this fd is used for the first iochannel, there's risk that the
> QIOChannel gets freed and the fd closed while outgoing_args.fd still
> has it available. This could lead to an fd-reuse bug.
> 
> Duplicate the outgoing_args fd to avoid this issue.
> 
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Fabiano Rosas]

The fd: URI has supported migration to a file or socket since before
QEMU 8.2. In 8.2 we added the file: URI that supported migration to a
file. So now we have two ways (three if you count exec:>cat) to
migrate to a file. Fine.

However,

In 8.2 we also added the new qmp_migrate API that uses a JSON channel
list instead of the URI. It added two migration transports SOCKET and
FILE. It was decided that the new API would classify the fd migration
as a type of socket migration, neglecting the fact that the fd.c code
also supported file migrations.

In 9.0 we're adding support for fd + multifd + mapped-ram, which is
tied to the file migration. This was implemented in fd.c, which is
only reachable when the SOCKET address type is used.

The result of this is that we're asking users of the new API to create   (1)
something called a "socket" to perform migration to a plain file. And
creating something called a "file" provides no way of passing in a
file descriptor. This is confusing.

The new API also parses the old-style URI into the new-style data
structures, so the old and correct fd:<filefd> now also considers fd:
to be socket-related only.

Unlike the other migration addresses, the fd comes already setup from
the user, it's not just a string that QEMU will use to start a
connection. We need to actually fetch the fd from the monitor before
even being able to poke at it to know if it is a socket.

Aside from the issue (1) above, the current approach of parsing the fd
into a SOCKET and only later deciding if it is file-backed doesn't
work well now that we're adding multifd support for fd: and file:
migration via the mapped-ram feature. With a larger number of
combinations, we need to be able to know upfront when an fd is backed
by a plain file vs. a socket.

We're currently using a trick of allowing socket channels to pass some   (2)
validation that they shouldn't, to only later verify if the fd was
indeed a socket.

To clean this up I'm proposing we start requiring users of the new API
to use the "file" structure when the migration stream will end up in a
file and use the "socket" structure when the fd points to an actual
socket. This improves (1).

To keep backward compatibility, I'm still accepting everything that
was accepted before and only changing the structures internally to
allow the rest of the code to rely on the MIGRATION_ADDRESS_TYPE. This
addresses (2).

We can then slowly deprecate the wrong way, i.e. using type socket for
fd + file migration.

In this patch you'll find:

- a new function migrate_resolve_fd() that fetches the fd from the
  monitor and converts the MigrationAddress into the correct type;

- the removal of the hacks for (2);

- a helper function to convert the resolved fd that's represented as a
  string for storage in the MigrationAddress back into an integer for
  consumption.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/fd.c        |  15 +------
 migration/file.c      |  14 +++---
 migration/migration.c | 100 ++++++++++++++++++++++++++++++++++++------
 migration/migration.h |   1 +
 qapi/migration.json   |  11 ++++-
 5 files changed, 107 insertions(+), 34 deletions(-)

diff --git a/migration/fd.c b/migration/fd.c
index fe0d096abd..2d4414c7ea 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -20,9 +20,6 @@
 #include "fd.h"
 #include "file.h"
 #include "migration.h"
-#include "monitor/monitor.h"
-#include "io/channel-file.h"
-#include "io/channel-socket.h"
 #include "io/channel-util.h"
 #include "options.h"
 #include "trace.h"
@@ -48,8 +45,7 @@ void fd_cleanup_outgoing_migration(void)
 void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
 {
     QIOChannel *ioc;
-    int fd = monitor_get_fd(monitor_cur(), fdname, errp);
-    int newfd;
+    int newfd, fd = migrate_fd_str_to_int(fdname, errp);
 
     if (fd == -1) {
         return;
@@ -91,7 +87,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
 void fd_start_incoming_migration(const char *fdname, Error **errp)
 {
     QIOChannel *ioc;
-    int fd = monitor_fd_param(monitor_cur(), fdname, errp);
+    int fd = migrate_fd_str_to_int(fdname, errp);
     if (fd == -1) {
         return;
     }
@@ -105,13 +101,6 @@ void fd_start_incoming_migration(const char *fdname, Error **errp)
     }
 
     if (migrate_multifd()) {
-        if (fd_is_socket(fd)) {
-            error_setg(errp,
-                       "Multifd migration to a socket FD is not supported");
-            object_unref(ioc);
-            return;
-        }
-
         file_create_incoming_channels(ioc, errp);
     } else {
         qio_channel_set_name(ioc, "migration-fd-incoming");
diff --git a/migration/file.c b/migration/file.c
index b6e8ba13f2..b29521ffa7 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -59,12 +59,6 @@ bool file_send_channel_create(gpointer opaque, Error **errp)
     int fd = fd_args_get_fd();
 
     if (fd && fd != -1) {
-        if (fd_is_socket(fd)) {
-            error_setg(errp,
-                       "Multifd migration to a socket FD is not supported");
-            goto out;
-        }
-
         ioc = qio_channel_file_new_dupfd(fd, errp);
     } else {
         ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp);
@@ -95,6 +89,10 @@ void file_start_outgoing_migration(MigrationState *s,
     uint64_t offset = file_args->offset;
     QIOChannel *ioc;
 
+    if (file_args->fd) {
+        return fd_start_outgoing_migration(s, file_args->fd, errp);
+    }
+
     trace_migration_file_outgoing(filename);
 
     fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC,
@@ -163,6 +161,10 @@ void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp)
     QIOChannelFile *fioc = NULL;
     uint64_t offset = file_args->offset;
 
+    if (file_args->fd) {
+        return fd_start_incoming_migration(file_args->fd, errp);
+    }
+
     trace_migration_file_incoming(filename);
 
     fioc = qio_channel_file_new_path(filename, O_RDONLY, 0, errp);
diff --git a/migration/migration.c b/migration/migration.c
index f60bd371e3..88162205a3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -140,10 +140,6 @@ static bool transport_supports_multi_channels(MigrationAddress *addr)
     if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
         SocketAddress *saddr = &addr->u.socket;
 
-        if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
-            return migrate_mapped_ram();
-        }
-
         return (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
                 saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
                 saddr->type == SOCKET_ADDRESS_TYPE_VSOCK);
@@ -165,15 +161,6 @@ static bool transport_supports_seeking(MigrationAddress *addr)
         return true;
     }
 
-    /*
-     * At this point QEMU has not yet fetched the fd passed in by the
-     * user, so we cannot know for sure whether it refers to a plain
-     * file or a socket. Let it through anyway and check at fd.c.
-     */
-    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
-        return addr->u.socket.type == SOCKET_ADDRESS_TYPE_FD;
-    }
-
     return false;
 }
 
@@ -608,6 +595,85 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
     return true;
 }
 
+/*
+ * Unlike other types of migration addresses, a file descriptor is not
+ * just a string that QEMU uses to initiate a connection. An fd has to
+ * be prepared in advance by the user or management app.  We only know
+ * whether a file descriptor refers to a socket or a file after
+ * retrieving it from the monitor.
+ *
+ * If the method being used to reference the fds for migration is the
+ * old "fd:" URI, migrate_uri_parse() always parses it into a
+ * MIGRATION_ADDRESS_TYPE_SOCKET.
+ *
+ * If the method is the new channels API, a file descriptor might come
+ * via either the MIGRATION_ADDRESS_TYPE_SOCKET or the
+ * MIGRATION_ADDRESS_TYPE_FILE. For backward compatibility with 8.2
+ * which allowed a file migration with MIGRATION_ADDRESS_TYPE_SOCKET,
+ * this is still allowed.
+ *
+ * This function converts all of the above into
+ * MIGRATION_ADDRESS_TYPE_FILE so the rest of code doesn't need to
+ * care about none of this.
+ */
+static bool migrate_resolve_fd(MigrationAddress *addr, Error **errp)
+{
+    int fd;
+
+    if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) {
+        FileMigrationArgs *fargs = &addr->u.file;
+
+        if (fargs->fd) {
+            fd = monitor_fd_param(monitor_cur(), fargs->fd, errp);
+            if (fd == -1) {
+                return false;
+            }
+
+            if (!fd_is_socket(fd)) {
+                addr->u.file.fd = g_strdup_printf("%d", fd);
+            } else {
+                error_setg(errp,
+                           "Migration transport 'file' does not support socket file descriptors");
+                return false;
+            }
+        }
+    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
+        SocketAddress *saddr = &addr->u.socket;
+
+        if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
+            fd = monitor_fd_param(monitor_cur(), saddr->u.fd.str, errp);
+            if (fd == -1) {
+                return false;
+            }
+
+            g_free(saddr->u.fd.str);
+
+            if (!fd_is_socket(fd)) {
+                addr->transport = MIGRATION_ADDRESS_TYPE_FILE;
+                addr->u.file.fd = g_strdup_printf("%d", fd);
+                addr->u.file.filename = NULL;
+                addr->u.file.offset = 0;
+            } else {
+                saddr->u.fd.str = g_strdup_printf("%d", fd);
+            }
+        }
+    }
+
+    return true;
+}
+
+int migrate_fd_str_to_int(const char *fdname, Error **errp)
+{
+    int err, fd;
+
+    err = qemu_strtoi(fdname, NULL, 0, &fd);
+    if (err) {
+        error_setg_errno(errp, err, "failed to convert fd");
+        return -1;
+    }
+    return fd;
+}
+
 static void qemu_start_incoming_migration(const char *uri, bool has_channels,
                                           MigrationChannelList *channels,
                                           Error **errp)
@@ -641,6 +707,10 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
         addr = channel->addr;
     }
 
+    if (!migrate_resolve_fd(addr, errp)) {
+        return;
+    }
+
     /* transport mechanism not suitable for migration? */
     if (!migration_channels_and_transport_compatible(addr, errp)) {
         return;
@@ -2086,6 +2156,10 @@ void qmp_migrate(const char *uri, bool has_channels,
         addr = channel->addr;
     }
 
+    if (!migrate_resolve_fd(addr, errp)) {
+        return;
+    }
+
     /* transport mechanism not suitable for migration? */
     if (!migration_channels_and_transport_compatible(addr, errp)) {
         return;
diff --git a/migration/migration.h b/migration/migration.h
index 8045e39c26..e265d1732c 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -543,5 +543,6 @@ int migration_rp_wait(MigrationState *s);
  * to remember the target is always the migration thread.
  */
 void migration_rp_kick(MigrationState *s);
+int migrate_fd_str_to_int(const char *fdname, Error **errp);
 
 #endif
diff --git a/qapi/migration.json b/qapi/migration.json
index aa1b39bce1..37f4b9c6fb 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1656,13 +1656,20 @@
 #
 # @filename: The file to receive the migration stream
 #
+# @fd: A file descriptor name or number.  File descriptors must be
+#     first added with the 'getfd' command. (since 9.0).
+#
 # @offset: The file offset where the migration stream will start
 #
+# Since 9.0, all members are optional, but at least one of @filename
+# or @fd are required.
+#
 # Since: 8.2
 ##
 { 'struct': 'FileMigrationArgs',
-  'data': { 'filename': 'str',
-            'offset': 'uint64' } }
+  'data': { '*filename': 'str',
+            '*fd': 'str',
+            '*offset': 'uint64' } }
 
 ##
 # @MigrationExecCommand:
-- 
2.35.3

Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Daniel P. Berrangé]

On Fri, Mar 15, 2024 at 12:20:40AM -0300, Fabiano Rosas wrote:
> The fd: URI has supported migration to a file or socket since before
> QEMU 8.2. In 8.2 we added the file: URI that supported migration to a
> file. So now we have two ways (three if you count exec:>cat) to
> migrate to a file. Fine.
> 
> However,
> 
> In 8.2 we also added the new qmp_migrate API that uses a JSON channel
> list instead of the URI. It added two migration transports SOCKET and
> FILE. It was decided that the new API would classify the fd migration
> as a type of socket migration, neglecting the fact that the fd.c code
> also supported file migrations.
> 
> In 9.0 we're adding support for fd + multifd + mapped-ram, which is
> tied to the file migration. This was implemented in fd.c, which is
> only reachable when the SOCKET address type is used.
> 
> The result of this is that we're asking users of the new API to create   (1)
> something called a "socket" to perform migration to a plain file. And
> creating something called a "file" provides no way of passing in a
> file descriptor. This is confusing.

The 'file:' protocol eventually calls into qemu_open, and this
transparently allows for FD passing using /dev/fdset/NNN syntax
to pass in FDs. 


> diff --git a/qapi/migration.json b/qapi/migration.json
> index aa1b39bce1..37f4b9c6fb 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1656,13 +1656,20 @@
>  #
>  # @filename: The file to receive the migration stream
>  #
> +# @fd: A file descriptor name or number.  File descriptors must be
> +#     first added with the 'getfd' command. (since 9.0).
> +#
>  # @offset: The file offset where the migration stream will start
>  #
> +# Since 9.0, all members are optional, but at least one of @filename
> +# or @fd are required.
> +#
>  # Since: 8.2
>  ##
>  { 'struct': 'FileMigrationArgs',
> -  'data': { 'filename': 'str',
> -            'offset': 'uint64' } }
> +  'data': { '*filename': 'str',
> +            '*fd': 'str',
> +            '*offset': 'uint64' } }

Adding 'fd' here is not desirable, because 'filename' is
resolved via qemu_open which allows for FD passing without
introducing any new syntax in interfaces which take filenames.

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

Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Fabiano Rosas]

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

> On Fri, Mar 15, 2024 at 12:20:40AM -0300, Fabiano Rosas wrote:
>> The fd: URI has supported migration to a file or socket since before
>> QEMU 8.2. In 8.2 we added the file: URI that supported migration to a
>> file. So now we have two ways (three if you count exec:>cat) to
>> migrate to a file. Fine.
>> 
>> However,
>> 
>> In 8.2 we also added the new qmp_migrate API that uses a JSON channel
>> list instead of the URI. It added two migration transports SOCKET and
>> FILE. It was decided that the new API would classify the fd migration
>> as a type of socket migration, neglecting the fact that the fd.c code
>> also supported file migrations.
>> 
>> In 9.0 we're adding support for fd + multifd + mapped-ram, which is
>> tied to the file migration. This was implemented in fd.c, which is
>> only reachable when the SOCKET address type is used.
>> 
>> The result of this is that we're asking users of the new API to create   (1)
>> something called a "socket" to perform migration to a plain file. And
>> creating something called a "file" provides no way of passing in a
>> file descriptor. This is confusing.
>
> The 'file:' protocol eventually calls into qemu_open, and this
> transparently allows for FD passing using /dev/fdset/NNN syntax
> to pass in FDs. 
>

Ok, that's technically correct. But it works differently from
fd:fdname.

/dev/fdset requires the user to switch from getfd to add-fd and it
requires QEMU and libvirt/user to synchronize on the open() flags being
used. QEMU cannot just receive any fd, it must be an fd that matches the
flags QEMU passed into qio_channel_file_new_path().

Users will have to adapt to the new API anyway, so it might be ok to
require these changes around it as well.

To keep compatibility, we'd continue to accept the fd that comes from
"fd:" or SOCKET_ADDRESS_TYPE_FD. But we'd be making it harder for users
of the fd: syntax to move to the new API.

I also don't know how we would deal with fdset when (if) we add support
for passing in more channels in the new API. It supports multiple fds,
but how do we deal with something like:

"[ { 'channel-type': 'main',"
"    'addr': { 'transport': 'file',"
"              'filename': '/dev/fdset/1' } },
"  { 'channel-type': 'second',"
"    'addr': { 'transport': 'file',"
"              'filename': '/dev/fdset/2' } } ]",

Maybe that's too far ahead for this discussion.

>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index aa1b39bce1..37f4b9c6fb 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1656,13 +1656,20 @@
>>  #
>>  # @filename: The file to receive the migration stream
>>  #
>> +# @fd: A file descriptor name or number.  File descriptors must be
>> +#     first added with the 'getfd' command. (since 9.0).
>> +#
>>  # @offset: The file offset where the migration stream will start
>>  #
>> +# Since 9.0, all members are optional, but at least one of @filename
>> +# or @fd are required.
>> +#
>>  # Since: 8.2
>>  ##
>>  { 'struct': 'FileMigrationArgs',
>> -  'data': { 'filename': 'str',
>> -            'offset': 'uint64' } }
>> +  'data': { '*filename': 'str',
>> +            '*fd': 'str',
>> +            '*offset': 'uint64' } }
>
> Adding 'fd' here is not desirable, because 'filename' is
> resolved via qemu_open which allows for FD passing without
> introducing any new syntax in interfaces which take filenames.
>
> With regards,
> Daniel

Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Daniel P. Berrangé]

On Fri, Mar 15, 2024 at 09:13:52AM -0300, Fabiano Rosas wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Mar 15, 2024 at 12:20:40AM -0300, Fabiano Rosas wrote:
> >> The fd: URI has supported migration to a file or socket since before
> >> QEMU 8.2. In 8.2 we added the file: URI that supported migration to a
> >> file. So now we have two ways (three if you count exec:>cat) to
> >> migrate to a file. Fine.
> >> 
> >> However,
> >> 
> >> In 8.2 we also added the new qmp_migrate API that uses a JSON channel
> >> list instead of the URI. It added two migration transports SOCKET and
> >> FILE. It was decided that the new API would classify the fd migration
> >> as a type of socket migration, neglecting the fact that the fd.c code
> >> also supported file migrations.
> >> 
> >> In 9.0 we're adding support for fd + multifd + mapped-ram, which is
> >> tied to the file migration. This was implemented in fd.c, which is
> >> only reachable when the SOCKET address type is used.
> >> 
> >> The result of this is that we're asking users of the new API to create   (1)
> >> something called a "socket" to perform migration to a plain file. And
> >> creating something called a "file" provides no way of passing in a
> >> file descriptor. This is confusing.
> >
> > The 'file:' protocol eventually calls into qemu_open, and this
> > transparently allows for FD passing using /dev/fdset/NNN syntax
> > to pass in FDs. 
> >
> 
> Ok, that's technically correct. But it works differently from
> fd:fdname.
> 
> /dev/fdset requires the user to switch from getfd to add-fd and it
> requires QEMU and libvirt/user to synchronize on the open() flags being
> used. QEMU cannot just receive any fd, it must be an fd that matches the
> flags QEMU passed into qio_channel_file_new_path().
> 
> Users will have to adapt to the new API anyway, so it might be ok to
> require these changes around it as well.

Passing plain files to QEMU is a new feature, so apps have to
adapt their code already. The expectations around flags are
not especially hard - RDONLY|WRONLY|RDWR, optionally with
O_DIRECT depending on what migrate feature is enabled.

> To keep compatibility, we'd continue to accept the fd that comes from
> "fd:" or SOCKET_ADDRESS_TYPE_FD. But we'd be making it harder for users
> of the fd: syntax to move to the new API.
> 
> I also don't know how we would deal with fdset when (if) we add support
> for passing in more channels in the new API. It supports multiple fds,
> but how do we deal with something like:
> 
> "[ { 'channel-type': 'main',"
> "    'addr': { 'transport': 'file',"
> "              'filename': '/dev/fdset/1' } },
> "  { 'channel-type': 'second',"
> "    'addr': { 'transport': 'file',"
> "              'filename': '/dev/fdset/2' } } ]",

Having multiple fdsets should "just work" - qemu_open() handles everything
behind the scenes, so the rest of QEMU shouldn't need to know anything
about FD passing of plain files.

Similarly for socket FD passing, if the QEMU code works with a
SocketAddress struct in its public API, FD passing of of sockets
should again "just work" without any special logic.

The only reason why 'migrate' has this "fd:name" protocol, is because
this pre-dates the introduction of "SocketAddress" in QAPI.

With the switch to "MigrateAddress" exposing "SocketAddress" directly,
'migrate' has normalized itself with best practice.

> 
> Maybe that's too far ahead for this discussion.
> 
> >> diff --git a/qapi/migration.json b/qapi/migration.json
> >> index aa1b39bce1..37f4b9c6fb 100644
> >> --- a/qapi/migration.json
> >> +++ b/qapi/migration.json
> >> @@ -1656,13 +1656,20 @@
> >>  #
> >>  # @filename: The file to receive the migration stream
> >>  #
> >> +# @fd: A file descriptor name or number.  File descriptors must be
> >> +#     first added with the 'getfd' command. (since 9.0).
> >> +#
> >>  # @offset: The file offset where the migration stream will start
> >>  #
> >> +# Since 9.0, all members are optional, but at least one of @filename
> >> +# or @fd are required.
> >> +#
> >>  # Since: 8.2
> >>  ##
> >>  { 'struct': 'FileMigrationArgs',
> >> -  'data': { 'filename': 'str',
> >> -            'offset': 'uint64' } }
> >> +  'data': { '*filename': 'str',
> >> +            '*fd': 'str',
> >> +            '*offset': 'uint64' } }
> >
> > Adding 'fd' here is not desirable, because 'filename' is
> > resolved via qemu_open which allows for FD passing without
> > introducing any new syntax in interfaces which take filenames.
> >
> > With regards,
> > Daniel
> 

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

Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Peter Xu]

[I queued patch 1-2 into -stable, leaving this patch for further
 discussions]

On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote:
> The 'file:' protocol eventually calls into qemu_open, and this
> transparently allows for FD passing using /dev/fdset/NNN syntax
> to pass in FDs. 

If it always use /dev/fdsets for files, does it mean that the newly added
SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can
drop them)?

What about the old getfd?  Is it obsolete because it relies on monitor
object?  Or maybe it's still in use?

It would be greatly helpful if there can be a summary of how libvirt uses
fd for migration purpose.

Thanks,

-- 
Peter Xu

Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> [I queued patch 1-2 into -stable, leaving this patch for further
>  discussions]
>
> On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote:
>> The 'file:' protocol eventually calls into qemu_open, and this
>> transparently allows for FD passing using /dev/fdset/NNN syntax
>> to pass in FDs. 
>
> If it always use /dev/fdsets for files, does it mean that the newly added
> SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can
> drop them)?

We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the
MigrationAddress was added. So this:

'channels': [ { 'channel-type': 'main',
                'addr': { 'transport': 'socket',
                'type': 'fd',
                'str': 'fdname' } } ]

works without multifd and without mapped-ram if the fd is a file or
socket.

So yes, you're correct, but given we already have this^ it would be
perhaps more confusing for users to allow it, but not allow the very
same JSON when multifd=true, mapped-ram=true.

That's the only reason I didn't propose reverting commit decdc76772
("migration/multifd: Add mapped-ram support to fd: URI").

For mapped-ram in libvirt, we'll definitely not use
SOCKET_ADDRESS_TYPE_FD (as in the JSON), because I don't think libvirt
supports the new API.

As for SOCKET_ADDRESS_TYPE_FD as in "fd:", we could use it when
direct-io is disabled. With direct-io, the fdset will be required.

>
> What about the old getfd?  Is it obsolete because it relies on monitor
> object?  Or maybe it's still in use?
>
> It would be greatly helpful if there can be a summary of how libvirt uses
> fd for migration purpose.
>
> Thanks,

Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Peter Xu]

On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > [I queued patch 1-2 into -stable, leaving this patch for further
> >  discussions]
> >
> > On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote:
> >> The 'file:' protocol eventually calls into qemu_open, and this
> >> transparently allows for FD passing using /dev/fdset/NNN syntax
> >> to pass in FDs. 
> >
> > If it always use /dev/fdsets for files, does it mean that the newly added
> > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can
> > drop them)?
> 
> We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the
> MigrationAddress was added. So this:
> 
> 'channels': [ { 'channel-type': 'main',
>                 'addr': { 'transport': 'socket',
>                 'type': 'fd',
>                 'str': 'fdname' } } ]
> 
> works without multifd and without mapped-ram if the fd is a file or
> socket.
> 
> So yes, you're correct, but given we already have this^ it would be
> perhaps more confusing for users to allow it, but not allow the very
> same JSON when multifd=true, mapped-ram=true.

I don't think the fd: protocol (no matter the old "fd:", or the new JSON
format) is trivial to use. If libvirt didn't use it I won't be surprised to
see nobody using it.  I want us to avoid working on things that nobody is
using, or has a better replacement.

So even if Libvirt supports both, I'm wondering whether /dev/fdset/ works
for all the cases that libvirt needs.  I am aware that the old getfd has
the monitor limitation so that if the QMP disconnected and reconnect, the
fd can be gone.  However I'm not sure whether that's the only reason to
have add-fd, and also not sure whether it means add-fd is always preferred,
so that maybe we can consider obsolete getfd?

> 
> That's the only reason I didn't propose reverting commit decdc76772
> ("migration/multifd: Add mapped-ram support to fd: URI").
> 
> For mapped-ram in libvirt, we'll definitely not use
> SOCKET_ADDRESS_TYPE_FD (as in the JSON), because I don't think libvirt
> supports the new API.
> 
> As for SOCKET_ADDRESS_TYPE_FD as in "fd:", we could use it when
> direct-io is disabled. With direct-io, the fdset will be required.
> 
> >
> > What about the old getfd?  Is it obsolete because it relies on monitor
> > object?  Or maybe it's still in use?
> >
> > It would be greatly helpful if there can be a summary of how libvirt uses
> > fd for migration purpose.
> >
> > Thanks,
> 

-- 
Peter Xu

Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Daniel P. Berrangé]

On Fri, Mar 15, 2024 at 04:54:27PM -0400, Peter Xu wrote:
> On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote:
> > Peter Xu <peterx@redhat.com> writes:
> > 
> > > [I queued patch 1-2 into -stable, leaving this patch for further
> > >  discussions]
> > >
> > > On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote:
> > >> The 'file:' protocol eventually calls into qemu_open, and this
> > >> transparently allows for FD passing using /dev/fdset/NNN syntax
> > >> to pass in FDs. 
> > >
> > > If it always use /dev/fdsets for files, does it mean that the newly added
> > > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can
> > > drop them)?
> > 
> > We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the
> > MigrationAddress was added. So this:
> > 
> > 'channels': [ { 'channel-type': 'main',
> >                 'addr': { 'transport': 'socket',
> >                 'type': 'fd',
> >                 'str': 'fdname' } } ]
> > 
> > works without multifd and without mapped-ram if the fd is a file or
> > socket.
> > 
> > So yes, you're correct, but given we already have this^ it would be
> > perhaps more confusing for users to allow it, but not allow the very
> > same JSON when multifd=true, mapped-ram=true.
> 
> I don't think the fd: protocol (no matter the old "fd:", or the new JSON
> format) is trivial to use. If libvirt didn't use it I won't be surprised to
> see nobody using it.  I want us to avoid working on things that nobody is
> using, or has a better replacement.
> 
> So even if Libvirt supports both, I'm wondering whether /dev/fdset/ works
> for all the cases that libvirt needs.  I am aware that the old getfd has
> the monitor limitation so that if the QMP disconnected and reconnect, the
> fd can be gone.  However I'm not sure whether that's the only reason to
> have add-fd, and also not sure whether it means add-fd is always preferred,
> so that maybe we can consider obsolete getfd?

Historically libvirt primariily uses the 'fd:' protocol, with a
socket FD. It never gives QEMU a plain file FD, since it has
always added its "iohelper" as a MITM, in order to add O_DIRECT
on top.

The 'getfd' command is something that is needed when talking to
QEMU for any API that involves a "SocketAddress" QAPI type,
which is applicable for migration.

With the introduction of 'MigrationAddress', the 'socket' protocol
is backed by 'SocketAddress' and thus supports FD passing for
sockets (or potentally pipes too), in combination with 'getfd'.

With the 'file' protocol in 'MigrationAddress', since it gets
backed by qemu_open(), then /dev/fdset/NN and 'add-fd' provide
passing for plain files.



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

Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Peter Xu]

On Tue, Mar 19, 2024 at 04:25:32PM +0000, Daniel P. Berrangé wrote:
> On Fri, Mar 15, 2024 at 04:54:27PM -0400, Peter Xu wrote:
> > On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote:
> > > Peter Xu <peterx@redhat.com> writes:
> > > 
> > > > [I queued patch 1-2 into -stable, leaving this patch for further
> > > >  discussions]
> > > >
> > > > On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote:
> > > >> The 'file:' protocol eventually calls into qemu_open, and this
> > > >> transparently allows for FD passing using /dev/fdset/NNN syntax
> > > >> to pass in FDs. 
> > > >
> > > > If it always use /dev/fdsets for files, does it mean that the newly added
> > > > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can
> > > > drop them)?
> > > 
> > > We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the
> > > MigrationAddress was added. So this:
> > > 
> > > 'channels': [ { 'channel-type': 'main',
> > >                 'addr': { 'transport': 'socket',
> > >                 'type': 'fd',
> > >                 'str': 'fdname' } } ]
> > > 
> > > works without multifd and without mapped-ram if the fd is a file or
> > > socket.
> > > 
> > > So yes, you're correct, but given we already have this^ it would be
> > > perhaps more confusing for users to allow it, but not allow the very
> > > same JSON when multifd=true, mapped-ram=true.
> > 
> > I don't think the fd: protocol (no matter the old "fd:", or the new JSON
> > format) is trivial to use. If libvirt didn't use it I won't be surprised to
> > see nobody using it.  I want us to avoid working on things that nobody is
> > using, or has a better replacement.
> > 
> > So even if Libvirt supports both, I'm wondering whether /dev/fdset/ works
> > for all the cases that libvirt needs.  I am aware that the old getfd has
> > the monitor limitation so that if the QMP disconnected and reconnect, the
> > fd can be gone.  However I'm not sure whether that's the only reason to
> > have add-fd, and also not sure whether it means add-fd is always preferred,
> > so that maybe we can consider obsolete getfd?
> 
> Historically libvirt primariily uses the 'fd:' protocol, with a
> socket FD. It never gives QEMU a plain file FD, since it has
> always added its "iohelper" as a MITM, in order to add O_DIRECT
> on top.
> 
> The 'getfd' command is something that is needed when talking to
> QEMU for any API that involves a "SocketAddress" QAPI type,
> which is applicable for migration.
> 
> With the introduction of 'MigrationAddress', the 'socket' protocol
> is backed by 'SocketAddress' and thus supports FD passing for
> sockets (or potentally pipes too), in combination with 'getfd'.
> 
> With the 'file' protocol in 'MigrationAddress', since it gets
> backed by qemu_open(), then /dev/fdset/NN and 'add-fd' provide
> passing for plain files.

I see.  I assume it means we still have multiple users of getfd so it's
still in use where add-fd is not yet avaiable.

But then, SOCKET_ADDRESS_TYPE_FD is then not used for libvirt in the whole
mapped-ram effort, neither do we need any support on file migrations over
"fd", e.g. fd_start_incoming_migration() for files. So we can drop these
parts, am I right?

Thanks,

-- 
Peter Xu

Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Daniel P. Berrangé]

On Tue, Mar 19, 2024 at 03:25:18PM -0400, Peter Xu wrote:
> On Tue, Mar 19, 2024 at 04:25:32PM +0000, Daniel P. Berrangé wrote:
> > On Fri, Mar 15, 2024 at 04:54:27PM -0400, Peter Xu wrote:
> > > On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote:
> > > > Peter Xu <peterx@redhat.com> writes:
> > > > 
> > > > > [I queued patch 1-2 into -stable, leaving this patch for further
> > > > >  discussions]
> > > > >
> > > > > On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote:
> > > > >> The 'file:' protocol eventually calls into qemu_open, and this
> > > > >> transparently allows for FD passing using /dev/fdset/NNN syntax
> > > > >> to pass in FDs. 
> > > > >
> > > > > If it always use /dev/fdsets for files, does it mean that the newly added
> > > > > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can
> > > > > drop them)?
> > > > 
> > > > We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the
> > > > MigrationAddress was added. So this:
> > > > 
> > > > 'channels': [ { 'channel-type': 'main',
> > > >                 'addr': { 'transport': 'socket',
> > > >                 'type': 'fd',
> > > >                 'str': 'fdname' } } ]
> > > > 
> > > > works without multifd and without mapped-ram if the fd is a file or
> > > > socket.
> > > > 
> > > > So yes, you're correct, but given we already have this^ it would be
> > > > perhaps more confusing for users to allow it, but not allow the very
> > > > same JSON when multifd=true, mapped-ram=true.
> > > 
> > > I don't think the fd: protocol (no matter the old "fd:", or the new JSON
> > > format) is trivial to use. If libvirt didn't use it I won't be surprised to
> > > see nobody using it.  I want us to avoid working on things that nobody is
> > > using, or has a better replacement.
> > > 
> > > So even if Libvirt supports both, I'm wondering whether /dev/fdset/ works
> > > for all the cases that libvirt needs.  I am aware that the old getfd has
> > > the monitor limitation so that if the QMP disconnected and reconnect, the
> > > fd can be gone.  However I'm not sure whether that's the only reason to
> > > have add-fd, and also not sure whether it means add-fd is always preferred,
> > > so that maybe we can consider obsolete getfd?
> > 
> > Historically libvirt primariily uses the 'fd:' protocol, with a
> > socket FD. It never gives QEMU a plain file FD, since it has
> > always added its "iohelper" as a MITM, in order to add O_DIRECT
> > on top.
> > 
> > The 'getfd' command is something that is needed when talking to
> > QEMU for any API that involves a "SocketAddress" QAPI type,
> > which is applicable for migration.
> > 
> > With the introduction of 'MigrationAddress', the 'socket' protocol
> > is backed by 'SocketAddress' and thus supports FD passing for
> > sockets (or potentally pipes too), in combination with 'getfd'.
> > 
> > With the 'file' protocol in 'MigrationAddress', since it gets
> > backed by qemu_open(), then /dev/fdset/NN and 'add-fd' provide
> > passing for plain files.
> 
> I see.  I assume it means we still have multiple users of getfd so it's
> still in use where add-fd is not yet avaiable.
> 
> But then, SOCKET_ADDRESS_TYPE_FD is then not used for libvirt in the whole
> mapped-ram effort, neither do we need any support on file migrations over
> "fd", e.g. fd_start_incoming_migration() for files. So we can drop these
> parts, am I right?

Correct, libvirt hasn't got any impl for 'mapped-ram' yet, at least
not something merged.

Since this is new functionality, libvirt could go straight for the
'file' protocol / address type.

At some point I think we can stop using 'fd' for traditional migration
too and pass the socket address to QEMU and let QEMU open the socket.

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

Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Peter Xu]

On Tue, Mar 19, 2024 at 07:52:47PM +0000, Daniel P. Berrangé wrote:
> On Tue, Mar 19, 2024 at 03:25:18PM -0400, Peter Xu wrote:
> > On Tue, Mar 19, 2024 at 04:25:32PM +0000, Daniel P. Berrangé wrote:
> > > On Fri, Mar 15, 2024 at 04:54:27PM -0400, Peter Xu wrote:
> > > > On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote:
> > > > > Peter Xu <peterx@redhat.com> writes:
> > > > > 
> > > > > > [I queued patch 1-2 into -stable, leaving this patch for further
> > > > > >  discussions]
> > > > > >
> > > > > > On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote:
> > > > > >> The 'file:' protocol eventually calls into qemu_open, and this
> > > > > >> transparently allows for FD passing using /dev/fdset/NNN syntax
> > > > > >> to pass in FDs. 
> > > > > >
> > > > > > If it always use /dev/fdsets for files, does it mean that the newly added
> > > > > > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can
> > > > > > drop them)?
> > > > > 
> > > > > We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the
> > > > > MigrationAddress was added. So this:
> > > > > 
> > > > > 'channels': [ { 'channel-type': 'main',
> > > > >                 'addr': { 'transport': 'socket',
> > > > >                 'type': 'fd',
> > > > >                 'str': 'fdname' } } ]
> > > > > 
> > > > > works without multifd and without mapped-ram if the fd is a file or
> > > > > socket.
> > > > > 
> > > > > So yes, you're correct, but given we already have this^ it would be
> > > > > perhaps more confusing for users to allow it, but not allow the very
> > > > > same JSON when multifd=true, mapped-ram=true.
> > > > 
> > > > I don't think the fd: protocol (no matter the old "fd:", or the new JSON
> > > > format) is trivial to use. If libvirt didn't use it I won't be surprised to
> > > > see nobody using it.  I want us to avoid working on things that nobody is
> > > > using, or has a better replacement.
> > > > 
> > > > So even if Libvirt supports both, I'm wondering whether /dev/fdset/ works
> > > > for all the cases that libvirt needs.  I am aware that the old getfd has
> > > > the monitor limitation so that if the QMP disconnected and reconnect, the
> > > > fd can be gone.  However I'm not sure whether that's the only reason to
> > > > have add-fd, and also not sure whether it means add-fd is always preferred,
> > > > so that maybe we can consider obsolete getfd?
> > > 
> > > Historically libvirt primariily uses the 'fd:' protocol, with a
> > > socket FD. It never gives QEMU a plain file FD, since it has
> > > always added its "iohelper" as a MITM, in order to add O_DIRECT
> > > on top.
> > > 
> > > The 'getfd' command is something that is needed when talking to
> > > QEMU for any API that involves a "SocketAddress" QAPI type,
> > > which is applicable for migration.
> > > 
> > > With the introduction of 'MigrationAddress', the 'socket' protocol
> > > is backed by 'SocketAddress' and thus supports FD passing for
> > > sockets (or potentally pipes too), in combination with 'getfd'.
> > > 
> > > With the 'file' protocol in 'MigrationAddress', since it gets
> > > backed by qemu_open(), then /dev/fdset/NN and 'add-fd' provide
> > > passing for plain files.
> > 
> > I see.  I assume it means we still have multiple users of getfd so it's
> > still in use where add-fd is not yet avaiable.
> > 
> > But then, SOCKET_ADDRESS_TYPE_FD is then not used for libvirt in the whole
> > mapped-ram effort, neither do we need any support on file migrations over
> > "fd", e.g. fd_start_incoming_migration() for files. So we can drop these
> > parts, am I right?
> 
> Correct, libvirt hasn't got any impl for 'mapped-ram' yet, at least
> not something merged.
> 
> Since this is new functionality, libvirt could go straight for the
> 'file' protocol / address type.
> 
> At some point I think we can stop using 'fd' for traditional migration
> too and pass the socket address to QEMU and let QEMU open the socket.

Thanks for confirming this, that sounds good.  I quickly discussed this
with Fabiano just now, I think there's a plan we start to mark fd migration
deprecated for the next release (9.1), then there is a chance we drop it in
migration for 9.3.  That'll copy libvirt list so we can re-check there.

Fabiano will then prepare patches to remove the "fd:" support on file
migrations; that will be for 9.0.

Thanks,

-- 
Peter Xu