| Message ID | 20220627090203.87-5-xieyongji@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix some coverity issues on VDUSE | expand |
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?
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
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?
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
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.
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
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:
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(-)