[PATCH] [RFC] vhost-user: clean up set_mem_table functions

[PATCH] [RFC] vhost-user: clean up set_mem_table functions

[Raphael Norwitz]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 533 bytes --]

The functions sending vhost-user set memory table messages are getting
convoluted. The amount of nested logic is getting in the way of my
development and it looks like some identical logic should be refractored
out anyways. Here’s an RFC which cleans these functions up a bit.

Raphael

Raphael Norwitz (1):
  vhost-user: Refractor vhost_user_set_mem_table Functions

 hw/virtio/vhost-user.c | 140 +++++++++++++++++++++++--------------------------
 1 file changed, 65 insertions(+), 75 deletions(-)

-- 
1.8.3.1

[PATCH] vhost-user: Refractor vhost_user_set_mem_table Functions

[Raphael Norwitz]

vhost_user_set_mem_table() and vhost_user_set_mem_table_postcopy()
have gotten convoluted, and have some identical code.

This change moves the logic populating the VhostUserMemory struct
and fds array from vhost_user_set_mem_table() and
vhost_user_set_mem_table_postcopy() to a new function,
vhost_user_fill_set_mem_table_msg().

No functionality is impacted.

Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
---
 hw/virtio/vhost-user.c | 140 +++++++++++++++++++++++--------------------------
 1 file changed, 65 insertions(+), 75 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 02a9b25..183587e 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -405,31 +405,16 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
     return 0;
 }
 
-static int vhost_user_set_mem_table_postcopy(struct vhost_dev *dev,
-                                             struct vhost_memory *mem)
+static int vhost_user_fill_set_mem_table_msg(struct vhost_user *u,
+                                             struct vhost_dev *dev,
+                                             struct VhostUserMsg *msg,
+                                             int *fds,
+                                             size_t *fd_num,
+                                             bool postcopy)
 {
-    struct vhost_user *u = dev->opaque;
-    int fds[VHOST_MEMORY_MAX_NREGIONS];
     int i, fd;
-    size_t fd_num = 0;
-    VhostUserMsg msg_reply;
-    int region_i, msg_i;
 
-    VhostUserMsg msg = {
-        .hdr.request = VHOST_USER_SET_MEM_TABLE,
-        .hdr.flags = VHOST_USER_VERSION,
-    };
-
-    if (u->region_rb_len < dev->mem->nregions) {
-        u->region_rb = g_renew(RAMBlock*, u->region_rb, dev->mem->nregions);
-        u->region_rb_offset = g_renew(ram_addr_t, u->region_rb_offset,
-                                      dev->mem->nregions);
-        memset(&(u->region_rb[u->region_rb_len]), '\0',
-               sizeof(RAMBlock *) * (dev->mem->nregions - u->region_rb_len));
-        memset(&(u->region_rb_offset[u->region_rb_len]), '\0',
-               sizeof(ram_addr_t) * (dev->mem->nregions - u->region_rb_len));
-        u->region_rb_len = dev->mem->nregions;
-    }
+    msg->hdr.request = VHOST_USER_SET_MEM_TABLE;
 
     for (i = 0; i < dev->mem->nregions; ++i) {
         struct vhost_memory_region *reg = dev->mem->regions + i;
@@ -441,37 +426,75 @@ static int vhost_user_set_mem_table_postcopy(struct vhost_dev *dev,
                                      &offset);
         fd = memory_region_get_fd(mr);
         if (fd > 0) {
-            trace_vhost_user_set_mem_table_withfd(fd_num, mr->name,
-                                                  reg->memory_size,
-                                                  reg->guest_phys_addr,
-                                                  reg->userspace_addr, offset);
-            u->region_rb_offset[i] = offset;
-            u->region_rb[i] = mr->ram_block;
-            msg.payload.memory.regions[fd_num].userspace_addr =
+            if (postcopy) {
+                trace_vhost_user_set_mem_table_withfd(*fd_num, mr->name,
+                                                      reg->memory_size,
+                                                      reg->guest_phys_addr,
+                                                      reg->userspace_addr, offset);
+                u->region_rb_offset[i] = offset;
+                u->region_rb[i] = mr->ram_block;
+            } else if (*fd_num == VHOST_MEMORY_MAX_NREGIONS) {
+                error_report("Failed preparing vhost-user memory table msg");
+                return -1;
+            }
+            msg->payload.memory.regions[*fd_num].userspace_addr =
                 reg->userspace_addr;
-            msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
-            msg.payload.memory.regions[fd_num].guest_phys_addr =
+            msg->payload.memory.regions[*fd_num].memory_size  = reg->memory_size;
+            msg->payload.memory.regions[*fd_num].guest_phys_addr =
                 reg->guest_phys_addr;
-            msg.payload.memory.regions[fd_num].mmap_offset = offset;
-            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
-            fds[fd_num++] = fd;
-        } else {
+            msg->payload.memory.regions[*fd_num].mmap_offset = offset;
+            assert(*fd_num < VHOST_MEMORY_MAX_NREGIONS);
+            fds[(*fd_num)++] = fd;
+        } else if (postcopy) {
             u->region_rb_offset[i] = 0;
             u->region_rb[i] = NULL;
         }
     }
 
-    msg.payload.memory.nregions = fd_num;
+    msg->payload.memory.nregions = *fd_num;
 
-    if (!fd_num) {
+    if (!*fd_num && postcopy) {
         error_report("Failed initializing vhost-user memory map, "
                      "consider using -object memory-backend-file share=on");
         return -1;
     }
 
-    msg.hdr.size = sizeof(msg.payload.memory.nregions);
-    msg.hdr.size += sizeof(msg.payload.memory.padding);
-    msg.hdr.size += fd_num * sizeof(VhostUserMemoryRegion);
+    msg->hdr.size = sizeof(msg->payload.memory.nregions);
+    msg->hdr.size += sizeof(msg->payload.memory.padding);
+    msg->hdr.size += *fd_num * sizeof(VhostUserMemoryRegion);
+
+    return 1;
+}
+
+
+static int vhost_user_set_mem_table_postcopy(struct vhost_dev *dev,
+                                             struct vhost_memory *mem)
+{
+    struct vhost_user *u = dev->opaque;
+    int fds[VHOST_MEMORY_MAX_NREGIONS];
+    size_t fd_num = 0;
+    VhostUserMsg msg_reply;
+    int region_i, msg_i;
+
+    VhostUserMsg msg = {
+        .hdr.flags = VHOST_USER_VERSION,
+    };
+
+    if (u->region_rb_len < dev->mem->nregions) {
+        u->region_rb = g_renew(RAMBlock*, u->region_rb, dev->mem->nregions);
+        u->region_rb_offset = g_renew(ram_addr_t, u->region_rb_offset,
+                                      dev->mem->nregions);
+        memset(&(u->region_rb[u->region_rb_len]), '\0',
+               sizeof(RAMBlock *) * (dev->mem->nregions - u->region_rb_len));
+        memset(&(u->region_rb_offset[u->region_rb_len]), '\0',
+               sizeof(ram_addr_t) * (dev->mem->nregions - u->region_rb_len));
+        u->region_rb_len = dev->mem->nregions;
+    }
+ 
+    if (vhost_user_fill_set_mem_table_msg(u, dev, &msg, fds, &fd_num,
+                                          true) < 0) {
+        return -1;
+    }
 
     if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
         return -1;
@@ -543,7 +566,6 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
 {
     struct vhost_user *u = dev->opaque;
     int fds[VHOST_MEMORY_MAX_NREGIONS];
-    int i, fd;
     size_t fd_num = 0;
     bool do_postcopy = u->postcopy_listen && u->postcopy_fd.handler;
     bool reply_supported = virtio_has_feature(dev->protocol_features,
@@ -557,7 +579,6 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
     }
 
     VhostUserMsg msg = {
-        .hdr.request = VHOST_USER_SET_MEM_TABLE,
         .hdr.flags = VHOST_USER_VERSION,
     };
 
@@ -565,42 +586,11 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
         msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
     }
 
-    for (i = 0; i < dev->mem->nregions; ++i) {
-        struct vhost_memory_region *reg = dev->mem->regions + i;
-        ram_addr_t offset;
-        MemoryRegion *mr;
-
-        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
-        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
-                                     &offset);
-        fd = memory_region_get_fd(mr);
-        if (fd > 0) {
-            if (fd_num == VHOST_MEMORY_MAX_NREGIONS) {
-                error_report("Failed preparing vhost-user memory table msg");
-                return -1;
-            }
-            msg.payload.memory.regions[fd_num].userspace_addr =
-                reg->userspace_addr;
-            msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
-            msg.payload.memory.regions[fd_num].guest_phys_addr =
-                reg->guest_phys_addr;
-            msg.payload.memory.regions[fd_num].mmap_offset = offset;
-            fds[fd_num++] = fd;
-        }
-    }
-
-    msg.payload.memory.nregions = fd_num;
-
-    if (!fd_num) {
-        error_report("Failed initializing vhost-user memory map, "
-                     "consider using -object memory-backend-file share=on");
+    if (vhost_user_fill_set_mem_table_msg(u, dev, &msg, fds, &fd_num,
+                                          false) < 0) {
         return -1;
     }
 
-    msg.hdr.size = sizeof(msg.payload.memory.nregions);
-    msg.hdr.size += sizeof(msg.payload.memory.padding);
-    msg.hdr.size += fd_num * sizeof(VhostUserMemoryRegion);
-
     if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
         return -1;
     }
-- 
1.8.3.1

Re: [PATCH] vhost-user: Refractor vhost_user_set_mem_table Functions

[no-reply]

Patchew URL: https://patchew.org/QEMU/1572477125-25344-2-git-send-email-raphael.norwitz@nutanix.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH] vhost-user: Refractor vhost_user_set_mem_table Functions
Type: series
Message-id: 1572477125-25344-2-git-send-email-raphael.norwitz@nutanix.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
3cd9eee vhost-user: Refractor vhost_user_set_mem_table Functions

=== OUTPUT BEGIN ===
WARNING: line over 80 characters
#53: FILE: hw/virtio/vhost-user.c:433:
+                                                      reg->userspace_addr, offset);

WARNING: line over 80 characters
#62: FILE: hw/virtio/vhost-user.c:442:
+            msg->payload.memory.regions[*fd_num].memory_size  = reg->memory_size;

ERROR: trailing whitespace
#145: FILE: hw/virtio/vhost-user.c:493:
+ $

total: 1 errors, 2 warnings, 190 lines checked

Commit 3cd9eee6765f (vhost-user: Refractor vhost_user_set_mem_table Functions) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/1572477125-25344-2-git-send-email-raphael.norwitz@nutanix.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com