Philippe Mathieu-Daudé <phi...@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 <faro...@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é <phi...@linaro.org>
>
>> +
>> + trace_multifd_recv_unfill(p->id, p->packet_num, p->flags,
>> + p->next_packet_size);
>> +
>> + return ret;
>> +}
