[PATCH v6 0/2] check magic value for deciding the mapping of channels

[PATCH v6 0/2] check magic value for deciding the mapping of channels

[manish.mishra]

Current logic assumes that channel connections on the destination side are
always established in the same order as the source and the first one will
always be the main channel followed by the multifid or post-copy
preemption channel. This may not be always true, as even if a channel has a
connection established on the source side it can be in the pending state on
the destination side and a newer connection can be established first.
Basically causing out of order mapping of channels on the destination side.
Currently, all channels except post-copy preempt send a magic number, this
patch uses that magic number to decide the type of channel. This logic is
applicable only for precopy(multifd) live migration, as mentioned, the
post-copy preempt channel does not send any magic number. Also, tls live
migrations already does tls handshake before creating other channels, so
this issue is not possible with tls, hence this logic is avoided for tls
live migrations. This patch uses MSG_PEEK to check the magic number of
channels so that current data/control stream management remains
un-effected.

v2:
  TLS does not support MSG_PEEK, so V1 was broken for tls live
  migrations. For tls live migration, while initializing main channel
  tls handshake is done before we can create other channels, so this
  issue is not possible for tls live migrations. In V2 added a check
  to avoid checking magic number for tls live migration and fallback
  to older method to decide mapping of channels on destination side.

v3:
  1. Split change in two patches, io patch for read_peek routines,
     migration patch for migration related changes.
  2. Add flags to io_readv calls to get extra read flags like
     MSG_PEEK.
  3. Some other minor fixes.

v4:
  1. Removed common *all_eof routines for read peek and added one
     specific to live migration.
  2. Updated to use qemu_co_sleep_ns instead of qio_channel_yield.
  3. Some other minor fixes.

v5:
  1. Handle busy-wait in migration_channel_read_peek due partial reads.

v6:
  With earlier patch, multifd_load_setup was done only in
  migration_incoming_setup but if multifd channel is received before
  default channel, multifd channels will be uninitialized. Moved
  multifd_load_setup to migration_ioc_process_incoming.
  

manish.mishra (2):
  io: Add support for MSG_PEEK for socket channel
  migration: check magic value for deciding the mapping of channels

 chardev/char-socket.c               |  4 +--
 include/io/channel.h                |  6 ++++
 io/channel-buffer.c                 |  1 +
 io/channel-command.c                |  1 +
 io/channel-file.c                   |  1 +
 io/channel-null.c                   |  1 +
 io/channel-socket.c                 | 17 ++++++++-
 io/channel-tls.c                    |  1 +
 io/channel-websock.c                |  1 +
 io/channel.c                        | 16 ++++++---
 migration/channel-block.c           |  1 +
 migration/channel.c                 | 45 ++++++++++++++++++++++++
 migration/channel.h                 |  5 +++
 migration/migration.c               | 54 ++++++++++++++++++++---------
 migration/multifd.c                 | 19 +++++-----
 migration/multifd.h                 |  2 +-
 migration/postcopy-ram.c            |  5 +--
 migration/postcopy-ram.h            |  2 +-
 scsi/qemu-pr-helper.c               |  2 +-
 tests/qtest/tpm-emu.c               |  2 +-
 tests/unit/test-io-channel-socket.c |  1 +
 util/vhost-user-server.c            |  2 +-
 22 files changed, 148 insertions(+), 41 deletions(-)

-- 
2.22.3

[PATCH v6 1/2] io: Add support for MSG_PEEK for socket channel

[manish.mishra]

MSG_PEEK peeks at the channel, The data is treated as unread and
the next read shall still return this data. This support is
currently added only for socket class. Extra parameter 'flags'
is added to io_readv calls to pass extra read flags like MSG_PEEK.

Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
---
 chardev/char-socket.c               |  4 ++--
 include/io/channel.h                |  6 ++++++
 io/channel-buffer.c                 |  1 +
 io/channel-command.c                |  1 +
 io/channel-file.c                   |  1 +
 io/channel-null.c                   |  1 +
 io/channel-socket.c                 | 17 ++++++++++++++++-
 io/channel-tls.c                    |  1 +
 io/channel-websock.c                |  1 +
 io/channel.c                        | 16 ++++++++++++----
 migration/channel-block.c           |  1 +
 scsi/qemu-pr-helper.c               |  2 +-
 tests/qtest/tpm-emu.c               |  2 +-
 tests/unit/test-io-channel-socket.c |  1 +
 util/vhost-user-server.c            |  2 +-
 15 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 29ffe5075e..c2265436ac 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -283,11 +283,11 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf, size_t len)
     if (qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) {
         ret = qio_channel_readv_full(s->ioc, &iov, 1,
                                      &msgfds, &msgfds_num,
-                                     NULL);
+                                     0, NULL);
     } else {
         ret = qio_channel_readv_full(s->ioc, &iov, 1,
                                      NULL, NULL,
-                                     NULL);
+                                     0, NULL);
     }
 
     if (msgfds_num) {
diff --git a/include/io/channel.h b/include/io/channel.h
index f1b7e05f81..5b41d02b2b 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -34,6 +34,8 @@ OBJECT_DECLARE_TYPE(QIOChannel, QIOChannelClass,
 
 #define QIO_CHANNEL_WRITE_FLAG_ZERO_COPY 0x1
 
+#define QIO_CHANNEL_READ_FLAG_MSG_PEEK 0x1
+
 typedef enum QIOChannelFeature QIOChannelFeature;
 
 enum QIOChannelFeature {
@@ -41,6 +43,7 @@ enum QIOChannelFeature {
     QIO_CHANNEL_FEATURE_SHUTDOWN,
     QIO_CHANNEL_FEATURE_LISTEN,
     QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY,
+    QIO_CHANNEL_FEATURE_READ_MSG_PEEK,
 };
 
 
@@ -114,6 +117,7 @@ struct QIOChannelClass {
                         size_t niov,
                         int **fds,
                         size_t *nfds,
+                        int flags,
                         Error **errp);
     int (*io_close)(QIOChannel *ioc,
                     Error **errp);
@@ -188,6 +192,7 @@ void qio_channel_set_name(QIOChannel *ioc,
  * @niov: the length of the @iov array
  * @fds: pointer to an array that will received file handles
  * @nfds: pointer filled with number of elements in @fds on return
+ * @flags: read flags (QIO_CHANNEL_READ_FLAG_*)
  * @errp: pointer to a NULL-initialized error object
  *
  * Read data from the IO channel, storing it in the
@@ -224,6 +229,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc,
                                size_t niov,
                                int **fds,
                                size_t *nfds,
+                               int flags,
                                Error **errp);
 
 
diff --git a/io/channel-buffer.c b/io/channel-buffer.c
index bf52011be2..8096180f85 100644
--- a/io/channel-buffer.c
+++ b/io/channel-buffer.c
@@ -54,6 +54,7 @@ static ssize_t qio_channel_buffer_readv(QIOChannel *ioc,
                                         size_t niov,
                                         int **fds,
                                         size_t *nfds,
+                                        int flags,
                                         Error **errp)
 {
     QIOChannelBuffer *bioc = QIO_CHANNEL_BUFFER(ioc);
diff --git a/io/channel-command.c b/io/channel-command.c
index 74516252ba..e7edd091af 100644
--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -203,6 +203,7 @@ static ssize_t qio_channel_command_readv(QIOChannel *ioc,
                                          size_t niov,
                                          int **fds,
                                          size_t *nfds,
+                                         int flags,
                                          Error **errp)
 {
     QIOChannelCommand *cioc = QIO_CHANNEL_COMMAND(ioc);
diff --git a/io/channel-file.c b/io/channel-file.c
index b67687c2aa..d76663e6ae 100644
--- a/io/channel-file.c
+++ b/io/channel-file.c
@@ -86,6 +86,7 @@ static ssize_t qio_channel_file_readv(QIOChannel *ioc,
                                       size_t niov,
                                       int **fds,
                                       size_t *nfds,
+                                      int flags,
                                       Error **errp)
 {
     QIOChannelFile *fioc = QIO_CHANNEL_FILE(ioc);
diff --git a/io/channel-null.c b/io/channel-null.c
index 75e3781507..4fafdb770d 100644
--- a/io/channel-null.c
+++ b/io/channel-null.c
@@ -60,6 +60,7 @@ qio_channel_null_readv(QIOChannel *ioc,
                        size_t niov,
                        int **fds G_GNUC_UNUSED,
                        size_t *nfds G_GNUC_UNUSED,
+                       int flags,
                        Error **errp)
 {
     QIOChannelNull *nioc = QIO_CHANNEL_NULL(ioc);
diff --git a/io/channel-socket.c b/io/channel-socket.c
index b76dca9cc1..dfb8cb6c40 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -173,6 +173,8 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
     }
 #endif
 
+    qio_channel_set_feature(QIO_CHANNEL(ioc), QIO_CHANNEL_FEATURE_READ_MSG_PEEK);
+
     return 0;
 }
 
@@ -406,6 +408,8 @@ qio_channel_socket_accept(QIOChannelSocket *ioc,
     }
 #endif /* WIN32 */
 
+    qio_channel_set_feature(QIO_CHANNEL(cioc), QIO_CHANNEL_FEATURE_READ_MSG_PEEK);
+
     trace_qio_channel_socket_accept_complete(ioc, cioc, cioc->fd);
     return cioc;
 
@@ -496,6 +500,7 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc,
                                         size_t niov,
                                         int **fds,
                                         size_t *nfds,
+                                        int flags,
                                         Error **errp)
 {
     QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
@@ -517,6 +522,10 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc,
 
     }
 
+    if (flags & QIO_CHANNEL_READ_FLAG_MSG_PEEK) {
+        sflags |= MSG_PEEK;
+    }
+
  retry:
     ret = recvmsg(sioc->fd, &msg, sflags);
     if (ret < 0) {
@@ -624,11 +633,17 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc,
                                         size_t niov,
                                         int **fds,
                                         size_t *nfds,
+                                        int flags,
                                         Error **errp)
 {
     QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
     ssize_t done = 0;
     ssize_t i;
+    int sflags = 0;
+
+    if (flags & QIO_CHANNEL_READ_FLAG_MSG_PEEK) {
+        sflags |= MSG_PEEK;
+    }
 
     for (i = 0; i < niov; i++) {
         ssize_t ret;
@@ -636,7 +651,7 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc,
         ret = recv(sioc->fd,
                    iov[i].iov_base,
                    iov[i].iov_len,
-                   0);
+                   sflags);
         if (ret < 0) {
             if (errno == EAGAIN) {
                 if (done) {
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 4ce890a538..c730cb8ec5 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -260,6 +260,7 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
                                      size_t niov,
                                      int **fds,
                                      size_t *nfds,
+                                     int flags,
                                      Error **errp)
 {
     QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
diff --git a/io/channel-websock.c b/io/channel-websock.c
index fb4932ade7..a12acc27cf 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -1081,6 +1081,7 @@ static ssize_t qio_channel_websock_readv(QIOChannel *ioc,
                                          size_t niov,
                                          int **fds,
                                          size_t *nfds,
+                                         int flags,
                                          Error **errp)
 {
     QIOChannelWebsock *wioc = QIO_CHANNEL_WEBSOCK(ioc);
diff --git a/io/channel.c b/io/channel.c
index 0640941ac5..a8c7f11649 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -52,6 +52,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc,
                                size_t niov,
                                int **fds,
                                size_t *nfds,
+                               int flags,
                                Error **errp)
 {
     QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
@@ -63,7 +64,14 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc,
         return -1;
     }
 
-    return klass->io_readv(ioc, iov, niov, fds, nfds, errp);
+    if ((flags & QIO_CHANNEL_READ_FLAG_MSG_PEEK) &&
+        !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
+        error_setg_errno(errp, EINVAL,
+                         "Channel does not support peek read");
+        return -1;
+    }
+
+    return klass->io_readv(ioc, iov, niov, fds, nfds, flags, errp);
 }
 
 
@@ -146,7 +154,7 @@ int qio_channel_readv_full_all_eof(QIOChannel *ioc,
     while ((nlocal_iov > 0) || local_fds) {
         ssize_t len;
         len = qio_channel_readv_full(ioc, local_iov, nlocal_iov, local_fds,
-                                     local_nfds, errp);
+                                     local_nfds, 0, errp);
         if (len == QIO_CHANNEL_ERR_BLOCK) {
             if (qemu_in_coroutine()) {
                 qio_channel_yield(ioc, G_IO_IN);
@@ -284,7 +292,7 @@ ssize_t qio_channel_readv(QIOChannel *ioc,
                           size_t niov,
                           Error **errp)
 {
-    return qio_channel_readv_full(ioc, iov, niov, NULL, NULL, errp);
+    return qio_channel_readv_full(ioc, iov, niov, NULL, NULL, 0, errp);
 }
 
 
@@ -303,7 +311,7 @@ ssize_t qio_channel_read(QIOChannel *ioc,
                          Error **errp)
 {
     struct iovec iov = { .iov_base = buf, .iov_len = buflen };
-    return qio_channel_readv_full(ioc, &iov, 1, NULL, NULL, errp);
+    return qio_channel_readv_full(ioc, &iov, 1, NULL, NULL, 0, errp);
 }
 
 
diff --git a/migration/channel-block.c b/migration/channel-block.c
index f4ab53acdb..b7374363c3 100644
--- a/migration/channel-block.c
+++ b/migration/channel-block.c
@@ -53,6 +53,7 @@ qio_channel_block_readv(QIOChannel *ioc,
                         size_t niov,
                         int **fds,
                         size_t *nfds,
+                        int flags,
                         Error **errp)
 {
     QIOChannelBlock *bioc = QIO_CHANNEL_BLOCK(ioc);
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 196b78c00d..199227a556 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -614,7 +614,7 @@ static int coroutine_fn prh_read(PRHelperClient *client, void *buf, int sz,
         iov.iov_base = buf;
         iov.iov_len = sz;
         n_read = qio_channel_readv_full(QIO_CHANNEL(client->ioc), &iov, 1,
-                                        &fds, &nfds, errp);
+                                        &fds, &nfds, 0, errp);
 
         if (n_read == QIO_CHANNEL_ERR_BLOCK) {
             qio_channel_yield(QIO_CHANNEL(client->ioc), G_IO_IN);
diff --git a/tests/qtest/tpm-emu.c b/tests/qtest/tpm-emu.c
index 2994d1cf42..3cf1acaf7d 100644
--- a/tests/qtest/tpm-emu.c
+++ b/tests/qtest/tpm-emu.c
@@ -106,7 +106,7 @@ void *tpm_emu_ctrl_thread(void *data)
         int *pfd = NULL;
         size_t nfd = 0;
 
-        qio_channel_readv_full(ioc, &iov, 1, &pfd, &nfd, &error_abort);
+        qio_channel_readv_full(ioc, &iov, 1, &pfd, &nfd, 0, &error_abort);
         cmd = be32_to_cpu(cmd);
         g_assert_cmpint(cmd, ==, CMD_SET_DATAFD);
         g_assert_cmpint(nfd, ==, 1);
diff --git a/tests/unit/test-io-channel-socket.c b/tests/unit/test-io-channel-socket.c
index b36a5d972a..b964bb202d 100644
--- a/tests/unit/test-io-channel-socket.c
+++ b/tests/unit/test-io-channel-socket.c
@@ -460,6 +460,7 @@ static void test_io_channel_unix_fd_pass(void)
                            G_N_ELEMENTS(iorecv),
                            &fdrecv,
                            &nfdrecv,
+                           0,
                            &error_abort);
 
     g_assert(nfdrecv == G_N_ELEMENTS(fdsend));
diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index 232984ace6..145eb17c08 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -116,7 +116,7 @@ vu_message_read(VuDev *vu_dev, int conn_fd, VhostUserMsg *vmsg)
          * qio_channel_readv_full may have short reads, keeping calling it
          * until getting VHOST_USER_HDR_SIZE or 0 bytes in total
          */
-        rc = qio_channel_readv_full(ioc, &iov, 1, &fds, &nfds, &local_err);
+        rc = qio_channel_readv_full(ioc, &iov, 1, &fds, &nfds, 0, &local_err);
         if (rc < 0) {
             if (rc == QIO_CHANNEL_ERR_BLOCK) {
                 assert(local_err == NULL);
-- 
2.22.3

[PATCH v6 2/2] migration: check magic value for deciding the mapping of channels

[manish.mishra]

Current logic assumes that channel connections on the destination side are
always established in the same order as the source and the first one will
always be the main channel followed by the multifid or post-copy
preemption channel. This may not be always true, as even if a channel has a
connection established on the source side it can be in the pending state on
the destination side and a newer connection can be established first.
Basically causing out of order mapping of channels on the destination side.
Currently, all channels except post-copy preempt send a magic number, this
patch uses that magic number to decide the type of channel. This logic is
applicable only for precopy(multifd) live migration, as mentioned, the
post-copy preempt channel does not send any magic number. Also, tls live
migrations already does tls handshake before creating other channels, so
this issue is not possible with tls, hence this logic is avoided for tls
live migrations. This patch uses read peek to check the magic number of
channels so that current data/control stream management remains
un-effected.

Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
---
 migration/channel.c      | 45 +++++++++++++++++++++++++++++++++
 migration/channel.h      |  5 ++++
 migration/migration.c    | 54 ++++++++++++++++++++++++++++------------
 migration/multifd.c      | 19 +++++++-------
 migration/multifd.h      |  2 +-
 migration/postcopy-ram.c |  5 +---
 migration/postcopy-ram.h |  2 +-
 7 files changed, 101 insertions(+), 31 deletions(-)

diff --git a/migration/channel.c b/migration/channel.c
index 1b0815039f..fc0228a198 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -92,3 +92,48 @@ void migration_channel_connect(MigrationState *s,
     migrate_fd_connect(s, error);
     error_free(error);
 }
+
+
+/**
+ * @migration_channel_read_peek - Peek at migration channel, without
+ *     actually removing it from channel buffer.
+ *
+ * @ioc: the channel object
+ * @buf: the memory region to read data into
+ * @buflen: the number of bytes to read in @buf
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Returns 0 if successful, returns -1 and sets @errp if fails.
+ */
+int migration_channel_read_peek(QIOChannel *ioc,
+                                const char *buf,
+                                const size_t buflen,
+                                Error **errp)
+{
+    ssize_t len = 0;
+    struct iovec iov = { .iov_base = (char *)buf, .iov_len = buflen };
+
+    while (true) {
+        len = qio_channel_readv_full(ioc, &iov, 1, NULL,
+                                     NULL, QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
+
+        if (len <= 0 && len != QIO_CHANNEL_ERR_BLOCK) {
+            error_setg(errp,
+                       "Failed to peek at channel");
+            return -1;
+        }
+
+        if (len == buflen) {
+            break;
+        }
+
+        /* 1ms sleep. */
+        if (qemu_in_coroutine()) {
+            qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
+        } else {
+            g_usleep(1000);
+        }
+    }
+
+    return 0;
+}
diff --git a/migration/channel.h b/migration/channel.h
index 67a461c28a..5bdb8208a7 100644
--- a/migration/channel.h
+++ b/migration/channel.h
@@ -24,4 +24,9 @@ void migration_channel_connect(MigrationState *s,
                                QIOChannel *ioc,
                                const char *hostname,
                                Error *error_in);
+
+int migration_channel_read_peek(QIOChannel *ioc,
+                                const char *buf,
+                                const size_t buflen,
+                                Error **errp);
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index 52b5d39244..bbc9ce3ca6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -31,6 +31,7 @@
 #include "migration.h"
 #include "savevm.h"
 #include "qemu-file.h"
+#include "channel.h"
 #include "migration/vmstate.h"
 #include "block/block.h"
 #include "qapi/error.h"
@@ -663,10 +664,6 @@ static bool migration_incoming_setup(QEMUFile *f, Error **errp)
 {
     MigrationIncomingState *mis = migration_incoming_get_current();
 
-    if (multifd_load_setup(errp) != 0) {
-        return false;
-    }
-
     if (!mis->from_src_file) {
         mis->from_src_file = f;
     }
@@ -733,31 +730,56 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
 {
     MigrationIncomingState *mis = migration_incoming_get_current();
     Error *local_err = NULL;
-    bool start_migration;
     QEMUFile *f;
+    bool default_channel = true;
+    uint32_t channel_magic = 0;
+    int ret = 0;
 
-    if (!mis->from_src_file) {
-        /* The first connection (multifd may have multiple) */
+    if (migrate_use_multifd() && !migrate_postcopy_ram() &&
+        qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
+        /*
+         * With multiple channels, it is possible that we receive channels
+         * out of order on destination side, causing incorrect mapping of
+         * source channels on destination side. Check channel MAGIC to
+         * decide type of channel. Please note this is best effort, postcopy
+         * preempt channel does not send any magic number so avoid it for
+         * postcopy live migration. Also tls live migration already does
+         * tls handshake while initializing main channel so with tls this
+         * issue is not possible.
+         */
+        ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
+                                          sizeof(channel_magic), &local_err);
+
+        if (ret != 0) {
+            error_propagate(errp, local_err);
+            return;
+        }
+
+        default_channel = (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC));
+    } else {
+        default_channel = !mis->from_src_file;
+    }
+
+    if (multifd_load_setup(errp) != 0) {
+        error_setg(errp, "Failed to setup multifd channels");
+        return;
+    }
+
+    if (default_channel) {
         f = qemu_file_new_input(ioc);
 
         if (!migration_incoming_setup(f, errp)) {
             return;
         }
-
-        /*
-         * Common migration only needs one channel, so we can start
-         * right now.  Some features need more than one channel, we wait.
-         */
-        start_migration = !migration_needs_multiple_sockets();
     } else {
         /* Multiple connections */
         assert(migration_needs_multiple_sockets());
         if (migrate_use_multifd()) {
-            start_migration = multifd_recv_new_channel(ioc, &local_err);
+            multifd_recv_new_channel(ioc, &local_err);
         } else {
             assert(migrate_postcopy_preempt());
             f = qemu_file_new_input(ioc);
-            start_migration = postcopy_preempt_new_channel(mis, f);
+            postcopy_preempt_new_channel(mis, f);
         }
         if (local_err) {
             error_propagate(errp, local_err);
@@ -765,7 +787,7 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
         }
     }
 
-    if (start_migration) {
+    if (migration_has_all_channels()) {
         /* If it's a recovery, we're done */
         if (postcopy_try_recover()) {
             return;
diff --git a/migration/multifd.c b/migration/multifd.c
index 000ca4d4ec..eeb4fb87ee 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1164,9 +1164,14 @@ int multifd_load_setup(Error **errp)
     uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size();
     uint8_t i;
 
-    if (!migrate_use_multifd()) {
+    /*
+     * Return successfully if multiFD recv state is already initialised
+     * or multiFD is not enabled.
+     */
+    if (multifd_recv_state || !migrate_use_multifd()) {
         return 0;
     }
+
     if (!migrate_multi_channels_is_allowed()) {
         error_setg(errp, "multifd is not supported by current protocol");
         return -1;
@@ -1227,11 +1232,9 @@ bool multifd_recv_all_channels_created(void)
 
 /*
  * Try to receive all multifd channels to get ready for the migration.
- * - Return true and do not set @errp when correctly receiving all channels;
- * - Return false and do not set @errp when correctly receiving the current one;
- * - Return false and set @errp when failing to receive the current channel.
+ * Sets @errp when failing to receive the current channel.
  */
-bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
+void multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
 {
     MultiFDRecvParams *p;
     Error *local_err = NULL;
@@ -1244,7 +1247,7 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
                                 "failed to receive packet"
                                 " via multifd channel %d: ",
                                 qatomic_read(&multifd_recv_state->count));
-        return false;
+        return;
     }
     trace_multifd_recv_new_channel(id);
 
@@ -1254,7 +1257,7 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
                    id);
         multifd_recv_terminate_threads(local_err);
         error_propagate(errp, local_err);
-        return false;
+        return;
     }
     p->c = ioc;
     object_ref(OBJECT(ioc));
@@ -1265,6 +1268,4 @@ bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
     qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
                        QEMU_THREAD_JOINABLE);
     qatomic_inc(&multifd_recv_state->count);
-    return qatomic_read(&multifd_recv_state->count) ==
-           migrate_multifd_channels();
 }
diff --git a/migration/multifd.h b/migration/multifd.h
index e2802a9ce2..ff3aa2e2e9 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -18,7 +18,7 @@ void multifd_save_cleanup(void);
 int multifd_load_setup(Error **errp);
 int multifd_load_cleanup(Error **errp);
 bool multifd_recv_all_channels_created(void);
-bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
+void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
 void multifd_recv_sync_main(void);
 int multifd_send_sync_main(QEMUFile *f);
 int multifd_queue_page(QEMUFile *f, RAMBlock *block, ram_addr_t offset);
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index b9a37ef255..f84f783ab4 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1539,7 +1539,7 @@ void postcopy_unregister_shared_ufd(struct PostCopyFD *pcfd)
     }
 }
 
-bool postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file)
+void postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file)
 {
     /*
      * The new loading channel has its own threads, so it needs to be
@@ -1548,9 +1548,6 @@ bool postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file)
     qemu_file_set_blocking(file, true);
     mis->postcopy_qemufile_dst = file;
     trace_postcopy_preempt_new_channel();
-
-    /* Start the migration immediately */
-    return true;
 }
 
 /*
diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
index 6147bf7d1d..25881c4127 100644
--- a/migration/postcopy-ram.h
+++ b/migration/postcopy-ram.h
@@ -190,7 +190,7 @@ enum PostcopyChannels {
     RAM_CHANNEL_MAX,
 };
 
-bool postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file);
+void postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file);
 int postcopy_preempt_setup(MigrationState *s, Error **errp);
 int postcopy_preempt_wait_channel(MigrationState *s);
 
-- 
2.22.3

Re: [PATCH v6 0/2] check magic value for deciding the mapping of channels

[manish.mishra]

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

Hi Everyone,

I was just checking if it was not missed in holidays and was received. :)

Thanks

Manish Mishra

On 21/12/22 12:14 am, manish.mishra wrote:
> Current logic assumes that channel connections on the destination side are
> always established in the same order as the source and the first one will
> always be the main channel followed by the multifid or post-copy
> preemption channel. This may not be always true, as even if a channel has a
> connection established on the source side it can be in the pending state on
> the destination side and a newer connection can be established first.
> Basically causing out of order mapping of channels on the destination side.
> Currently, all channels except post-copy preempt send a magic number, this
> patch uses that magic number to decide the type of channel. This logic is
> applicable only for precopy(multifd) live migration, as mentioned, the
> post-copy preempt channel does not send any magic number. Also, tls live
> migrations already does tls handshake before creating other channels, so
> this issue is not possible with tls, hence this logic is avoided for tls
> live migrations. This patch uses MSG_PEEK to check the magic number of
> channels so that current data/control stream management remains
> un-effected.
>
> v2:
>    TLS does not support MSG_PEEK, so V1 was broken for tls live
>    migrations. For tls live migration, while initializing main channel
>    tls handshake is done before we can create other channels, so this
>    issue is not possible for tls live migrations. In V2 added a check
>    to avoid checking magic number for tls live migration and fallback
>    to older method to decide mapping of channels on destination side.
>
> v3:
>    1. Split change in two patches, io patch for read_peek routines,
>       migration patch for migration related changes.
>    2. Add flags to io_readv calls to get extra read flags like
>       MSG_PEEK.
>    3. Some other minor fixes.
>
> v4:
>    1. Removed common *all_eof routines for read peek and added one
>       specific to live migration.
>    2. Updated to use qemu_co_sleep_ns instead of qio_channel_yield.
>    3. Some other minor fixes.
>
> v5:
>    1. Handle busy-wait in migration_channel_read_peek due partial reads.
>
> v6:
>    With earlier patch, multifd_load_setup was done only in
>    migration_incoming_setup but if multifd channel is received before
>    default channel, multifd channels will be uninitialized. Moved
>    multifd_load_setup to migration_ioc_process_incoming.
>    
>
> manish.mishra (2):
>    io: Add support for MSG_PEEK for socket channel
>    migration: check magic value for deciding the mapping of channels
>
>   chardev/char-socket.c               |  4 +--
>   include/io/channel.h                |  6 ++++
>   io/channel-buffer.c                 |  1 +
>   io/channel-command.c                |  1 +
>   io/channel-file.c                   |  1 +
>   io/channel-null.c                   |  1 +
>   io/channel-socket.c                 | 17 ++++++++-
>   io/channel-tls.c                    |  1 +
>   io/channel-websock.c                |  1 +
>   io/channel.c                        | 16 ++++++---
>   migration/channel-block.c           |  1 +
>   migration/channel.c                 | 45 ++++++++++++++++++++++++
>   migration/channel.h                 |  5 +++
>   migration/migration.c               | 54 ++++++++++++++++++++---------
>   migration/multifd.c                 | 19 +++++-----
>   migration/multifd.h                 |  2 +-
>   migration/postcopy-ram.c            |  5 +--
>   migration/postcopy-ram.h            |  2 +-
>   scsi/qemu-pr-helper.c               |  2 +-
>   tests/qtest/tpm-emu.c               |  2 +-
>   tests/unit/test-io-channel-socket.c |  1 +
>   util/vhost-user-server.c            |  2 +-
>   22 files changed, 148 insertions(+), 41 deletions(-)
>

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

Re: [PATCH v6 1/2] io: Add support for MSG_PEEK for socket channel

[Juan Quintela]

"manish.mishra" <manish.mishra@nutanix.com> wrote:
> MSG_PEEK peeks at the channel, The data is treated as unread and
> the next read shall still return this data. This support is
> currently added only for socket class. Extra parameter 'flags'
> is added to io_readv calls to pass extra read flags like MSG_PEEK.
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: manish.mishra <manish.mishra@nutanix.com>

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

queued.

Re: [PATCH v6 2/2] migration: check magic value for deciding the mapping of channels

[Juan Quintela]

"manish.mishra" <manish.mishra@nutanix.com> wrote:
> Current logic assumes that channel connections on the destination side are
> always established in the same order as the source and the first one will
> always be the main channel followed by the multifid or post-copy
> preemption channel. This may not be always true, as even if a channel has a
> connection established on the source side it can be in the pending state on
> the destination side and a newer connection can be established first.
> Basically causing out of order mapping of channels on the destination side.
> Currently, all channels except post-copy preempt send a magic number, this
> patch uses that magic number to decide the type of channel. This logic is
> applicable only for precopy(multifd) live migration, as mentioned, the
> post-copy preempt channel does not send any magic number. Also, tls live
> migrations already does tls handshake before creating other channels, so
> this issue is not possible with tls, hence this logic is avoided for tls
> live migrations. This patch uses read peek to check the magic number of
> channels so that current data/control stream management remains
> un-effected.
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: manish.mishra <manish.mishra@nutanix.com>

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

Re: [PATCH v6 0/2] check magic value for deciding the mapping of channels

[Juan Quintela]

"manish.mishra" <manish.mishra@nutanix.com> wrote:
> Hi Everyone,
>
> I was just checking if it was not missed in holidays and was received. :)

Queued.

Sorry for the delay.

Re: [PATCH v6 1/2] io: Add support for MSG_PEEK for socket channel

[Juan Quintela]

"manish.mishra" <manish.mishra@nutanix.com> wrote:
> MSG_PEEK peeks at the channel, The data is treated as unread and
> the next read shall still return this data. This support is
> currently added only for socket class. Extra parameter 'flags'
> is added to io_readv calls to pass extra read flags like MSG_PEEK.
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: manish.mishra <manish.mishra@nutanix.com>


This change breaks RDMA migration.

FAILED: libcommon.fa.p/migration_rdma.c.o
cc -m64 -mcx16 -Ilibcommon.fa.p -I/usr/include/pixman-1 -I/usr/include/libpng16 -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/nss3 -I/usr/include/nspr4 -I/usr/include/PCSC -I/usr/include/p11-kit-1 -I/usr/include/libusb-1.0 -I/usr/include/SDL2 -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 -I/usr/include/slirp -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /mnt/code/qemu/full/linux-headers -isystem linux-headers -iquote . -iquote /mnt/code/qemu/full -iquote /mnt/code/qemu/full/include -iquote /mnt/code/qemu/full/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -D_REENTRANT -Wno-undef -DSTRUCT_IOVEC_DEFINED -MD -MQ libcommon.fa.p/migration_rdma.c.o -MF libcommon.fa.p/migration_rdma.c.o.d -o libcommon.fa.p/migration_rdma.c.o -c ../../../../mnt/code/qemu/full/migration/rdma.c
../../../../mnt/code/qemu/full/migration/rdma.c: In function ‘qio_channel_rdma_class_init’:
../../../../mnt/code/qemu/full/migration/rdma.c:4020:25: error: assignment to ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, int,  Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, int,  Error **)’} from incompatible pointer type ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, Error **)’} [-Werror=incompatible-pointer-types]
 4020 |     ioc_klass->io_readv = qio_channel_rdma_readv;
      |                         ^
cc1: all warnings being treated as errors

And I don't really know how to fix it, because the problem is that rdma
don't use qio_channel_readv_full() at all.

Sorry, I have to drop the series until a fix is found.

Later, Juan.

Re: [PATCH v6 1/2] io: Add support for MSG_PEEK for socket channel

[Daniel P. Berrangé]

On Thu, Feb 02, 2023 at 01:22:12PM +0100, Juan Quintela wrote:
> "manish.mishra" <manish.mishra@nutanix.com> wrote:
> > MSG_PEEK peeks at the channel, The data is treated as unread and
> > the next read shall still return this data. This support is
> > currently added only for socket class. Extra parameter 'flags'
> > is added to io_readv calls to pass extra read flags like MSG_PEEK.
> >
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> > Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
> 
> 
> This change breaks RDMA migration.
> 
> FAILED: libcommon.fa.p/migration_rdma.c.o
> cc -m64 -mcx16 -Ilibcommon.fa.p -I/usr/include/pixman-1 -I/usr/include/libpng16 -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/nss3 -I/usr/include/nspr4 -I/usr/include/PCSC -I/usr/include/p11-kit-1 -I/usr/include/libusb-1.0 -I/usr/include/SDL2 -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 -I/usr/include/slirp -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /mnt/code/qemu/full/linux-headers -isystem linux-headers -iquote . -iquote /mnt/code/qemu/full -iquote /mnt/code/qemu/full/include -iquote /mnt/code/qemu/full/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -D_REENTRANT -Wno-undef -DSTRUCT_IOVEC_DEFINED -MD -MQ libcommon.fa.p/migration_rdma.c.o -MF libcommon.fa.p/migration_rdma.c.o.d -o libcommon.fa.p/migration_rdma.c.o -c ../../../../mnt/code/qemu/full/migration/rdma.c
> ../../../../mnt/code/qemu/full/migration/rdma.c: In function ‘qio_channel_rdma_class_init’:
> ../../../../mnt/code/qemu/full/migration/rdma.c:4020:25: error: assignment to ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, int,  Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, int,  Error **)’} from incompatible pointer type ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, Error **)’} [-Werror=incompatible-pointer-types]
>  4020 |     ioc_klass->io_readv = qio_channel_rdma_readv;
>       |                         ^
> cc1: all warnings being treated as errors
> 
> And I don't really know how to fix it, because the problem is that rdma
> don't use qio_channel_readv_full() at all.

Likely qio_channel_rdma_readv just adds the 'int flags' param added.
It doesn't need to actually do anything with the flags as they are
checked before

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

Re: [PATCH v6 1/2] io: Add support for MSG_PEEK for socket channel

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, Feb 02, 2023 at 01:22:12PM +0100, Juan Quintela wrote:
>> "manish.mishra" <manish.mishra@nutanix.com> wrote:
>> > MSG_PEEK peeks at the channel, The data is treated as unread and
>> > the next read shall still return this data. This support is
>> > currently added only for socket class. Extra parameter 'flags'
>> > is added to io_readv calls to pass extra read flags like MSG_PEEK.
>> >
>> > Reviewed-by: Peter Xu <peterx@redhat.com>
>> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
>> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>> > Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
>> 
>> 
>> This change breaks RDMA migration.
>> 
>> FAILED: libcommon.fa.p/migration_rdma.c.o
>> cc -m64 -mcx16 -Ilibcommon.fa.p -I/usr/include/pixman-1 -I/usr/include/libpng16 -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/nss3 -I/usr/include/nspr4 -I/usr/include/PCSC -I/usr/include/p11-kit-1 -I/usr/include/libusb-1.0 -I/usr/include/SDL2 -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 -I/usr/include/slirp -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /mnt/code/qemu/full/linux-headers -isystem linux-headers -iquote . -iquote /mnt/code/qemu/full -iquote /mnt/code/qemu/full/include -iquote /mnt/code/qemu/full/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -D_REENTRANT -Wno-undef -DSTRUCT_IOVEC_DEFINED -MD -MQ libcommon.fa.p/migration_rdma.c.o -MF libcommon.fa.p/migration_rdma.c.o.d -o libcommon.fa.p/migration_rdma.c.o -c ../../../../mnt/code/qemu/full/migration/rdma.c
>> ../../../../mnt/code/qemu/full/migration/rdma.c: In function ‘qio_channel_rdma_class_init’:
>> ../../../../mnt/code/qemu/full/migration/rdma.c:4020:25: error: assignment to ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, int,  Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, int,  Error **)’} from incompatible pointer type ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, Error **)’} [-Werror=incompatible-pointer-types]
>>  4020 |     ioc_klass->io_readv = qio_channel_rdma_readv;
>>       |                         ^
>> cc1: all warnings being treated as errors
>> 
>> And I don't really know how to fix it, because the problem is that rdma
>> don't use qio_channel_readv_full() at all.
>
> Likely qio_channel_rdma_readv just adds the 'int flags' param added.
> It doesn't need to actually do anything with the flags as they are
> checked before

I can do that.  That would fix the compilation issue.

But will rdma work?  Because it fakes a qio channel, so what is going to
implement the MSG_PEEK functionality for it?  It don't end calling
recv() at all.


Later, Juan.

Re: [PATCH v6 1/2] io: Add support for MSG_PEEK for socket channel

[Daniel P. Berrangé]

On Thu, Feb 02, 2023 at 01:51:28PM +0100, Juan Quintela wrote:
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> > On Thu, Feb 02, 2023 at 01:22:12PM +0100, Juan Quintela wrote:
> >> "manish.mishra" <manish.mishra@nutanix.com> wrote:
> >> > MSG_PEEK peeks at the channel, The data is treated as unread and
> >> > the next read shall still return this data. This support is
> >> > currently added only for socket class. Extra parameter 'flags'
> >> > is added to io_readv calls to pass extra read flags like MSG_PEEK.
> >> >
> >> > Reviewed-by: Peter Xu <peterx@redhat.com>
> >> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> >> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> >> > Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
> >> 
> >> 
> >> This change breaks RDMA migration.
> >> 
> >> FAILED: libcommon.fa.p/migration_rdma.c.o
> >> cc -m64 -mcx16 -Ilibcommon.fa.p -I/usr/include/pixman-1 -I/usr/include/libpng16 -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/nss3 -I/usr/include/nspr4 -I/usr/include/PCSC -I/usr/include/p11-kit-1 -I/usr/include/libusb-1.0 -I/usr/include/SDL2 -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 -I/usr/include/slirp -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /mnt/code/qemu/full/linux-headers -isystem linux-headers -iquote . -iquote /mnt/code/qemu/full -iquote /mnt/code/qemu/full/include -iquote /mnt/code/qemu/full/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -D_REENTRANT -Wno-undef -DSTRUCT_IOVEC_DEFINED -MD -MQ libcommon.fa.p/migration_rdma.c.o -MF libcommon.fa.p/migration_rdma.c.o.d -o libcommon.fa.p/migration_rdma.c.o -c ../../../../mnt/code/qemu/full/migration/rdma.c
> >> ../../../../mnt/code/qemu/full/migration/rdma.c: In function ‘qio_channel_rdma_class_init’:
> >> ../../../../mnt/code/qemu/full/migration/rdma.c:4020:25: error: assignment to ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, int,  Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, int,  Error **)’} from incompatible pointer type ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, Error **)’} [-Werror=incompatible-pointer-types]
> >>  4020 |     ioc_klass->io_readv = qio_channel_rdma_readv;
> >>       |                         ^
> >> cc1: all warnings being treated as errors
> >> 
> >> And I don't really know how to fix it, because the problem is that rdma
> >> don't use qio_channel_readv_full() at all.
> >
> > Likely qio_channel_rdma_readv just adds the 'int flags' param added.
> > It doesn't need to actually do anything with the flags as they are
> > checked before
> 
> I can do that.  That would fix the compilation issue.
> 
> But will rdma work?  Because it fakes a qio channel, so what is going to
> implement the MSG_PEEK functionality for it?  It don't end calling
> recv() at all.

It is no problem - the qio_channel_readv method changes in this patch
add:

+    if ((flags & QIO_CHANNEL_READ_FLAG_MSG_PEEK) &&
+        !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
+        error_setg_errno(errp, EINVAL,
+                         "Channel does not support peek read");
+        return -1;
+    }


so it is impossible for qio_channel_rdma_readv to be invoked with
flags having MSG_PEEK set, thus RDMA can ignore the whole concept.

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

Re: [PATCH v6 1/2] io: Add support for MSG_PEEK for socket channel

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, Feb 02, 2023 at 01:51:28PM +0100, Juan Quintela wrote:
>> Daniel P. Berrangé <berrange@redhat.com> wrote:
>> > On Thu, Feb 02, 2023 at 01:22:12PM +0100, Juan Quintela wrote:
>> >> "manish.mishra" <manish.mishra@nutanix.com> wrote:
>> >> > MSG_PEEK peeks at the channel, The data is treated as unread and
>> >> > the next read shall still return this data. This support is
>> >> > currently added only for socket class. Extra parameter 'flags'
>> >> > is added to io_readv calls to pass extra read flags like MSG_PEEK.
>> >> >
>> >> > Reviewed-by: Peter Xu <peterx@redhat.com>
>> >> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
>> >> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>> >> > Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
>> >> 
>> >> 
>> >> This change breaks RDMA migration.
>> >> 
>> >> FAILED: libcommon.fa.p/migration_rdma.c.o
>> >> cc -m64 -mcx16 -Ilibcommon.fa.p -I/usr/include/pixman-1 -I/usr/include/libpng16 -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/nss3 -I/usr/include/nspr4 -I/usr/include/PCSC -I/usr/include/p11-kit-1 -I/usr/include/libusb-1.0 -I/usr/include/SDL2 -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 -I/usr/include/slirp -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /mnt/code/qemu/full/linux-headers -isystem linux-headers -iquote . -iquote /mnt/code/qemu/full -iquote /mnt/code/qemu/full/include -iquote /mnt/code/qemu/full/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -D_REENTRANT -Wno-undef -DSTRUCT_IOVEC_DEFINED -MD -MQ libcommon.fa.p/migration_rdma.c.o -MF libcommon.fa.p/migration_rdma.c.o.d -o libcommon.fa.p/migration_rdma.c.o -c ../../../../mnt/code/qemu/full/migration/rdma.c
>> >> ../../../../mnt/code/qemu/full/migration/rdma.c: In function ‘qio_channel_rdma_class_init’:
>> >> ../../../../mnt/code/qemu/full/migration/rdma.c:4020:25: error: assignment to ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, int,  Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, int,  Error **)’} from incompatible pointer type ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, Error **)’} [-Werror=incompatible-pointer-types]
>> >>  4020 |     ioc_klass->io_readv = qio_channel_rdma_readv;
>> >>       |                         ^
>> >> cc1: all warnings being treated as errors
>> >> 
>> >> And I don't really know how to fix it, because the problem is that rdma
>> >> don't use qio_channel_readv_full() at all.
>> >
>> > Likely qio_channel_rdma_readv just adds the 'int flags' param added.
>> > It doesn't need to actually do anything with the flags as they are
>> > checked before
>> 
>> I can do that.  That would fix the compilation issue.
>> 
>> But will rdma work?  Because it fakes a qio channel, so what is going to
>> implement the MSG_PEEK functionality for it?  It don't end calling
>> recv() at all.
>
> It is no problem - the qio_channel_readv method changes in this patch
> add:
>
> +    if ((flags & QIO_CHANNEL_READ_FLAG_MSG_PEEK) &&
> +        !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
> +        error_setg_errno(errp, EINVAL,
> +                         "Channel does not support peek read");
> +        return -1;
> +    }
>
>
> so it is impossible for qio_channel_rdma_readv to be invoked with
> flags having MSG_PEEK set, thus RDMA can ignore the whole concept.

And as we require MSG_PEEK to do migration, we have lost RDMA migration
in the process.

The following patch on the series use this functionality to read the
beggining of the streams in the channels.

Later, Juan.

Re: [PATCH v6 1/2] io: Add support for MSG_PEEK for socket channel

[Daniel P. Berrangé]

On Thu, Feb 02, 2023 at 02:39:05PM +0100, Juan Quintela wrote:
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> > On Thu, Feb 02, 2023 at 01:51:28PM +0100, Juan Quintela wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> wrote:
> >> > On Thu, Feb 02, 2023 at 01:22:12PM +0100, Juan Quintela wrote:
> >> >> "manish.mishra" <manish.mishra@nutanix.com> wrote:
> >> >> > MSG_PEEK peeks at the channel, The data is treated as unread and
> >> >> > the next read shall still return this data. This support is
> >> >> > currently added only for socket class. Extra parameter 'flags'
> >> >> > is added to io_readv calls to pass extra read flags like MSG_PEEK.
> >> >> >
> >> >> > Reviewed-by: Peter Xu <peterx@redhat.com>
> >> >> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> >> >> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> >> >> > Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
> >> >> 
> >> >> 
> >> >> This change breaks RDMA migration.
> >> >> 
> >> >> FAILED: libcommon.fa.p/migration_rdma.c.o
> >> >> cc -m64 -mcx16 -Ilibcommon.fa.p -I/usr/include/pixman-1 -I/usr/include/libpng16 -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/nss3 -I/usr/include/nspr4 -I/usr/include/PCSC -I/usr/include/p11-kit-1 -I/usr/include/libusb-1.0 -I/usr/include/SDL2 -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 -I/usr/include/slirp -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /mnt/code/qemu/full/linux-headers -isystem linux-headers -iquote . -iquote /mnt/code/qemu/full -iquote /mnt/code/qemu/full/include -iquote /mnt/code/qemu/full/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -D_REENTRANT -Wno-undef -DSTRUCT_IOVEC_DEFINED -MD -MQ libcommon.fa.p/migration_rdma.c.o -MF libcommon.fa.p/migration_rdma.c.o.d -o libcommon.fa.p/migration_rdma.c.o -c ../../../../mnt/code/qemu/full/migration/rdma.c
> >> >> ../../../../mnt/code/qemu/full/migration/rdma.c: In function ‘qio_channel_rdma_class_init’:
> >> >> ../../../../mnt/code/qemu/full/migration/rdma.c:4020:25: error: assignment to ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, int,  Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, int,  Error **)’} from incompatible pointer type ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, Error **)’} [-Werror=incompatible-pointer-types]
> >> >>  4020 |     ioc_klass->io_readv = qio_channel_rdma_readv;
> >> >>       |                         ^
> >> >> cc1: all warnings being treated as errors
> >> >> 
> >> >> And I don't really know how to fix it, because the problem is that rdma
> >> >> don't use qio_channel_readv_full() at all.
> >> >
> >> > Likely qio_channel_rdma_readv just adds the 'int flags' param added.
> >> > It doesn't need to actually do anything with the flags as they are
> >> > checked before
> >> 
> >> I can do that.  That would fix the compilation issue.
> >> 
> >> But will rdma work?  Because it fakes a qio channel, so what is going to
> >> implement the MSG_PEEK functionality for it?  It don't end calling
> >> recv() at all.
> >
> > It is no problem - the qio_channel_readv method changes in this patch
> > add:
> >
> > +    if ((flags & QIO_CHANNEL_READ_FLAG_MSG_PEEK) &&
> > +        !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
> > +        error_setg_errno(errp, EINVAL,
> > +                         "Channel does not support peek read");
> > +        return -1;
> > +    }
> >
> >
> > so it is impossible for qio_channel_rdma_readv to be invoked with
> > flags having MSG_PEEK set, thus RDMA can ignore the whole concept.
> 
> And as we require MSG_PEEK to do migration, we have lost RDMA migration
> in the process.
> 
> The following patch on the series use this functionality to read the
> beggining of the streams in the channels.

It guards that usage of MSG_PEEK with

   if (... && qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {


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

Re: [PATCH v6 1/2] io: Add support for MSG_PEEK for socket channel

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, Feb 02, 2023 at 02:39:05PM +0100, Juan Quintela wrote:
>> Daniel P. Berrangé <berrange@redhat.com> wrote:
>> > On Thu, Feb 02, 2023 at 01:51:28PM +0100, Juan Quintela wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> wrote:
>> >> > On Thu, Feb 02, 2023 at 01:22:12PM +0100, Juan Quintela wrote:
>> >> >> "manish.mishra" <manish.mishra@nutanix.com> wrote:
>> >> >> > MSG_PEEK peeks at the channel, The data is treated as unread and
>> >> >> > the next read shall still return this data. This support is
>> >> >> > currently added only for socket class. Extra parameter 'flags'
>> >> >> > is added to io_readv calls to pass extra read flags like MSG_PEEK.
>> >> >> >
>> >> >> > Reviewed-by: Peter Xu <peterx@redhat.com>
>> >> >> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
>> >> >> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>> >> >> > Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
>> >> >> 
>> >> >> 
>> >> >> This change breaks RDMA migration.
>> >> >> 
>> >> >> FAILED: libcommon.fa.p/migration_rdma.c.o
>> >> >> cc -m64 -mcx16 -Ilibcommon.fa.p -I/usr/include/pixman-1 -I/usr/include/libpng16 -I/usr/include/spice-server -I/usr/include/spice-1 -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/nss3 -I/usr/include/nspr4 -I/usr/include/PCSC -I/usr/include/p11-kit-1 -I/usr/include/libusb-1.0 -I/usr/include/SDL2 -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 -I/usr/include/slirp -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /mnt/code/qemu/full/linux-headers -isystem linux-headers -iquote . -iquote /mnt/code/qemu/full -iquote /mnt/code/qemu/full/include -iquote /mnt/code/qemu/full/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -D_REENTRANT -Wno-undef -DSTRUCT_IOVEC_DEFINED -MD -MQ libcommon.fa.p/migration_rdma.c.o -MF libcommon.fa.p/migration_rdma.c.o.d -o libcommon.fa.p/migration_rdma.c.o -c ../../../../mnt/code/qemu/full/migration/rdma.c
>> >> >> ../../../../mnt/code/qemu/full/migration/rdma.c: In function ‘qio_channel_rdma_class_init’:
>> >> >> ../../../../mnt/code/qemu/full/migration/rdma.c:4020:25: error: assignment to ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, int,  Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, int,  Error **)’} from incompatible pointer type ‘ssize_t (*)(QIOChannel *, const struct iovec *, size_t,  int **, size_t *, Error **)’ {aka ‘long int (*)(QIOChannel *, const struct iovec *, long unsigned int,  int **, long unsigned int *, Error **)’} [-Werror=incompatible-pointer-types]
>> >> >>  4020 |     ioc_klass->io_readv = qio_channel_rdma_readv;
>> >> >>       |                         ^
>> >> >> cc1: all warnings being treated as errors
>> >> >> 
>> >> >> And I don't really know how to fix it, because the problem is that rdma
>> >> >> don't use qio_channel_readv_full() at all.
>> >> >
>> >> > Likely qio_channel_rdma_readv just adds the 'int flags' param added.
>> >> > It doesn't need to actually do anything with the flags as they are
>> >> > checked before
>> >> 
>> >> I can do that.  That would fix the compilation issue.
>> >> 
>> >> But will rdma work?  Because it fakes a qio channel, so what is going to
>> >> implement the MSG_PEEK functionality for it?  It don't end calling
>> >> recv() at all.
>> >
>> > It is no problem - the qio_channel_readv method changes in this patch
>> > add:
>> >
>> > +    if ((flags & QIO_CHANNEL_READ_FLAG_MSG_PEEK) &&
>> > +        !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
>> > +        error_setg_errno(errp, EINVAL,
>> > +                         "Channel does not support peek read");
>> > +        return -1;
>> > +    }
>> >
>> >
>> > so it is impossible for qio_channel_rdma_readv to be invoked with
>> > flags having MSG_PEEK set, thus RDMA can ignore the whole concept.
>> 
>> And as we require MSG_PEEK to do migration, we have lost RDMA migration
>> in the process.
>> 
>> The following patch on the series use this functionality to read the
>> beggining of the streams in the channels.
>
> It guards that usage of MSG_PEEK with
>
>    if (... && qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {

You win.

They are back.

Thanks very much for the explanation.

Re: [PATCH v6 0/2] check magic value for deciding the mapping of channels

[manish.mishra]

On 31/01/23 8:47 pm, Peter Xu wrote:
> On Tue, Jan 31, 2023 at 08:29:08PM +0530, manish.mishra wrote:
>> Hi Peter, Daniel,
>>
>> Just a gentle reminder on this patch if it can be merged, and really
>> sorry i see now earlier reminders i sent were on v6[0/2] and somehow you
>> were not CCed on that earlier. You were CCed just on v6[1/2] and v6[2,2]
>> so that's why probably missed it.
> Yes I think so.  For some reason I guess Juan missed this set when sending
> the most recent PR. We should pick them up soon.
>

Thanks Peter :)

Re: [PATCH v6 0/2] check magic value for deciding the mapping of channels

[Peter Xu]

On Tue, Jan 31, 2023 at 08:29:08PM +0530, manish.mishra wrote:
> Hi Peter, Daniel,
> 
> Just a gentle reminder on this patch if it can be merged, and really
> sorry i see now earlier reminders i sent were on v6[0/2] and somehow you
> were not CCed on that earlier. You were CCed just on v6[1/2] and v6[2,2]
> so that's why probably missed it.

Yes I think so.  For some reason I guess Juan missed this set when sending
the most recent PR. We should pick them up soon.

-- 
Peter Xu

Re: [PATCH v6 0/2] check magic value for deciding the mapping of channels

[manish.mishra]

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

Hi Peter, Daniel,

Just a gentle reminder on this patch if it can be merged, and really sorry i see now earlier reminders i sent were on v6[0/2] and somehow you were not CCed on that earlier. You were CCed just on v6[1/2] and v6[2,2] so that's why probably missed it.

Thanks

Manish Mishra

On 21/12/22 12:06 am, manish.mishra wrote:
> Current logic assumes that channel connections on the destination side are
> always established in the same order as the source and the first one will
> always be the main channel followed by the multifid or post-copy
> preemption channel. This may not be always true, as even if a channel has a
> connection established on the source side it can be in the pending state on
> the destination side and a newer connection can be established first.
> Basically causing out of order mapping of channels on the destination side.
> Currently, all channels except post-copy preempt send a magic number, this
> patch uses that magic number to decide the type of channel. This logic is
> applicable only for precopy(multifd) live migration, as mentioned, the
> post-copy preempt channel does not send any magic number. Also, tls live
> migrations already does tls handshake before creating other channels, so
> this issue is not possible with tls, hence this logic is avoided for tls
> live migrations. This patch uses MSG_PEEK to check the magic number of
> channels so that current data/control stream management remains
> un-effected.
>
> v2:
>    TLS does not support MSG_PEEK, so V1 was broken for tls live
>    migrations. For tls live migration, while initializing main channel
>    tls handshake is done before we can create other channels, so this
>    issue is not possible for tls live migrations. In V2 added a check
>    to avoid checking magic number for tls live migration and fallback
>    to older method to decide mapping of channels on destination side.
>
> v3:
>    1. Split change in two patches, io patch for read_peek routines,
>       migration patch for migration related changes.
>    2. Add flags to io_readv calls to get extra read flags like
>       MSG_PEEK.
>    3. Some other minor fixes.
>
> v4:
>    1. Removed common *all_eof routines for read peek and added one
>       specific to live migration.
>    2. Updated to use qemu_co_sleep_ns instead of qio_channel_yield.
>    3. Some other minor fixes.
>
> v5:
>    1. Handle busy-wait in migration_channel_read_peek due partial reads.
>
> v6:
>    With earlier patch, multifd_load_setup was done only in
>    migration_incoming_setup but if multifd channel is received before
>    default channel, multifd channels will be uninitialized. Moved
>    multifd_load_setup to migration_ioc_process_incoming.
>    
>
> manish.mishra (2):
>    io: Add support for MSG_PEEK for socket channel
>    migration: check magic value for deciding the mapping of channels
>
>   chardev/char-socket.c               |  4 +--
>   include/io/channel.h                |  6 ++++
>   io/channel-buffer.c                 |  1 +
>   io/channel-command.c                |  1 +
>   io/channel-file.c                   |  1 +
>   io/channel-null.c                   |  1 +
>   io/channel-socket.c                 | 17 ++++++++-
>   io/channel-tls.c                    |  1 +
>   io/channel-websock.c                |  1 +
>   io/channel.c                        | 16 ++++++---
>   migration/channel-block.c           |  1 +
>   migration/channel.c                 | 45 ++++++++++++++++++++++++
>   migration/channel.h                 |  5 +++
>   migration/migration.c               | 54 ++++++++++++++++++++---------
>   migration/multifd.c                 | 19 +++++-----
>   migration/multifd.h                 |  2 +-
>   migration/postcopy-ram.c            |  5 +--
>   migration/postcopy-ram.h            |  2 +-
>   scsi/qemu-pr-helper.c               |  2 +-
>   tests/qtest/tpm-emu.c               |  2 +-
>   tests/unit/test-io-channel-socket.c |  1 +
>   util/vhost-user-server.c            |  2 +-
>   22 files changed, 148 insertions(+), 41 deletions(-)
>

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

[PATCH v6 0/2] check magic value for deciding the mapping of channels

[manish.mishra]

Current logic assumes that channel connections on the destination side are
always established in the same order as the source and the first one will
always be the main channel followed by the multifid or post-copy
preemption channel. This may not be always true, as even if a channel has a
connection established on the source side it can be in the pending state on
the destination side and a newer connection can be established first.
Basically causing out of order mapping of channels on the destination side.
Currently, all channels except post-copy preempt send a magic number, this
patch uses that magic number to decide the type of channel. This logic is
applicable only for precopy(multifd) live migration, as mentioned, the
post-copy preempt channel does not send any magic number. Also, tls live
migrations already does tls handshake before creating other channels, so
this issue is not possible with tls, hence this logic is avoided for tls
live migrations. This patch uses MSG_PEEK to check the magic number of
channels so that current data/control stream management remains
un-effected.

v2:
  TLS does not support MSG_PEEK, so V1 was broken for tls live
  migrations. For tls live migration, while initializing main channel
  tls handshake is done before we can create other channels, so this
  issue is not possible for tls live migrations. In V2 added a check
  to avoid checking magic number for tls live migration and fallback
  to older method to decide mapping of channels on destination side.

v3:
  1. Split change in two patches, io patch for read_peek routines,
     migration patch for migration related changes.
  2. Add flags to io_readv calls to get extra read flags like
     MSG_PEEK.
  3. Some other minor fixes.

v4:
  1. Removed common *all_eof routines for read peek and added one
     specific to live migration.
  2. Updated to use qemu_co_sleep_ns instead of qio_channel_yield.
  3. Some other minor fixes.

v5:
  1. Handle busy-wait in migration_channel_read_peek due partial reads.

v6:
  With earlier patch, multifd_load_setup was done only in
  migration_incoming_setup but if multifd channel is received before
  default channel, multifd channels will be uninitialized. Moved
  multifd_load_setup to migration_ioc_process_incoming.
  

manish.mishra (2):
  io: Add support for MSG_PEEK for socket channel
  migration: check magic value for deciding the mapping of channels

 chardev/char-socket.c               |  4 +--
 include/io/channel.h                |  6 ++++
 io/channel-buffer.c                 |  1 +
 io/channel-command.c                |  1 +
 io/channel-file.c                   |  1 +
 io/channel-null.c                   |  1 +
 io/channel-socket.c                 | 17 ++++++++-
 io/channel-tls.c                    |  1 +
 io/channel-websock.c                |  1 +
 io/channel.c                        | 16 ++++++---
 migration/channel-block.c           |  1 +
 migration/channel.c                 | 45 ++++++++++++++++++++++++
 migration/channel.h                 |  5 +++
 migration/migration.c               | 54 ++++++++++++++++++++---------
 migration/multifd.c                 | 19 +++++-----
 migration/multifd.h                 |  2 +-
 migration/postcopy-ram.c            |  5 +--
 migration/postcopy-ram.h            |  2 +-
 scsi/qemu-pr-helper.c               |  2 +-
 tests/qtest/tpm-emu.c               |  2 +-
 tests/unit/test-io-channel-socket.c |  1 +
 util/vhost-user-server.c            |  2 +-
 22 files changed, 148 insertions(+), 41 deletions(-)

-- 
2.22.3