[libvirt] [PATCH 0/4] Add caching of QEMU probed capabilities

Probing capabilities takes 200-300ms per binary and we have as many as 26 binaries. This noticably slows down libvirtd startup. It does not look like performance of probing QEMU can be improved, so this series introduces caching of the capabilities information. So the first time libvirtd starts it'll be slow, but thereafter it is fast. The cache is invalidated any time the QEMU binary timestamp changes or the libvirtd binary or driver module timestamp changes. Daniel P. Berrange (4): Add helper APIs for generating cryptographic hashes Convert lock driver plugins to use new crypto APIs Cache result of QEMU capabilities extraction Refresh qemu capabilities if libvirtd binary changes .gitignore | 1 + daemon/libvirtd.c | 2 + include/libvirt/virterror.h | 1 + po/POTFILES.in | 1 + src/Makefile.am | 1 + src/driver.c | 2 + src/libvirt_private.syms | 6 + src/locking/lock_driver_lockd.c | 32 +-- src/locking/lock_driver_sanlock.c | 42 +--- src/qemu/qemu_capabilities.c | 418 +++++++++++++++++++++++++++++++++++++- src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_driver.c | 1 + src/util/vircrypto.c | 80 ++++++++ src/util/vircrypto.h | 40 ++++ src/util/virerror.c | 1 + src/util/virutil.c | 23 +++ src/util/virutil.h | 4 + tests/Makefile.am | 5 + tests/vircryptotest.c | 90 ++++++++ 19 files changed, 682 insertions(+), 70 deletions(-) create mode 100644 src/util/vircrypto.c create mode 100644 src/util/vircrypto.h create mode 100644 tests/vircryptotest.c -- 1.8.5.3

GNULIB provides APIs for calculating md5 and sha256 hashes, but these APIs only return you raw byte arrays. Most users in libvirt want the hash in printable string format. Add some helper APIs in util/vircrypto.{c,h} for doing this. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- .gitignore | 1 + include/libvirt/virterror.h | 1 + po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 4 ++ src/util/vircrypto.c | 80 ++++++++++++++++++++++++++++++++++++++++ src/util/vircrypto.h | 40 ++++++++++++++++++++ src/util/virerror.c | 1 + tests/Makefile.am | 5 +++ tests/vircryptotest.c | 90 +++++++++++++++++++++++++++++++++++++++++++++ 10 files changed, 224 insertions(+) create mode 100644 src/util/vircrypto.c create mode 100644 src/util/vircrypto.h create mode 100644 tests/vircryptotest.c diff --git a/.gitignore b/.gitignore index cb60734..1973640 100644 --- a/.gitignore +++ b/.gitignore @@ -198,6 +198,7 @@ /tests/virbuftest /tests/vircapstest /tests/vircgrouptest +/tests/vircryptotest /tests/virdbustest /tests/virdrivermoduletest /tests/virendiantest diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index cc6569d..495c121 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -121,6 +121,7 @@ typedef enum { VIR_FROM_ACCESS = 55, /* Error from access control manager */ VIR_FROM_SYSTEMD = 56, /* Error from systemd code */ VIR_FROM_BHYVE = 57, /* Error from bhyve driver */ + VIR_FROM_CRYPTO = 58, /* Error from crypto code */ # ifdef VIR_ENUM_SENTINELS VIR_ERR_DOMAIN_LAST diff --git a/po/POTFILES.in b/po/POTFILES.in index bcc0ee0..a8a5975 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -158,6 +158,7 @@ src/util/vircgroup.c src/util/virclosecallbacks.c src/util/vircommand.c src/util/virconf.c +src/util/vircrypto.c src/util/virdbus.c src/util/virdnsmasq.c src/util/vireventpoll.c diff --git a/src/Makefile.am b/src/Makefile.am index 25d0370..4bc2df4 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -93,6 +93,7 @@ UTIL_SOURCES = \ util/virclosecallbacks.c util/virclosecallbacks.h \ util/vircommand.c util/vircommand.h \ util/virconf.c util/virconf.h \ + util/vircrypto.c util/vircrypto.h \ util/virdbus.c util/virdbus.h util/virdbuspriv.h \ util/virdnsmasq.c util/virdnsmasq.h \ util/virebtables.c util/virebtables.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 80070c5..00e3d9c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1149,6 +1149,10 @@ virConfWriteFile; virConfWriteMem; +# util/vircrypto.h +virCryptoHashString; + + # util/virdbus.h virDBusCallMethod; virDBusCloseSystemBus; diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c new file mode 100644 index 0000000..bd00644 --- /dev/null +++ b/src/util/vircrypto.c @@ -0,0 +1,80 @@ +/* + * vircrypto.c: cryptographic helper APIs + * + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "vircrypto.h" +#include "virerror.h" +#include "viralloc.h" + +#include "md5.h" +#include "sha256.h" + +#define VIR_FROM_THIS VIR_FROM_CRYPTO + +static const char hex[] = { '0', '1', '2', '3', '4', '5', '6', '7', + '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' }; + +struct virHashInfo { + void *(*func)(const char *buf, size_t len, void *res); + size_t hashlen; +} hashinfo[] = { + { md5_buffer, MD5_DIGEST_SIZE }, + { sha256_buffer, SHA256_DIGEST_SIZE }, +}; + +#define VIR_CRYPTO_LARGEST_DIGEST_SIZE SHA256_DIGEST_SIZE + +verify(ARRAY_CARDINALITY(hashinfo) == VIR_CRYPTO_HASH_LAST); + +int +virCryptoHashString(virCryptoHash hash, + const char *input, + char **output) +{ + unsigned char buf[VIR_CRYPTO_LARGEST_DIGEST_SIZE]; + size_t hashstrlen; + size_t i; + + if (hash >= VIR_CRYPTO_HASH_LAST) { + virReportError(VIR_ERR_INVALID_ARG, + _("Unknown crypto hash %d"), hash); + return -1; + } + + hashstrlen = (hashinfo[hash].hashlen * 2) + 1; + + if (!(hashinfo[hash].func(input, strlen(input), buf))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to compute hash of data")); + return -1; + } + + if (VIR_ALLOC_N(*output, hashstrlen) < 0) + return -1; + + for (i = 0; i < hashinfo[hash].hashlen; i++) { + (*output)[i*2] = hex[(buf[i] >> 4) & 0xf]; + (*output)[(i*2)+1] = hex[buf[i] & 0xf]; + } + (*output)[hashstrlen] = '\0'; + + return 0; +} diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h new file mode 100644 index 0000000..f67d49d --- /dev/null +++ b/src/util/vircrypto.h @@ -0,0 +1,40 @@ +/* + * vircrypto.h: cryptographic helper APIs + * + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#ifndef __VIR_CRYPTO_H__ +# define __VIR_CRYPTO_H__ + +# include "internal.h" + +typedef enum { + VIR_CRYPTO_HASH_MD5, /* Don't use this except for historic compat */ + VIR_CRYPTO_HASH_SHA256, + + VIR_CRYPTO_HASH_LAST +} virCryptoHash; + +int +virCryptoHashString(virCryptoHash hash, + const char *input, + char **output) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_RETURN_CHECK; + +#endif /* __VIR_CRYPTO_H__ */ diff --git a/src/util/virerror.c b/src/util/virerror.c index 820e1ad..3eb4f5d 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -125,6 +125,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, "Access Manager", /* 55 */ "Systemd", "Bhyve", + "Crypto", ) diff --git a/tests/Makefile.am b/tests/Makefile.am index cd91734..67f6600 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -135,6 +135,7 @@ test_programs = virshtest sockettest \ virauthconfigtest \ virbitmaptest \ vircgrouptest \ + vircryptotest \ virpcitest \ virendiantest \ virfiletest \ @@ -812,6 +813,10 @@ vircgroupmock_la_CFLAGS = $(AM_CFLAGS) vircgroupmock_la_LDFLAGS = -module -avoid-version \ -rpath /evil/libtool/hack/to/force/shared/lib/creation +vircryptotest_SOURCES = \ + vircryptotest.c testutils.h testutils.c +vircryptotest_LDADD = $(LDADDS) + virpcitest_SOURCES = \ virpcitest.c testutils.h testutils.c virpcitest_LDADD = $(LDADDS) diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c new file mode 100644 index 0000000..270d0a9 --- /dev/null +++ b/tests/vircryptotest.c @@ -0,0 +1,90 @@ +/* + * vircrypto.h: cryptographic helper APIs + * + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "vircrypto.h" + +#include "testutils.h" + + +struct testCryptoHashData { + virCryptoHash hash; + const char *input; + const char *output; +}; + +static int +testCryptoHash(const void *opaque) +{ + const struct testCryptoHashData *data = opaque; + char *actual = NULL; + int ret = -1; + + if (virCryptoHashString(data->hash, data->input, &actual) < 0) { + fprintf(stderr, "Failed to generate crypto hash\n"); + goto cleanup; + } + + if (STRNEQ_NULLABLE(data->output, actual)) { + fprintf(stderr, "Expected hash '%s' but got '%s'\n", + data->output, NULLSTR(actual)); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(actual); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + +#define VIR_CRYPTO_HASH(h, i, o) \ + do { \ + struct testCryptoHashData data = { \ + .hash = h, \ + .input = i, \ + .output = o, \ + }; \ + if (virtTestRun("Hash " i, testCryptoHash, &data) < 0) \ + ret = -1; \ + } while (0) + + VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_MD5, "", "d41d8cd98f00b204e9800998ecf8427e"); + VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_SHA256, "", "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"); + + VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_MD5, " ", "7215ee9c7d9dc229d2921a40e899ec5f"); + VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_SHA256, " ", "36a9e7f1c95b82ffb99743e0c5c4ce95d83c9a430aac59f84ef3cbfab6145068"); + + VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_MD5, "\n", "68b329da9893e34099c7d8ad5cb9c940"); + VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_SHA256, "\n", "01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b"); + + VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_MD5, "The quick brown fox", "a2004f37730b9445670a738fa0fc9ee5"); + VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_SHA256, "The quick brown fox", "5cac4f980fedc3d3f1f99b4be3472c9b30d56523e632d151237ec9309048bda9"); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN(mymain) -- 1.8.5.3

On 03/05/2014 10:53 AM, Daniel P. Berrange wrote:
GNULIB provides APIs for calculating md5 and sha256 hashes, but these APIs only return you raw byte arrays. Most users in libvirt want the hash in printable string format. Add some helper APIs in util/vircrypto.{c,h} for doing this.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- .gitignore | 1 + include/libvirt/virterror.h | 1 + po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 4 ++ src/util/vircrypto.c | 80 ++++++++++++++++++++++++++++++++++++++++ src/util/vircrypto.h | 40 ++++++++++++++++++++ src/util/virerror.c | 1 + tests/Makefile.am | 5 +++ tests/vircryptotest.c | 90 +++++++++++++++++++++++++++++++++++++++++++++ 10 files changed, 224 insertions(+) create mode 100644 src/util/vircrypto.c create mode 100644 src/util/vircrypto.h create mode 100644 tests/vircryptotest.c
+#define VIR_FROM_THIS VIR_FROM_CRYPTO + +static const char hex[] = { '0', '1', '2', '3', '4', '5', '6', '7', + '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' };
Why not the more compact: static const char hex[] = "0123456789abcdef"; virbuffer.c and virsystemd.c also have variants of this table.
+ + for (i = 0; i < hashinfo[hash].hashlen; i++) { + (*output)[i*2] = hex[(buf[i] >> 4) & 0xf]; + (*output)[(i*2)+1] = hex[buf[i] & 0xf];
Spaces around operators * and + ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Conver the sanlock and lockd lock driver plugins over to use the new virCryptoHashString APIs instead of having their own duplicated code. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/locking/lock_driver_lockd.c | 32 ++--------------------------- src/locking/lock_driver_sanlock.c | 42 ++++++++++----------------------------- 2 files changed, 12 insertions(+), 62 deletions(-) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index f3b9467..1b92d68 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -24,6 +24,7 @@ #include "lock_driver.h" #include "virconf.h" #include "viralloc.h" +#include "vircrypto.h" #include "virlog.h" #include "viruuid.h" #include "virfile.h" @@ -31,7 +32,6 @@ #include "rpc/virnetclient.h" #include "lock_protocol.h" #include "configmake.h" -#include "sha256.h" #include "virstring.h" #define VIR_FROM_THIS VIR_FROM_LOCKING @@ -505,34 +505,6 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock, } -static const char hex[] = { '0', '1', '2', '3', '4', '5', '6', '7', - '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' }; - -static char *virLockManagerLockDaemonDiskLeaseName(const char *path) -{ - unsigned char buf[SHA256_DIGEST_SIZE]; - char *ret; - size_t i; - - if (!(sha256_buffer(path, strlen(path), buf))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to compute sha256 checksum")); - return NULL; - } - - if (VIR_ALLOC_N(ret, (SHA256_DIGEST_SIZE * 2) + 1) < 0) - return NULL; - - for (i = 0; i < SHA256_DIGEST_SIZE; i++) { - ret[i*2] = hex[(buf[i] >> 4) & 0xf]; - ret[(i*2)+1] = hex[buf[i] & 0xf]; - } - ret[(SHA256_DIGEST_SIZE * 2) + 1] = '\0'; - - return ret; -} - - static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, unsigned int type, const char *name, @@ -605,7 +577,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, if (driver->fileLockSpaceDir) { if (VIR_STRDUP(newLockspace, driver->fileLockSpaceDir) < 0) goto error; - if (!(newName = virLockManagerLockDaemonDiskLeaseName(name))) + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0) goto error; autoCreate = true; VIR_DEBUG("Using indirect lease %s for %s", newName, name); diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index f11f3c6..0c87048 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -40,8 +40,8 @@ #include "virlog.h" #include "virerror.h" #include "viralloc.h" +#include "vircrypto.h" #include "virfile.h" -#include "md5.h" #include "virconf.h" #include "virstring.h" @@ -509,36 +509,6 @@ static void virLockManagerSanlockFree(virLockManagerPtr lock) } -static const char hex[] = { '0', '1', '2', '3', '4', '5', '6', '7', - '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' }; - -static int virLockManagerSanlockDiskLeaseName(const char *path, - char *str, - size_t strbuflen) -{ - unsigned char buf[MD5_DIGEST_SIZE]; - size_t i; - - if (strbuflen < ((MD5_DIGEST_SIZE * 2) + 1)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("String length too small to store md5 checksum")); - return -1; - } - - if (!(md5_buffer(path, strlen(path), buf))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to compute md5 checksum")); - return -1; - } - - for (i = 0; i < MD5_DIGEST_SIZE; i++) { - str[i*2] = hex[(buf[i] >> 4) & 0xf]; - str[(i*2)+1] = hex[buf[i] & 0xf]; - } - str[(MD5_DIGEST_SIZE*2)+1] = '\0'; - return 0; -} - static int virLockManagerSanlockAddLease(virLockManagerPtr lock, const char *name, size_t nparams, @@ -606,6 +576,7 @@ static int virLockManagerSanlockAddDisk(virLockManagerPtr lock, int ret = -1; struct sanlk_resource *res = NULL; char *path = NULL; + char *hash = NULL; if (nparams) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -618,8 +589,14 @@ static int virLockManagerSanlockAddDisk(virLockManagerPtr lock, res->flags = shared ? SANLK_RES_SHARED : 0; res->num_disks = 1; - if (virLockManagerSanlockDiskLeaseName(name, res->name, SANLK_NAME_LEN) < 0) + if (virCryptoHashString(VIR_CRYPTO_HASH_MD5, name, &hash) < 0) + goto cleanup; + if (!virStrcpy(res->name, hash, SANLK_NAME_LEN)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("MD5 hash '%s' unexpectedly larger than %d characters"), + hash, (SANLK_NAME_LEN - 1)); goto cleanup; + } if (virAsprintf(&path, "%s/%s", driver->autoDiskLeasePath, res->name) < 0) @@ -649,6 +626,7 @@ cleanup: if (ret == -1) VIR_FREE(res); VIR_FREE(path); + VIR_FREE(hash); return ret; } -- 1.8.5.3

On 03/05/2014 10:53 AM, Daniel P. Berrange wrote:
Conver the sanlock and lockd lock driver plugins over to use
s/Conver/Convert/
the new virCryptoHashString APIs instead of having their own duplicated code.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/locking/lock_driver_lockd.c | 32 ++--------------------------- src/locking/lock_driver_sanlock.c | 42 ++++++++++----------------------------- 2 files changed, 12 insertions(+), 62 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Extracting capabilities from QEMU takes a notable amount of time when all QEMU binaries are installed. Each system emulator needs about 200-300ms multiplied by 26 binaries == ~5-8 seconds. This change causes the QEMU driver to save an XML file containing the content of the virQEMUCaps object instance in the cache dir eg /var/cache/libvirt/qemu/capabilities/$SHA256(binarypath).xml or $HOME/.cache/libvirt/qemu/cache/capabilities/$SHA256(binarypath).xml We attempt to load this and only if it fails, do we fallback to probing the QEMU binary. The timestamp of the file is compared to the timestamp of the QEMU binary and discarded if the QEMU binary is newer. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 412 ++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_driver.c | 1 + 3 files changed, 407 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index cae25e0..e3ed960 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -25,6 +25,7 @@ #include "qemu_capabilities.h" #include "viralloc.h" +#include "vircrypto.h" #include "virlog.h" #include "virerror.h" #include "virfile.h" @@ -44,6 +45,7 @@ #include <unistd.h> #include <sys/wait.h> #include <stdarg.h> +#include <utime.h> #define VIR_FROM_THIS VIR_FROM_QEMU @@ -281,7 +283,7 @@ struct _virQEMUCapsCache { virMutex lock; virHashTablePtr binaries; char *libDir; - char *runDir; + char *cacheDir; uid_t runUid; gid_t runGid; }; @@ -2339,6 +2341,386 @@ int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps, } +/* + * Parsing a doc that looks like + * + * <qemuCaps> + * <usedQMP/> + * <flag name='foo'/> + * <flag name='bar'/> + * ... + * <cpu name="pentium3"/> + * ... + * <machine name="pc-1.0" alias="pc" maxCpus="4"/> + * ... + * </qemuCaps> + */ +static int +virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const char *filename) +{ + xmlDocPtr doc = NULL; + int ret = -1; + size_t i; + int n; + xmlNodePtr *nodes = NULL; + xmlXPathContextPtr ctxt = NULL; + char *str; + + if (!(doc = virXMLParseFile(filename))) + goto cleanup; + + if (!(ctxt = xmlXPathNewContext(doc))) { + virReportOOMError(); + goto cleanup; + } + + ctxt->node = xmlDocGetRootElement(doc); + + if (STRNEQ((const char *)ctxt->node->name, "qemuCaps")) { + virReportError(VIR_ERR_XML_ERROR, + _("unexpected root element <%s>, " + "expecting <qemuCaps>"), + ctxt->node->name); + goto cleanup; + } + + qemuCaps->usedQMP = virXPathBoolean("count(.//usedQMP) > 0", + ctxt) > 0; + + if ((n = virXPathNodeSet(".//flag", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse qemu capabilities flags")); + goto cleanup; + } + VIR_DEBUG("Got flags %d", n); + if (n > 0) { + for (i = 0; i < n; i++) { + int flag; + if (!(str = virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing flag name in QEMU capabilities cache")); + goto cleanup; + } + flag = virQEMUCapsTypeFromString(str); + if (flag < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown qemu capabilities flag %s"), str); + VIR_FREE(str); + goto cleanup; + } + VIR_FREE(str); + virQEMUCapsSet(qemuCaps, flag); + } + } + VIR_FREE(nodes); + + if (virXPathUInt("string(.//version)", ctxt, &qemuCaps->version) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing version in QEMU capabilities cache")); + goto cleanup; + } + + if (virXPathUInt("string(.//kvmVersion)", ctxt, &qemuCaps->kvmVersion) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing version in QEMU capabilities cache")); + goto cleanup; + } + + if (!(str = virXPathString("string(.//arch)", ctxt))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing arch in QEMU capabilities cache")); + goto cleanup; + } + if (!(qemuCaps->arch = virArchFromString(str))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown arch %s in QEMU capabilities cache"), str); + goto cleanup; + } + + if ((n = virXPathNodeSet(".//cpu", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse qemu capabilities cpus")); + goto cleanup; + } + if (n > 0) { + qemuCaps->ncpuDefinitions = n; + if (VIR_ALLOC_N(qemuCaps->cpuDefinitions, + qemuCaps->ncpuDefinitions) < 0) + goto cleanup; + + for (i = 0; i < n; i++) { + if (!(str = virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing cpu name in QEMU capabilities cache")); + goto cleanup; + } + qemuCaps->cpuDefinitions[i] = str; + } + } + VIR_FREE(nodes); + + + if ((n = virXPathNodeSet(".//machine", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse qemu capabilities machines")); + goto cleanup; + } + if (n > 0) { + qemuCaps->nmachineTypes = n; + if (VIR_ALLOC_N(qemuCaps->machineTypes, + qemuCaps->nmachineTypes) < 0 || + VIR_ALLOC_N(qemuCaps->machineAliases, + qemuCaps->nmachineTypes) < 0 || + VIR_ALLOC_N(qemuCaps->machineMaxCpus, + qemuCaps->nmachineTypes) < 0) + goto cleanup; + + for (i = 0; i < n; i++) { + if (!(str = virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing machine name in QEMU capabilities cache")); + goto cleanup; + } + qemuCaps->machineTypes[i] = str; + + qemuCaps->machineAliases[i] = virXMLPropString(nodes[i], "alias"); + + str = virXMLPropString(nodes[i], "maxCpus"); + if (str && + virStrToLong_ui(str, NULL, 10, &(qemuCaps->machineMaxCpus[i])) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed machine cpu count in QEMU capabilities cache")); + goto cleanup; + } + } + } + VIR_FREE(nodes); + + ret = 0; + cleanup: + VIR_FREE(nodes); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(doc); + return ret; +} + + +static int +virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *xml = NULL; + int ret = -1; + size_t i; + struct utimbuf ut; + + virBufferAddLit(&buf, "<qemuCaps>\n"); + + if (qemuCaps->usedQMP) + virBufferAddLit(&buf, " <usedQMP/>\n"); + + for (i = 0; i < QEMU_CAPS_LAST; i++) { + if (virQEMUCapsGet(qemuCaps, i)) { + virBufferAsprintf(&buf, " <flag name='%s'/>\n", + virQEMUCapsTypeToString(i)); + } + } + + virBufferAsprintf(&buf, " <version>%d</version>\n", + qemuCaps->version); + + virBufferAsprintf(&buf, " <kvmVersion>%d</kvmVersion>\n", + qemuCaps->kvmVersion); + + virBufferAsprintf(&buf, " <arch>%s</arch>\n", + virArchToString(qemuCaps->arch)); + + for (i = 0; i < qemuCaps->ncpuDefinitions; i++) { + virBufferEscapeString(&buf, " <cpu name='%s'/>\n", + qemuCaps->cpuDefinitions[i]); + } + + for (i = 0; i < qemuCaps->nmachineTypes; i++) { + virBufferEscapeString(&buf, " <machine name='%s'", + qemuCaps->machineTypes[i]); + if (qemuCaps->machineAliases[i]) + virBufferEscapeString(&buf, " alias='%s'", + qemuCaps->machineAliases[i]); + virBufferAsprintf(&buf, " maxCpus='%u'/>\n", + qemuCaps->machineMaxCpus[i]); + } + + virBufferAddLit(&buf, "</qemuCaps>\n"); + + if (virBufferError(&buf)) + goto cleanup; + + xml = virBufferContentAndReset(&buf); + + if (virFileWriteStr(filename, xml, 0600) < 0) { + virReportSystemError(errno, + _("Failed to save '%s' for '%s'"), + filename, qemuCaps->binary); + goto cleanup; + } + + ut.actime = qemuCaps->mtime; + ut.modtime = qemuCaps->mtime; + if (utime(filename, &ut) < 0) { + virReportSystemError(errno, + _("Failed to set mtime on '%s' for '%s'"), + filename, qemuCaps->binary); + ignore_value(unlink(filename)); + goto cleanup; + } + + VIR_DEBUG("Saved caps '%s' for '%s' with %lld", + filename, qemuCaps->binary, (long long)qemuCaps->mtime); + + ret = 0; + cleanup: + VIR_FREE(xml); + return ret; +} + +static int +virQEMUCapsRememberCached(virQEMUCapsPtr qemuCaps, const char *cacheDir) +{ + char *capsdir = NULL; + char *capsfile = NULL; + int ret = -1; + char *binaryhash = NULL; + + if (virAsprintf(&capsdir, "%s/capabilities", cacheDir) < 0) + goto cleanup; + + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, + qemuCaps->binary, + &binaryhash) < 0) + goto cleanup; + + if (virAsprintf(&capsfile, "%s/%s.xml", capsdir, binaryhash) < 0) + goto cleanup; + + if (virFileMakePath(capsdir) < 0) { + virReportSystemError(errno, + _("Unable to create directory '%s'"), + capsdir); + goto cleanup; + } + + if (virQEMUCapsSaveCache(qemuCaps, capsfile) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(binaryhash); + VIR_FREE(capsfile); + VIR_FREE(capsdir); + return ret; +} + + +static void +virQEMUCapsReset(virQEMUCapsPtr qemuCaps) +{ + size_t i; + + virBitmapClearAll(qemuCaps->flags); + qemuCaps->version = qemuCaps->kvmVersion = 0; + qemuCaps->arch = VIR_ARCH_NONE; + + for (i = 0; i < qemuCaps->ncpuDefinitions; i++) { + VIR_FREE(qemuCaps->cpuDefinitions[i]); + } + VIR_FREE(qemuCaps->cpuDefinitions); + qemuCaps->ncpuDefinitions = 0; + + for (i = 0; i < qemuCaps->nmachineTypes; i++) { + VIR_FREE(qemuCaps->machineTypes[i]); + VIR_FREE(qemuCaps->machineAliases[i]); + } + VIR_FREE(qemuCaps->machineTypes); + VIR_FREE(qemuCaps->machineAliases); + qemuCaps->nmachineTypes = 0; +} + + +static int +virQEMUCapsInitCached(virQEMUCapsPtr qemuCaps, const char *cacheDir) +{ + char *capsdir = NULL; + char *capsfile = NULL; + int ret = -1; + char *binaryhash = NULL; + struct stat sb; + + if (virAsprintf(&capsdir, "%s/capabilities", cacheDir) < 0) + goto cleanup; + + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, + qemuCaps->binary, + &binaryhash) < 0) + goto cleanup; + + if (virAsprintf(&capsfile, "%s/%s.xml", capsdir, binaryhash) < 0) + goto cleanup; + + if (virFileMakePath(capsdir) < 0) { + virReportSystemError(errno, + _("Unable to create directory '%s'"), + capsdir); + goto cleanup; + } + + if (stat(capsfile, &sb) < 0) { + if (errno == ENOENT) { + VIR_DEBUG("No cached capabilities '%s' for '%s'", + capsfile, qemuCaps->binary); + ret = 0; + goto cleanup; + } + virReportSystemError(errno, + _("Unable to access cache '%s' for '%s'"), + capsfile, qemuCaps->binary); + goto cleanup; + } + + /* Discard if cache is older that QEMU binary */ + /* XXX must also compare to libvirtd timestamp */ + if (sb.st_mtime < qemuCaps->mtime) { + VIR_DEBUG("Outdated cached capabilities '%s' for '%s' (%lld vs %lld)", + capsfile, qemuCaps->binary, + (long long)sb.st_mtime, (long long)qemuCaps->mtime); + ignore_value(unlink(capsfile)); + ret = 0; + goto cleanup; + } + + if (virQEMUCapsLoadCache(qemuCaps, capsfile) < 0) { + virErrorPtr err = virGetLastError(); + VIR_WARN("Failed to load cached caps from '%s' for '%s': %s", + capsfile, qemuCaps->binary, err ? NULLSTR(err->message) : + _("unknown error")); + virResetLastError(); + ret = 0; + virQEMUCapsReset(qemuCaps); + goto cleanup; + } + + VIR_DEBUG("Loaded '%s' for '%s' mtime %lld usedQMP=%d", + capsfile, qemuCaps->binary, + (long long)qemuCaps->mtime, qemuCaps->usedQMP); + + ret = 1; + cleanup: + VIR_FREE(binaryhash); + VIR_FREE(capsfile); + VIR_FREE(capsdir); + return ret; +} + + #define QEMU_SYSTEM_PREFIX "qemu-system-" static int @@ -2779,6 +3161,7 @@ virQEMUCapsLogProbeFailure(const char *binary) virQEMUCapsPtr virQEMUCapsNewForBinary(const char *binary, const char *libDir, + const char *cacheDir, uid_t runUid, gid_t runGid) { @@ -2808,15 +3191,23 @@ virQEMUCapsPtr virQEMUCapsNewForBinary(const char *binary, goto error; } - if ((rv = virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid)) < 0) { - virQEMUCapsLogProbeFailure(binary); + if ((rv = virQEMUCapsInitCached(qemuCaps, cacheDir)) < 0) goto error; - } - if (!qemuCaps->usedQMP && - virQEMUCapsInitHelp(qemuCaps, runUid, runGid) < 0) { - virQEMUCapsLogProbeFailure(binary); - goto error; + if (rv == 0) { + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0) { + virQEMUCapsLogProbeFailure(binary); + goto error; + } + + if (!qemuCaps->usedQMP && + virQEMUCapsInitHelp(qemuCaps, runUid, runGid) < 0) { + virQEMUCapsLogProbeFailure(binary); + goto error; + } + + if (virQEMUCapsRememberCached(qemuCaps, cacheDir) < 0) + goto error; } return qemuCaps; @@ -2851,6 +3242,7 @@ virQEMUCapsHashDataFree(void *payload, const void *key ATTRIBUTE_UNUSED) virQEMUCapsCachePtr virQEMUCapsCacheNew(const char *libDir, + const char *cacheDir, uid_t runUid, gid_t runGid) { @@ -2870,6 +3262,8 @@ virQEMUCapsCacheNew(const char *libDir, goto error; if (VIR_STRDUP(cache->libDir, libDir) < 0) goto error; + if (VIR_STRDUP(cache->cacheDir, cacheDir) < 0) + goto error; cache->runUid = runUid; cache->runGid = runGid; @@ -2899,6 +3293,7 @@ virQEMUCapsCacheLookup(virQEMUCapsCachePtr cache, const char *binary) VIR_DEBUG("Creating capabilities for %s", binary); ret = virQEMUCapsNewForBinary(binary, cache->libDir, + cache->cacheDir, cache->runUid, cache->runGid); if (ret) { VIR_DEBUG("Caching capabilities %p for %s", @@ -2938,6 +3333,7 @@ virQEMUCapsCacheFree(virQEMUCapsCachePtr cache) return; VIR_FREE(cache->libDir); + VIR_FREE(cache->cacheDir); virHashFree(cache->binaries); virMutexDestroy(&cache->lock); VIR_FREE(cache); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index b5445e7..a9082d5 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -218,6 +218,7 @@ virQEMUCapsPtr virQEMUCapsNew(void); virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps); virQEMUCapsPtr virQEMUCapsNewForBinary(const char *binary, const char *libDir, + const char *cacheDir, uid_t runUid, gid_t runGid); @@ -262,6 +263,7 @@ bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps); virQEMUCapsCachePtr virQEMUCapsCacheNew(const char *libDir, + const char *cacheDir, uid_t uid, gid_t gid); virQEMUCapsPtr virQEMUCapsCacheLookup(virQEMUCapsCachePtr cache, const char *binary); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9aad2dc..51c9ed0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -744,6 +744,7 @@ qemuStateInitialize(bool privileged, } qemu_driver->qemuCapsCache = virQEMUCapsCacheNew(cfg->libDir, + cfg->cacheDir, run_uid, run_gid); if (!qemu_driver->qemuCapsCache) -- 1.8.5.3

+ /* Discard if cache is older that QEMU binary */ + /* XXX must also compare to libvirtd timestamp */ + if (sb.st_mtime < qemuCaps->mtime) { I think looking at the mtime isn't sufficent here. Tools like dpkg set
Hi Daniel, On Wed, Mar 05, 2014 at 05:53:53PM +0000, Daniel P. Berrange wrote: [..snip..] the mtime to the time the package was built at not the installation time so we might end up updating qemu but still having an mtime older than the time the xml was created (same holds for downgrades to older qemu versions) or are we simply requiring distributors to clean the cache upon package upgrades (which would mandate using a trigger)? Maybe we need to actually calculate the binaries checksum not only the pathname? We could then simply just wipe alle checksums upon libvirt upgrades to get rid of old entries. Cheers, -- Guido

On 03/05/2014 01:40 PM, Guido Günther wrote:
+ /* Discard if cache is older that QEMU binary */ + /* XXX must also compare to libvirtd timestamp */ + if (sb.st_mtime < qemuCaps->mtime) { I think looking at the mtime isn't sufficent here. Tools like dpkg set
Hi Daniel, On Wed, Mar 05, 2014 at 05:53:53PM +0000, Daniel P. Berrange wrote: [..snip..] the mtime to the time the package was built at not the installation time so we might end up updating qemu but still having an mtime older than the time the xml was created (same holds for downgrades to older qemu versions) or are we simply requiring distributors to clean the cache upon package upgrades (which would mandate using a trigger)?
ctime is better than mtime, because ctime can't be faked. On the other hand, ctime changes in more situations than mtime (chmod, anyone?) - but then again, any time ctime changes, we probably DO want to retest (a chmod may change our ability to execute a given binary). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Wed, Mar 05, 2014 at 09:40:34PM +0100, Guido Günther wrote:
+ /* Discard if cache is older that QEMU binary */ + /* XXX must also compare to libvirtd timestamp */ + if (sb.st_mtime < qemuCaps->mtime) { I think looking at the mtime isn't sufficent here. Tools like dpkg set
Hi Daniel, On Wed, Mar 05, 2014 at 05:53:53PM +0000, Daniel P. Berrange wrote: [..snip..] the mtime to the time the package was built at not the installation time so we might end up updating qemu but still having an mtime older than the time the xml was created (same holds for downgrades to older qemu versions) or are we simply requiring distributors to clean the cache upon package upgrades (which would mandate using a trigger)?
NB the existing code already has the flaw you speak of - we cache the capabilities in memory currently and only invalidate when mtime is outdated.
Maybe we need to actually calculate the binaries checksum not only the pathname? We could then simply just wipe alle checksums upon libvirt upgrades to get rid of old entries.
Checksumming the binary content is reasonably heavyweight, but we could just use ctime like Eric suggests Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 03/05/2014 10:53 AM, Daniel P. Berrange wrote:
Extracting capabilities from QEMU takes a notable amount of time when all QEMU binaries are installed. Each system emulator needs about 200-300ms multiplied by 26 binaries == ~5-8 seconds.
This change causes the QEMU driver to save an XML file containing the content of the virQEMUCaps object instance in the cache dir eg /var/cache/libvirt/qemu/capabilities/$SHA256(binarypath).xml or $HOME/.cache/libvirt/qemu/cache/capabilities/$SHA256(binarypath).xml
We attempt to load this and only if it fails, do we fallback to probing the QEMU binary. The timestamp of the file is compared to the timestamp of the QEMU binary and discarded if the QEMU binary is newer.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 412 ++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_driver.c | 1 + 3 files changed, 407 insertions(+), 8 deletions(-)
@@ -44,6 +45,7 @@ #include <unistd.h> #include <sys/wait.h> #include <stdarg.h> +#include <utime.h>
<utime.h> (and the utime() function) is deprecated by POSIX and therefore no longer portable because it corrupts sub-second timestamps. Better is to use gnulib's utimensat (or futimens). But what timestamps do we actually have to munge?
+ + ut.actime = qemuCaps->mtime; + ut.modtime = qemuCaps->mtime; + if (utime(filename, &ut) < 0) { + virReportSystemError(errno, + _("Failed to set mtime on '%s' for '%s'"), + filename, qemuCaps->binary);
Why not just store the qemu binary timestamp in the XML, rather than playing games with the mtime of the xml file?
+ + /* Discard if cache is older that QEMU binary */ + /* XXX must also compare to libvirtd timestamp */ + if (sb.st_mtime < qemuCaps->mtime) {
This ignores subsecond timestamps; you may want to use gnulib's stat-time module to do a more accurate check. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Wed, Mar 05, 2014 at 05:53:48PM -0700, Eric Blake wrote:
On 03/05/2014 10:53 AM, Daniel P. Berrange wrote:
Extracting capabilities from QEMU takes a notable amount of time when all QEMU binaries are installed. Each system emulator needs about 200-300ms multiplied by 26 binaries == ~5-8 seconds.
This change causes the QEMU driver to save an XML file containing the content of the virQEMUCaps object instance in the cache dir eg /var/cache/libvirt/qemu/capabilities/$SHA256(binarypath).xml or $HOME/.cache/libvirt/qemu/cache/capabilities/$SHA256(binarypath).xml
We attempt to load this and only if it fails, do we fallback to probing the QEMU binary. The timestamp of the file is compared to the timestamp of the QEMU binary and discarded if the QEMU binary is newer.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 412 ++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_driver.c | 1 + 3 files changed, 407 insertions(+), 8 deletions(-)
@@ -44,6 +45,7 @@ #include <unistd.h> #include <sys/wait.h> #include <stdarg.h> +#include <utime.h>
<utime.h> (and the utime() function) is deprecated by POSIX and therefore no longer portable because it corrupts sub-second timestamps. Better is to use gnulib's utimensat (or futimens). But what timestamps do we actually have to munge?
+ + ut.actime = qemuCaps->mtime; + ut.modtime = qemuCaps->mtime; + if (utime(filename, &ut) < 0) { + virReportSystemError(errno, + _("Failed to set mtime on '%s' for '%s'"), + filename, qemuCaps->binary);
Why not just store the qemu binary timestamp in the XML, rather than playing games with the mtime of the xml file?
I wanted to be able to detect out of date cache files without having to go through parsing them. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Currently the QEMU capabilities cache files on disk are only invalidated if the QEMU binary changes. New versions of libvirt, however, may want to extract more information from existing binaries. Thus we must take into account modification time of the libvirtd daemon and/or any loadable driver modules when checking cache validity Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/libvirtd.c | 2 ++ src/driver.c | 2 ++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_capabilities.c | 24 +++++++++++++++--------- src/util/virutil.c | 23 +++++++++++++++++++++++ src/util/virutil.h | 4 ++++ 6 files changed, 48 insertions(+), 9 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 72f0e81..d8226a4 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1152,6 +1152,8 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } + virUpdateSelfLastModified(argv[0]); + if (strstr(argv[0], "lt-libvirtd") || strstr(argv[0], "/daemon/.libs/libvirtd")) { char *tmp = strrchr(argv[0], '/'); diff --git a/src/driver.c b/src/driver.c index ab2a253..2acf39c 100644 --- a/src/driver.c +++ b/src/driver.c @@ -74,6 +74,8 @@ virDriverLoadModule(const char *name) goto cleanup; } + virUpdateSelfLastModified(modfile); + handle = dlopen(modfile, RTLD_NOW | RTLD_GLOBAL); if (!handle) { VIR_ERROR(_("failed to load module %s %s"), modfile, dlerror()); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 00e3d9c..82a8918 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1954,6 +1954,7 @@ virGetGroupID; virGetGroupList; virGetGroupName; virGetHostname; +virGetSelfLastModified; virGetUnprivSGIOSysfsPath; virGetUserCacheDirectory; virGetUserConfigDirectory; @@ -1983,6 +1984,7 @@ virSetNonBlock; virSetUIDGID; virSetUIDGIDWithCaps; virStrIsPrint; +virUpdateSelfLastModified; virValidateWWN; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e3ed960..3dfaf48 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2564,18 +2564,21 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) goto cleanup; } - ut.actime = qemuCaps->mtime; - ut.modtime = qemuCaps->mtime; + ut.modtime = virGetSelfLastModified(); + if (qemuCaps->mtime > ut.modtime) + ut.modtime = qemuCaps->mtime; + + ut.actime = ut.modtime; if (utime(filename, &ut) < 0) { virReportSystemError(errno, - _("Failed to set mtime on '%s' for '%s'"), - filename, qemuCaps->binary); + _("Failed to set mtime on '%s' for '%s' to %lld"), + filename, qemuCaps->binary, (long long)ut.modtime); ignore_value(unlink(filename)); goto cleanup; } VIR_DEBUG("Saved caps '%s' for '%s' with %lld", - filename, qemuCaps->binary, (long long)qemuCaps->mtime); + filename, qemuCaps->binary, (long long)ut.modtime); ret = 0; cleanup: @@ -2654,6 +2657,7 @@ virQEMUCapsInitCached(virQEMUCapsPtr qemuCaps, const char *cacheDir) int ret = -1; char *binaryhash = NULL; struct stat sb; + time_t selfmtime = virGetSelfLastModified(); if (virAsprintf(&capsdir, "%s/capabilities", cacheDir) < 0) goto cleanup; @@ -2687,11 +2691,13 @@ virQEMUCapsInitCached(virQEMUCapsPtr qemuCaps, const char *cacheDir) } /* Discard if cache is older that QEMU binary */ - /* XXX must also compare to libvirtd timestamp */ - if (sb.st_mtime < qemuCaps->mtime) { - VIR_DEBUG("Outdated cached capabilities '%s' for '%s' (%lld vs %lld)", + if (sb.st_mtime < qemuCaps->mtime || + sb.st_mtime < selfmtime) { + VIR_DEBUG("Outdated cached capabilities '%s' for '%s' (%lld,%lld vs %lld)", capsfile, qemuCaps->binary, - (long long)sb.st_mtime, (long long)qemuCaps->mtime); + (long long)sb.st_mtime, + (long long)selfmtime, + (long long)qemuCaps->mtime); ignore_value(unlink(capsfile)); ret = 0; goto cleanup; diff --git a/src/util/virutil.c b/src/util/virutil.c index 7a2fbb0..0e3fbd7 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2173,3 +2173,26 @@ bool virIsSUID(void) { return getuid() != geteuid(); } + + +static time_t selfLastModified; + +time_t virGetSelfLastModified(void) +{ + return selfLastModified; +} + + +void virUpdateSelfLastModified(const char *path) +{ + struct stat sb; + + if (stat(path, &sb) < 0) + return; + + if (sb.st_mtime > selfLastModified) { + VIR_DEBUG("Setting self last modified to %lld for '%s'", + (long long)sb.st_mtime, path); + selfLastModified = sb.st_mtime; + } +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 029265c..0bf7687 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -194,4 +194,8 @@ const char *virGetEnvBlockSUID(const char *name); const char *virGetEnvAllowSUID(const char *name); bool virIsSUID(void); + +time_t virGetSelfLastModified(void); +void virUpdateSelfLastModified(const char *path); + #endif /* __VIR_UTIL_H__ */ -- 1.8.5.3

On 03/05/2014 10:53 AM, Daniel P. Berrange wrote:
Currently the QEMU capabilities cache files on disk are only invalidated if the QEMU binary changes. New versions of libvirt, however, may want to extract more information from existing binaries. Thus we must take into account modification time of the libvirtd daemon and/or any loadable driver modules when checking cache validity
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
+++ b/src/qemu/qemu_capabilities.c @@ -2564,18 +2564,21 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) goto cleanup; }
- ut.actime = qemuCaps->mtime; - ut.modtime = qemuCaps->mtime; + ut.modtime = virGetSelfLastModified(); + if (qemuCaps->mtime > ut.modtime) + ut.modtime = qemuCaps->mtime; +
I'm still thinking that mtime is a bit dangerous, compared to ctime.
+ ut.actime = ut.modtime; if (utime(filename, &ut) < 0) { virReportSystemError(errno, - _("Failed to set mtime on '%s' for '%s'"), - filename, qemuCaps->binary); + _("Failed to set mtime on '%s' for '%s' to %lld"), + filename, qemuCaps->binary, (long long)ut.modtime);
If you go by ctime, you can't use utime() (or the modern futimens counterpart). I'm also still not convinced that just storing the time in the xml file is going to be any slower than playing file timestamp games. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange
-
Eric Blake
-
Guido Günther