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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Mar 07, 2018 at 12:00:07PM +0100, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  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, &qemu_uuid.data, sizeof(msg.uuid));
> +    ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), &local_err);
> +    if (ret != 0) {
> +        terminate_multifd_send_threads(local_err);
> +        return NULL;
> +    }
> +
>      while (true) {
>          qemu_sem_wait(&p->sem);
>          qemu_mutex_lock(&p->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 = &multifd_recv_state->params[i];
> -    i++;
> +    ret = qio_channel_read_all(ioc, (char *)&msg, sizeof(msg), &local_err);
> +    if (ret != 0) {
> +        terminate_multifd_recv_threads(local_err);
> +        return;
> +    }
> +
> +    if (memcmp(msg.uuid, &qemu_uuid, sizeof(qemu_uuid))) {
> +        char *uuid = qemu_uuid_unparse_strdup(&qemu_uuid);
> +        error_setg(&local_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 = &multifd_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(&local_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 :|