On Tue, 7 Nov 2023 at 09:24, David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote: > > From: David Woodhouse <dwmw@xxxxxxxxxxxx> > > 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