conf: Sanitize handling of <auth> and <encryption> placement for disks

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@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
index c5677cdb058b8d1438310dc8a8c86b57a9723d09..c0c6f25d10e3e4cf2911aa1bbfcc6f96e890ea80 100644 (file)
--- 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 83748354b0cda36b33b481cc7bb455f7408c5ce7..84eadb56593f6b3d2c902106d97352be71958f6e 100644 (file)
--- 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 4afd8f04bc3b35de226ddd9708e67aed89c23298..ddc75d8de26706207ff72bc454978fc49b56809a 100644 (file)
--- 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 37b5c2fdf7adb8425a9909d80d1b81c78f069c81..98c296b2764454e30bac1921533a0981ab49d7ae 100644 (file)
--- 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 800aa44f750c17d6eaa3ad97957d62e66d27d344..a1b250fd0b8ce7a91490cf364a1a76deacc9ff66 100644 (file)
--- 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 ff9a6c66637c3381da405308716301d89eec89d6..535a94e2398fdab71ad9927802a4de02f6a799de 100644 (file)
--- 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 d56d5c45e3bc1379636f8776b9505d2adc48cff8..c68bdc9680630f980c91a81e8b2a9b0193292d84 100644 (file)
--- 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 82c11311e2de95f1aee0d6a199006b326db479a2..8a715d19895c4c054d4d4b670038e79253ca8929 100644 (file)
--- 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 ac1480de4e659e58518384c43f132381565d46f2..98f47f0e41bab9473758e9118dd129194bace6b9 100644 (file)
--- 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;