[PATCH 1/2] conf: Sanitize handling of <auth> and <encryption> placement for disks
Peter Krempa
pkrempa at redhat.com
Thu May 7 13:26:54 UTC 2020
Modern way to store <auth> and <encryption> of a <disk> is under
<source>. This was added to mirror how <backingStore> handles these and
in fact they are relevant to the source rather than to any other part of
the disk. Historically we allowed them to be directly under <disk> and
we need to keep compatibility.
This wasn't a problem until introduction of -blockdev in qemu using of
<auth> or <encryption> plainly wouldn't work with backing chains.
Now that it works in backing chains and can be moved back and forth
using snapshots/block-commit we need to ensure that the original
placement is properly kept even if the source changes.
To achieve the above semantics we need to store the preferred placement
with the disk definition rather than the storage source definitions and
also ensure that the modern way is chosen when the VM started with
<source/encryption> only in the backing store.
https://bugzilla.redhat.com/show_bug.cgi?id=1822878
Signed-off-by: Peter Krempa <pkrempa at redhat.com>
---
src/conf/backup_conf.c | 3 +-
src/conf/domain_conf.c | 97 ++++++++++++++++-----------------------
src/conf/domain_conf.h | 6 ++-
src/conf/snapshot_conf.c | 4 +-
src/qemu/qemu_domain.c | 6 ++-
src/util/virstoragefile.c | 1 -
src/util/virstoragefile.h | 2 -
tests/qemublocktest.c | 4 +-
tests/virstoragetest.c | 3 +-
9 files changed, 57 insertions(+), 69 deletions(-)
diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
index c5677cdb05..c0c6f25d10 100644
--- a/src/conf/backup_conf.c
+++ b/src/conf/backup_conf.c
@@ -349,7 +349,8 @@ virDomainBackupDiskDefFormat(virBufferPtr buf,
virStorageFileFormatTypeToString(disk->store->format));
if (virDomainDiskSourceFormat(&childBuf, disk->store, sourcename,
- 0, false, storageSourceFormatFlags, true, NULL) < 0)
+ 0, false, storageSourceFormatFlags,
+ false, false, NULL) < 0)
return -1;
}
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 83748354b0..84eadb5659 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10469,31 +10469,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
if (virDomainStorageSourceParse(cur, ctxt, def->src, flags, xmlopt) < 0)
goto error;
- /* If we've already found an <auth> as a child of <disk> and
- * we find one as a child of <source>, then force an error to
- * avoid ambiguity */
- if (authdef && def->src->auth) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("an <auth> definition already found for "
- "the <disk> definition"));
- goto error;
- }
-
- if (def->src->auth)
- def->src->authInherited = true;
-
- /* Similarly for <encryption> - it's a child of <source> too
- * and we cannot find in both places */
- if (encryption && def->src->encryption) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("an <encryption> definition already found for "
- "the <disk> definition"));
- goto error;
- }
-
- if (def->src->encryption)
- def->src->encryptionInherited = true;
-
source = true;
startupPolicy = virXMLPropString(cur, "startupPolicy");
@@ -10558,17 +10533,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
} else if (!authdef &&
virXMLNodeNameEqual(cur, "auth")) {
- /* If we've already parsed <source> and found an <auth> child,
- * then generate an error to avoid ambiguity */
- if (def->src->authInherited) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("an <auth> definition already found for "
- "disk source"));
- goto error;
- }
-
if (!(authdef = virStorageAuthDefParse(cur, ctxt)))
goto error;
+ def->diskElementAuth = true;
} else if (virXMLNodeNameEqual(cur, "iotune")) {
if (virDomainDiskDefIotuneParse(def, ctxt) < 0)
goto error;
@@ -10580,17 +10547,11 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
def->transient = true;
} else if (!encryption &&
virXMLNodeNameEqual(cur, "encryption")) {
- /* If we've already parsed <source> and found an <encryption> child,
- * then generate an error to avoid ambiguity */
- if (def->src->encryptionInherited) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("an <encryption> definition already found for "
- "disk source"));
- goto error;
- }
-
if (!(encryption = virStorageEncryptionParseNode(cur, ctxt)))
goto error;
+
+ def->diskElementEnc = true;
+
} else if (!serial &&
virXMLNodeNameEqual(cur, "serial")) {
serial = (char *)xmlNodeGetContent(cur);
@@ -10783,10 +10744,31 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
}
def->dst = g_steal_pointer(&target);
- if (authdef)
+ if (authdef) {
+ /* If we've already parsed <source> and found an <auth> child,
+ * then generate an error to avoid ambiguity */
+ if (def->src->auth) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("an <auth> definition already found for "
+ "disk source"));
+ goto error;
+ }
+
def->src->auth = g_steal_pointer(&authdef);
- if (encryption)
+ }
+
+ if (encryption) {
+ /* If we've already parsed <source> and found an <encryption> child,
+ * then generate an error to avoid ambiguity */
+ if (def->src->encryption) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("an <encryption> definition already found for "
+ "disk source"));
+ goto error;
+ }
+
def->src->encryption = g_steal_pointer(&encryption);
+ }
def->domain_name = g_steal_pointer(&domain_name);
def->serial = g_steal_pointer(&serial);
def->wwn = g_steal_pointer(&wwn);
@@ -25044,7 +25026,8 @@ virDomainDiskSourceFormatSlices(virBufferPtr buf,
* @policy: startup policy attribute value, if necessary
* @attrIndex: the 'index' attribute of <source> is formatted if true
* @flags: XML formatter flags
- * @formatsecrets: Force formatting of <auth> and <encryption> under <source>
+ * @skipAuth: Skip formatting of <auth>
+ * @skipEnc: Skip formatting of <encryption>
* regardless of the original definition state
* @xmlopt: XML formatter callbacks
*
@@ -25058,7 +25041,8 @@ virDomainDiskSourceFormat(virBufferPtr buf,
int policy,
bool attrIndex,
unsigned int flags,
- bool formatsecrets,
+ bool skipAuth,
+ bool skipEnc,
virDomainXMLOptionPtr xmlopt)
{
g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
@@ -25117,13 +25101,10 @@ virDomainDiskSourceFormat(virBufferPtr buf,
* <auth> for a volume source type. The <auth> information is
* kept in the storage pool and would be overwritten anyway.
* So avoid formatting it for volumes. */
- if (src->auth && (src->authInherited || formatsecrets) &&
- src->type != VIR_STORAGE_TYPE_VOLUME)
+ if (src->auth && !skipAuth && src->type != VIR_STORAGE_TYPE_VOLUME)
virStorageAuthDefFormat(&childBuf, src->auth);
- /* If we found encryption as a child of <source>, then format it
- * as we found it. */
- if (src->encryption && (src->encryptionInherited || formatsecrets) &&
+ if (src->encryption && !skipEnc &&
virStorageEncryptionFormat(&childBuf, src->encryption) < 0)
return -1;
@@ -25184,7 +25165,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf,
virBufferAsprintf(&childBuf, "<format type='%s'/>\n",
virStorageFileFormatTypeToString(backingStore->format));
if (virDomainDiskSourceFormat(&childBuf, backingStore, "source", 0, false,
- flags, true, xmlopt) < 0)
+ flags, false, false, xmlopt) < 0)
return -1;
if (virDomainDiskBackingStoreFormat(&childBuf, backingStore, xmlopt, flags) < 0)
@@ -25346,7 +25327,7 @@ virDomainDiskDefFormatMirror(virBufferPtr buf,
virBufferEscapeString(&childBuf, "<format type='%s'/>\n", formatStr);
if (virDomainDiskSourceFormat(&childBuf, disk->mirror, "source", 0, true,
- flags, true, xmlopt) < 0)
+ flags, false, false, xmlopt) < 0)
return -1;
if (virDomainDiskBackingStoreFormat(&childBuf, disk->mirror, xmlopt, flags) < 0)
@@ -25441,11 +25422,13 @@ virDomainDiskDefFormat(virBufferPtr buf,
/* Format as child of <disk> if defined there; otherwise,
* if defined as child of <source>, then format later */
- if (def->src->auth && !def->src->authInherited)
+ if (def->src->auth && def->diskElementAuth)
virStorageAuthDefFormat(buf, def->src->auth);
if (virDomainDiskSourceFormat(buf, def->src, "source", def->startupPolicy,
- true, flags, false, xmlopt) < 0)
+ true, flags,
+ def->diskElementAuth, def->diskElementEnc,
+ xmlopt) < 0)
return -1;
/* Don't format backingStore to inactive XMLs until the code for
@@ -25491,7 +25474,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
/* If originally found as a child of <disk>, then format thusly;
* otherwise, will be formatted as child of <source> */
- if (def->src->encryption && !def->src->encryptionInherited &&
+ if (def->src->encryption && def->diskElementEnc &&
virStorageEncryptionFormat(buf, def->src->encryption) < 0)
return -1;
if (virDomainDeviceInfoFormat(buf, &def->info,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 4afd8f04bc..ddc75d8de2 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -579,6 +579,9 @@ struct _virDomainDiskDef {
unsigned int queues;
int model; /* enum virDomainDiskModel */
virDomainVirtioOptionsPtr virtio;
+
+ bool diskElementAuth;
+ bool diskElementEnc;
};
@@ -3233,7 +3236,8 @@ int virDomainDiskSourceFormat(virBufferPtr buf,
int policy,
bool attrIndex,
unsigned int flags,
- bool formatsecrets,
+ bool skipAuth,
+ bool skipEnc,
virDomainXMLOptionPtr xmlopt);
int
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 37b5c2fdf7..98c296b276 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -818,8 +818,8 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
if (disk->src->format > 0)
virBufferEscapeString(buf, "<driver type='%s'/>\n",
virStorageFileFormatTypeToString(disk->src->format));
- if (virDomainDiskSourceFormat(buf, disk->src, "source", 0, false, 0, true,
- xmlopt) < 0)
+ if (virDomainDiskSourceFormat(buf, disk->src, "source", 0, false, 0,
+ false, false, xmlopt) < 0)
return -1;
virBufferAdjustIndent(buf, -2);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9c629c31a3..a22cb4ed30 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2603,7 +2603,8 @@ qemuDomainObjPrivateXMLFormatBlockjobFormatSource(virBufferPtr buf,
virStorageTypeToString(src->type),
virStorageFileFormatTypeToString(src->format));
- if (virDomainDiskSourceFormat(&childBuf, src, "source", 0, true, xmlflags, true, xmlopt) < 0)
+ if (virDomainDiskSourceFormat(&childBuf, src, "source", 0, true, xmlflags,
+ false, false, xmlopt) < 0)
return -1;
if (chain &&
@@ -2823,7 +2824,8 @@ qemuDomainObjPrivateXMLFormatNBDMigrationSource(virBufferPtr buf,
virStorageFileFormatTypeToString(src->format));
if (virDomainDiskSourceFormat(&childBuf, src, "source", 0, false,
- VIR_DOMAIN_DEF_FORMAT_STATUS, true, xmlopt) < 0)
+ VIR_DOMAIN_DEF_FORMAT_STATUS,
+ false, false, xmlopt) < 0)
return -1;
virXMLFormatElement(buf, "migrationSource", &attrBuf, &childBuf);
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index ff9a6c6663..535a94e239 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2999,7 +2999,6 @@ virStorageSourceParseRBDColonString(const char *rbdstr,
authdef->secrettype = g_strdup(virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH));
src->auth = g_steal_pointer(&authdef);
- src->authInherited = true;
/* Cannot formulate a secretType (eg, usage or uuid) given
* what is provided.
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index d56d5c45e3..c68bdc9680 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -291,9 +291,7 @@ struct _virStorageSource {
virStorageNetCookieDefPtr *cookies;
virStorageSourcePoolDefPtr srcpool;
virStorageAuthDefPtr auth;
- bool authInherited;
virStorageEncryptionPtr encryption;
- bool encryptionInherited;
virStoragePRDefPtr pr;
virTristateBool sslverify;
/* both values below have 0 as default value */
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index 8001807552..9ac506c78b 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -111,8 +111,8 @@ testBackingXMLjsonXML(const void *args)
return -1;
}
- if (virDomainDiskSourceFormat(&buf, jsonsrc, "source", 0, false, 0, true,
- NULL) < 0 ||
+ if (virDomainDiskSourceFormat(&buf, jsonsrc, "source", 0, false, 0,
+ false, false, NULL) < 0 ||
!(actualxml = virBufferContentAndReset(&buf))) {
fprintf(stderr, "failed to format disk source xml\n");
return -1;
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index ac1480de4e..98f47f0e41 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -614,7 +614,8 @@ testBackingParse(const void *args)
return -1;
}
- if (virDomainDiskSourceFormat(&buf, src, "source", 0, false, xmlformatflags, true, NULL) < 0 ||
+ if (virDomainDiskSourceFormat(&buf, src, "source", 0, false, xmlformatflags,
+ false, false, NULL) < 0 ||
!(xml = virBufferContentAndReset(&buf))) {
fprintf(stderr, "failed to format disk source xml\n");
return -1;
--
2.26.2
More information about the libvir-list
mailing list