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>

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;
     }
 

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) &&
+    if (src->auth && !skipAuth && src->type != VIR_STORAGE_TYPE_VOLUME)
-        src->type != VIR_STORAGE_TYPE_VOLUME)
         virStorageAuthDefFormat(&childBuf, src->auth);
 
-    /* If we found encryption as a child of <source>, then format it
+    if (src->encryption && !skipEnc &&
-     * as we found it. */
-    if (src->encryption && (src->encryptionInherited || formatsecrets) &&
         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,

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

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,
+    if (virDomainDiskSourceFormat(buf, disk->src, "source", 0, false, 0,
-                                  xmlopt) < 0)
+                                  false, false, xmlopt) < 0)
         return -1;
 
     virBufferAdjustIndent(buf, -2);

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);

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.

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 */

tests/qemublocktest.c

@@ -111,8 +111,8 @@ testBackingXMLjsonXML(const void *args)
         return -1;
     }
 
-    if (virDomainDiskSourceFormat(&buf, jsonsrc, "source", 0, false, 0, true,
+    if (virDomainDiskSourceFormat(&buf, jsonsrc, "source", 0, false, 0,
-                                  NULL) < 0 ||
+                                  false, false, NULL) < 0 ||
         !(actualxml = virBufferContentAndReset(&buf))) {
         fprintf(stderr, "failed to format disk source xml\n");
         return -1;

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;