Re: [Qemu-devel] [PATCH v10 21/24] migration: Transmit initial package through the multifd channels

[Daniel P . Berrangé]

On Wed, Mar 07, 2018 at 12:00:07PM +0100, Juan Quintela wrote:
> Signed-off-by: Juan Quintela 
> ---
>  migration/ram.c | 50 +-
>  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 1aab96bd5e..4efac0c20c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -52,6 +52,8 @@
>  #include "qemu/rcu_queue.h"
>  #include "migration/colo.h"
>  #include "migration/block.h"
> +#include "sysemu/sysemu.h"
> +#include "qemu/uuid.h"
>  
>  /***/
>  /* ram save/restore */
> @@ -491,12 +493,30 @@ static void multifd_send_sync_main(void)
>  trace_multifd_send_sync_main();
>  }
>  
> +typedef struct {
> +uint32_t version;
> +unsigned char uuid[16]; /* QemuUUID */
> +uint8_t id;
> +} __attribute__((packed)) MultiFDInit_t;
> +
>  static void *multifd_send_thread(void *opaque)
>  {
>  MultiFDSendParams *p = opaque;
> +MultiFDInit_t msg;
> +Error *local_err = NULL;
> +size_t ret;
>  
>  trace_multifd_send_thread_start(p->id);
>  
> +msg.version = 1;

I'm thinking we should standardize byte-order for this, as you could be
migrating between qemu-system-x86_64 running TCG on PPC, to a another
qemu-system-x86_64 running TCG on AArch64, and so have mixed-endianness.

Just a 'msg.version = htonl(1) call to set network byte order ?

> +msg.id = p->id;
> +memcpy(msg.uuid, _uuid.data, sizeof(msg.uuid));
> +ret = qio_channel_write_all(p->c, (char *), sizeof(msg), _err);
> +if (ret != 0) {
> +terminate_multifd_send_threads(local_err);
> +return NULL;
> +}
> +
>  while (true) {
>  qemu_sem_wait(>sem);
>  qemu_mutex_lock(>mutex);
> @@ -730,12 +750,32 @@ bool multifd_recv_all_channels_created(void)
>  void multifd_recv_new_channel(QIOChannel *ioc)
>  {
>  MultiFDRecvParams *p;
> -/* we need to invent channels id's until we transmit */
> -/* we will remove this on a later patch */
> -static int i = 0;
> +MultiFDInit_t msg;
> +Error *local_err = NULL;
> +size_t ret;
>  
> -p = _recv_state->params[i];
> -i++;
> +ret = qio_channel_read_all(ioc, (char *), sizeof(msg), _err);
> +if (ret != 0) {
> +terminate_multifd_recv_threads(local_err);
> +return;
> +}
> +
> +if (memcmp(msg.uuid, _uuid, sizeof(qemu_uuid))) {
> +char *uuid = qemu_uuid_unparse_strdup(_uuid);
> +error_setg(_err, "multifd: received uuid '%s' and expected "
> +   "uuid '%s' for channel %hhd", msg.uuid, uuid, msg.id);
> +g_free(uuid);
> +terminate_multifd_recv_threads(local_err);
> +return;
> +}
> +
> +p = _recv_state->params[msg.id];

And  ntohl(msg.id) here

Also, since we're indexnig into an array using data off the network,
we should validate the index is in range to avoid out of bounds memory
access.

> +if (p->c != NULL) {
> +error_setg(_err, "multifd: received id '%d' already setup'",
> +   msg.id);
> +terminate_multifd_recv_threads(local_err);
> +return;
> +}
>  p->c = ioc;
>  socket_recv_channel_ref(ioc);


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

[Qemu-devel] [PATCH v10 21/24] migration: Transmit initial package through the multifd channels

[Juan Quintela]

Signed-off-by: Juan Quintela 
---
 migration/ram.c | 50 +-
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 1aab96bd5e..4efac0c20c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -52,6 +52,8 @@
 #include "qemu/rcu_queue.h"
 #include "migration/colo.h"
 #include "migration/block.h"
+#include "sysemu/sysemu.h"
+#include "qemu/uuid.h"
 
 /***/
 /* ram save/restore */
@@ -491,12 +493,30 @@ static void multifd_send_sync_main(void)
 trace_multifd_send_sync_main();
 }
 
+typedef struct {
+uint32_t version;
+unsigned char uuid[16]; /* QemuUUID */
+uint8_t id;
+} __attribute__((packed)) MultiFDInit_t;
+
 static void *multifd_send_thread(void *opaque)
 {
 MultiFDSendParams *p = opaque;
+MultiFDInit_t msg;
+Error *local_err = NULL;
+size_t ret;
 
 trace_multifd_send_thread_start(p->id);
 
+msg.version = 1;
+msg.id = p->id;
+memcpy(msg.uuid, _uuid.data, sizeof(msg.uuid));
+ret = qio_channel_write_all(p->c, (char *), sizeof(msg), _err);
+if (ret != 0) {
+terminate_multifd_send_threads(local_err);
+return NULL;
+}
+
 while (true) {
 qemu_sem_wait(>sem);
 qemu_mutex_lock(>mutex);
@@ -730,12 +750,32 @@ bool multifd_recv_all_channels_created(void)
 void multifd_recv_new_channel(QIOChannel *ioc)
 {
 MultiFDRecvParams *p;
-/* we need to invent channels id's until we transmit */
-/* we will remove this on a later patch */
-static int i = 0;
+MultiFDInit_t msg;
+Error *local_err = NULL;
+size_t ret;
 
-p = _recv_state->params[i];
-i++;
+ret = qio_channel_read_all(ioc, (char *), sizeof(msg), _err);
+if (ret != 0) {
+terminate_multifd_recv_threads(local_err);
+return;
+}
+
+if (memcmp(msg.uuid, _uuid, sizeof(qemu_uuid))) {
+char *uuid = qemu_uuid_unparse_strdup(_uuid);
+error_setg(_err, "multifd: received uuid '%s' and expected "
+   "uuid '%s' for channel %hhd", msg.uuid, uuid, msg.id);
+g_free(uuid);
+terminate_multifd_recv_threads(local_err);
+return;
+}
+
+p = _recv_state->params[msg.id];
+if (p->c != NULL) {
+error_setg(_err, "multifd: received id '%d' already setup'",
+   msg.id);
+terminate_multifd_recv_threads(local_err);
+return;
+}
 p->c = ioc;
 socket_recv_channel_ref(ioc);
 
-- 
2.14.3