On Tue, 7 Nov 2023 at 09:24, David Woodhouse <dw...@infradead.org> wrote:
>
> From: David Woodhouse <d...@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):
> +/*
> + * 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)...
> + 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.
> + 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);
thanks
-- PMM
