Use proper enum type and refactor the formatter accordingly. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 12 ++++-------- src/conf/storage_source_conf.h | 2 +- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4435ee2ad4..d7f167a469 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7011,7 +7011,6 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, { virStorageSourcePoolDef *source; int ret = -1; - g_autofree char *mode = NULL; *srcpool = NULL; @@ -7019,7 +7018,6 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, source->pool = virXMLPropString(node, "pool"); source->volume = virXMLPropString(node, "volume"); - mode = virXMLPropString(node, "mode"); /* CD-ROM and Floppy allows no source */ if (!source->pool && !source->volume) { @@ -7033,13 +7031,11 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, goto cleanup; } - if (mode && - (source->mode = virStorageSourcePoolModeTypeFromString(mode)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown source mode '%1$s' for volume type disk"), - mode); + if (virXMLPropEnum(node, "mode", + virStorageSourcePoolModeTypeFromString, + VIR_XML_PROP_NONZERO, + &source->mode) < 0) goto cleanup; - } *srcpool = g_steal_pointer(&source); ret = 0; diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h index fc6c67f426..bfa8d625e5 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -201,7 +201,7 @@ struct _virStorageSourcePoolDef { int voltype; /* virStorageVolType, internal only */ int pooltype; /* virStoragePoolType from storage_conf.h, internal only */ virStorageType actualtype; /* internal only */ - int mode; /* virStorageSourcePoolMode, currently makes sense only for iscsi pool */ + virStorageSourcePoolMode mode; /* currently makes sense only for iscsi pool */ }; -- 2.41.0

Register autoptr cleanup function for virStorageSourcePoolDef and refactor the parser to simplify the logic. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 35 +++++++++++----------------------- src/conf/storage_source_conf.h | 1 + 2 files changed, 12 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d7f167a469..3e0989e2e8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7005,44 +7005,31 @@ virDomainLeaseDefParseXML(xmlNodePtr node, return NULL; } -static int -virDomainDiskSourcePoolDefParse(xmlNodePtr node, - virStorageSourcePoolDef **srcpool) +static virStorageSourcePoolDef * +virDomainDiskSourcePoolDefParse(xmlNodePtr node) { - virStorageSourcePoolDef *source; - int ret = -1; - - *srcpool = NULL; - - source = g_new0(virStorageSourcePoolDef, 1); + g_autoptr(virStorageSourcePoolDef) source = g_new0(virStorageSourcePoolDef, 1); source->pool = virXMLPropString(node, "pool"); source->volume = virXMLPropString(node, "volume"); - /* CD-ROM and Floppy allows no source */ - if (!source->pool && !source->volume) { - ret = 0; - goto cleanup; - } + /* CD-ROM and Floppy allows no source -> empty pool */ + if (!source->pool && !source->volume) + return g_steal_pointer(&source); if (!source->pool || !source->volume) { virReportError(VIR_ERR_XML_ERROR, "%s", _("'pool' and 'volume' must be specified together for 'pool' type source")); - goto cleanup; + return NULL; } if (virXMLPropEnum(node, "mode", virStorageSourcePoolModeTypeFromString, VIR_XML_PROP_NONZERO, &source->mode) < 0) - goto cleanup; - - *srcpool = g_steal_pointer(&source); - ret = 0; + return NULL; - cleanup: - virStorageSourcePoolDefFree(source); - return ret; + return g_steal_pointer(&source); } @@ -7482,7 +7469,7 @@ virDomainStorageSourceParse(xmlNodePtr node, return -1; break; case VIR_STORAGE_TYPE_VOLUME: - if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0) + if (!(src->srcpool = virDomainDiskSourcePoolDefParse(node))) return -1; break; case VIR_STORAGE_TYPE_NVME: @@ -8660,7 +8647,7 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt, units = virXMLPropString(source_node, "units"); } else if (def->type == VIR_DOMAIN_FS_TYPE_VOLUME) { def->src->type = VIR_STORAGE_TYPE_VOLUME; - if (virDomainDiskSourcePoolDefParse(source_node, &def->src->srcpool) < 0) + if (!(def->src->srcpool = virDomainDiskSourcePoolDefParse(source_node))) goto error; } } diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h index bfa8d625e5..0cd5cd0192 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -498,6 +498,7 @@ virStorageSourceInitChainElement(virStorageSource *newelem, void virStorageSourcePoolDefFree(virStorageSourcePoolDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSourcePoolDef, virStorageSourcePoolDefFree); void virStorageSourceClear(virStorageSource *def); -- 2.41.0

If a disk definition was already translated re-doing it makes no sense. Skip the translation if the 'actualtype' is already populated. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e0989e2e8..e128457b00 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30529,7 +30529,7 @@ virDomainDiskTranslateSourcePool(virDomainDiskDef *def) virStorageSource *n; for (n = def->src; virStorageSourceIsBacking(n); n = n->backingStore) { - if (n->type != VIR_STORAGE_TYPE_VOLUME || !n->srcpool) + if (n->type != VIR_STORAGE_TYPE_VOLUME || !n->srcpool || n->srcpool->actualtype != VIR_STORAGE_TYPE_NONE) continue; if (!conn) { -- 2.41.0

Re-translating a disk type='volume' definition from a storage pool is not a good idea in cases when the volume might have changed or we might not have access to the storage driver. Specific cases are if a storage pool is not activated on daemon restart, then re-connecting to a VM fails, or if the virt-aa-helper program tries to setup labelling for apparmor. Add a new flag which will preserve the translated data in the definition. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 69 ++++++++++++++++++++++++++++++------------ src/conf/domain_conf.h | 4 +++ 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e128457b00..9b636215e9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7006,7 +7006,8 @@ virDomainLeaseDefParseXML(xmlNodePtr node, } static virStorageSourcePoolDef * -virDomainDiskSourcePoolDefParse(xmlNodePtr node) +virDomainDiskSourcePoolDefParse(xmlNodePtr node, + virDomainDefParseFlags flags) { g_autoptr(virStorageSourcePoolDef) source = g_new0(virStorageSourcePoolDef, 1); @@ -7029,6 +7030,14 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node) &source->mode) < 0) return NULL; + if (flags & VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED) { + if (virXMLPropEnum(node, "actualType", + virStorageTypeFromString, + VIR_XML_PROP_NONZERO, + &source->actualtype) < 0) + return NULL; + } + return g_steal_pointer(&source); } @@ -7448,12 +7457,22 @@ virDomainStorageSourceParse(xmlNodePtr node, unsigned int flags, virDomainXMLOption *xmlopt) { + virStorageType actualType = src->type; VIR_XPATH_NODE_AUTORESTORE(ctxt) xmlNodePtr tmp; ctxt->node = node; - switch (src->type) { + if (src->type == VIR_STORAGE_TYPE_VOLUME) { + if (!(src->srcpool = virDomainDiskSourcePoolDefParse(node, flags))) + return -1; + + /* If requested we need to also parse the translated volume runtime data */ + if (flags & VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED) + actualType = virStorageSourceGetActualType(src); + } + + switch (actualType) { case VIR_STORAGE_TYPE_FILE: src->path = virXMLPropString(node, "file"); src->fdgroup = virXMLPropString(node, "fdgroup"); @@ -7469,8 +7488,7 @@ virDomainStorageSourceParse(xmlNodePtr node, return -1; break; case VIR_STORAGE_TYPE_VOLUME: - if (!(src->srcpool = virDomainDiskSourcePoolDefParse(node))) - return -1; + /* parsed above */ break; case VIR_STORAGE_TYPE_NVME: if (virDomainDiskSourceNVMeParse(node, ctxt, src) < 0) @@ -7488,7 +7506,7 @@ virDomainStorageSourceParse(xmlNodePtr node, case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk type %1$s"), - virStorageTypeToString(src->type)); + virStorageTypeToString(actualType)); return -1; } @@ -8647,7 +8665,7 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt, units = virXMLPropString(source_node, "units"); } else if (def->type == VIR_DOMAIN_FS_TYPE_VOLUME) { def->src->type = VIR_STORAGE_TYPE_VOLUME; - if (!(def->src->srcpool = virDomainDiskSourcePoolDefParse(source_node))) + if (!(def->src->srcpool = virDomainDiskSourcePoolDefParse(source_node, flags))) goto error; } } @@ -22335,10 +22353,28 @@ virDomainDiskSourceFormat(virBuffer *buf, bool skipEnc, virDomainXMLOption *xmlopt) { + virStorageType actualType = src->type; g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); - switch (src->type) { + if (src->type == VIR_STORAGE_TYPE_VOLUME) { + if (src->srcpool) { + virBufferEscapeString(&attrBuf, " pool='%s'", src->srcpool->pool); + virBufferEscapeString(&attrBuf, " volume='%s'", src->srcpool->volume); + if (src->srcpool->mode) + virBufferAsprintf(&attrBuf, " mode='%s'", + virStorageSourcePoolModeTypeToString(src->srcpool->mode)); + } + + if (flags & VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED && + src->srcpool->actualtype != VIR_STORAGE_TYPE_NONE) { + virBufferAsprintf(&attrBuf, " actualType='%s'", + virStorageTypeToString(src->srcpool->actualtype)); + actualType = virStorageSourceGetActualType(src); + } + } + + switch (actualType) { case VIR_STORAGE_TYPE_FILE: virBufferEscapeString(&attrBuf, " file='%s'", src->path); virBufferEscapeString(&attrBuf, " fdgroup='%s'", src->fdgroup); @@ -22357,15 +22393,7 @@ virDomainDiskSourceFormat(virBuffer *buf, break; case VIR_STORAGE_TYPE_VOLUME: - if (src->srcpool) { - virBufferEscapeString(&attrBuf, " pool='%s'", src->srcpool->pool); - virBufferEscapeString(&attrBuf, " volume='%s'", - src->srcpool->volume); - if (src->srcpool->mode) - virBufferAsprintf(&attrBuf, " mode='%s'", - virStorageSourcePoolModeTypeToString(src->srcpool->mode)); - } - + /* formatted above */ break; case VIR_STORAGE_TYPE_NVME: @@ -22383,13 +22411,13 @@ virDomainDiskSourceFormat(virBuffer *buf, case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk type %1$d"), src->type); + _("unexpected disk type %1$d"), actualType); return -1; } virDomainDiskSourceFormatSlices(&childBuf, src); - if (src->type != VIR_STORAGE_TYPE_NETWORK) + if (actualType != VIR_STORAGE_TYPE_NETWORK) virDomainSourceDefFormatSeclabel(&childBuf, src->nseclabels, src->seclabels, flags); @@ -22408,7 +22436,7 @@ virDomainDiskSourceFormat(virBuffer *buf, if (src->pr) virStoragePRDefFormat(&childBuf, src->pr, flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE); - if (policy && src->type != VIR_STORAGE_TYPE_NETWORK) + if (policy && actualType != VIR_STORAGE_TYPE_NETWORK) virBufferEscapeString(&attrBuf, " startupPolicy='%s'", virDomainStartupPolicyTypeToString(policy)); @@ -27512,7 +27540,8 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, VIR_DOMAIN_DEF_FORMAT_STATUS | VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET | VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES | - VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST, + VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST | + VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED, -1); if (!(type = virDomainVirtTypeToString(def->virtType))) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 16289789c2..9e6dd930fa 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3734,6 +3734,8 @@ typedef enum { * post parse callbacks before starting. Failure of the post parse callback * is recorded as def->postParseFail */ VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL = 1 << 11, + /* Parse the translated disk type='volume' data if present */ + VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED = 1 << 12, } virDomainDefParseFlags; typedef enum { @@ -3749,6 +3751,8 @@ typedef enum { VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM = 1 << 6, VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT = 1 << 7, VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST = 1 << 8, + /* format disk type='volume' translated data if present */ + VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED = 1 << 9, } virDomainDefFormatFlags; /* Use these flags to skip specific domain ABI consistency checks done -- 2.41.0

Enable the flags in the status xml2xmtest and add an exaple to the test data. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/qemustatusxml2xmldata/modern-in.xml | 4 ++-- tests/qemustatusxml2xmltest.c | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/qemustatusxml2xmldata/modern-in.xml b/tests/qemustatusxml2xmldata/modern-in.xml index e139c8d38c..67e0aa4952 100644 --- a/tests/qemustatusxml2xmldata/modern-in.xml +++ b/tests/qemustatusxml2xmldata/modern-in.xml @@ -378,9 +378,9 @@ <target dev='vdc' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> </disk> - <disk type='file' device='cdrom'> + <disk type='volume' device='cdrom'> <driver name='qemu' type='raw'/> - <source file='/var/lib/libvirt/images/systemrescuecd-x86-4.9.5.iso'/> + <source pool='testpool' volume='testvolume' actualType='file' file='/var/lib/libvirt/images/systemrescuecd-x86-4.9.5.iso'/> <backingStore/> <target dev='hda' bus='ide'/> <readonly/> diff --git a/tests/qemustatusxml2xmltest.c b/tests/qemustatusxml2xmltest.c index 418a724b94..f1589345c3 100644 --- a/tests/qemustatusxml2xmltest.c +++ b/tests/qemustatusxml2xmltest.c @@ -30,7 +30,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque) VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | - VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL))) { + VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL | + VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED))) { VIR_TEST_DEBUG("\nfailed to parse '%s'", data->infile); goto cleanup; } @@ -40,7 +41,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque) VIR_DOMAIN_DEF_FORMAT_STATUS | VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET | VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES | - VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST))) { + VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST | + VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED))) { VIR_TEST_DEBUG("\nfailed to format back '%s'", data->infile); goto cleanup; } -- 2.41.0

Re-translating the disk source pools when reconnecting to a VM makes no sense as the volume might have changed or pool became inactive. The VM still uses the original volume though. Failing to re-translate the pool also causes the VM to be killed. Fix this by storing the original translation in the status XML. Resolves: https://issues.redhat.com/browse/RHEL-7345 Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 3 ++- src/conf/virdomainobjlist.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9b636215e9..80f467ae7a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28247,7 +28247,8 @@ virDomainObjSave(virDomainObj *obj, VIR_DOMAIN_DEF_FORMAT_STATUS | VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET | VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES | - VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST); + VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST | + VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED); g_autofree char *xml = NULL; diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 6be5de5e2b..0bd833257d 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -532,7 +532,8 @@ virDomainObjListLoadStatus(virDomainObjList *doms, VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | - VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL))) + VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL | + VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED))) goto error; virUUIDFormat(obj->def->uuid, uuidstr); -- 2.41.0

The 'virt-aa-helper' process gets a XML of the VM it needs to create a profile for. For a disk type='volume' this XML contained only the pool and volume name. The 'virt-aa-helper' needs a local path though for anything it needs to label. This means that we'd either need to invoke connection to the storage driver and re-resolve the volume. Alternative which makes more sense is to pass the proper data in the XML already passed to it via the new XML formatter and parser flags. This was indirectly reported upstream in https://gitlab.com/libvirt/libvirt/-/issues/546 The configuration in the issue above was created by Cockpit on Debian. Since Cockpit is getting more popular it's more likely that users will be impacted by this problem. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/security/security_apparmor.c | 8 ++++++-- src/security/virt-aa-helper.c | 3 ++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index bce797de7c..6fd0aedacf 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -159,13 +159,17 @@ load_profile(virSecurityManager *mgr G_GNUC_UNUSED, bool append) { bool create = true; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; g_autofree char *xml = NULL; g_autoptr(virCommand) cmd = NULL; - xml = virDomainDefFormat(def, NULL, VIR_DOMAIN_DEF_FORMAT_SECURE); - if (!xml) + if (virDomainDefFormatInternal(def, NULL, &buf, + VIR_DOMAIN_DEF_FORMAT_SECURE | + VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED) < 0) return -1; + xml = virBufferContentAndReset(&buf); + if (profile_status_file(profile) >= 0) create = false; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 0855eb68ca..be13979cef 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -654,7 +654,8 @@ get_definition(vahControl * ctl, const char *xmlStr) ctl->def = virDomainDefParseString(xmlStr, ctl->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | + VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED); if (ctl->def == NULL) { vah_error(ctl, 0, _("could not parse XML")); -- 2.41.0