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

manish.mishra posted 2 patches 2 months, 1 week ago
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Juan Quintela <quintela@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Fam Zheng <fam@euphon.net>, Stefan Berger <stefanb@linux.vnet.ibm.com>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Coiby Xu <Coiby.Xu@gmail.com>
[PATCH v4 2/2] migration: check magic value for deciding the mapping of channels
Posted by manish.mishra 2 months, 1 week ago
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. Berrangé <berrange@redhat.co
Suggested-by: Daniel P. Berrangé <berrange@redhat.com
Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
---
 migration/channel.c      | 46 ++++++++++++++++++++++++++++++++++++++++
 migration/channel.h      |  5 +++++
 migration/migration.c    | 45 ++++++++++++++++++++++++++++-----------
 migration/multifd.c      | 12 ++++-------
 migration/multifd.h      |  2 +-
 migration/postcopy-ram.c |  5 +----
 migration/postcopy-ram.h |  2 +-
 7 files changed, 91 insertions(+), 26 deletions(-)

diff --git a/migration/channel.c b/migration/channel.c
index 1b0815039f..a4600f52c5 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -92,3 +92,49 @@ void migration_channel_connect(MigrationState *s,
     migrate_fd_connect(s, error);
     error_free(error);
 }
+
+
+/**
+ * @migration_channel_read_peek - Read from the peek of 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 (len < buflen) {
+       len = qio_channel_readv_full(ioc, &iov, 1, NULL,
+                                    NULL, QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
+
+       if (len == QIO_CHANNEL_ERR_BLOCK) {
+            if (qemu_in_coroutine()) {
+                /* 1ms sleep. */
+                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
+            } else {
+                qio_channel_wait(ioc, G_IO_IN);
+            }
+            continue;
+       }
+       if (len == 0) {
+           error_setg(errp,
+                      "Unexpected end-of-file on channel");
+           return -1;
+       }
+       if (len < 0) {
+           return -1;
+       }
+   }
+
+   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 f485eea5fb..8509f20a80 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"
@@ -733,31 +734,51 @@ 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 (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 +786,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 509bbbe3bf..b54b6e7528 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1228,11 +1228,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;
@@ -1245,7 +1243,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);
 
@@ -1255,7 +1253,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));
@@ -1266,6 +1264,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 519f498643..913e4ba274 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 v4 2/2] migration: check magic value for deciding the mapping of channels
Posted by Daniel P. Berrangé 2 months, 1 week ago
On Wed, Nov 23, 2022 at 03:05:27PM +0000, 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 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. Berrangé <berrange@redhat.co
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com
> Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
> ---
>  migration/channel.c      | 46 ++++++++++++++++++++++++++++++++++++++++
>  migration/channel.h      |  5 +++++
>  migration/migration.c    | 45 ++++++++++++++++++++++++++++-----------
>  migration/multifd.c      | 12 ++++-------
>  migration/multifd.h      |  2 +-
>  migration/postcopy-ram.c |  5 +----
>  migration/postcopy-ram.h |  2 +-
>  7 files changed, 91 insertions(+), 26 deletions(-)
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index 1b0815039f..a4600f52c5 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -92,3 +92,49 @@ void migration_channel_connect(MigrationState *s,
>      migrate_fd_connect(s, error);
>      error_free(error);
>  }
> +
> +
> +/**
> + * @migration_channel_read_peek - Read from the peek of 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 (len < buflen) {
> +       len = qio_channel_readv_full(ioc, &iov, 1, NULL,
> +                                    NULL, QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
> +
> +       if (len == QIO_CHANNEL_ERR_BLOCK) {
> +            if (qemu_in_coroutine()) {
> +                /* 1ms sleep. */
> +                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
> +            } else {
> +                qio_channel_wait(ioc, G_IO_IN);
> +            }
> +            continue;
> +       }
> +       if (len == 0) {
> +           error_setg(errp,
> +                      "Unexpected end-of-file on channel");
> +           return -1;
> +       }
> +       if (len < 0) {
> +           return -1;
> +       }
> +   }

This busy waits when len > 0 and < buflen


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


Re: [PATCH v4 2/2] migration: check magic value for deciding the mapping of channels
Posted by manish.mishra 2 months, 1 week ago
On 23/11/22 9:28 pm, Daniel P. Berrangé wrote:
> On Wed, Nov 23, 2022 at 03:05:27PM +0000, 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 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. Berrangé <berrange@redhat.co
>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com
>> Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
>> ---
>>   migration/channel.c      | 46 ++++++++++++++++++++++++++++++++++++++++
>>   migration/channel.h      |  5 +++++
>>   migration/migration.c    | 45 ++++++++++++++++++++++++++++-----------
>>   migration/multifd.c      | 12 ++++-------
>>   migration/multifd.h      |  2 +-
>>   migration/postcopy-ram.c |  5 +----
>>   migration/postcopy-ram.h |  2 +-
>>   7 files changed, 91 insertions(+), 26 deletions(-)
>>
>> diff --git a/migration/channel.c b/migration/channel.c
>> index 1b0815039f..a4600f52c5 100644
>> --- a/migration/channel.c
>> +++ b/migration/channel.c
>> @@ -92,3 +92,49 @@ void migration_channel_connect(MigrationState *s,
>>       migrate_fd_connect(s, error);
>>       error_free(error);
>>   }
>> +
>> +
>> +/**
>> + * @migration_channel_read_peek - Read from the peek of 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 (len < buflen) {
>> +       len = qio_channel_readv_full(ioc, &iov, 1, NULL,
>> +                                    NULL, QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
>> +
>> +       if (len == QIO_CHANNEL_ERR_BLOCK) {
>> +            if (qemu_in_coroutine()) {
>> +                /* 1ms sleep. */
>> +                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
>> +            } else {
>> +                qio_channel_wait(ioc, G_IO_IN);
>> +            }
>> +            continue;
>> +       }
>> +       if (len == 0) {
>> +           error_setg(errp,
>> +                      "Unexpected end-of-file on channel");
>> +           return -1;
>> +       }
>> +       if (len < 0) {
>> +           return -1;
>> +       }
>> +   }
> This busy waits when len > 0 and < buflen
>
>
> With regards,
> Daniel

Sorry,   Daniel, may be i misunderstood something from earlier discussions. I thought we discussed we may not prevent it from looping multiple times but we can  qemu_co_sleep_ns after every retry to deal with busy wait?

Thanks

Manish Mishra



Re: [PATCH v4 2/2] migration: check magic value for deciding the mapping of channels
Posted by Daniel P. Berrangé 2 months, 1 week ago
On Wed, Nov 23, 2022 at 09:34:35PM +0530, manish.mishra wrote:
> 
> On 23/11/22 9:28 pm, Daniel P. Berrangé wrote:
> > On Wed, Nov 23, 2022 at 03:05:27PM +0000, 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 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. Berrangé <berrange@redhat.co
> > > Suggested-by: Daniel P. Berrangé <berrange@redhat.com
> > > Signed-off-by: manish.mishra <manish.mishra@nutanix.com>
> > > ---
> > >   migration/channel.c      | 46 ++++++++++++++++++++++++++++++++++++++++
> > >   migration/channel.h      |  5 +++++
> > >   migration/migration.c    | 45 ++++++++++++++++++++++++++++-----------
> > >   migration/multifd.c      | 12 ++++-------
> > >   migration/multifd.h      |  2 +-
> > >   migration/postcopy-ram.c |  5 +----
> > >   migration/postcopy-ram.h |  2 +-
> > >   7 files changed, 91 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/migration/channel.c b/migration/channel.c
> > > index 1b0815039f..a4600f52c5 100644
> > > --- a/migration/channel.c
> > > +++ b/migration/channel.c
> > > @@ -92,3 +92,49 @@ void migration_channel_connect(MigrationState *s,
> > >       migrate_fd_connect(s, error);
> > >       error_free(error);
> > >   }
> > > +
> > > +
> > > +/**
> > > + * @migration_channel_read_peek - Read from the peek of 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 (len < buflen) {
> > > +       len = qio_channel_readv_full(ioc, &iov, 1, NULL,
> > > +                                    NULL, QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
> > > +
> > > +       if (len == QIO_CHANNEL_ERR_BLOCK) {
> > > +            if (qemu_in_coroutine()) {
> > > +                /* 1ms sleep. */
> > > +                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
> > > +            } else {
> > > +                qio_channel_wait(ioc, G_IO_IN);
> > > +            }
> > > +            continue;
> > > +       }
> > > +       if (len == 0) {
> > > +           error_setg(errp,
> > > +                      "Unexpected end-of-file on channel");
> > > +           return -1;
> > > +       }
> > > +       if (len < 0) {
> > > +           return -1;
> > > +       }
> > > +   }
> > This busy waits when len > 0 and < buflen
> > 
> > 
> > With regards,
> > Daniel
> 
> Sorry,   Daniel, may be i misunderstood something from earlier
> discussions. I thought we discussed we may not prevent it from
> looping multiple times but we can  qemu_co_sleep_ns after every
> retry to deal with busy wait?

You're only calling  qemu_co_sleep_ns when you get ERR_BLOCK
though. That will happen the first time when 0 bytes are
pending. Once 2 bytes arrive and we're waiting for 2 more,
this code will busy wait instead of calling qemu_co_sleep_ns
again, because len==2 in that case.



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


Re: [PATCH v4 2/2] migration: check magic value for deciding the mapping of channels
Posted by Peter Xu 2 months, 1 week ago
On Wed, Nov 23, 2022 at 03:05:27PM +0000, manish.mishra wrote:
> +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 (len < buflen) {
> +       len = qio_channel_readv_full(ioc, &iov, 1, NULL,
> +                                    NULL, QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
> +
> +       if (len == QIO_CHANNEL_ERR_BLOCK) {

This needs to take care of partial len too?

> +            if (qemu_in_coroutine()) {
> +                /* 1ms sleep. */
> +                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
> +            } else {
> +                qio_channel_wait(ioc, G_IO_IN);
> +            }
> +            continue;
> +       }
> +       if (len == 0) {
> +           error_setg(errp,
> +                      "Unexpected end-of-file on channel");
> +           return -1;
> +       }
> +       if (len < 0) {
> +           return -1;
> +       }
> +   }
> +
> +   return 0;
> +}

-- 
Peter Xu
Re: [PATCH v4 2/2] migration: check magic value for deciding the mapping of channels
Posted by manish.mishra 2 months, 1 week ago
On 23/11/22 9:22 pm, Peter Xu wrote:
> On Wed, Nov 23, 2022 at 03:05:27PM +0000, manish.mishra wrote:
>> +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 (len < buflen) {
>> +       len = qio_channel_readv_full(ioc, &iov, 1, NULL,
>> +                                    NULL, QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
>> +
>> +       if (len == QIO_CHANNEL_ERR_BLOCK) {
> This needs to take care of partial len too?


sorry Peter, I did not quite understand it. Can you please give some more details.

>
>> +            if (qemu_in_coroutine()) {
>> +                /* 1ms sleep. */
>> +                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
>> +            } else {
>> +                qio_channel_wait(ioc, G_IO_IN);
>> +            }
>> +            continue;
>> +       }
>> +       if (len == 0) {
>> +           error_setg(errp,
>> +                      "Unexpected end-of-file on channel");
>> +           return -1;
>> +       }
>> +       if (len < 0) {
>> +           return -1;
>> +       }
>> +   }
>> +
>> +   return 0;
>> +}

Thanks

Manish Mishra
Re: [PATCH v4 2/2] migration: check magic value for deciding the mapping of channels
Posted by Peter Xu 2 months, 1 week ago
On Wed, Nov 23, 2022 at 09:28:14PM +0530, manish.mishra wrote:
> 
> On 23/11/22 9:22 pm, Peter Xu wrote:
> > On Wed, Nov 23, 2022 at 03:05:27PM +0000, manish.mishra wrote:
> > > +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 (len < buflen) {
> > > +       len = qio_channel_readv_full(ioc, &iov, 1, NULL,
> > > +                                    NULL, QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
> > > +
> > > +       if (len == QIO_CHANNEL_ERR_BLOCK) {
> > This needs to take care of partial len too?
> 
> 
> sorry Peter, I did not quite understand it. Can you please give some more details.

As Daniel pointed out, I think if we peek partially it'll go into a loop.
Since we shouldn't read 0 anyway here, maybe you can directly write it as:

while (true) {
  len = read();
  if (len <= 0 && len != QIO_CHANNEL_ERR_BLOCK) {
      error_setg(...);
      return -1;
  }
  if (len == buflen) {
      break;
  }
  /* For either partial peek or QIO_CHANNEL_ERR_BLOCK, retry with timeout */
  sleep();
}

> 
> > 
> > > +            if (qemu_in_coroutine()) {
> > > +                /* 1ms sleep. */
> > > +                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
> > > +            } else {
> > > +                qio_channel_wait(ioc, G_IO_IN);
> > > +            }
> > > +            continue;
> > > +       }
> > > +       if (len == 0) {
> > > +           error_setg(errp,
> > > +                      "Unexpected end-of-file on channel");
> > > +           return -1;
> > > +       }
> > > +       if (len < 0) {
> > > +           return -1;
> > > +       }
> > > +   }
> > > +
> > > +   return 0;
> > > +}
> 
> Thanks
> 
> Manish Mishra
> 

-- 
Peter Xu
Re: [PATCH v4 2/2] migration: check magic value for deciding the mapping of channels
Posted by manish.mishra 2 months, 1 week ago
On 23/11/22 9:57 pm, Peter Xu wrote:
> On Wed, Nov 23, 2022 at 09:28:14PM +0530, manish.mishra wrote:
>> On 23/11/22 9:22 pm, Peter Xu wrote:
>>> On Wed, Nov 23, 2022 at 03:05:27PM +0000, manish.mishra wrote:
>>>> +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 (len < buflen) {
>>>> +       len = qio_channel_readv_full(ioc, &iov, 1, NULL,
>>>> +                                    NULL, QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
>>>> +
>>>> +       if (len == QIO_CHANNEL_ERR_BLOCK) {
>>> This needs to take care of partial len too?
>>
>> sorry Peter, I did not quite understand it. Can you please give some more details.
> As Daniel pointed out, I think if we peek partially it'll go into a loop.
> Since we shouldn't read 0 anyway here, maybe you can directly write it as:
>
> while (true) {
>    len = read();
>    if (len <= 0 && len != QIO_CHANNEL_ERR_BLOCK) {
>        error_setg(...);
>        return -1;
>    }
>    if (len == buflen) {
>        break;
>    }
>    /* For either partial peek or QIO_CHANNEL_ERR_BLOCK, retry with timeout */
>    sleep();
> }


Yes, got it, sorry, will update it.

Thanks

Manish Mishra

>>>> +            if (qemu_in_coroutine()) {
>>>> +                /* 1ms sleep. */
>>>> +                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
>>>> +            } else {
>>>> +                qio_channel_wait(ioc, G_IO_IN);
>>>> +            }
>>>> +            continue;
>>>> +       }
>>>> +       if (len == 0) {
>>>> +           error_setg(errp,
>>>> +                      "Unexpected end-of-file on channel");
>>>> +           return -1;
>>>> +       }
>>>> +       if (len < 0) {
>>>> +           return -1;
>>>> +       }
>>>> +   }
>>>> +
>>>> +   return 0;
>>>> +}
>> Thanks
>>
>> Manish Mishra
>>