qemu: don't continue loading caps if outdated

The XML format used for QEMU capabilities is not required to be
stable across releases, as we invalidate the cache whenever the
libvirt binary changes.

We none the less always try to parse te entire XML file before
we do any validity checks. Thus if we change the format of any
part of the data, or change permitted values for enums, then
libvirtd logs will be spammed with errors.

These are not in fact errors, but an expected scenario.

This change makes the loading code validate the cache timestamp
against the libvirtd timestamp immediately. If they don't match
then we stop loading the rest of the XML file.

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

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 267cbcf883d99631ecfd5b04824eca340cae083a..268e32f801fcd7b4d7a84013a417caa821231932 100644 (file)
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1811,14 +1811,6 @@ virQEMUCapsNewBinary(const char *binary)
 }
 
 
-void
-virQEMUCapsSetInvalidation(virQEMUCapsPtr qemuCaps,
-                           bool enabled)
-{
-    qemuCaps->invalidation = enabled;
-}
-
-
 static int
 virQEMUCapsHostCPUDataCopy(virQEMUCapsHostCPUDataPtr dst,
                            virQEMUCapsHostCPUDataPtr src)
@@ -4209,11 +4201,14 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps, xmlXPathContextPtr ctxt)
  *   <machine name='pc-1.0' alias='pc' hotplugCpus='yes' maxCpus='4' default='yes' numaMemSupported='yes'/>
  *   ...
  * </qemuCaps>
+ *
+ * Returns 0 on success, 1 if outdated, -1 on error
  */
 int
 virQEMUCapsLoadCache(virArch hostArch,
                      virQEMUCapsPtr qemuCaps,
-                     const char *filename)
+                     const char *filename,
+                     bool skipInvalidation)
 {
     xmlDocPtr doc = NULL;
     int ret = -1;
@@ -4241,6 +4236,31 @@ virQEMUCapsLoadCache(virArch hostArch,
         goto cleanup;
     }
 
+    if (virXPathLongLong("string(./selfctime)", ctxt, &l) < 0) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("missing selfctime in QEMU capabilities XML"));
+        goto cleanup;
+    }
+    qemuCaps->libvirtCtime = (time_t)l;
+
+    qemuCaps->libvirtVersion = 0;
+    if (virXPathULong("string(./selfvers)", ctxt, &lu) == 0)
+        qemuCaps->libvirtVersion = lu;
+
+    if (!skipInvalidation &&
+        (qemuCaps->libvirtCtime != virGetSelfLastChanged() ||
+         qemuCaps->libvirtVersion != LIBVIR_VERSION_NUMBER)) {
+        VIR_DEBUG("Outdated capabilities in %s: libvirt changed "
+                  "(%lld vs %lld, %lu vs %lu), stopping load",
+                  qemuCaps->binary,
+                  (long long)qemuCaps->libvirtCtime,
+                  (long long)virGetSelfLastChanged(),
+                  (unsigned long)qemuCaps->libvirtVersion,
+                  (unsigned long)LIBVIR_VERSION_NUMBER);
+        ret = 1;
+        goto cleanup;
+    }
+
     if (!(str = virXPathString("string(./emulator)", ctxt))) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("missing emulator in QEMU capabilities cache"));
@@ -4260,17 +4280,6 @@ virQEMUCapsLoadCache(virArch hostArch,
     }
     qemuCaps->ctime = (time_t)l;
 
-    if (virXPathLongLong("string(./selfctime)", ctxt, &l) < 0) {
-        virReportError(VIR_ERR_XML_ERROR, "%s",
-                       _("missing selfctime in QEMU capabilities XML"));
-        goto cleanup;
-    }
-    qemuCaps->libvirtCtime = (time_t)l;
-
-    qemuCaps->libvirtVersion = 0;
-    if (virXPathULong("string(./selfvers)", ctxt, &lu) == 0)
-        qemuCaps->libvirtVersion = lu;
-
     if ((n = virXPathNodeSet("./flag", ctxt, &nodes)) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("failed to parse qemu capabilities flags"));
@@ -4422,6 +4431,9 @@ virQEMUCapsLoadCache(virArch hostArch,
     if (virXPathBoolean("boolean(./kvmSupportsSecureGuest)", ctxt) > 0)
         qemuCaps->kvmSupportsSecureGuest = true;
 
+    if (skipInvalidation)
+        qemuCaps->invalidation = false;
+
     ret = 0;
  cleanup:
     VIR_FREE(str);
@@ -5493,16 +5505,23 @@ virQEMUCapsNewData(const char *binary,
 static void *
 virQEMUCapsLoadFile(const char *filename,
                     const char *binary,
-                    void *privData)
+                    void *privData,
+                    bool *outdated)
 {
     virQEMUCapsPtr qemuCaps = virQEMUCapsNewBinary(binary);
     virQEMUCapsCachePrivPtr priv = privData;
+    int ret;
 
     if (!qemuCaps)
         return NULL;
 
-    if (virQEMUCapsLoadCache(priv->hostArch, qemuCaps, filename) < 0)
+    ret = virQEMUCapsLoadCache(priv->hostArch, qemuCaps, filename, false);
+    if (ret < 0)
         goto error;
+    if (ret == 1) {
+        *outdated = true;
+        goto error;
+    }
 
     return qemuCaps;
 

diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h
index 5d2f448e413149e121090791e0cde4ee9d1f3ded..f6c06ea0081ec7c00aacd5fb4cf6b0e66fd13c14 100644 (file)
--- a/src/qemu/qemu_capspriv.h
+++ b/src/qemu/qemu_capspriv.h
@@ -37,12 +37,10 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
                                 unsigned int microcodeVersion,
                                 const char *kernelVersion);
 
-void virQEMUCapsSetInvalidation(virQEMUCapsPtr qemuCaps,
-                                bool enabled);
-
 int virQEMUCapsLoadCache(virArch hostArch,
                          virQEMUCapsPtr qemuCaps,
-                         const char *filename);
+                         const char *filename,
+                         bool skipInvalidation);
 char *virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps);
 
 int

diff --git a/src/util/virfilecache.c b/src/util/virfilecache.c
index aecabf173d65a1b14651783258b948fc92d61183..2162917b11d8ac1bb3ecb502c8101de75b3d3575 100644 (file)
--- a/src/util/virfilecache.c
+++ b/src/util/virfilecache.c
@@ -130,6 +130,7 @@ virFileCacheLoad(virFileCachePtr cache,
     g_autofree char *file = NULL;
     int ret = -1;
     void *loadData = NULL;
+    bool outdated = false;
 
     *data = NULL;
 
@@ -148,10 +149,12 @@ virFileCacheLoad(virFileCachePtr cache,
         goto cleanup;
     }
 
-    if (!(loadData = cache->handlers.loadFile(file, name, cache->priv))) {
-        VIR_WARN("Failed to load cached data from '%s' for '%s': %s",
-                 file, name, virGetLastErrorMessage());
-        virResetLastError();
+    if (!(loadData = cache->handlers.loadFile(file, name, cache->priv, &outdated))) {
+        if (!outdated) {
+            VIR_WARN("Failed to load cached data from '%s' for '%s': %s",
+                     file, name, virGetLastErrorMessage());
+            virResetLastError();
+        }
         ret = 0;
         goto cleanup;
     }

diff --git a/src/util/virfilecache.h b/src/util/virfilecache.h
index 006a9717cbae7b5e11c3a630f9179f2bd006180e..9a7edf07e6c53af0d1c2695a6bc309c653268b5f 100644 (file)
--- a/src/util/virfilecache.h
+++ b/src/util/virfilecache.h
@@ -62,15 +62,20 @@ typedef void *
  * @filename: name of a file with cached data
  * @name: name of the cached data
  * @priv: private data created together with cache
+ * @outdated: set to true if data was outdated
  *
- * Loads the cached data from a file @filename.
+ * Loads the cached data from a file @filename. If
+ * NULL is returned, then @oudated indicates whether
+ * this was due to the data being outdated, or an
+ * error loading the cache.
  *
- * Returns cached data object or NULL on error.
+ * Returns cached data object or NULL on outdated data or error.
  */
 typedef void *
 (*virFileCacheLoadFilePtr)(const char *filename,
                            const char *name,
-                           void *priv);
+                           void *priv,
+                           bool *outdated);
 
 /**
  * virFileCacheSaveFilePtr:

diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 4dcc3089dd73c1887c0984c1b8055fc3d1501ad8..e3b1e2813ba56b9651fb1cb23c1572c6f8c39253 100644 (file)
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -293,10 +293,9 @@ qemuTestParseCapabilitiesArch(virArch arch,
                                               virArchToString(arch));
 
     if (!(qemuCaps = virQEMUCapsNewBinary(binary)) ||
-        virQEMUCapsLoadCache(arch, qemuCaps, capsFile) < 0)
+        virQEMUCapsLoadCache(arch, qemuCaps, capsFile, true) < 0)
         goto error;
 
-    virQEMUCapsSetInvalidation(qemuCaps, false);
     return qemuCaps;
 
  error:

diff --git a/tests/virfilecachetest.c b/tests/virfilecachetest.c
index 6d280b3bec0d6f72a158273c0a4d482612d95394..34e0d0ab2f5d5a653d14ac1ee0a811fa2963434b 100644 (file)
--- a/tests/virfilecachetest.c
+++ b/tests/virfilecachetest.c
@@ -110,7 +110,8 @@ testFileCacheNewData(const char *name G_GNUC_UNUSED,
 static void *
 testFileCacheLoadFile(const char *filename,
                       const char *name G_GNUC_UNUSED,
-                      void *priv G_GNUC_UNUSED)
+                      void *priv G_GNUC_UNUSED,
+                      bool *outdated G_GNUC_UNUSED)
 {
     testFileCacheObjPtr obj;
     char *data;