Re: [PULL 06/15] hw/xen: automatically assign device index to block devices

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 4247 bytes --]

On Thu, 2023-11-09 at 14:33 +0000, Peter Maydell wrote:
> On Tue, 7 Nov 2023 at 09:24, David Woodhouse <dwmw2@infradead.org> wrote:
> > 
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > There's no need to force the user to assign a vdev. We can automatically
> > assign one, starting at xvda and searching until we find the first disk
> > name that's unused.
> > 
> > This means we can now allow '-drive if=xen,file=xxx' to work without an
> > explicit separate -driver argument, just like if=virtio.
> > 
> > Rip out the legacy handling from the xenpv machine, which was scribbling
> > over any disks configured by the toolstack, and didn't work with anything
> > but raw images.
> 
> Hi; Coverity points out an issue in this code (CID 1523906):

Thanks! I think this one is a false positive, although I'm happy to
explore possible cleanups which make it clearer both to the human
reader and to Coverity.

> > +/*
> > + * Find a free device name in the xvda → xvdfan range and set it in
> > + * blockdev->props.vdev. Our definition of "free" is that there must
> > + * be no other disk or partition with the same disk number.
> > + *
> > + * You are technically permitted to have all of hda, hda1, sda, sda1,
> > + * xvda and xvda1 as *separate* PV block devices with separate backing
> > + * stores. That doesn't make it a good idea. This code will skip xvda
> > + * if *any* of those "conflicting" devices already exists.
> > + *
> > + * The limit of xvdfan (disk 4095) is fairly arbitrary just to avoid a
> > + * stupidly sized bitmap, but Linux as of v6.6 doesn't support anything
> > + * higher than that anyway.
> > + */
> > +static bool xen_block_find_free_vdev(XenBlockDevice *blockdev, Error **errp)
> > +{
> > +    XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(blockdev)));
> > +    unsigned long used_devs[BITS_TO_LONGS(MAX_AUTO_VDEV)];
> > +    XenBlockVdev *vdev = &blockdev->props.vdev;
> > +    char fe_path[XENSTORE_ABS_PATH_MAX + 1];
> > +    char **existing_frontends;
> > +    unsigned int nr_existing = 0;
> > +    unsigned int vdev_nr;
> > +    int i, disk = 0;
> > +
> > +    snprintf(fe_path, sizeof(fe_path), "/local/domain/%u/device/vbd",
> > +             blockdev->xendev.frontend_id);
> > +
> > +    existing_frontends = qemu_xen_xs_directory(xenbus->xsh, XBT_NULL, fe_path,
> > +                                               &nr_existing);
> > +    if (!existing_frontends && errno != ENOENT) {
> 
> Here we check whether existing_frontends is NULL, implying it
> might be NULL (and the && in the condition means we might not
> take this error-exit path even if it is NULL)...

True, but nr_existing will be zero in that case, and we'll never go
into the loop where existing_frontends[] is dereferenced.

I suppose we could add something like this. Would it help Coverity to
realise that it's a false positive?

    /*
     * If the directory didn't exist (the ENOENT case) then nr_existing
     * will still be zero, and the loop below won't dereference the
     * existing_frontends pointer which is NULL.
     */
    assert(existing_frontends || !nr_existing);


> > +        error_setg_errno(errp, errno, "cannot read %s", fe_path);
> > +        return false;
> > +    }
> > +
> > +    memset(used_devs, 0, sizeof(used_devs));
> > +    for (i = 0; i < nr_existing; i++) {
> > +        if (qemu_strtoui(existing_frontends[i], NULL, 10, &vdev_nr)) {
> 
> ...but here we deref existing_frontends, implying it can't be NULL.
> 

If you got to this line of code, it can't.

> > +            free(existing_frontends[i]);
> > +            continue;
> > +        }
> > +
> > +        free(existing_frontends[i]);
> > +
> > +        disk = vdev_to_diskno(vdev_nr);
> > +        if (disk < 0 || disk >= MAX_AUTO_VDEV) {
> > +            continue;
> > +        }
> > +
> > +        set_bit(disk, used_devs);
> > +    }
> > +    free(existing_frontends);


*Here* it can be NULL. But free(NULL) is fine.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]