* [PATCH 1/6] vhost: fix vduse device creation error handling
2024-10-23 15:15 [PATCH 0/6] VDUSE reconnection fixes and cleanup Maxime Coquelin
@ 2024-10-23 15:15 ` Maxime Coquelin
2024-10-23 16:06 ` David Marchand
2024-10-23 15:15 ` [PATCH 2/6] vhost: fix possible TOCTOU in VDUSE dev creation Maxime Coquelin
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Maxime Coquelin @ 2024-10-23 15:15 UTC (permalink / raw)
To: dev, david.marchand, chenbox; +Cc: Maxime Coquelin
This patch fixes missing reconnection log unmapping
miss in the error path at VDUSE creation time.
Coverity issue: 445525
Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
lib/vhost/vduse.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index f9ac317438..89bc29a33f 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -488,7 +488,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
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 = NULL;
+ struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
bool reconnect = false;
if (vduse.fdset == NULL) {
@@ -530,13 +530,13 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
ret = rte_vhost_driver_get_features(path, &features);
if (ret < 0) {
VHOST_CONFIG_LOG(name, ERR, "Failed to get backend features");
- goto out_free;
+ goto out_ctrl_close;
}
ret = rte_vhost_driver_get_queue_num(path, &max_queue_pairs);
if (ret < 0) {
VHOST_CONFIG_LOG(name, ERR, "Failed to get max queue pairs");
- goto out_free;
+ goto out_ctrl_close;
}
VHOST_CONFIG_LOG(path, INFO, "VDUSE max queue pairs: %u", max_queue_pairs);
@@ -584,7 +584,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
"Features mismatch between backend (0x%" PRIx64 ") & reconnection file (0x%" PRIx64 ")",
features, reconnect_log->features);
ret = -1;
- goto out_ctrl_close;
+ goto out_log_unmap;
}
if (reconnect_log->nr_vrings != total_queues) {
@@ -592,7 +592,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
"Queues number mismatch between backend (%u) and reconnection file (%u)",
total_queues, reconnect_log->nr_vrings);
ret = -1;
- goto out_ctrl_close;
+ goto out_log_unmap;
}
} else {
reco_fd = open(reconnect_file, O_CREAT | O_EXCL | O_RDWR, 0600);
@@ -633,7 +633,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
if (!dev_config) {
VHOST_CONFIG_LOG(name, ERR, "Failed to allocate VDUSE config");
ret = -1;
- goto out_ctrl_close;
+ goto out_log_unmap;
}
vnet_config.max_virtqueue_pairs = max_queue_pairs;
@@ -649,16 +649,16 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
memcpy(dev_config->config, &vnet_config, sizeof(vnet_config));
ret = ioctl(control_fd, VDUSE_CREATE_DEV, dev_config);
+ free(dev_config);
+ dev_config = NULL;
if (ret < 0) {
VHOST_CONFIG_LOG(name, ERR, "Failed to create VDUSE device: %s",
strerror(errno));
- goto out_free;
+ goto out_log_unmap;
}
memcpy(&reconnect_log->config, &vnet_config, sizeof(vnet_config));
reconnect_log->nr_vrings = total_queues;
- free(dev_config);
- dev_config = NULL;
}
dev_fd = open(path, O_RDWR);
@@ -693,6 +693,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
dev->vduse_ctrl_fd = control_fd;
dev->vduse_dev_fd = dev_fd;
dev->reconnect_log = reconnect_log;
+ reconnect_log = MAP_FAILED;
if (reconnect)
dev->status = dev->reconnect_log->status;
@@ -768,8 +769,9 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
if (dev_fd >= 0)
close(dev_fd);
ioctl(control_fd, VDUSE_DESTROY_DEV, name);
-out_free:
- free(dev_config);
+out_log_unmap:
+ if (reconnect_log != MAP_FAILED)
+ munmap(reconnect_log, sizeof(*reconnect_log));
out_ctrl_close:
close(control_fd);
--
2.46.2
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/6] vhost: fix vduse device creation error handling
2024-10-23 15:15 ` [PATCH 1/6] vhost: fix vduse device creation error handling Maxime Coquelin
@ 2024-10-23 16:06 ` David Marchand
2024-10-23 18:55 ` Maxime Coquelin
0 siblings, 1 reply; 15+ messages in thread
From: David Marchand @ 2024-10-23 16:06 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: dev, chenbox
On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch fixes missing reconnection log unmapping
> miss in the error path at VDUSE creation time.
>
> Coverity issue: 445525
> Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
VDUSE* in the title, for consistency please.
Reviewed-by: David Marchand <david.marchand@redhat.com>
One comment below.
> ---
> lib/vhost/vduse.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
> index f9ac317438..89bc29a33f 100644
> --- a/lib/vhost/vduse.c
> +++ b/lib/vhost/vduse.c
> @@ -488,7 +488,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
> struct vduse_dev_config *dev_config = NULL;
dev_config could be allocated on the stack, rather than the heap.
But at least, move dev_config declaration in the block where it is
used, this will limit the chance a future change introduces a leak.
> const char *name = path + strlen("/dev/vduse/");
> char reconnect_file[PATH_MAX];
> - struct vhost_reconnect_data *reconnect_log = NULL;
> + struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
> bool reconnect = false;
>
> if (vduse.fdset == NULL) {
--
David Marchand
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/6] vhost: fix vduse device creation error handling
2024-10-23 16:06 ` David Marchand
@ 2024-10-23 18:55 ` Maxime Coquelin
0 siblings, 0 replies; 15+ messages in thread
From: Maxime Coquelin @ 2024-10-23 18:55 UTC (permalink / raw)
To: David Marchand; +Cc: dev, chenbox
On 10/23/24 18:06, David Marchand wrote:
> On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> This patch fixes missing reconnection log unmapping
>> miss in the error path at VDUSE creation time.
>>
>> Coverity issue: 445525
>> Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> VDUSE* in the title, for consistency please.
Fixed in v2.
> Reviewed-by: David Marchand <david.marchand@redhat.com>
>
> One comment below.
>
>> ---
>> lib/vhost/vduse.c | 24 +++++++++++++-----------
>> 1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
>> index f9ac317438..89bc29a33f 100644
>> --- a/lib/vhost/vduse.c
>> +++ b/lib/vhost/vduse.c
>> @@ -488,7 +488,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>> struct vduse_dev_config *dev_config = NULL;
>
> dev_config could be allocated on the stack, rather than the heap.
>
> But at least, move dev_config declaration in the block where it is
> used, this will limit the chance a future change introduces a leak.
>
I moved dev_config declaration in the block, having it declared in the
heap makes it less readable IMHO.
>
>> const char *name = path + strlen("/dev/vduse/");
>> char reconnect_file[PATH_MAX];
>> - struct vhost_reconnect_data *reconnect_log = NULL;
>> + struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
>> bool reconnect = false;
>>
>> if (vduse.fdset == NULL) {
>
>
Thanks,
Maxime
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/6] vhost: fix possible TOCTOU in VDUSE dev creation
2024-10-23 15:15 [PATCH 0/6] VDUSE reconnection fixes and cleanup Maxime Coquelin
2024-10-23 15:15 ` [PATCH 1/6] vhost: fix vduse device creation error handling Maxime Coquelin
@ 2024-10-23 15:15 ` Maxime Coquelin
2024-10-23 16:06 ` David Marchand
2024-10-23 15:15 ` [PATCH 3/6] vhost: fix VDUSE reconnect device start failure Maxime Coquelin
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Maxime Coquelin @ 2024-10-23 15:15 UTC (permalink / raw)
To: dev, david.marchand, chenbox; +Cc: Maxime Coquelin
This patch fixes a possible TOCTOU on the VDUSE
device chardev opening at device creation time.
Coverity issue: 445526
Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
lib/vhost/vduse.c | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index 89bc29a33f..d1373d0549 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -547,7 +547,8 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
else
total_queues += 1; /* Includes ctrl queue */
- if (access(path, F_OK) == 0) {
+ dev_fd = open(path, O_RDWR);
+ if (dev_fd >= 0) {
VHOST_CONFIG_LOG(name, INFO, "Device already exists, reconnecting...");
reconnect = true;
@@ -560,7 +561,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
VHOST_CONFIG_LOG(name, ERR, "Failed to open reconnect file %s (%s)",
reconnect_file, strerror(errno));
ret = -1;
- goto out_ctrl_close;
+ goto out_dev_close;
}
reconnect_log = mmap(NULL, sizeof(*reconnect_log), PROT_READ | PROT_WRITE,
@@ -570,7 +571,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
VHOST_CONFIG_LOG(name, ERR, "Failed to mmap reconnect file %s (%s)",
reconnect_file, strerror(errno));
ret = -1;
- goto out_ctrl_close;
+ goto out_dev_close;
}
if (reconnect_log->version != VHOST_RECONNECT_VERSION) {
@@ -594,7 +595,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
ret = -1;
goto out_log_unmap;
}
- } else {
+ } else if (errno == ENOENT) {
reco_fd = open(reconnect_file, O_CREAT | O_EXCL | O_RDWR, 0600);
if (reco_fd < 0) {
if (errno == EEXIST) {
@@ -659,34 +660,39 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
memcpy(&reconnect_log->config, &vnet_config, sizeof(vnet_config));
reconnect_log->nr_vrings = total_queues;
- }
- dev_fd = open(path, O_RDWR);
- if (dev_fd < 0) {
+ dev_fd = open(path, O_RDWR);
+ if (dev_fd < 0) {
+ VHOST_CONFIG_LOG(name, ERR, "Failed to open newly created device %s: %s",
+ path, strerror(errno));
+ ret = -1;
+ goto out_log_unmap;
+ }
+ } else {
VHOST_CONFIG_LOG(name, ERR, "Failed to open device %s: %s",
path, strerror(errno));
ret = -1;
- goto out_dev_close;
+ goto out_ctrl_close;
}
ret = fcntl(dev_fd, F_SETFL, O_NONBLOCK);
if (ret < 0) {
VHOST_CONFIG_LOG(name, ERR, "Failed to set chardev as non-blocking: %s",
strerror(errno));
- goto out_dev_close;
+ goto out_log_unmap;
}
vid = vhost_new_device(&vduse_backend_ops);
if (vid < 0) {
VHOST_CONFIG_LOG(name, ERR, "Failed to create new Vhost device");
ret = -1;
- goto out_dev_close;
+ goto out_log_unmap;
}
dev = get_device(vid);
if (!dev) {
ret = -1;
- goto out_dev_close;
+ goto out_dev_destroy;
}
strncpy(dev->ifname, path, IF_NAME_SZ - 1);
@@ -765,13 +771,13 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
out_dev_destroy:
vhost_destroy_device(vid);
+out_log_unmap:
+ if (reconnect_log != MAP_FAILED)
+ munmap(reconnect_log, sizeof(*reconnect_log));
out_dev_close:
if (dev_fd >= 0)
close(dev_fd);
ioctl(control_fd, VDUSE_DESTROY_DEV, name);
-out_log_unmap:
- if (reconnect_log != MAP_FAILED)
- munmap(reconnect_log, sizeof(*reconnect_log));
out_ctrl_close:
close(control_fd);
--
2.46.2
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 3/6] vhost: fix VDUSE reconnect device start failure
2024-10-23 15:15 [PATCH 0/6] VDUSE reconnection fixes and cleanup Maxime Coquelin
2024-10-23 15:15 ` [PATCH 1/6] vhost: fix vduse device creation error handling Maxime Coquelin
2024-10-23 15:15 ` [PATCH 2/6] vhost: fix possible TOCTOU in VDUSE dev creation Maxime Coquelin
@ 2024-10-23 15:15 ` Maxime Coquelin
2024-10-23 16:06 ` David Marchand
2024-10-23 15:15 ` [PATCH 4/6] vhost: refactor VDUSE reconnection log mapping Maxime Coquelin
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Maxime Coquelin @ 2024-10-23 15:15 UTC (permalink / raw)
To: dev, david.marchand, chenbox; +Cc: Maxime Coquelin
This patch fixes a FD leak in the VDUSE device reconnect
code fails to start the device.
Also take the opportunity to refactor the related code into
a dedicated function.
Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
lib/vhost/vduse.c | 63 +++++++++++++++++++++++++++++++----------------
1 file changed, 42 insertions(+), 21 deletions(-)
diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index d1373d0549..c8c4761293 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -476,6 +476,46 @@ vduse_reconnect_handler(int fd, void *arg, int *remove)
*remove = 1;
}
+static int
+vduse_reconnect_start_device(struct virtio_net *dev)
+{
+ int fd, ret;
+
+ /*
+ * Make vduse_device_start() being executed in the same
+ * context for both reconnection and fresh startup.
+ */
+ fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+ if (fd < 0) {
+ VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to create reconnect efd: %s",
+ strerror(errno));
+ ret = -1;
+ goto out_err;
+ }
+
+ ret = fdset_add(vduse.fdset, fd, vduse_reconnect_handler, NULL, dev);
+ if (ret) {
+ VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to add reconnect efd %d to vduse fdset",
+ fd);
+ goto out_err_close;
+ }
+
+ ret = eventfd_write(fd, (eventfd_t)1);
+ if (ret < 0) {
+ VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to write to reconnect eventfd");
+ goto out_err_fdset;
+ }
+
+ return 0;
+
+out_err_fdset:
+ fdset_del(vduse.fdset, fd);
+out_err_close:
+ close(fd);
+out_err:
+ return ret;
+}
+
int
vduse_device_create(const char *path, bool compliant_ol_flags)
{
@@ -741,28 +781,9 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
}
if (reconnect && dev->status & VIRTIO_DEVICE_STATUS_DRIVER_OK) {
- /*
- * Make vduse_device_start() being executed in the same
- * context for both reconnection and fresh startup.
- */
- reco_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
- if (reco_fd < 0) {
- VHOST_CONFIG_LOG(name, ERR, "Failed to create reco_fd: %s",
- strerror(errno));
- ret = -1;
- goto out_dev_destroy;
- }
-
- ret = fdset_add(vduse.fdset, reco_fd, vduse_reconnect_handler, NULL, dev);
+ ret = vduse_reconnect_start_device(dev);
if (ret) {
- VHOST_CONFIG_LOG(name, ERR, "Failed to add reconnect fd %d to vduse fdset",
- reco_fd);
- goto out_dev_destroy;
- }
-
- ret = eventfd_write(reco_fd, (eventfd_t)1);
- if (ret < 0) {
- VHOST_CONFIG_LOG(name, ERR, "Failed to write to reconnect eventfd");
+ VHOST_CONFIG_LOG(name, ERR, "Failed to start device at reconnection");
goto out_dev_destroy;
}
}
--
2.46.2
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/6] vhost: fix VDUSE reconnect device start failure
2024-10-23 15:15 ` [PATCH 3/6] vhost: fix VDUSE reconnect device start failure Maxime Coquelin
@ 2024-10-23 16:06 ` David Marchand
2024-10-23 18:57 ` Maxime Coquelin
0 siblings, 1 reply; 15+ messages in thread
From: David Marchand @ 2024-10-23 16:06 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: dev, chenbox
On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch fixes a FD leak in the VDUSE device reconnect
> code fails to start the device.
>
> Also take the opportunity to refactor the related code into
> a dedicated function.
>
> Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/vhost/vduse.c | 63 +++++++++++++++++++++++++++++++----------------
> 1 file changed, 42 insertions(+), 21 deletions(-)
>
> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
> index d1373d0549..c8c4761293 100644
> --- a/lib/vhost/vduse.c
> +++ b/lib/vhost/vduse.c
> @@ -476,6 +476,46 @@ vduse_reconnect_handler(int fd, void *arg, int *remove)
> *remove = 1;
> }
>
> +static int
> +vduse_reconnect_start_device(struct virtio_net *dev)
> +{
> + int fd, ret;
> +
> + /*
> + * Make vduse_device_start() being executed in the same
> + * context for both reconnection and fresh startup.
> + */
> + fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> + if (fd < 0) {
> + VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to create reconnect efd: %s",
> + strerror(errno));
> + ret = -1;
> + goto out_err;
> + }
> +
> + ret = fdset_add(vduse.fdset, fd, vduse_reconnect_handler, NULL, dev);
> + if (ret) {
> + VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to add reconnect efd %d to vduse fdset",
> + fd);
> + goto out_err_close;
> + }
> +
> + ret = eventfd_write(fd, (eventfd_t)1);
> + if (ret < 0) {
> + VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to write to reconnect eventfd");
> + goto out_err_fdset;
> + }
> +
> + return 0;
> +
> +out_err_fdset:
> + fdset_del(vduse.fdset, fd);
> +out_err_close:
> + close(fd);
> +out_err:
> + return ret;
> +}
> +
> int
> vduse_device_create(const char *path, bool compliant_ol_flags)
> {
> @@ -741,28 +781,9 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
> }
>
> if (reconnect && dev->status & VIRTIO_DEVICE_STATUS_DRIVER_OK) {
> - /*
> - * Make vduse_device_start() being executed in the same
> - * context for both reconnection and fresh startup.
> - */
> - reco_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> - if (reco_fd < 0) {
> - VHOST_CONFIG_LOG(name, ERR, "Failed to create reco_fd: %s",
> - strerror(errno));
> - ret = -1;
> - goto out_dev_destroy;
> - }
> -
> - ret = fdset_add(vduse.fdset, reco_fd, vduse_reconnect_handler, NULL, dev);
> + ret = vduse_reconnect_start_device(dev);
> if (ret) {
> - VHOST_CONFIG_LOG(name, ERR, "Failed to add reconnect fd %d to vduse fdset",
> - reco_fd);
> - goto out_dev_destroy;
> - }
> -
> - ret = eventfd_write(reco_fd, (eventfd_t)1);
> - if (ret < 0) {
> - VHOST_CONFIG_LOG(name, ERR, "Failed to write to reconnect eventfd");
> + VHOST_CONFIG_LOG(name, ERR, "Failed to start device at reconnection");
We don't need a new log, there is already one for each error case in
vduse_reconnect_start_device().
--
David Marchand
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/6] vhost: fix VDUSE reconnect device start failure
2024-10-23 16:06 ` David Marchand
@ 2024-10-23 18:57 ` Maxime Coquelin
0 siblings, 0 replies; 15+ messages in thread
From: Maxime Coquelin @ 2024-10-23 18:57 UTC (permalink / raw)
To: David Marchand; +Cc: dev, chenbox
On 10/23/24 18:06, David Marchand wrote:
> On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> This patch fixes a FD leak in the VDUSE device reconnect
>> code fails to start the device.
>>
>> Also take the opportunity to refactor the related code into
>> a dedicated function.
>>
>> Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>> lib/vhost/vduse.c | 63 +++++++++++++++++++++++++++++++----------------
>> 1 file changed, 42 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
>> index d1373d0549..c8c4761293 100644
>> --- a/lib/vhost/vduse.c
>> +++ b/lib/vhost/vduse.c
>> @@ -476,6 +476,46 @@ vduse_reconnect_handler(int fd, void *arg, int *remove)
>> *remove = 1;
>> }
>>
>> +static int
>> +vduse_reconnect_start_device(struct virtio_net *dev)
>> +{
>> + int fd, ret;
>> +
>> + /*
>> + * Make vduse_device_start() being executed in the same
>> + * context for both reconnection and fresh startup.
>> + */
>> + fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
>> + if (fd < 0) {
>> + VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to create reconnect efd: %s",
>> + strerror(errno));
>> + ret = -1;
>> + goto out_err;
>> + }
>> +
>> + ret = fdset_add(vduse.fdset, fd, vduse_reconnect_handler, NULL, dev);
>> + if (ret) {
>> + VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to add reconnect efd %d to vduse fdset",
>> + fd);
>> + goto out_err_close;
>> + }
>> +
>> + ret = eventfd_write(fd, (eventfd_t)1);
>> + if (ret < 0) {
>> + VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to write to reconnect eventfd");
>> + goto out_err_fdset;
>> + }
>> +
>> + return 0;
>> +
>> +out_err_fdset:
>> + fdset_del(vduse.fdset, fd);
>> +out_err_close:
>> + close(fd);
>> +out_err:
>> + return ret;
>> +}
>> +
>> int
>> vduse_device_create(const char *path, bool compliant_ol_flags)
>> {
>> @@ -741,28 +781,9 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>> }
>>
>> if (reconnect && dev->status & VIRTIO_DEVICE_STATUS_DRIVER_OK) {
>> - /*
>> - * Make vduse_device_start() being executed in the same
>> - * context for both reconnection and fresh startup.
>> - */
>> - reco_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
>> - if (reco_fd < 0) {
>> - VHOST_CONFIG_LOG(name, ERR, "Failed to create reco_fd: %s",
>> - strerror(errno));
>> - ret = -1;
>> - goto out_dev_destroy;
>> - }
>> -
>> - ret = fdset_add(vduse.fdset, reco_fd, vduse_reconnect_handler, NULL, dev);
>> + ret = vduse_reconnect_start_device(dev);
>> if (ret) {
>> - VHOST_CONFIG_LOG(name, ERR, "Failed to add reconnect fd %d to vduse fdset",
>> - reco_fd);
>> - goto out_dev_destroy;
>> - }
>> -
>> - ret = eventfd_write(reco_fd, (eventfd_t)1);
>> - if (ret < 0) {
>> - VHOST_CONFIG_LOG(name, ERR, "Failed to write to reconnect eventfd");
>> + VHOST_CONFIG_LOG(name, ERR, "Failed to start device at reconnection");
>
> We don't need a new log, there is already one for each error case in
> vduse_reconnect_start_device().
>
>
Removed in v2.
Thanks,
Maxime
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/6] vhost: refactor VDUSE reconnection log mapping
2024-10-23 15:15 [PATCH 0/6] VDUSE reconnection fixes and cleanup Maxime Coquelin
` (2 preceding siblings ...)
2024-10-23 15:15 ` [PATCH 3/6] vhost: fix VDUSE reconnect device start failure Maxime Coquelin
@ 2024-10-23 15:15 ` Maxime Coquelin
2024-10-23 16:07 ` David Marchand
2024-10-23 15:15 ` [PATCH 5/6] vhost: fix and refactor VDUSE reconnect log check Maxime Coquelin
2024-10-23 15:15 ` [PATCH 6/6] vhost: move VDUSE reconnection after device is created Maxime Coquelin
5 siblings, 1 reply; 15+ messages in thread
From: Maxime Coquelin @ 2024-10-23 15:15 UTC (permalink / raw)
To: dev, david.marchand, chenbox; +Cc: Maxime Coquelin
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.coquelin@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)
+{
+ 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;
+ }
+
+ 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");
+ goto out_ctrl_close;
}
if (reconnect_log->version != VHOST_RECONNECT_VERSION) {
@@ -636,34 +673,9 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
goto out_log_unmap;
}
} else if (errno == ENOENT) {
- reco_fd = open(reconnect_file, O_CREAT | O_EXCL | O_RDWR, 0600);
- if (reco_fd < 0) {
- if (errno == EEXIST) {
- VHOST_CONFIG_LOG(name, ERR, "Reconnect file %s exists but not the device",
- reconnect_file);
- } else {
- VHOST_CONFIG_LOG(name, ERR, "Failed to open reconnect file %s (%s)",
- reconnect_file, strerror(errno));
- }
- ret = -1;
- goto out_ctrl_close;
- }
-
- ret = ftruncate(reco_fd, sizeof(*reconnect_log));
+ ret = vduse_reconnect_log_map(name, &reconnect_log, true);
if (ret < 0) {
- VHOST_CONFIG_LOG(name, ERR, "Failed to truncate reconnect file %s (%s)",
- reconnect_file, strerror(errno));
- close(reco_fd);
- goto out_ctrl_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;
+ VHOST_CONFIG_LOG(name, ERR, "Failed to create reconnect log file");
goto out_ctrl_close;
}
--
2.46.2
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 4/6] vhost: refactor VDUSE reconnection log mapping
2024-10-23 15:15 ` [PATCH 4/6] vhost: refactor VDUSE reconnection log mapping Maxime Coquelin
@ 2024-10-23 16:07 ` David Marchand
2024-10-23 19:04 ` Maxime Coquelin
0 siblings, 1 reply; 15+ messages in thread
From: David Marchand @ 2024-10-23 16:07 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: dev, chenbox
On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
<maxime.coquelin@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.coquelin@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.
> +{
> + 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.
> + }
> +
> + 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.
> + goto out_ctrl_close;
> }
>
> if (reconnect_log->version != VHOST_RECONNECT_VERSION) {
--
David Marchand
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 4/6] vhost: refactor VDUSE reconnection log mapping
2024-10-23 16:07 ` David Marchand
@ 2024-10-23 19:04 ` Maxime Coquelin
0 siblings, 0 replies; 15+ messages in thread
From: Maxime Coquelin @ 2024-10-23 19:04 UTC (permalink / raw)
To: David Marchand; +Cc: dev, chenbox
On 10/23/24 18:07, David Marchand wrote:
> On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
> <maxime.coquelin@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.coquelin@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) {
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/6] vhost: fix and refactor VDUSE reconnect log check
2024-10-23 15:15 [PATCH 0/6] VDUSE reconnection fixes and cleanup Maxime Coquelin
` (3 preceding siblings ...)
2024-10-23 15:15 ` [PATCH 4/6] vhost: refactor VDUSE reconnection log mapping Maxime Coquelin
@ 2024-10-23 15:15 ` Maxime Coquelin
2024-10-23 16:08 ` David Marchand
2024-10-23 15:15 ` [PATCH 6/6] vhost: move VDUSE reconnection after device is created Maxime Coquelin
5 siblings, 1 reply; 15+ messages in thread
From: Maxime Coquelin @ 2024-10-23 15:15 UTC (permalink / raw)
To: dev, david.marchand, chenbox; +Cc: Maxime Coquelin
This patch fixes missing error handling in checking the
reconnect log version, and takes the opportunity to move
all the checks into a dedicated function to simply VDUSE
device creation code.
Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
lib/vhost/vduse.c | 51 +++++++++++++++++++++++++++++------------------
1 file changed, 32 insertions(+), 19 deletions(-)
diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index 985d4cc58d..0ce17e9690 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -533,6 +533,35 @@ vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco
return ret;
}
+static int
+vduse_reconnect_log_check(struct vhost_reconnect_data *reco_log,
+ const char *dev_name,
+ uint64_t features, uint32_t total_queues)
+{
+ if (reco_log->version != VHOST_RECONNECT_VERSION) {
+ VHOST_CONFIG_LOG(dev_name, ERR,
+ "Version mismatch between backend (0x%x) & reconnection file (0x%x)",
+ VHOST_RECONNECT_VERSION, reco_log->version);
+ return -1;
+ }
+
+ if ((reco_log->features & features) != reco_log->features) {
+ VHOST_CONFIG_LOG(dev_name, ERR,
+ "Features mismatch between backend (0x%" PRIx64 ") & reconnection file (0x%" PRIx64 ")",
+ features, reco_log->features);
+ return -1;
+ }
+
+ if (reco_log->nr_vrings != total_queues) {
+ VHOST_CONFIG_LOG(dev_name, ERR,
+ "Queues number mismatch between backend (%u) and reconnection file (%u)",
+ total_queues, reco_log->nr_vrings);
+ return -1;
+ }
+
+ return 0;
+}
+
static void
vduse_reconnect_handler(int fd, void *arg, int *remove)
{
@@ -651,25 +680,9 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
goto out_ctrl_close;
}
- if (reconnect_log->version != VHOST_RECONNECT_VERSION) {
- VHOST_CONFIG_LOG(name, ERR,
- "Version mismatch between backend (0x%x) & reconnection file (0x%x)",
- VHOST_RECONNECT_VERSION, reconnect_log->version);
- }
-
- if ((reconnect_log->features & features) != reconnect_log->features) {
- VHOST_CONFIG_LOG(name, ERR,
- "Features mismatch between backend (0x%" PRIx64 ") & reconnection file (0x%" PRIx64 ")",
- features, reconnect_log->features);
- ret = -1;
- goto out_log_unmap;
- }
-
- if (reconnect_log->nr_vrings != total_queues) {
- VHOST_CONFIG_LOG(name, ERR,
- "Queues number mismatch between backend (%u) and reconnection file (%u)",
- total_queues, reconnect_log->nr_vrings);
- ret = -1;
+ ret = vduse_reconnect_log_check(reconnect_log, name, features, total_queues);
+ if (ret < 0) {
+ VHOST_CONFIG_LOG(name, ERR, "Reconnection log file check failed");
goto out_log_unmap;
}
} else if (errno == ENOENT) {
--
2.46.2
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 5/6] vhost: fix and refactor VDUSE reconnect log check
2024-10-23 15:15 ` [PATCH 5/6] vhost: fix and refactor VDUSE reconnect log check Maxime Coquelin
@ 2024-10-23 16:08 ` David Marchand
0 siblings, 0 replies; 15+ messages in thread
From: David Marchand @ 2024-10-23 16:08 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: dev, chenbox
On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch fixes missing error handling in checking the
> reconnect log version, and takes the opportunity to move
> all the checks into a dedicated function to simply VDUSE
> device creation code.
>
> Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Same comment, no additional log.
--
David Marchand
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 6/6] vhost: move VDUSE reconnection after device is created
2024-10-23 15:15 [PATCH 0/6] VDUSE reconnection fixes and cleanup Maxime Coquelin
` (4 preceding siblings ...)
2024-10-23 15:15 ` [PATCH 5/6] vhost: fix and refactor VDUSE reconnect log check Maxime Coquelin
@ 2024-10-23 15:15 ` Maxime Coquelin
5 siblings, 0 replies; 15+ messages in thread
From: Maxime Coquelin @ 2024-10-23 15:15 UTC (permalink / raw)
To: dev, david.marchand, chenbox; +Cc: Maxime Coquelin
This patch moves the VDUSE reconnection log mapping and
setup after the Vhost device is created in order to
simplify the device creation and its error path.
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
lib/vhost/vduse.c | 120 +++++++++++++++++++++-------------------------
1 file changed, 55 insertions(+), 65 deletions(-)
diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index 0ce17e9690..68d0e2c5d8 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -471,19 +471,20 @@ vduse_reconnect_path_init(void)
}
static int
-vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco_log, bool create)
+vduse_reconnect_log_map(struct virtio_net *dev, bool create)
{
char reco_file[PATH_MAX];
int fd, ret;
+ const char *name = dev->ifname + strlen("/dev/vduse/");
if (vduse_reconnect_path_init() < 0) {
- VHOST_CONFIG_LOG(dev_name, ERR, "Failed to initialize reconnect path");
+ VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to initialize reconnect path");
return -1;
}
- ret = snprintf(reco_file, sizeof(reco_file), "%s/%s", vduse_reconnect_dir, dev_name);
+ ret = snprintf(reco_file, sizeof(reco_file), "%s/%s", vduse_reconnect_dir, name);
if (ret < 0 || ret == sizeof(reco_file)) {
- VHOST_CONFIG_LOG(dev_name, ERR, "Failed to create vduse reconnect path name");
+ VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to create vduse reconnect path name");
return -1;
}
@@ -491,18 +492,18 @@ vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco
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",
+ VHOST_CONFIG_LOG(dev->ifname, 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)",
+ VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to open reconnect file %s (%s)",
reco_file, strerror(errno));
}
return -1;
}
- ret = ftruncate(fd, sizeof(**reco_log));
+ ret = ftruncate(fd, sizeof(*dev->reconnect_log));
if (ret < 0) {
- VHOST_CONFIG_LOG(dev_name, ERR, "Failed to truncate reconnect file %s (%s)",
+ VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to truncate reconnect file %s (%s)",
reco_file, strerror(errno));
goto out_close;
}
@@ -510,17 +511,18 @@ vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco
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);
+ VHOST_CONFIG_LOG(dev->ifname, ERR, "Missing reconnect file (%s)", reco_file);
else
- VHOST_CONFIG_LOG(dev_name, ERR, "Failed to open reconnect file %s (%s)",
+ VHOST_CONFIG_LOG(dev->ifname, 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)",
+ dev->reconnect_log = mmap(NULL, sizeof(*dev->reconnect_log), PROT_READ | PROT_WRITE,
+ MAP_SHARED, fd, 0);
+ if (dev->reconnect_log == MAP_FAILED) {
+ VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to mmap reconnect file %s (%s)",
reco_file, strerror(errno));
ret = -1;
goto out_close;
@@ -534,28 +536,26 @@ vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco
}
static int
-vduse_reconnect_log_check(struct vhost_reconnect_data *reco_log,
- const char *dev_name,
- uint64_t features, uint32_t total_queues)
+vduse_reconnect_log_check(struct virtio_net *dev, uint64_t features, uint32_t total_queues)
{
- if (reco_log->version != VHOST_RECONNECT_VERSION) {
- VHOST_CONFIG_LOG(dev_name, ERR,
+ if (dev->reconnect_log->version != VHOST_RECONNECT_VERSION) {
+ VHOST_CONFIG_LOG(dev->ifname, ERR,
"Version mismatch between backend (0x%x) & reconnection file (0x%x)",
- VHOST_RECONNECT_VERSION, reco_log->version);
+ VHOST_RECONNECT_VERSION, dev->reconnect_log->version);
return -1;
}
- if ((reco_log->features & features) != reco_log->features) {
- VHOST_CONFIG_LOG(dev_name, ERR,
+ if ((dev->reconnect_log->features & features) != dev->reconnect_log->features) {
+ VHOST_CONFIG_LOG(dev->ifname, ERR,
"Features mismatch between backend (0x%" PRIx64 ") & reconnection file (0x%" PRIx64 ")",
- features, reco_log->features);
+ features, dev->reconnect_log->features);
return -1;
}
- if (reco_log->nr_vrings != total_queues) {
- VHOST_CONFIG_LOG(dev_name, ERR,
+ if (dev->reconnect_log->nr_vrings != total_queues) {
+ VHOST_CONFIG_LOG(dev->ifname, ERR,
"Queues number mismatch between backend (%u) and reconnection file (%u)",
- total_queues, reco_log->nr_vrings);
+ total_queues, dev->reconnect_log->nr_vrings);
return -1;
}
@@ -624,7 +624,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/");
- struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
bool reconnect = false;
if (vduse.fdset == NULL) {
@@ -673,33 +672,13 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
if (dev_fd >= 0) {
VHOST_CONFIG_LOG(name, INFO, "Device already exists, reconnecting...");
reconnect = true;
-
- ret = vduse_reconnect_log_map(name, &reconnect_log, false);
- if (ret < 0) {
- VHOST_CONFIG_LOG(name, ERR, "Failed to map reconnect log file");
- goto out_ctrl_close;
- }
-
- ret = vduse_reconnect_log_check(reconnect_log, name, features, total_queues);
- if (ret < 0) {
- VHOST_CONFIG_LOG(name, ERR, "Reconnection log file check failed");
- goto out_log_unmap;
- }
} else if (errno == ENOENT) {
- ret = vduse_reconnect_log_map(name, &reconnect_log, true);
- if (ret < 0) {
- VHOST_CONFIG_LOG(name, ERR, "Failed to create reconnect log file");
- goto out_ctrl_close;
- }
-
- reconnect_log->version = VHOST_RECONNECT_VERSION;
-
dev_config = malloc(offsetof(struct vduse_dev_config, config) +
sizeof(vnet_config));
if (!dev_config) {
VHOST_CONFIG_LOG(name, ERR, "Failed to allocate VDUSE config");
ret = -1;
- goto out_log_unmap;
+ goto out_ctrl_close;
}
vnet_config.max_virtqueue_pairs = max_queue_pairs;
@@ -720,18 +699,15 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
if (ret < 0) {
VHOST_CONFIG_LOG(name, ERR, "Failed to create VDUSE device: %s",
strerror(errno));
- goto out_log_unmap;
+ goto out_ctrl_close;
}
- memcpy(&reconnect_log->config, &vnet_config, sizeof(vnet_config));
- reconnect_log->nr_vrings = total_queues;
-
dev_fd = open(path, O_RDWR);
if (dev_fd < 0) {
VHOST_CONFIG_LOG(name, ERR, "Failed to open newly created device %s: %s",
path, strerror(errno));
ret = -1;
- goto out_log_unmap;
+ goto out_ctrl_close;
}
} else {
VHOST_CONFIG_LOG(name, ERR, "Failed to open device %s: %s",
@@ -744,14 +720,14 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
if (ret < 0) {
VHOST_CONFIG_LOG(name, ERR, "Failed to set chardev as non-blocking: %s",
strerror(errno));
- goto out_log_unmap;
+ goto out_dev_close;
}
vid = vhost_new_device(&vduse_backend_ops);
if (vid < 0) {
VHOST_CONFIG_LOG(name, ERR, "Failed to create new Vhost device");
ret = -1;
- goto out_log_unmap;
+ goto out_dev_close;
}
dev = get_device(vid);
@@ -763,10 +739,25 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
strncpy(dev->ifname, path, IF_NAME_SZ - 1);
dev->vduse_ctrl_fd = control_fd;
dev->vduse_dev_fd = dev_fd;
- dev->reconnect_log = reconnect_log;
- reconnect_log = MAP_FAILED;
- if (reconnect)
+
+ ret = vduse_reconnect_log_map(dev, !reconnect);
+ if (ret < 0) {
+ VHOST_CONFIG_LOG(name, ERR, "Failed to map reconnect log file");
+ goto out_dev_destroy;
+ }
+
+ if (reconnect) {
+ ret = vduse_reconnect_log_check(dev, features, total_queues);
+ if (ret < 0) {
+ VHOST_CONFIG_LOG(name, ERR, "Reconnection log file check failed");
+ goto out_log_unmap;
+ }
dev->status = dev->reconnect_log->status;
+ } else {
+ dev->reconnect_log->version = VHOST_RECONNECT_VERSION;
+ dev->reconnect_log->nr_vrings = total_queues;
+ memcpy(&dev->reconnect_log->config, &vnet_config, sizeof(vnet_config));
+ }
vhost_setup_virtio_net(dev->vid, true, compliant_ol_flags, true, true);
@@ -777,11 +768,11 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
ret = alloc_vring_queue(dev, i);
if (ret) {
VHOST_CONFIG_LOG(name, ERR, "Failed to alloc vring %d metadata", i);
- goto out_dev_destroy;
+ goto out_log_unmap;
}
vq = dev->virtqueue[i];
- vq->reconnect_log = &reconnect_log->vring[i];
+ vq->reconnect_log = &dev->reconnect_log->vring[i];
if (reconnect)
continue;
@@ -792,7 +783,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
ret = ioctl(dev->vduse_dev_fd, VDUSE_VQ_SETUP, &vq_cfg);
if (ret) {
VHOST_CONFIG_LOG(name, ERR, "Failed to set-up VQ %d", i);
- goto out_dev_destroy;
+ goto out_log_unmap;
}
}
@@ -802,24 +793,23 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
if (ret) {
VHOST_CONFIG_LOG(name, ERR, "Failed to add fd %d to vduse fdset",
dev->vduse_dev_fd);
- goto out_dev_destroy;
+ goto out_log_unmap;
}
if (reconnect && dev->status & VIRTIO_DEVICE_STATUS_DRIVER_OK) {
ret = vduse_reconnect_start_device(dev);
if (ret) {
VHOST_CONFIG_LOG(name, ERR, "Failed to start device at reconnection");
- goto out_dev_destroy;
+ goto out_log_unmap;
}
}
return 0;
+out_log_unmap:
+ munmap(dev->reconnect_log, sizeof(*dev->reconnect_log));
out_dev_destroy:
vhost_destroy_device(vid);
-out_log_unmap:
- if (reconnect_log != MAP_FAILED)
- munmap(reconnect_log, sizeof(*reconnect_log));
out_dev_close:
if (dev_fd >= 0)
close(dev_fd);
--
2.46.2
^ permalink raw reply [flat|nested] 15+ messages in thread