Re: [libvirt] [PATCH v1 11/31] conf: Format and parse NVMe type disk

On 7/16/19 3:00 PM, Peter Krempa wrote: > On Thu, Jul 11, 2019 at 17:53:58 +0200, Michal Privoznik wrote: > > To simplify implementation, some restrictions are added. For > > instance, an NVMe disk can't go to any bus but virtio and has to > > be type of 'disk' and can't have startupPolicy set. > > > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > > --- > > src/conf/domain_conf.c | 129 +++++++++++++++++++++++++ > > src/libvirt_private.syms | 1 + > > src/qemu/qemu_block.c | 1 + > > src/qemu/qemu_command.c | 1 + > > src/qemu/qemu_driver.c | 4 + > > src/qemu/qemu_migration.c | 1 + > > src/util/virstoragefile.c | 59 +++++++++++ > > src/util/virstoragefile.h | 15 +++ > > src/xenconfig/xen_xl.c | 1 + > > tests/qemuxml2argvdata/disk-nvme.xml | 12 ++- > > tests/qemuxml2xmloutdata/disk-nvme.xml | 1 + > > tests/qemuxml2xmltest.c | 1 + > > 12 files changed, 224 insertions(+), 2 deletions(-) > > create mode 120000 tests/qemuxml2xmloutdata/disk-nvme.xml > > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > > index 3323c9a5b1..73f5e1fa0f 100644 > > --- a/src/conf/domain_conf.c > > +++ b/src/conf/domain_conf.c > > [...] > > > @@ -5938,6 +5943,38 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk) > > return -1; > > } > > + if (disk->src->type == VIR_STORAGE_TYPE_NVME) { > > + /* These might not be valid for all hypervisors, but be > > + * strict now and possibly refine in the future. */ > > + if (disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) { > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > > + _("Unsupported disk type '%s' for NVMe disk"), > > + virDomainDiskDeviceTypeToString(disk->device)); > > + return -1; > > + } > > + > > + if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > > + _("Unsupported bus '%s' for NVMe disk"), > > + virDomainDiskBusTypeToString(disk->bus)); > > + return -1; > > + } > > + > > + if (disk->startupPolicy != VIR_DOMAIN_STARTUP_POLICY_DEFAULT && > > + disk->startupPolicy != VIR_DOMAIN_STARTUP_POLICY_MANDATORY) { > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > > + _("Unsupported startup policy '%s' for NVMe disk"), > > + virDomainStartupPolicyTypeToString(disk->startupPolicy)); > > + return -1; > > + } > > + > > + if (disk->src->shared) { > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > > + _("Unsupported <shareable/> for NVMe disk")); > > + return -1; > > + } > > + } > > + > > return 0; > > } > > As noted in the other thread, this really should be extracted, placed in > the validation callback rather than post parse and must iterate the > backing chain if you want this to keep working with -blockdev. I'm not sure I understand what you mean. This function is called virDomainDiskDefValidate() and therefore it is validation callback rather than post parse callback. Where do you want me to put these checks?

> > > @@ -9184,6 +9221,76 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node, > > } > > +static int > > +virDomainDiskSourceNVMeParse(xmlNodePtr node, > > + xmlXPathContextPtr ctxt, > > + virStorageSourcePtr src) > > +{ > > + VIR_AUTOPTR(virStorageSourceNVMeDef) nvme = NULL; > > + VIR_AUTOFREE(char *) type = NULL; > > + VIR_AUTOFREE(char *) namespace = NULL; > > + VIR_AUTOFREE(char *) managed = NULL; > > + xmlNodePtr address; > > + > > + if (VIR_ALLOC(nvme) < 0) > > + return -1; > > + > > + if (!(type = virXMLPropString(node, "type"))) { > > + virReportError(VIR_ERR_XML_ERROR, "%s", > > + _("missing 'type' attribute to disk source")); > > + return -1; > > + } > > + > > + if (STRNEQ(type, "pci")) { > > + virReportError(VIR_ERR_XML_ERROR, > > + _("unsupported source type '%s'"), > > + type); > > + return -1; > > + } > > + > > + if (!(namespace = virXMLPropString(node, "namespace"))) { > > + virReportError(VIR_ERR_XML_ERROR, "%s", > > + _("missing 'namespace' attribute to disk source")); > > + return -1; > > + } > > + > > + if (virStrToLong_ul(namespace, NULL, 10, &nvme->namespace) < 0) { > > + virReportError(VIR_ERR_XML_ERROR, > > + _("malformed namespace '%s'"), > > + namespace); > > + return -1; > > + } > > + > > + /* NVMe namespaces start from 1 */ > > + if (nvme->namespace == 0) { > > + virReportError(VIR_ERR_XML_ERROR, "%s", > > + _("NVMe namespace can't be zero")); > > + return -1; > > + } > > + > > + if ((managed = virXMLPropString(node, "managed"))) { > > + if ((nvme->managed = virTristateBoolTypeFromString(managed)) <= 0) { > > + virReportError(VIR_ERR_XML_ERROR, > > + _("malformed managed value '%s'"), > > + managed); > > + return -1; > > + } > > + } > > + > > + if (!(address = virXPathNode("./address", ctxt))) { > > + virReportError(VIR_ERR_XML_ERROR, "%s", > > + _("NVMe disk source is missing address")); > > + return -1; > > + } > > I'm displeased that this is yet another function adding validation in > the parser. You don't make the status quo any worse though so this is > not a request to change it. What validation do you mean? namespace != 0 check? Well, that stems straight from NVMe specs, so it is completely independent of any driver. Or do you mean this !address check? Well, it's needed later in the parsing.

> > > + > > + if (virPCIDeviceAddressParseXML(address, &nvme->pciAddr) < 0) > > + return -1; > > + > > + VIR_STEAL_PTR(src->nvme, nvme); > > + return 0; > > +} > > + > > + > > static int > > virDomainDiskSourcePRParse(xmlNodePtr node, > > xmlXPathContextPtr ctxt, > > [...] > > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > > index 2436f5051b..87adccab3d 100644 > > --- a/src/qemu/qemu_migration.c > > +++ b/src/qemu/qemu_migration.c > > Missing change to 'qemuMigrationSrcIsSafe' to reject VMs containing NVMe > disks not having shared source. A NVMe disk can't be shared. It's even explicitly denied in the validation callback. The reason is that there is no way to share a single PCI device with multiple domains.

But this is a good point. I'll probably put it into a different commt though. Even though I'm changing some parts of qemu driver here it's only because of the way we handle switch() with enums. > > Plus that function should probably check the full backing chain rather > than the top element only. Pre-existing, but I can try to fix it. Okay.