[PATCH 0/4] Fix some coverity issues on VDUSE

[PATCH 0/4] Fix some coverity issues on VDUSE

[Xie Yongji]

This series fixes some issues reported by coverity.

Patch 1 fixes an incorrect function name.

Patch 2 fixes Coverity CID 1490224.

Patch 3 fixes Coverity CID 1490226, 1490223.

Patch 4 fixes Coverity CID 1490222, 1490227.

Xie Yongji (4):
  libvduse: Fix the incorrect function name
  libvduse: Replace strcpy() with strncpy()
  libvduse: Pass positive value to strerror()
  libvduse: Check the return value of some ioctls

 subprojects/libvduse/libvduse.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH 1/4] libvduse: Fix the incorrect function name

[Xie Yongji]

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

Re: [PATCH 1/4] libvduse: Fix the incorrect function name

[Markus Armbruster]

Xie Yongji <xieyongji@bytedance.com> writes:

> 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>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH 2/4] libvduse: Replace strcpy() with strncpy()

[Xie Yongji]

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

Re: [PATCH 2/4] libvduse: Replace strcpy() with strncpy()

[Richard Henderson]

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~

Re: [PATCH 2/4] libvduse: Replace strcpy() with strncpy()

[Yongji Xie]

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

Re: [PATCH 2/4] libvduse: Replace strcpy() with strncpy()

[Markus Armbruster]

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>

[PATCH 3/4] libvduse: Pass positive value to strerror()

[Xie Yongji]

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

Re: [PATCH 3/4] libvduse: Pass positive value to strerror()

[Richard Henderson]

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~

Re: [PATCH 3/4] libvduse: Pass positive value to strerror()

[Markus Armbruster]

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>

[PATCH 4/4] libvduse: Check the return value of some ioctls

[Xie Yongji]

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

Re: [PATCH 4/4] libvduse: Check the return value of some ioctls

[Markus Armbruster]

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?

Re: [PATCH 4/4] libvduse: Check the return value of some ioctls

[Yongji Xie]

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

Re: [PATCH 4/4] libvduse: Check the return value of some ioctls

[Markus Armbruster]

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?

Re: [PATCH 4/4] libvduse: Check the return value of some ioctls

[Yongji Xie]

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

Re: [PATCH 4/4] libvduse: Check the return value of some ioctls

[Markus Armbruster]

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.

Re: [PATCH 4/4] libvduse: Check the return value of some ioctls

[Yongji Xie]

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