| Message ID | 1c312ba88e0928527dad6bc2e1b73d8cfe4b7f48.camel@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/xen: clean up xen_block_find_free_vdev() to avoid Coverity false positive | expand |
On 09/11/2023 15:30, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > Coverity couldn't see that nr_existing was always going to be zero when > qemu_xen_xs_directory() returned NULL in the ENOENT case (CID 1523906). > > Perhaps more to the point, neither could Peter at first glance. Improve > the code to hopefully make it clearer to Coverity and human reviewers > alike. > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > --- > hw/block/xen-block.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > Reviewed-by: Paul Durrant <paul@xen.org>
On Thu, 9 Nov 2023 at 15:30, David Woodhouse <dwmw2@infradead.org> wrote: > > From: David Woodhouse <dwmw@amazon.co.uk> > > Coverity couldn't see that nr_existing was always going to be zero when > qemu_xen_xs_directory() returned NULL in the ENOENT case (CID 1523906). > > Perhaps more to the point, neither could Peter at first glance. Improve > the code to hopefully make it clearer to Coverity and human reviewers > alike. > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > --- > hw/block/xen-block.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c > index 6d64ede94f..aed1d5c330 100644 > --- a/hw/block/xen-block.c > +++ b/hw/block/xen-block.c > @@ -91,9 +91,27 @@ static bool xen_block_find_free_vdev(XenBlockDevice *blockdev, Error **errp) > > existing_frontends = qemu_xen_xs_directory(xenbus->xsh, XBT_NULL, fe_path, > &nr_existing); > - if (!existing_frontends && errno != ENOENT) { > - error_setg_errno(errp, errno, "cannot read %s", fe_path); > - return false; > + if (!existing_frontends) { > + if (errno == ENOENT) { > + /* > + * If the frontend directory doesn't exist because there are > + * no existing vbd devices, that's fine. Just ensure that we > + * don't dereference the NULL existing_frontends pointer, by > + * checking that nr_existing is zero so the loop below is not > + * entered. > + * > + * In fact this is redundant since nr_existing is initialized > + * to zero, but setting it again here makes it abundantly clear > + * to Coverity, and to the human reader who doesn't know the > + * semantics of qemu_xen_xs_directory() off the top of their > + * head. > + */ > + nr_existing = 0; You could alternatively assert(nr_existing == 0); here, but I don't feel strongly about that. > + } else { > + /* All other errors accessing the frontend directory are fatal. */ > + error_setg_errno(errp, errno, "cannot read %s", fe_path); > + return false; > + } > } > > memset(used_devs, 0, sizeof(used_devs)); > -- > 2.34.1 thanks -- PMM
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c index 6d64ede94f..aed1d5c330 100644 --- a/hw/block/xen-block.c +++ b/hw/block/xen-block.c @@ -91,9 +91,27 @@ static bool xen_block_find_free_vdev(XenBlockDevice *blockdev, Error **errp) existing_frontends = qemu_xen_xs_directory(xenbus->xsh, XBT_NULL, fe_path, &nr_existing); - if (!existing_frontends && errno != ENOENT) { - error_setg_errno(errp, errno, "cannot read %s", fe_path); - return false; + if (!existing_frontends) { + if (errno == ENOENT) { + /* + * If the frontend directory doesn't exist because there are + * no existing vbd devices, that's fine. Just ensure that we + * don't dereference the NULL existing_frontends pointer, by + * checking that nr_existing is zero so the loop below is not + * entered. + * + * In fact this is redundant since nr_existing is initialized + * to zero, but setting it again here makes it abundantly clear + * to Coverity, and to the human reader who doesn't know the + * semantics of qemu_xen_xs_directory() off the top of their + * head. + */ + nr_existing = 0; + } else { + /* All other errors accessing the frontend directory are fatal. */ + error_setg_errno(errp, errno, "cannot read %s", fe_path); + return false; + } } memset(used_devs, 0, sizeof(used_devs));