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