Re: [libvirt PATCH 4/6] conf: implement support for vhostuser disk

On Tue, Feb 02, 2021 at 16:04:10 +0100, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > > We need to find a better way to validate different combinations of XML > elements and attributes. > > src/conf/domain_conf.c | 85 ++++++++ > src/conf/domain_validate.c | 187 ++++++++++++++++++ > src/conf/storage_source_conf.c | 3 + > src/conf/storage_source_conf.h | 4 + > src/libxl/xen_xl.c | 1 + > src/qemu/qemu_block.c | 4 + > src/qemu/qemu_command.c | 1 + > src/qemu/qemu_migration.c | 2 + > src/qemu/qemu_snapshot.c | 4 + > src/storage_file/storage_source.c | 1 + > tests/qemuxml2argvdata/disk-vhostuser.xml | 29 +++ > .../disk-vhostuser.x86_64-latest.xml | 48 +++++ > tests/qemuxml2xmltest.c | 1 + > 13 files changed, 370 insertions(+) > create mode 100644 tests/qemuxml2argvdata/disk-vhostuser.xml > create mode 100644 tests/qemuxml2xmloutdata/disk-vhostuser.x86_64-latest.xml > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 97fa841bff..43552c36c3 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -5228,6 +5228,11 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk, > disk->src->nvme->managed = VIR_TRISTATE_BOOL_YES; > } /* vhost-user doesn't allow us to snapshot, disable snapshots by default */ > + if (disk->src->type == VIR_STORAGE_TYPE_VHOST_USER && > + disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT) { > + disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; > + } > + > if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && > virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) { > return -1; > @@ -8361,6 +8366,55 @@ virDomainDiskSourceNVMeParse(xmlNodePtr node, > } > > > +static int > +virDomainChrSourceReconnectDefParseXML(virDomainChrSourceReconnectDefPtr def, > + xmlNodePtr node, > + xmlXPathContextPtr ctxt); [... see below note on forward decls.] > + > + > +static int > +virDomainDiskSourceVHostUserParse(xmlNodePtr node, > + virStorageSourcePtr src, > + virDomainXMLOptionPtr xmlopt, > + xmlXPathContextPtr ctxt) > +{ > + g_autofree char *type = virXMLPropString(node, "type"); > + g_autofree char *path = virXMLPropString(node, "path"); > + > + if (!type) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing 'type' attribute for vhostuser disk source")); > + return -1; > + } > + > + if (STRNEQ(type, "unix")) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("invalid 'type' attribute for vhostuser disk source")); > + return -1; > + } > + > + if (!path) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing 'path' attribute for vhostuser disk source")); > + return -1; > + } > + > + if (!(src->vhostuser = virDomainChrSourceDefNew(xmlopt))) > + return -1; > + > + src->vhostuser->type = virDomainChrTypeFromString(type); > + src->vhostuser->data.nix.path = g_steal_pointer(&path); 'path' doesn't really need a temp variable.

> + > + if (virDomainChrSourceReconnectDefParseXML(&src->vhostuser->data.nix.reconnect, > + node, > + ctxt) < 0) { > + return -1; > + } > + > + return 0; > +} [...] > @@ -23939,6 +23997,23 @@ virDomainDiskSourceNVMeFormat(virBufferPtr attrBuf, > } > > > +static void > +virDomainChrSourceReconnectDefFormat(virBufferPtr buf, > + virDomainChrSourceReconnectDefPtr def); > + > + [...] > @@ -23989,6 +24064,12 @@ virDomainDiskSourceFormatSlices(virBufferPtr buf, > } > > > +static void > +virDomainChrSourceDefFormat(virBufferPtr buf, > + virDomainChrSourceDefPtr def, > + unsigned int flags); > + > + Preferably put the forward declarations at the top of the file. [...] > diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c > index c56b03ff3a..c60300e750 100644 > --- a/src/conf/domain_validate.c > +++ b/src/conf/domain_validate.c [...] > @@ -254,6 +255,187 @@ virDomainCheckVirtioOptionsAreAbsent(virDomainVirtioOptionsPtr virtio) > } > > > +static int > +virDomainDiskVhostUserValidate(const virDomainDiskDef *disk) > +{ > + if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("vhostuser disk supports only virtio bus")); > + return -1; > + } > + > + if (disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("only snapshot=no is supported with vhostuser disk")); > + return -1; > + } > + > + /* Unsupported drive attributes */ s/drive/dirver/

> + > + if (disk->cachemode != VIR_DOMAIN_DISK_CACHE_DEFAULT) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("cache is not supported with vhostuser disk")); > + return -1; > + } > + > + if (disk->error_policy || disk->rerror_policy) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("error_policy is not supported with vhostuser disk")); > + return -1; > + } > + > + if (disk->iomode) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("io is not supported with vhostuser disk")); > + return -1; > + } > + > + if (disk->ioeventfd != VIR_TRISTATE_SWITCH_ABSENT) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("ioeventfd is not supported with vhostuser disk")); > + return -1; > + } > + > + if (disk->copy_on_read) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("copy_on_read is not supported with vhostuser disk")); > + return -1; > + } > + > + if (disk->discard) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("discard is not supported with vhostuser disk")); > + return -1; > + } > + > + if (disk->iothread) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("iothread is not supported with vhostuser disk")); > + return -1; > + } > + > + if (disk->detect_zeroes) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("detect_zeroes is not supported with vhostuser disk")); > + return -1; > + } > + > + if (disk->src->metadataCacheMaxSize > 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("metadata_cache is not supported with vhostuser disk")); > + return -1; > + } > + > + /* Unsupported driver elements */ s/driver/virtio/ ?

> + > + if (disk->virtio) { > + if (disk->virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("iommu is not supported with vhostuser disk")); > + return -1; > + } > + > + if (disk->virtio->ats != VIR_TRISTATE_SWITCH_ABSENT) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("ats is not supported with vhostuser disk")); > + return -1; > + } > + > + if (disk->virtio->packed != VIR_TRISTATE_SWITCH_ABSENT) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("packed is not supported with vhostuser disk")); > + return -1; > + } > + } [...] > diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c > index 6456100170..f6e81a7503 100644 > --- a/src/qemu/qemu_block.c > +++ b/src/qemu/qemu_block.c > @@ -1173,6 +1173,9 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src, > return NULL; > break; > > + case VIR_STORAGE_TYPE_VHOST_USER: > + break; This case must return NULL and an error per API contract of the function.

> + > case VIR_STORAGE_TYPE_VOLUME: > virReportError(VIR_ERR_INTERNAL_ERROR, > _("storage source pool '%s' volume '%s' is not translated"),