Re: [PATCH v4 10/16] migration/multifd: Isolate ram pages packet data

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 23/8/24 19:39, Fabiano Rosas wrote:
> While we cannot yet disentangle the multifd packet from page data, we
> can make the code a bit cleaner by setting the page-related fields in
> a separate function.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   migration/multifd.c    | 99 +++++++++++++++++++++++++-----------------
>   migration/trace-events |  5 ++-
>   2 files changed, 63 insertions(+), 41 deletions(-)


> -static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
> +static int multifd_ram_unfill_packet(MultiFDRecvParams *p, Error **errp)
>   {
>       MultiFDPacket_t *packet = p->packet;
>       uint32_t page_count = multifd_ram_page_count();
>       uint32_t page_size = multifd_ram_page_size();
>       int i;
>   
> -    packet->magic = be32_to_cpu(packet->magic);
> -    if (packet->magic != MULTIFD_MAGIC) {
> -        error_setg(errp, "multifd: received packet "
> -                   "magic %x and expected magic %x",
> -                   packet->magic, MULTIFD_MAGIC);
> -        return -1;
> -    }
> -
> -    packet->version = be32_to_cpu(packet->version);
> -    if (packet->version != MULTIFD_VERSION) {
> -        error_setg(errp, "multifd: received packet "
> -                   "version %u and expected version %u",
> -                   packet->version, MULTIFD_VERSION);
> -        return -1;
> -    }
> -
> -    p->flags = be32_to_cpu(packet->flags);
> -
>       packet->pages_alloc = be32_to_cpu(packet->pages_alloc);
>       /*
>        * If we received a packet that is 100 times bigger than expected
> @@ -511,13 +507,6 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>           return -1;
>       }
>   
> -    p->next_packet_size = be32_to_cpu(packet->next_packet_size);
> -    p->packet_num = be64_to_cpu(packet->packet_num);
> -    p->packets_recved++;
> -
> -    trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->zero_num,
> -                       p->flags, p->next_packet_size);
> -
>       if (p->normal_num == 0 && p->zero_num == 0) {
>           return 0;
>       }
> @@ -559,6 +548,40 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>       return 0;
>   }
>   
> +static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
> +{
> +    MultiFDPacket_t *packet = p->packet;
> +    int ret = 0;
> +
> +    packet->magic = be32_to_cpu(packet->magic);
> +    if (packet->magic != MULTIFD_MAGIC) {
> +        error_setg(errp, "multifd: received packet "
> +                   "magic %x and expected magic %x",
> +                   packet->magic, MULTIFD_MAGIC);
> +        return -1;
> +    }
> +
> +    packet->version = be32_to_cpu(packet->version);
> +    if (packet->version != MULTIFD_VERSION) {
> +        error_setg(errp, "multifd: received packet "
> +                   "version %u and expected version %u",
> +                   packet->version, MULTIFD_VERSION);
> +        return -1;
> +    }
> +
> +    p->flags = be32_to_cpu(packet->flags);
> +    p->next_packet_size = be32_to_cpu(packet->next_packet_size);
> +    p->packet_num = be64_to_cpu(packet->packet_num);
> +    p->packets_recved++;
> +
> +    ret = multifd_ram_unfill_packet(p, errp);

Pre-existing but since you modify this code, maybe cleaner to
"unfill" all packet fields, then check for sane magic/version
and return error, to not let a packet half-swapped. Otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +
> +    trace_multifd_recv_unfill(p->id, p->packet_num, p->flags,
> +                              p->next_packet_size);
> +
> +    return ret;
> +}