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

[Philippe Mathieu-Daudé]

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:

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