On 10/23/24 18:07, David Marchand wrote:
On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
<maxime.coque...@redhat.com> wrote:

This patch moves the VDUSE reconnection log mapping, as
well as creation if needed, into a dedicated function.

This is a preliminary rework to simplify VDUSE device
creation.

Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
---
  lib/vhost/vduse.c | 138 +++++++++++++++++++++++++---------------------
  1 file changed, 75 insertions(+), 63 deletions(-)

diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index c8c4761293..985d4cc58d 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -431,6 +431,9 @@ vduse_reconnect_path_init(void)
         const char *directory;
         int ret;

+       if (vduse_reconnect_path_set == true)
+               return 0;
+
         /* from RuntimeDirectory= see systemd.exec */
         directory = getenv("RUNTIME_DIRECTORY");
         if (directory == NULL) {
@@ -462,9 +465,74 @@ vduse_reconnect_path_init(void)
         VHOST_CONFIG_LOG("vduse", INFO, "Created VDUSE reconnect directory in 
%s",
                         vduse_reconnect_dir);

+       vduse_reconnect_path_set = true;
+
         return 0;
  }

+static int
+vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data 
**reco_log, bool create)

You can pass struct virtio_net *dev instead of patch 6.

At patch 4, the struct virtio_net *dev has not yet been allocated when
this function is called.

Or, maybe you mean I should squash patch 6 with this one?



+{
+       char reco_file[PATH_MAX];
+       int fd, ret;
+
+       if (vduse_reconnect_path_init() < 0) {
+               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to initialize reconnect 
path");
+                       return -1;

Weird indent.

Fixed.


+       }
+
+       ret = snprintf(reco_file, sizeof(reco_file), "%s/%s", 
vduse_reconnect_dir, dev_name);
+       if (ret < 0 || ret == sizeof(reco_file)) {
+               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to create vduse reconnect 
path name");
+               return -1;
+       }
+
+       if (create) {
+               fd = open(reco_file, O_CREAT | O_EXCL | O_RDWR, 0600);
+               if (fd < 0) {
+                       if (errno == EEXIST) {
+                               VHOST_CONFIG_LOG(dev_name, ERR, "Reconnect file %s 
exists but not the device",
+                                               reco_file);
+                       } else {
+                               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to open 
reconnect file %s (%s)",
+                                               reco_file, strerror(errno));
+                       }
+                       return -1;
+               }
+
+               ret = ftruncate(fd, sizeof(**reco_log));
+               if (ret < 0) {
+                       VHOST_CONFIG_LOG(dev_name, ERR, "Failed to truncate 
reconnect file %s (%s)",
+                                       reco_file, strerror(errno));
+                       goto out_close;
+               }
+       } else {
+               fd = open(reco_file, O_RDWR, 0600);
+               if (fd < 0) {
+                       if (errno == ENOENT)
+                               VHOST_CONFIG_LOG(dev_name, ERR, "Missing reconnect 
file (%s)", reco_file);
+                       else
+                               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to open 
reconnect file %s (%s)",
+                                               reco_file, strerror(errno));
+                       return -1;
+               }
+       }
+
+       *reco_log = mmap(NULL, sizeof(**reco_log), PROT_READ | PROT_WRITE, 
MAP_SHARED, fd, 0);
+       if (*reco_log == MAP_FAILED) {
+               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to mmap reconnect file %s 
(%s)",
+                               reco_file, strerror(errno));
+               ret = -1;
+               goto out_close;
+       }
+       ret = 0;
+
+out_close:
+       close(fd);
+
+       return ret;
+}
+
  static void
  vduse_reconnect_handler(int fd, void *arg, int *remove)
  {
@@ -519,7 +587,7 @@ vduse_reconnect_start_device(struct virtio_net *dev)
  int
  vduse_device_create(const char *path, bool compliant_ol_flags)
  {
-       int control_fd, dev_fd, vid, ret, reco_fd;
+       int control_fd, dev_fd, vid, ret;
         uint32_t i, max_queue_pairs, total_queues;
         struct virtio_net *dev;
         struct virtio_net_config vnet_config = {{ 0 }};
@@ -527,7 +595,6 @@ vduse_device_create(const char *path, bool 
compliant_ol_flags)
         uint64_t features;
         struct vduse_dev_config *dev_config = NULL;
         const char *name = path + strlen("/dev/vduse/");
-       char reconnect_file[PATH_MAX];
         struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
         bool reconnect = false;

@@ -539,20 +606,6 @@ vduse_device_create(const char *path, bool 
compliant_ol_flags)
                 }
         }

-       if (vduse_reconnect_path_set == false) {
-               if (vduse_reconnect_path_init() < 0) {
-                       VHOST_CONFIG_LOG(path, ERR, "failed to initialize reconnect 
path");
-                       return -1;
-               }
-               vduse_reconnect_path_set = true;
-       }
-
-       ret = snprintf(reconnect_file, sizeof(reconnect_file), "%s/%s", 
vduse_reconnect_dir, name);
-       if (ret < 0 || ret == sizeof(reconnect_file)) {
-               VHOST_CONFIG_LOG(name, ERR, "Failed to create vduse reconnect path 
name");
-               return -1;
-       }
-
         control_fd = open(VDUSE_CTRL_PATH, O_RDWR);
         if (control_fd < 0) {
                 VHOST_CONFIG_LOG(name, ERR, "Failed to open %s: %s",
@@ -592,26 +645,10 @@ vduse_device_create(const char *path, bool 
compliant_ol_flags)
                 VHOST_CONFIG_LOG(name, INFO, "Device already exists, 
reconnecting...");
                 reconnect = true;

-               reco_fd = open(reconnect_file, O_RDWR, 0600);
-               if (reco_fd < 0) {
-                       if (errno == ENOENT)
-                               VHOST_CONFIG_LOG(name, ERR, "Missing reconnect file 
(%s)",
-                                               reconnect_file);
-                       else
-                               VHOST_CONFIG_LOG(name, ERR, "Failed to open 
reconnect file %s (%s)",
-                                               reconnect_file, 
strerror(errno));
-                       ret = -1;
-                       goto out_dev_close;
-               }
-
-               reconnect_log = mmap(NULL, sizeof(*reconnect_log), PROT_READ | 
PROT_WRITE,
-                               MAP_SHARED, reco_fd, 0);
-               close(reco_fd);
-               if (reconnect_log == MAP_FAILED) {
-                       VHOST_CONFIG_LOG(name, ERR, "Failed to mmap reconnect file 
%s (%s)",
-                                       reconnect_file, strerror(errno));
-                       ret = -1;
-                       goto out_dev_close;
+               ret = vduse_reconnect_log_map(name, &reconnect_log, false);
+               if (ret < 0) {
+                       VHOST_CONFIG_LOG(name, ERR, "Failed to map reconnect log 
file");

No additional log, all error cases are covered, afaics.

Ack.



+                       goto out_ctrl_close;

and it should be out_dev_close here, will change in v2.

Thanks,
Maxime

                 }

                 if (reconnect_log->version != VHOST_RECONNECT_VERSION) {



Reply via email to