1. Patchew
  2. QEMU
  3. [PATCH 0/4] Fix some coverity issues on VDUSE
  4. View patch

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

Xie Yongji posted 4 patches 3 years, 7 months ago
Maintainers: Xie Yongji <xieyongji@bytedance.com>
There is a newer version of this series
[PATCH 4/4] libvduse: Check the return value of some ioctls
Posted by Xie Yongji 3 years, 7 months ago 
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
Posted by Markus Armbruster 3 years, 7 months ago 
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
Posted by Yongji Xie 3 years, 7 months ago 
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
Posted by Markus Armbruster 3 years, 7 months ago 
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
Posted by Yongji Xie 3 years, 7 months ago 
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
Posted by Markus Armbruster 3 years, 7 months ago 
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
Posted by Yongji Xie 3 years, 7 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.