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;
+        } 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

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

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>