* [PATCH 1/4] libvduse: Fix the incorrect function name
2022-06-27 9:01 [PATCH 0/4] Fix some coverity issues on VDUSE Xie Yongji
@ 2022-06-27 9:02 ` Xie Yongji
2022-06-29 9:37 ` Markus Armbruster
2022-06-27 9:02 ` [PATCH 2/4] libvduse: Replace strcpy() with strncpy() Xie Yongji
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Xie Yongji @ 2022-06-27 9:02 UTC (permalink / raw)
To: kwolf, stefanha, armbru; +Cc: qemu-block, qemu-devel
In vduse_name_is_valid(), we actually check whether
the name is invalid or not. So let's change the
function name to vduse_name_is_invalid() to match
the behavior.
Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
subprojects/libvduse/libvduse.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
index 9a2bcec282..6374933881 100644
--- a/subprojects/libvduse/libvduse.c
+++ b/subprojects/libvduse/libvduse.c
@@ -1193,7 +1193,7 @@ static int vduse_dev_init(VduseDev *dev, const char *name,
return 0;
}
-static inline bool vduse_name_is_valid(const char *name)
+static inline bool vduse_name_is_invalid(const char *name)
{
return strlen(name) >= VDUSE_NAME_MAX || strstr(name, "..");
}
@@ -1242,7 +1242,7 @@ VduseDev *vduse_dev_create_by_name(const char *name, uint16_t num_queues,
VduseDev *dev;
int ret;
- if (!name || vduse_name_is_valid(name) || !ops ||
+ if (!name || vduse_name_is_invalid(name) || !ops ||
!ops->enable_queue || !ops->disable_queue) {
fprintf(stderr, "Invalid parameter for vduse\n");
return NULL;
@@ -1276,7 +1276,7 @@ VduseDev *vduse_dev_create(const char *name, uint32_t device_id,
struct vduse_dev_config *dev_config;
size_t size = offsetof(struct vduse_dev_config, config);
- if (!name || vduse_name_is_valid(name) ||
+ if (!name || vduse_name_is_invalid(name) ||
!has_feature(features, VIRTIO_F_VERSION_1) || !config ||
!config_size || !ops || !ops->enable_queue || !ops->disable_queue) {
fprintf(stderr, "Invalid parameter for vduse\n");
--
2.20.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/4] libvduse: Replace strcpy() with strncpy()
2022-06-27 9:01 [PATCH 0/4] Fix some coverity issues on VDUSE Xie Yongji
2022-06-27 9:02 ` [PATCH 1/4] libvduse: Fix the incorrect function name Xie Yongji
@ 2022-06-27 9:02 ` Xie Yongji
2022-06-28 0:25 ` Richard Henderson
2022-06-29 9:38 ` Markus Armbruster
2022-06-27 9:02 ` [PATCH 3/4] libvduse: Pass positive value to strerror() Xie Yongji
2022-06-27 9:02 ` [PATCH 4/4] libvduse: Check the return value of some ioctls Xie Yongji
3 siblings, 2 replies; 17+ messages in thread
From: Xie Yongji @ 2022-06-27 9:02 UTC (permalink / raw)
To: kwolf, stefanha, armbru; +Cc: qemu-block, qemu-devel
Coverity reported a string overflow issue since we copied
"name" to "dev_config->name" without checking the length.
This should be a false positive since we already checked
the length of "name" in vduse_name_is_invalid(). But anyway,
let's replace strcpy() with strncpy() to fix the coverity
complaint.
Fixes: Coverity CID 1490224
Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
subprojects/libvduse/libvduse.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
index 6374933881..1e36227388 100644
--- a/subprojects/libvduse/libvduse.c
+++ b/subprojects/libvduse/libvduse.c
@@ -1309,7 +1309,8 @@ VduseDev *vduse_dev_create(const char *name, uint32_t device_id,
goto err_dev;
}
- strcpy(dev_config->name, name);
+ strncpy(dev_config->name, name, VDUSE_NAME_MAX);
+ dev_config->name[VDUSE_NAME_MAX - 1] = '\0';
dev_config->device_id = device_id;
dev_config->vendor_id = vendor_id;
dev_config->features = features;
--
2.20.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] libvduse: Replace strcpy() with strncpy()
2022-06-27 9:02 ` [PATCH 2/4] libvduse: Replace strcpy() with strncpy() Xie Yongji
@ 2022-06-28 0:25 ` Richard Henderson
2022-06-28 2:59 ` Yongji Xie
2022-06-29 9:38 ` Markus Armbruster
1 sibling, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2022-06-28 0:25 UTC (permalink / raw)
To: Xie Yongji, kwolf, stefanha, armbru; +Cc: qemu-block, qemu-devel
On 6/27/22 14:32, Xie Yongji wrote:
> - strcpy(dev_config->name, name);
> + strncpy(dev_config->name, name, VDUSE_NAME_MAX);
> + dev_config->name[VDUSE_NAME_MAX - 1] = '\0';
g_strlcpy
r~
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] libvduse: Replace strcpy() with strncpy()
2022-06-28 0:25 ` Richard Henderson
@ 2022-06-28 2:59 ` Yongji Xie
0 siblings, 0 replies; 17+ messages in thread
From: Yongji Xie @ 2022-06-28 2:59 UTC (permalink / raw)
To: Richard Henderson
Cc: Kevin Wolf, Stefan Hajnoczi, Markus Armbruster, qemu-block, qemu-devel
On Tue, Jun 28, 2022 at 8:26 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 6/27/22 14:32, Xie Yongji wrote:
> > - strcpy(dev_config->name, name);
> > + strncpy(dev_config->name, name, VDUSE_NAME_MAX);
> > + dev_config->name[VDUSE_NAME_MAX - 1] = '\0';
>
> g_strlcpy
>
Now we don't have a dependency on glib, so we use strncpy here.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] libvduse: Replace strcpy() with strncpy()
2022-06-27 9:02 ` [PATCH 2/4] libvduse: Replace strcpy() with strncpy() Xie Yongji
2022-06-28 0:25 ` Richard Henderson
@ 2022-06-29 9:38 ` Markus Armbruster
1 sibling, 0 replies; 17+ messages in thread
From: Markus Armbruster @ 2022-06-29 9:38 UTC (permalink / raw)
To: Xie Yongji; +Cc: kwolf, stefanha, armbru, qemu-block, qemu-devel
Xie Yongji <xieyongji@bytedance.com> writes:
> Coverity reported a string overflow issue since we copied
> "name" to "dev_config->name" without checking the length.
> This should be a false positive since we already checked
> the length of "name" in vduse_name_is_invalid(). But anyway,
> let's replace strcpy() with strncpy() to fix the coverity
> complaint.
Mention why you can't use something nicer from GLib?
> Fixes: Coverity CID 1490224
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/4] libvduse: Pass positive value to strerror()
2022-06-27 9:01 [PATCH 0/4] Fix some coverity issues on VDUSE Xie Yongji
2022-06-27 9:02 ` [PATCH 1/4] libvduse: Fix the incorrect function name Xie Yongji
2022-06-27 9:02 ` [PATCH 2/4] libvduse: Replace strcpy() with strncpy() Xie Yongji
@ 2022-06-27 9:02 ` Xie Yongji
2022-06-28 0:26 ` Richard Henderson
2022-06-29 9:38 ` Markus Armbruster
2022-06-27 9:02 ` [PATCH 4/4] libvduse: Check the return value of some ioctls Xie Yongji
3 siblings, 2 replies; 17+ messages in thread
From: Xie Yongji @ 2022-06-27 9:02 UTC (permalink / raw)
To: kwolf, stefanha, armbru; +Cc: qemu-block, qemu-devel
The value passed to strerror() should be positive.
So let's fix it.
Fixes: Coverity CID 1490226, 1490223
Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
subprojects/libvduse/libvduse.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
index 1e36227388..1a5981445c 100644
--- a/subprojects/libvduse/libvduse.c
+++ b/subprojects/libvduse/libvduse.c
@@ -1257,7 +1257,7 @@ VduseDev *vduse_dev_create_by_name(const char *name, uint16_t num_queues,
ret = vduse_dev_init(dev, name, num_queues, ops, priv);
if (ret < 0) {
fprintf(stderr, "Failed to init vduse device %s: %s\n",
- name, strerror(ret));
+ name, strerror(-ret));
free(dev);
return NULL;
}
@@ -1331,7 +1331,7 @@ VduseDev *vduse_dev_create(const char *name, uint32_t device_id,
ret = vduse_dev_init(dev, name, num_queues, ops, priv);
if (ret < 0) {
fprintf(stderr, "Failed to init vduse device %s: %s\n",
- name, strerror(ret));
+ name, strerror(-ret));
goto err;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] libvduse: Pass positive value to strerror()
2022-06-27 9:02 ` [PATCH 3/4] libvduse: Pass positive value to strerror() Xie Yongji
@ 2022-06-28 0:26 ` Richard Henderson
2022-06-29 9:38 ` Markus Armbruster
1 sibling, 0 replies; 17+ messages in thread
From: Richard Henderson @ 2022-06-28 0:26 UTC (permalink / raw)
To: Xie Yongji, kwolf, stefanha, armbru; +Cc: qemu-block, qemu-devel
On 6/27/22 14:32, Xie Yongji wrote:
> The value passed to strerror() should be positive.
> So let's fix it.
>
> Fixes: Coverity CID 1490226, 1490223
> Signed-off-by: Xie Yongji<xieyongji@bytedance.com>
> ---
> subprojects/libvduse/libvduse.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] libvduse: Pass positive value to strerror()
2022-06-27 9:02 ` [PATCH 3/4] libvduse: Pass positive value to strerror() Xie Yongji
2022-06-28 0:26 ` Richard Henderson
@ 2022-06-29 9:38 ` Markus Armbruster
1 sibling, 0 replies; 17+ messages in thread
From: Markus Armbruster @ 2022-06-29 9:38 UTC (permalink / raw)
To: Xie Yongji; +Cc: kwolf, stefanha, armbru, qemu-block, qemu-devel
Xie Yongji <xieyongji@bytedance.com> writes:
> The value passed to strerror() should be positive.
> So let's fix it.
>
> Fixes: Coverity CID 1490226, 1490223
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/4] libvduse: Check the return value of some ioctls
2022-06-27 9:01 [PATCH 0/4] Fix some coverity issues on VDUSE Xie Yongji
` (2 preceding siblings ...)
2022-06-27 9:02 ` [PATCH 3/4] libvduse: Pass positive value to strerror() Xie Yongji
@ 2022-06-27 9:02 ` Xie Yongji
2022-06-29 9:41 ` Markus Armbruster
3 siblings, 1 reply; 17+ messages in thread
From: Xie Yongji @ 2022-06-27 9:02 UTC (permalink / raw)
To: kwolf, stefanha, armbru; +Cc: qemu-block, qemu-devel
Coverity pointed out (CID 1490222, 1490227) that we called
ioctl somewhere without checking the return value. This
patch fixes these issues.
Fixes: Coverity CID 1490222, 1490227
Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
subprojects/libvduse/libvduse.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
index 1a5981445c..bf7302c60a 100644
--- a/subprojects/libvduse/libvduse.c
+++ b/subprojects/libvduse/libvduse.c
@@ -947,7 +947,10 @@ static void vduse_queue_disable(VduseVirtq *vq)
eventfd.index = vq->index;
eventfd.fd = VDUSE_EVENTFD_DEASSIGN;
- ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd);
+ if (ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd)) {
+ fprintf(stderr, "Failed to disable eventfd for vq[%d]: %s\n",
+ vq->index, strerror(errno));
+ }
close(vq->fd);
assert(vq->inuse == 0);
@@ -1337,7 +1340,10 @@ VduseDev *vduse_dev_create(const char *name, uint32_t device_id,
return dev;
err:
- ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name);
+ if (ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name)) {
+ fprintf(stderr, "Failed to destroy vduse device %s: %s\n",
+ name, strerror(errno));
+ }
err_dev:
close(ctrl_fd);
err_ctrl:
--
2.20.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] libvduse: Check the return value of some ioctls
2022-06-27 9:02 ` [PATCH 4/4] libvduse: Check the return value of some ioctls Xie Yongji
@ 2022-06-29 9:41 ` Markus Armbruster
2022-06-29 11:10 ` Yongji Xie
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2022-06-29 9:41 UTC (permalink / raw)
To: Xie Yongji; +Cc: kwolf, stefanha, qemu-block, qemu-devel
Xie Yongji <xieyongji@bytedance.com> writes:
> Coverity pointed out (CID 1490222, 1490227) that we called
> ioctl somewhere without checking the return value. This
> patch fixes these issues.
>
> Fixes: Coverity CID 1490222, 1490227
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
> subprojects/libvduse/libvduse.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
> index 1a5981445c..bf7302c60a 100644
> --- a/subprojects/libvduse/libvduse.c
> +++ b/subprojects/libvduse/libvduse.c
> @@ -947,7 +947,10 @@ static void vduse_queue_disable(VduseVirtq *vq)
>
> eventfd.index = vq->index;
> eventfd.fd = VDUSE_EVENTFD_DEASSIGN;
> - ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd);
> + if (ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd)) {
> + fprintf(stderr, "Failed to disable eventfd for vq[%d]: %s\n",
> + vq->index, strerror(errno));
> + }
> close(vq->fd);
>
> assert(vq->inuse == 0);
> @@ -1337,7 +1340,10 @@ VduseDev *vduse_dev_create(const char *name, uint32_t device_id,
>
> return dev;
> err:
> - ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name);
> + if (ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name)) {
> + fprintf(stderr, "Failed to destroy vduse device %s: %s\n",
> + name, strerror(errno));
> + }
> err_dev:
> close(ctrl_fd);
> err_ctrl:
Both errors are during cleanup that can't fail. The program continues
as if they didn't happen. Does the user need to know?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] libvduse: Check the return value of some ioctls
2022-06-29 9:41 ` Markus Armbruster
@ 2022-06-29 11:10 ` Yongji Xie
2022-06-29 11:39 ` Markus Armbruster
0 siblings, 1 reply; 17+ messages in thread
From: Yongji Xie @ 2022-06-29 11:10 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, qemu-devel
On Wed, Jun 29, 2022 at 5:41 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Xie Yongji <xieyongji@bytedance.com> writes:
>
> > Coverity pointed out (CID 1490222, 1490227) that we called
> > ioctl somewhere without checking the return value. This
> > patch fixes these issues.
> >
> > Fixes: Coverity CID 1490222, 1490227
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > ---
> > subprojects/libvduse/libvduse.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
> > index 1a5981445c..bf7302c60a 100644
> > --- a/subprojects/libvduse/libvduse.c
> > +++ b/subprojects/libvduse/libvduse.c
> > @@ -947,7 +947,10 @@ static void vduse_queue_disable(VduseVirtq *vq)
> >
> > eventfd.index = vq->index;
> > eventfd.fd = VDUSE_EVENTFD_DEASSIGN;
> > - ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd);
> > + if (ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd)) {
> > + fprintf(stderr, "Failed to disable eventfd for vq[%d]: %s\n",
> > + vq->index, strerror(errno));
> > + }
> > close(vq->fd);
> >
> > assert(vq->inuse == 0);
> > @@ -1337,7 +1340,10 @@ VduseDev *vduse_dev_create(const char *name, uint32_t device_id,
> >
> > return dev;
> > err:
> > - ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name);
> > + if (ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name)) {
> > + fprintf(stderr, "Failed to destroy vduse device %s: %s\n",
> > + name, strerror(errno));
> > + }
> > err_dev:
> > close(ctrl_fd);
> > err_ctrl:
>
> Both errors are during cleanup that can't fail. The program continues
> as if they didn't happen. Does the user need to know?
>
So I printed some error messages. I didn't find any other good way to
notify the users.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] libvduse: Check the return value of some ioctls
2022-06-29 11:10 ` Yongji Xie
@ 2022-06-29 11:39 ` Markus Armbruster
2022-06-29 12:37 ` Yongji Xie
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2022-06-29 11:39 UTC (permalink / raw)
To: Yongji Xie; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, qemu-devel
Yongji Xie <xieyongji@bytedance.com> writes:
> On Wed, Jun 29, 2022 at 5:41 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Xie Yongji <xieyongji@bytedance.com> writes:
>>
>> > Coverity pointed out (CID 1490222, 1490227) that we called
>> > ioctl somewhere without checking the return value. This
>> > patch fixes these issues.
>> >
>> > Fixes: Coverity CID 1490222, 1490227
>> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
>> > ---
>> > subprojects/libvduse/libvduse.c | 10 ++++++++--
>> > 1 file changed, 8 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
>> > index 1a5981445c..bf7302c60a 100644
>> > --- a/subprojects/libvduse/libvduse.c
>> > +++ b/subprojects/libvduse/libvduse.c
>> > @@ -947,7 +947,10 @@ static void vduse_queue_disable(VduseVirtq *vq)
>> >
>> > eventfd.index = vq->index;
>> > eventfd.fd = VDUSE_EVENTFD_DEASSIGN;
>> > - ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd);
>> > + if (ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd)) {
>> > + fprintf(stderr, "Failed to disable eventfd for vq[%d]: %s\n",
>> > + vq->index, strerror(errno));
>> > + }
>> > close(vq->fd);
>> >
>> > assert(vq->inuse == 0);
>> > @@ -1337,7 +1340,10 @@ VduseDev *vduse_dev_create(const char *name, uint32_t device_id,
>> >
>> > return dev;
>> > err:
>> > - ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name);
>> > + if (ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name)) {
>> > + fprintf(stderr, "Failed to destroy vduse device %s: %s\n",
>> > + name, strerror(errno));
>> > + }
>> > err_dev:
>> > close(ctrl_fd);
>> > err_ctrl:
>>
>> Both errors are during cleanup that can't fail. The program continues
>> as if they didn't happen. Does the user need to know?
>>
>
> So I printed some error messages. I didn't find any other good way to
> notify the users.
I can think of another way, either. But my question wasn't about "how",
it was about "why". The answer depends on the impact of these errors.
Which I can't judge. Can you?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] libvduse: Check the return value of some ioctls
2022-06-29 11:39 ` Markus Armbruster
@ 2022-06-29 12:37 ` Yongji Xie
2022-06-29 13:22 ` Markus Armbruster
0 siblings, 1 reply; 17+ messages in thread
From: Yongji Xie @ 2022-06-29 12:37 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, qemu-devel
On Wed, Jun 29, 2022 at 7:39 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Yongji Xie <xieyongji@bytedance.com> writes:
>
> > On Wed, Jun 29, 2022 at 5:41 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Xie Yongji <xieyongji@bytedance.com> writes:
> >>
> >> > Coverity pointed out (CID 1490222, 1490227) that we called
> >> > ioctl somewhere without checking the return value. This
> >> > patch fixes these issues.
> >> >
> >> > Fixes: Coverity CID 1490222, 1490227
> >> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> >> > ---
> >> > subprojects/libvduse/libvduse.c | 10 ++++++++--
> >> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
> >> > index 1a5981445c..bf7302c60a 100644
> >> > --- a/subprojects/libvduse/libvduse.c
> >> > +++ b/subprojects/libvduse/libvduse.c
> >> > @@ -947,7 +947,10 @@ static void vduse_queue_disable(VduseVirtq *vq)
> >> >
> >> > eventfd.index = vq->index;
> >> > eventfd.fd = VDUSE_EVENTFD_DEASSIGN;
> >> > - ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd);
> >> > + if (ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd)) {
> >> > + fprintf(stderr, "Failed to disable eventfd for vq[%d]: %s\n",
> >> > + vq->index, strerror(errno));
> >> > + }
> >> > close(vq->fd);
> >> >
> >> > assert(vq->inuse == 0);
> >> > @@ -1337,7 +1340,10 @@ VduseDev *vduse_dev_create(const char *name, uint32_t device_id,
> >> >
> >> > return dev;
> >> > err:
> >> > - ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name);
> >> > + if (ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name)) {
> >> > + fprintf(stderr, "Failed to destroy vduse device %s: %s\n",
> >> > + name, strerror(errno));
> >> > + }
> >> > err_dev:
> >> > close(ctrl_fd);
> >> > err_ctrl:
> >>
> >> Both errors are during cleanup that can't fail. The program continues
> >> as if they didn't happen. Does the user need to know?
> >>
> >
> > So I printed some error messages. I didn't find any other good way to
> > notify the users.
>
> I can think of another way, either. But my question wasn't about "how",
> it was about "why". The answer depends on the impact of these errors.
> Which I can't judge. Can you?
>
OK, I get your point. Actually users might have no way to handle those
errors. And there is no real impact on users since those errors mean
the resources have been cleaned up in other places or by other
processes. So I choose to ignore this error, but it triggers a
Coverity warning.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] libvduse: Check the return value of some ioctls
2022-06-29 12:37 ` Yongji Xie
@ 2022-06-29 13:22 ` Markus Armbruster
2022-06-29 13:28 ` Yongji Xie
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2022-06-29 13:22 UTC (permalink / raw)
To: Yongji Xie; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, qemu-devel
Yongji Xie <xieyongji@bytedance.com> writes:
> On Wed, Jun 29, 2022 at 7:39 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Yongji Xie <xieyongji@bytedance.com> writes:
>>
>> > On Wed, Jun 29, 2022 at 5:41 PM Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> Xie Yongji <xieyongji@bytedance.com> writes:
>> >>
>> >> > Coverity pointed out (CID 1490222, 1490227) that we called
>> >> > ioctl somewhere without checking the return value. This
>> >> > patch fixes these issues.
>> >> >
>> >> > Fixes: Coverity CID 1490222, 1490227
>> >> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
>> >> > ---
>> >> > subprojects/libvduse/libvduse.c | 10 ++++++++--
>> >> > 1 file changed, 8 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
>> >> > index 1a5981445c..bf7302c60a 100644
>> >> > --- a/subprojects/libvduse/libvduse.c
>> >> > +++ b/subprojects/libvduse/libvduse.c
>> >> > @@ -947,7 +947,10 @@ static void vduse_queue_disable(VduseVirtq *vq)
>> >> >
>> >> > eventfd.index = vq->index;
>> >> > eventfd.fd = VDUSE_EVENTFD_DEASSIGN;
>> >> > - ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd);
>> >> > + if (ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd)) {
>> >> > + fprintf(stderr, "Failed to disable eventfd for vq[%d]: %s\n",
>> >> > + vq->index, strerror(errno));
>> >> > + }
>> >> > close(vq->fd);
>> >> >
>> >> > assert(vq->inuse == 0);
>> >> > @@ -1337,7 +1340,10 @@ VduseDev *vduse_dev_create(const char *name, uint32_t device_id,
>> >> >
>> >> > return dev;
>> >> > err:
>> >> > - ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name);
>> >> > + if (ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name)) {
>> >> > + fprintf(stderr, "Failed to destroy vduse device %s: %s\n",
>> >> > + name, strerror(errno));
>> >> > + }
>> >> > err_dev:
>> >> > close(ctrl_fd);
>> >> > err_ctrl:
>> >>
>> >> Both errors are during cleanup that can't fail. The program continues
>> >> as if they didn't happen. Does the user need to know?
>> >>
>> >
>> > So I printed some error messages. I didn't find any other good way to
>> > notify the users.
>>
>> I can think of another way, either. But my question wasn't about "how",
>> it was about "why". The answer depends on the impact of these errors.
>> Which I can't judge. Can you?
>>
>
> OK, I get your point. Actually users might have no way to handle those
> errors. And there is no real impact on users since those errors mean
> the resources have been cleaned up in other places or by other
> processes. So I choose to ignore this error, but it triggers a
> Coverity warning.
If we genuinely want to ignore errors from ioctl(), we can mark the
Coverity complaint as false positive.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] libvduse: Check the return value of some ioctls
2022-06-29 13:22 ` Markus Armbruster
@ 2022-06-29 13:28 ` Yongji Xie
0 siblings, 0 replies; 17+ messages in thread
From: Yongji Xie @ 2022-06-29 13:28 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-block, qemu-devel
On Wed, Jun 29, 2022 at 9:22 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Yongji Xie <xieyongji@bytedance.com> writes:
>
> > On Wed, Jun 29, 2022 at 7:39 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Yongji Xie <xieyongji@bytedance.com> writes:
> >>
> >> > On Wed, Jun 29, 2022 at 5:41 PM Markus Armbruster <armbru@redhat.com> wrote:
> >> >>
> >> >> Xie Yongji <xieyongji@bytedance.com> writes:
> >> >>
> >> >> > Coverity pointed out (CID 1490222, 1490227) that we called
> >> >> > ioctl somewhere without checking the return value. This
> >> >> > patch fixes these issues.
> >> >> >
> >> >> > Fixes: Coverity CID 1490222, 1490227
> >> >> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> >> >> > ---
> >> >> > subprojects/libvduse/libvduse.c | 10 ++++++++--
> >> >> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >> >> >
> >> >> > diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
> >> >> > index 1a5981445c..bf7302c60a 100644
> >> >> > --- a/subprojects/libvduse/libvduse.c
> >> >> > +++ b/subprojects/libvduse/libvduse.c
> >> >> > @@ -947,7 +947,10 @@ static void vduse_queue_disable(VduseVirtq *vq)
> >> >> >
> >> >> > eventfd.index = vq->index;
> >> >> > eventfd.fd = VDUSE_EVENTFD_DEASSIGN;
> >> >> > - ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd);
> >> >> > + if (ioctl(dev->fd, VDUSE_VQ_SETUP_KICKFD, &eventfd)) {
> >> >> > + fprintf(stderr, "Failed to disable eventfd for vq[%d]: %s\n",
> >> >> > + vq->index, strerror(errno));
> >> >> > + }
> >> >> > close(vq->fd);
> >> >> >
> >> >> > assert(vq->inuse == 0);
> >> >> > @@ -1337,7 +1340,10 @@ VduseDev *vduse_dev_create(const char *name, uint32_t device_id,
> >> >> >
> >> >> > return dev;
> >> >> > err:
> >> >> > - ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name);
> >> >> > + if (ioctl(ctrl_fd, VDUSE_DESTROY_DEV, name)) {
> >> >> > + fprintf(stderr, "Failed to destroy vduse device %s: %s\n",
> >> >> > + name, strerror(errno));
> >> >> > + }
> >> >> > err_dev:
> >> >> > close(ctrl_fd);
> >> >> > err_ctrl:
> >> >>
> >> >> Both errors are during cleanup that can't fail. The program continues
> >> >> as if they didn't happen. Does the user need to know?
> >> >>
> >> >
> >> > So I printed some error messages. I didn't find any other good way to
> >> > notify the users.
> >>
> >> I can think of another way, either. But my question wasn't about "how",
> >> it was about "why". The answer depends on the impact of these errors.
> >> Which I can't judge. Can you?
> >>
> >
> > OK, I get your point. Actually users might have no way to handle those
> > errors. And there is no real impact on users since those errors mean
> > the resources have been cleaned up in other places or by other
> > processes. So I choose to ignore this error, but it triggers a
> > Coverity warning.
>
> If we genuinely want to ignore errors from ioctl(), we can mark the
> Coverity complaint as false positive.
>
OK, if so, I think I can drop this patch.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 17+ messages in thread