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

[Fabiano Rosas]

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

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

I don't even see the point of byte-swapping the packet
in-place. Probably better to have all the swaps into the variables in
the unfill function and leave the packet unchanged. I'll add another
patch to the series.

>
> 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;
>> +}