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