conf: move seclabel validation into post-parse phase

Currently the disk and chardev seclabels are validated immediately at
the time their data is parsed. This forces the parser to fill in the
top level secmodel at time of parsing which is an undesirable thing.
This validation conceptually should be done in the post-parse phase
instead.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

src/conf/domain_conf.c

@@ -5944,8 +5944,42 @@ virDomainDiskAddressDiskBusCompatibility(virDomainDiskBus bus,
 
 
 static int
-virDomainDiskDefValidate(const virDomainDiskDef *disk)
+virSecurityDeviceLabelDefValidateXML(virSecurityDeviceLabelDefPtr *seclabels,
+                                     size_t nseclabels,
+                                     virSecurityLabelDefPtr *vmSeclabels,
+                                     size_t nvmSeclabels)
 {
+    virSecurityDeviceLabelDefPtr seclabel;
+    size_t i;
+    size_t j;
+
+    for (i = 0; i < nseclabels; i++) {
+        seclabel = seclabels[i];
+
+        /* find the security label that it's being overridden */
+        for (j = 0; j < nvmSeclabels; j++) {
+            if (STRNEQ_NULLABLE(vmSeclabels[j]->model, seclabel->model))
+                continue;
+
+            if (!vmSeclabels[j]->relabel) {
+                virReportError(VIR_ERR_XML_ERROR, "%s",
+                               _("label overrides require relabeling to be "
+                                 "enabled at the domain level"));
+                return -1;
+            }
+        }
+    }
+
+    return 0;
+}
+
+
+static int
+virDomainDiskDefValidate(const virDomainDef *def,
+                         const virDomainDiskDef *disk)
+{
+    virStorageSourcePtr next;
+
     /* Validate LUN configuration */
     if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
         /* volumes haven't been translated at this point, so accept them */
@@ -5999,6 +6033,14 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk)
         return -1;
     }
 
+    for (next = disk->src; next; next = next->backingStore) {
+        if (virSecurityDeviceLabelDefValidateXML(next->seclabels,
+                                                 next->nseclabels,
+                                                 def->seclabels,
+                                                 def->nseclabels) < 0)
+            return -1;
+    }
+
     return 0;
 }
 
@@ -6022,10 +6064,11 @@ virDomainDefHasUSB(const virDomainDef *def)
 
 
 static int
-virDomainChrSourceDefValidate(const virDomainChrSourceDef *def,
+virDomainChrSourceDefValidate(const virDomainChrSourceDef *src_def,
-                              const virDomainChrDef *chr_def)
+                              const virDomainChrDef *chr_def,
+                              const virDomainDef *def)
 {
-    switch ((virDomainChrType) def->type) {
+    switch ((virDomainChrType) src_def->type) {
     case VIR_DOMAIN_CHR_TYPE_NULL:
     case VIR_DOMAIN_CHR_TYPE_PTY:
     case VIR_DOMAIN_CHR_TYPE_VC:
@@ -6037,7 +6080,7 @@ virDomainChrSourceDefValidate(const virDomainChrSourceDef *def,
     case VIR_DOMAIN_CHR_TYPE_FILE:
     case VIR_DOMAIN_CHR_TYPE_DEV:
     case VIR_DOMAIN_CHR_TYPE_PIPE:
-        if (!def->data.file.path) {
+        if (!src_def->data.file.path) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("Missing source path attribute for char device"));
             return -1;
@@ -6045,13 +6088,13 @@ virDomainChrSourceDefValidate(const virDomainChrSourceDef *def,
         break;
 
     case VIR_DOMAIN_CHR_TYPE_NMDM:
-        if (!def->data.nmdm.master) {
+        if (!src_def->data.nmdm.master) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("Missing master path attribute for nmdm device"));
             return -1;
         }
 
-        if (!def->data.nmdm.slave) {
+        if (!src_def->data.nmdm.slave) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("Missing slave path attribute for nmdm device"));
             return -1;
@@ -6059,19 +6102,19 @@ virDomainChrSourceDefValidate(const virDomainChrSourceDef *def,
         break;
 
     case VIR_DOMAIN_CHR_TYPE_TCP:
-        if (!def->data.tcp.host) {
+        if (!src_def->data.tcp.host) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("Missing source host attribute for char device"));
             return -1;
         }
 
-        if (!def->data.tcp.service) {
+        if (!src_def->data.tcp.service) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("Missing source service attribute for char device"));
             return -1;
         }
 
-        if (def->data.tcp.listen && def->data.tcp.reconnect.enabled) {
+        if (src_def->data.tcp.listen && src_def->data.tcp.reconnect.enabled) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("chardev reconnect is possible only for connect mode"));
             return -1;
@@ -6079,7 +6122,7 @@ virDomainChrSourceDefValidate(const virDomainChrSourceDef *def,
         break;
 
     case VIR_DOMAIN_CHR_TYPE_UDP:
-        if (!def->data.udp.connectService) {
+        if (!src_def->data.udp.connectService) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("Missing source service attribute for char device"));
             return -1;
@@ -6090,7 +6133,7 @@ virDomainChrSourceDefValidate(const virDomainChrSourceDef *def,
         /* The source path can be auto generated for certain specific
          * types of channels, but in most cases we should report an
          * error if the user didn't provide it */
-        if (!def->data.nix.path &&
+        if (!src_def->data.nix.path &&
             !(chr_def &&
               chr_def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
               (chr_def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN ||
@@ -6100,7 +6143,7 @@ virDomainChrSourceDefValidate(const virDomainChrSourceDef *def,
             return -1;
         }
 
-        if (def->data.nix.listen && def->data.nix.reconnect.enabled) {
+        if (src_def->data.nix.listen && src_def->data.nix.reconnect.enabled) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("chardev reconnect is possible only for connect mode"));
             return -1;
@@ -6108,13 +6151,13 @@ virDomainChrSourceDefValidate(const virDomainChrSourceDef *def,
         break;
 
     case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
-        if (!def->data.spiceport.channel) {
+        if (!src_def->data.spiceport.channel) {
             virReportError(VIR_ERR_XML_ERROR, "%s",
                            _("Missing source channel attribute for char device"));
             return -1;
         }
-        if (strspn(def->data.spiceport.channel,
+        if (strspn(src_def->data.spiceport.channel,
-                   SERIAL_CHANNEL_NAME_CHARS) < strlen(def->data.spiceport.channel)) {
+                   SERIAL_CHANNEL_NAME_CHARS) < strlen(src_def->data.spiceport.channel)) {
             virReportError(VIR_ERR_INVALID_ARG, "%s",
                            _("Invalid character in source channel for char device"));
             return -1;
@@ -6122,6 +6165,12 @@ virDomainChrSourceDefValidate(const virDomainChrSourceDef *def,
         break;
     }
 
+    if (virSecurityDeviceLabelDefValidateXML(src_def->seclabels,
+                                             src_def->nseclabels,
+                                             def->seclabels,
+                                             def->nseclabels) < 0)
+        return -1;
+
     return 0;
 }
 
@@ -6138,7 +6187,7 @@ virDomainRedirdevDefValidate(const virDomainDef *def,
         return -1;
     }
 
-    return virDomainChrSourceDefValidate(redirdev->source, NULL);
+    return virDomainChrSourceDefValidate(redirdev->source, NULL, def);
 }
 
 
@@ -6261,27 +6310,30 @@ virDomainControllerDefValidate(const virDomainControllerDef *controller)
 
 
 static int
-virDomainChrDefValidate(const virDomainChrDef *chr)
+virDomainChrDefValidate(const virDomainChrDef *chr,
+                        const virDomainDef *def)
 {
-    return virDomainChrSourceDefValidate(chr->source, chr);
+    return virDomainChrSourceDefValidate(chr->source, chr, def);
 }
 
 
 static int
-virDomainSmartcardDefValidate(const virDomainSmartcardDef *smartcard)
+virDomainSmartcardDefValidate(const virDomainSmartcardDef *smartcard,
+                              const virDomainDef *def)
 {
     if (smartcard->type == VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH)
-        return virDomainChrSourceDefValidate(smartcard->data.passthru, NULL);
+        return virDomainChrSourceDefValidate(smartcard->data.passthru, NULL, def);
 
     return 0;
 }
 
 
 static int
-virDomainRNGDefValidate(const virDomainRNGDef *rng)
+virDomainRNGDefValidate(const virDomainRNGDef *rng,
+                        const virDomainDef *def)
 {
     if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD)
-        return virDomainChrSourceDefValidate(rng->source.chardev, NULL);
+        return virDomainChrSourceDefValidate(rng->source.chardev, NULL, def);
 
     return 0;
 }
@@ -6478,7 +6530,7 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev,
 {
     switch ((virDomainDeviceType) dev->type) {
     case VIR_DOMAIN_DEVICE_DISK:
-        return virDomainDiskDefValidate(dev->data.disk);
+        return virDomainDiskDefValidate(def, dev->data.disk);
 
     case VIR_DOMAIN_DEVICE_REDIRDEV:
         return virDomainRedirdevDefValidate(def, dev->data.redirdev);
@@ -6490,13 +6542,13 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev,
         return virDomainControllerDefValidate(dev->data.controller);
 
     case VIR_DOMAIN_DEVICE_CHR:
-        return virDomainChrDefValidate(dev->data.chr);
+        return virDomainChrDefValidate(dev->data.chr, def);
 
     case VIR_DOMAIN_DEVICE_SMARTCARD:
-        return virDomainSmartcardDefValidate(dev->data.smartcard);
+        return virDomainSmartcardDefValidate(dev->data.smartcard, def);
 
     case VIR_DOMAIN_DEVICE_RNG:
-        return virDomainRNGDefValidate(dev->data.rng);
+        return virDomainRNGDefValidate(dev->data.rng, def);
 
     case VIR_DOMAIN_DEVICE_HOSTDEV:
         return virDomainHostdevDefValidate(dev->data.hostdev);
@@ -9052,37 +9104,6 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn,
 }
 
 
-static int
-virSecurityDeviceLabelDefValidateXML(virSecurityDeviceLabelDefPtr *seclabels,
-                                     size_t nseclabels,
-                                     virSecurityLabelDefPtr *vmSeclabels,
-                                     size_t nvmSeclabels)
-{
-    virSecurityDeviceLabelDefPtr seclabel;
-    size_t i;
-    size_t j;
-
-    for (i = 0; i < nseclabels; i++) {
-        seclabel = seclabels[i];
-
-        /* find the security label that it's being overridden */
-        for (j = 0; j < nvmSeclabels; j++) {
-            if (STRNEQ_NULLABLE(vmSeclabels[j]->model, seclabel->model))
-                continue;
-
-            if (!vmSeclabels[j]->relabel) {
-                virReportError(VIR_ERR_XML_ERROR, "%s",
-                               _("label overrides require relabeling to be "
-                                 "enabled at the domain level"));
-                return -1;
-            }
-        }
-    }
-
-    return 0;
-}
-
-
 /* Parse the XML definition for a lease
  */
 static virDomainLeaseDefPtr
@@ -9711,9 +9732,7 @@ virDomainDiskSourceDefParseAuthValidate(const virStorageSource *src)
 
 
 static int
-virDomainDiskDefParseValidate(const virDomainDiskDef *def,
+virDomainDiskDefParseValidate(const virDomainDiskDef *def)
-                              virSecurityLabelDefPtr *vmSeclabels,
-                              size_t nvmSeclabels)
 
 {
     virStorageSourcePtr next;
@@ -9804,12 +9823,6 @@ virDomainDiskDefParseValidate(const virDomainDiskDef *def,
                 return -1;
             }
         }
-
-        if (virSecurityDeviceLabelDefValidateXML(next->seclabels,
-                                                 next->nseclabels,
-                                                 vmSeclabels,
-                                                 nvmSeclabels) < 0)
-            return -1;
     }
 
     return 0;
@@ -9966,8 +9979,6 @@ static virDomainDiskDefPtr
 virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
                          xmlNodePtr node,
                          xmlXPathContextPtr ctxt,
-                         virSecurityLabelDefPtr* vmSeclabels,
-                         int nvmSeclabels,
                          unsigned int flags)
 {
     virDomainDiskDefPtr def;
@@ -10370,7 +10381,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
         virDomainDiskDefParsePrivateData(ctxt, def, xmlopt) < 0)
         goto error;
 
-    if (virDomainDiskDefParseValidate(def, vmSeclabels, nvmSeclabels) < 0)
+    if (virDomainDiskDefParseValidate(def) < 0)
         goto error;
 
  cleanup:
@@ -12660,9 +12671,7 @@ static int
 virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
                               xmlNodePtr cur, unsigned int flags,
                               virDomainChrDefPtr chr_def,
-                              xmlXPathContextPtr ctxt,
+                              xmlXPathContextPtr ctxt)
-                              virSecurityLabelDefPtr* vmSeclabels,
-                              int nvmSeclabels)
 {
     bool logParsed = false;
     bool protocolParsed = false;
@@ -12745,11 +12754,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
                 if (virSecurityDeviceLabelDefParseXML(&def->seclabels,
                                                       &def->nseclabels,
                                                       ctxt,
-                                                      flags) < 0 ||
+                                                      flags) < 0) {
-                    virSecurityDeviceLabelDefValidateXML(def->seclabels,
-                                                         def->nseclabels,
-                                                         vmSeclabels,
-                                                         nvmSeclabels) < 0) {
                     ctxt->node = saved_node;
                     goto error;
                 }
@@ -12886,8 +12891,6 @@ static virDomainChrDefPtr
 virDomainChrDefParseXML(virDomainXMLOptionPtr xmlopt,
                         xmlXPathContextPtr ctxt,
                         xmlNodePtr node,
-                        virSecurityLabelDefPtr* vmSeclabels,
-                        int nvmSeclabels,
                         unsigned int flags)
 {
     xmlNodePtr cur;
@@ -12934,7 +12937,7 @@ virDomainChrDefParseXML(virDomainXMLOptionPtr xmlopt,
         goto error;
 
     if (virDomainChrSourceDefParseXML(def->source, node->children, flags, def,
-                                      ctxt, vmSeclabels, nvmSeclabels) < 0)
+                                      ctxt) < 0)
         goto error;
 
     if (def->source->type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) {
@@ -13065,7 +13068,7 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt,
 
         cur = node->children;
         if (virDomainChrSourceDefParseXML(def->data.passthru, cur, flags,
-                                          NULL, ctxt, NULL, 0) < 0)
+                                          NULL, ctxt) < 0)
             goto error;
 
         if (def->data.passthru->type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) {
@@ -14740,7 +14743,7 @@ virDomainRNGDefParseXML(virDomainXMLOptionPtr xmlopt,
 
         if (virDomainChrSourceDefParseXML(def->source.chardev,
                                           backends[0]->children, flags,
-                                          NULL, ctxt, NULL, 0) < 0)
+                                          NULL, ctxt) < 0)
             goto error;
         break;
 
@@ -15761,7 +15764,7 @@ virDomainRedirdevDefParseXML(virDomainXMLOptionPtr xmlopt,
     /* boot gets parsed in virDomainDeviceInfoParseXML
      * source gets parsed in virDomainChrSourceDefParseXML */
     if (virDomainChrSourceDefParseXML(def->source, cur, flags,
-                                      NULL, ctxt, NULL, 0) < 0)
+                                      NULL, ctxt) < 0)
         goto error;
 
     if (def->source->type == VIR_DOMAIN_CHR_TYPE_SPICEVMC)
@@ -16461,8 +16464,6 @@ virDomainDeviceDefParse(const char *xmlStr,
     switch ((virDomainDeviceType) dev->type) {
     case VIR_DOMAIN_DEVICE_DISK:
         if (!(dev->data.disk = virDomainDiskDefParseXML(xmlopt, node, ctxt,
-                                                        def->seclabels,
-                                                        def->nseclabels,
                                                         flags)))
             return NULL;
         break;
@@ -16532,8 +16533,6 @@ virDomainDeviceDefParse(const char *xmlStr,
         if (!(dev->data.chr = virDomainChrDefParseXML(xmlopt,
                                                       ctxt,
                                                       node,
-                                                      def->seclabels,
-                                                      def->nseclabels,
                                                       flags)))
             return NULL;
         break;
@@ -16600,14 +16599,11 @@ virDomainDeviceDefParse(const char *xmlStr,
 
 virDomainDiskDefPtr
 virDomainDiskDefParse(const char *xmlStr,
-                      const virDomainDef *def,
                       virDomainXMLOptionPtr xmlopt,
                       unsigned int flags)
 {
     g_autoptr(xmlDoc) xml = NULL;
     g_autoptr(xmlXPathContext) ctxt = NULL;
-    virSecurityLabelDefPtr *seclabels = NULL;
-    size_t nseclabels = 0;
 
     if (!(xml = virXMLParseStringCtxt(xmlStr, _("(disk_definition)"), &ctxt)))
         return NULL;
@@ -16619,13 +16615,7 @@ virDomainDiskDefParse(const char *xmlStr,
         return NULL;
     }
 
-    if (def) {
+    return virDomainDiskDefParseXML(xmlopt, ctxt->node, ctxt, flags);
-        seclabels = def->seclabels;
-        nseclabels = def->nseclabels;
-    }
-
-    return virDomainDiskDefParseXML(xmlopt, ctxt->node, ctxt,
-                                    seclabels, nseclabels, flags);
 }
 
 
@@ -20815,8 +20805,6 @@ virDomainDefParseXML(xmlDocPtr xml,
         virDomainDiskDefPtr disk = virDomainDiskDefParseXML(xmlopt,
                                                             nodes[i],
                                                             ctxt,
-                                                            def->seclabels,
-                                                            def->nseclabels,
                                                             flags);
         if (!disk)
             goto error;
@@ -20967,8 +20955,6 @@ virDomainDefParseXML(xmlDocPtr xml,
         virDomainChrDefPtr chr = virDomainChrDefParseXML(xmlopt,
                                                          ctxt,
                                                          nodes[i],
-                                                         def->seclabels,
-                                                         def->nseclabels,
                                                          flags);
         if (!chr)
             goto error;
@@ -20995,8 +20981,6 @@ virDomainDefParseXML(xmlDocPtr xml,
         virDomainChrDefPtr chr = virDomainChrDefParseXML(xmlopt,
                                                          ctxt,
                                                          nodes[i],
-                                                         def->seclabels,
-                                                         def->nseclabels,
                                                          flags);
         if (!chr)
             goto error;
@@ -21025,8 +21009,6 @@ virDomainDefParseXML(xmlDocPtr xml,
         virDomainChrDefPtr chr = virDomainChrDefParseXML(xmlopt,
                                                          ctxt,
                                                          nodes[i],
-                                                         def->seclabels,
-                                                         def->nseclabels,
                                                          flags);
         if (!chr)
             goto error;
@@ -21045,8 +21027,6 @@ virDomainDefParseXML(xmlDocPtr xml,
         virDomainChrDefPtr chr = virDomainChrDefParseXML(xmlopt,
                                                          ctxt,
                                                          nodes[i],
-                                                         def->seclabels,
-                                                         def->nseclabels,
                                                          flags);
         if (!chr)
             goto error;

src/conf/domain_conf.h

@@ -3047,7 +3047,6 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr,
                                               void *parseOpaque,
                                               unsigned int flags);
 virDomainDiskDefPtr virDomainDiskDefParse(const char *xmlStr,
-                                          const virDomainDef *def,
                                           virDomainXMLOptionPtr xmlopt,
                                           unsigned int flags);
 virDomainDefPtr virDomainDefParseString(const char *xmlStr,

src/qemu/qemu_driver.c

@@ -18477,7 +18477,7 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml,
         }
     }
 
-    if (!(diskdef = virDomainDiskDefParse(destxml, vm->def, driver->xmlopt,
+    if (!(diskdef = virDomainDiskDefParse(destxml, driver->xmlopt,
                                           VIR_DOMAIN_DEF_PARSE_INACTIVE |
                                           VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)))
         goto cleanup;

tests/qemublocktest.c

@@ -200,7 +200,7 @@ testQemuDiskXMLToProps(const void *opaque)
         goto cleanup;
 
     /* qemu stores node names in the status XML portion */
-    if (!(disk = virDomainDiskDefParse(xmlstr, NULL, data->driver->xmlopt,
+    if (!(disk = virDomainDiskDefParse(xmlstr, data->driver->xmlopt,
                                        VIR_DOMAIN_DEF_PARSE_STATUS)))
         goto cleanup;