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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jul 17, 2019 at 17:05:08 +0200, Michal Privoznik wrote:
> 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@xxxxxxxxxx>
> > > ---
> > >   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?

Sorry I thought it was in post-parse. This is okay.

> 
> > 
> > > @@ -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.

Yes, but that stuff still does not have to be intermixed in the parser
code. It's a pre-existing mess though.

> 
> > 
> > > +
> > > +    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.

I meant that the NVMe disk is NOT available on the destination of the
migration. That means that 'qemuMigrationSrcIsSafe' must reject it as
not having a shared storage (note that "shared" here has a different
conotation as <shareable/>, just look at he named function).

> 
> 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.

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux