Re: [PATCH] hw/xen: clean up xen_block_find_free_vdev() to avoid Coverity false positive

[Peter Maydell]

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