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.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/qemu/qemu_capabilities.c | 48 ++++++++++++++++++++++++++----------
 src/util/virfilecache.c      | 11 ++++++---
 src/util/virfilecache.h      | 11 ++++++---
 tests/virfilecachetest.c     |  3 ++-
 4 files changed, 52 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 310be800e2..8694db2463 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4205,6 +4205,8 @@ 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,
@@ -4237,6 +4239,30 @@ 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 (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"));
@@ -4256,17 +4282,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"));
@@ -5489,16 +5504,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);
+    if (ret < 0)
         goto error;
+    if (ret == 1) {
+        *outdated = true;
+        goto error;
+    }
 
     return qemuCaps;
 
diff --git a/src/util/virfilecache.c b/src/util/virfilecache.c
index aecabf173d..2162917b11 100644
--- 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 006a9717cb..9a7edf07e6 100644
--- 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/virfilecachetest.c b/tests/virfilecachetest.c
index 6d280b3bec..34e0d0ab2f 100644
--- 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;
-- 
2.24.1

On 6/18/20 4:31 PM, Daniel P. Berrangé wrote:
> 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.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/qemu/qemu_capabilities.c | 48 ++++++++++++++++++++++++++----------
>   src/util/virfilecache.c      | 11 ++++++---
>   src/util/virfilecache.h      | 11 ++++++---
>   tests/virfilecachetest.c     |  3 ++-
>   4 files changed, 52 insertions(+), 21 deletions(-)
> 

While I agree with the idea, we need to think of tests.
The problem is that in tests/ we store some capabilities which don't 
have <selfvers/> and thus many of our tests fail to load them.

Michal

On Mon, Jun 22, 2020 at 05:30:39PM +0200, Michal Privoznik wrote:
> On 6/18/20 4:31 PM, Daniel P. Berrangé wrote:
> > 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.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   src/qemu/qemu_capabilities.c | 48 ++++++++++++++++++++++++++----------
> >   src/util/virfilecache.c      | 11 ++++++---
> >   src/util/virfilecache.h      | 11 ++++++---
> >   tests/virfilecachetest.c     |  3 ++-
> >   4 files changed, 52 insertions(+), 21 deletions(-)
> > 
> 
> While I agree with the idea, we need to think of tests.
> The problem is that in tests/ we store some capabilities which don't have
> <selfvers/> and thus many of our tests fail to load them.

Doh, I missed that.

We should fix the XML to be complete, and provide then mock the
check so it always succeeds.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|