[libvirt] [PATCH 00/11] introduce virFileCache and use it for qemu capabilities
Pavel Hrdina (11): util/virhash: add name parameter to virHashSearch util: introduce virFileCache tests: add virfilecachetest qemu: move libvirt ctime and version into _virQEMUCaps struct qemu: move virQEMUCapsIsValid before its usage and make it static qemu: move libvirt ctime and version check into virQEMUCapsIsValid qemu: don't pass qemuctime into virQEMUCapsIsValid qemu: separate virQEMUCapsInitCached out of virQEMUCapsNewForBinaryInternal qemu: introduce struct _virQEMUCapsCachePriv qemu: switch QEMU capabilities to use virFileCache qemu: privatize _virQEMUCapsCachePriv struct po/POTFILES.in | 1 + src/Makefile.am | 1 + src/conf/virdomainobjlist.c | 2 +- src/conf/virnetworkobj.c | 4 +- src/conf/virsecretobj.c | 2 +- src/libvirt_private.syms | 9 + src/qemu/qemu_capabilities.c | 517 +++++++-------------- src/qemu/qemu_capabilities.h | 22 +- src/qemu/qemu_capspriv.h | 18 +- src/qemu/qemu_conf.h | 3 +- src/qemu/qemu_driver.c | 2 +- src/util/virfilecache.c | 391 ++++++++++++++++ src/util/virfilecache.h | 136 ++++++ src/util/virhash.c | 11 +- src/util/virhash.h | 2 +- src/xen/xm_internal.c | 5 +- tests/Makefile.am | 12 + tests/qemucapabilitiestest.c | 4 +- tests/qemucapsprobe.c | 2 +- tests/testutilsqemu.c | 14 +- tests/testutilsqemu.h | 3 +- ...a15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache | 1 + ...ae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache | 1 + tests/virfilecachetest.c | 238 ++++++++++ tests/virhashtest.c | 2 +- 25 files changed, 1008 insertions(+), 395 deletions(-) create mode 100644 src/util/virfilecache.c create mode 100644 src/util/virfilecache.h create mode 100644 tests/virfilecachedata/5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache create mode 120000 tests/virfilecachedata/9ca150bf3119b75dcac8e8bae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache create mode 100644 tests/virfilecachetest.c -- 2.13.2
While searching for an element using a function it may be desirable to know the element key for future operation. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/virdomainobjlist.c | 2 +- src/conf/virnetworkobj.c | 4 ++-- src/conf/virsecretobj.c | 2 +- src/qemu/qemu_capabilities.c | 5 +++-- src/util/virhash.c | 11 ++++++++--- src/util/virhash.h | 2 +- src/xen/xm_internal.c | 5 +++-- tests/virhashtest.c | 2 +- 8 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index c121382bcb..7bd4cd29b9 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -119,7 +119,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms, { virDomainObjPtr obj; virObjectLock(doms); - obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id); + obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL); if (ref) { virObjectRef(obj); virObjectUnlock(doms); diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index 88e42b5b3e..ccde72e727 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -208,7 +208,7 @@ virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, { virNetworkObjPtr ret = NULL; - ret = virHashSearch(nets->objs, virNetworkObjSearchName, name); + ret = virHashSearch(nets->objs, virNetworkObjSearchName, name, NULL); if (ret) virObjectRef(ret); return ret; @@ -980,7 +980,7 @@ virNetworkObjBridgeInUse(virNetworkObjListPtr nets, struct virNetworkObjBridgeInUseHelperData data = {bridge, skipname}; virObjectLock(nets); - obj = virHashSearch(nets->objs, virNetworkObjBridgeInUseHelper, &data); + obj = virHashSearch(nets->objs, virNetworkObjBridgeInUseHelper, &data, NULL); virObjectUnlock(nets); return obj != NULL; diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 0311335ba3..8ba8784d1e 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -251,7 +251,7 @@ virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets, struct virSecretSearchData data = { .usageType = usageType, .usageID = usageID }; - obj = virHashSearch(secrets->objs, virSecretObjSearchName, &data); + obj = virHashSearch(secrets->objs, virSecretObjSearchName, &data, NULL); if (obj) virObjectRef(obj); return obj; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 61c9a10661..6659744a34 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5504,14 +5504,15 @@ virQEMUCapsCacheLookupByArch(virCapsPtr caps, struct virQEMUCapsSearchData data = { .arch = arch }; virMutexLock(&cache->lock); - ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, &data); + ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, &data, NULL); if (!ret) { /* If the first attempt at finding capabilities has failed, try * again using the QEMU target as lookup key instead */ target = virQEMUCapsFindTarget(virArchFromHost(), data.arch); if (target != data.arch) { data.arch = target; - ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, &data); + ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, + &data, NULL); } } diff --git a/src/util/virhash.c b/src/util/virhash.c index a8e0edfd3b..7fa2992f18 100644 --- a/src/util/virhash.c +++ b/src/util/virhash.c @@ -702,15 +702,18 @@ virHashRemoveAll(virHashTablePtr table) * @table: the hash table to search * @iter: an iterator to identify the desired element * @data: extra opaque information passed to the iter + * @name: the name of found user data, pass NULL to ignore * * Iterates over the hash table calling the 'iter' callback * for each element. The first element for which the iter * returns non-zero will be returned by this function. - * The elements are processed in a undefined order + * The elements are processed in a undefined order. Caller is + * responsible for freeing the @name. */ void *virHashSearch(const virHashTable *ctable, virHashSearcher iter, - const void *data) + const void *data, + void **name) { size_t i; @@ -730,6 +733,8 @@ void *virHashSearch(const virHashTable *ctable, for (entry = table->table[i]; entry; entry = entry->next) { if (iter(entry->payload, entry->name, data)) { table->iterating = false; + if (name) + *name = table->keyCopy(entry->name); return entry->payload; } } @@ -824,7 +829,7 @@ bool virHashEqual(const virHashTable *table1, virHashSize(table1) != virHashSize(table2)) return false; - virHashSearch(table1, virHashEqualSearcher, &data); + virHashSearch(table1, virHashEqualSearcher, &data, NULL); return data.equal; } diff --git a/src/util/virhash.h b/src/util/virhash.h index 61c172b9e0..00b2550e70 100644 --- a/src/util/virhash.h +++ b/src/util/virhash.h @@ -194,7 +194,7 @@ bool virHashEqual(const virHashTable *table1, int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data); ssize_t virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, const void *data); void *virHashSearch(const virHashTable *table, virHashSearcher iter, - const void *data); + const void *data, void **name); /* Convenience for when VIR_FREE(value) is sufficient as a data freer. */ void virHashValueFree(void *value, const void *name); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index fba814aa19..123f379572 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -880,7 +880,8 @@ xenXMDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) if (!xenInotifyActive(conn) && xenXMConfigCacheRefresh(conn) < 0) goto cleanup; - if (!(entry = virHashSearch(priv->configCache, xenXMDomainSearchForUUID, (const void *)uuid))) + if (!(entry = virHashSearch(priv->configCache, xenXMDomainSearchForUUID, + (const void *)uuid, NULL))) goto cleanup; ret = virDomainDefNewFull(entry->def->name, uuid, -1); @@ -971,7 +972,7 @@ xenXMDomainDefineXML(virConnectPtr conn, virDomainDefPtr def) * it has the same name */ if ((entry = virHashSearch(priv->configCache, xenXMDomainSearchForUUID, - (const void *)&(def->uuid))) != NULL) { + (const void *)&(def->uuid), NULL)) != NULL) { if ((entry->def != NULL) && (entry->def->name != NULL) && (STRNEQ(def->name, entry->def->name))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; diff --git a/tests/virhashtest.c b/tests/virhashtest.c index dbf07ae7f7..9407f98c4b 100644 --- a/tests/virhashtest.c +++ b/tests/virhashtest.c @@ -436,7 +436,7 @@ testHashSearch(const void *data ATTRIBUTE_UNUSED) if (!(hash = testHashInit(0))) return -1; - entry = virHashSearch(hash, testHashSearchIter, NULL); + entry = virHashSearch(hash, testHashSearchIter, NULL, NULL); if (!entry || STRNEQ(uuids_subset[testSearchIndex], entry)) { VIR_TEST_VERBOSE("\nvirHashSearch didn't find entry '%s'\n", -- 2.13.2
On Mon, Jul 10, 2017 at 14:46:40 +0200, Pavel Hrdina wrote:
While searching for an element using a function it may be desirable to know the element key for future operation.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/virdomainobjlist.c | 2 +- src/conf/virnetworkobj.c | 4 ++-- src/conf/virsecretobj.c | 2 +- src/qemu/qemu_capabilities.c | 5 +++-- src/util/virhash.c | 11 ++++++++--- src/util/virhash.h | 2 +- src/xen/xm_internal.c | 5 +++-- tests/virhashtest.c | 2 +- 8 files changed, 20 insertions(+), 13 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The new virFileCache will nicely handle the caching logic for any data that we would like to cache. For each type of data we will just need to implement few handlers that will take care of creating, validating, loading and saving the cached data. The cached data must be an instance of virObject. Currently we cache QEMU capabilities which will start using virFileCache. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 9 ++ src/util/virfilecache.c | 391 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfilecache.h | 136 +++++++++++++++++ 5 files changed, 538 insertions(+) create mode 100644 src/util/virfilecache.c create mode 100644 src/util/virfilecache.h diff --git a/po/POTFILES.in b/po/POTFILES.in index 275df1f297..ae81a57220 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -209,6 +209,7 @@ src/util/vireventpoll.c src/util/virfcp.c src/util/virfdstream.c src/util/virfile.c +src/util/virfilecache.c src/util/virfirewall.c src/util/virfirmware.c src/util/virhash.c diff --git a/src/Makefile.am b/src/Makefile.am index eae32dc58a..a58118db67 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -194,6 +194,7 @@ UTIL_SOURCES = \ util/virxdrdefs.h \ util/virxml.c util/virxml.h \ util/virmdev.c util/virmdev.h \ + util/virfilecache.c util/virfilecache.h \ $(NULL) EXTRA_DIST += \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 888412ac78..054a00c1d1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1707,6 +1707,15 @@ virFileWriteStr; virFindFileInPath; +# util/virfilecache.h +virFileCacheGetPriv; +virFileCacheInsertData; +virFileCacheLookup; +virFileCacheLookupByFunc; +virFileCacheNew; +virFileCacheSetPriv; + + # util/virfirewall.h virFirewallAddRuleFull; virFirewallApply; diff --git a/src/util/virfilecache.c b/src/util/virfilecache.c new file mode 100644 index 0000000000..0b0e361daa --- /dev/null +++ b/src/util/virfilecache.c @@ -0,0 +1,391 @@ +/* + * virfilecache.c: file caching for data + * + * Copyright (C) 2017 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 "internal.h" + +#include "viralloc.h" +#include "vircrypto.h" +#include "virerror.h" +#include "virfile.h" +#include "virfilecache.h" +#include "virhash.h" +#include "virlog.h" +#include "virobject.h" +#include "virstring.h" + +#include <sys/stat.h> +#include <sys/types.h> +#include <unistd.h> + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.filecache") + + +struct _virFileCache { + virObjectLockable object; + + virHashTablePtr table; + + char *dir; + void *priv; + + virFileCacheHandlers handlers; +}; + + +static virClassPtr virFileCacheClass; + + +static void +virFileCacheDispose(void *obj) +{ + virFileCachePtr cache = obj; + + VIR_FREE(cache->dir); + + virHashFree(cache->table); + + if (cache->priv && cache->handlers.privFree) + cache->handlers.privFree(cache->priv); +} + + +static int +virFileCacheOnceInit(void) +{ + if (!(virFileCacheClass = virClassNew(virClassForObjectLockable(), + "virFileCache", + sizeof(virFileCache), + virFileCacheDispose))) + return -1; + + return 0; +} + + +VIR_ONCE_GLOBAL_INIT(virFileCache) + + +static char * +virFileCacheGetFileName(virFileCachePtr cache, + const char *name) +{ + char *file = NULL; + char *namehash = NULL; + + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &namehash) < 0) + goto cleanup; + + if (virAsprintf(&file, "%s/%s.cache", cache->dir, namehash) < 0) + goto cleanup; + + if (virFileMakePath(cache->dir) < 0) { + virReportSystemError(errno, + _("Unable to create directory '%s'"), + cache->dir); + VIR_FREE(file); + file = NULL; + } + + cleanup: + VIR_FREE(namehash); + return file; +} + + +static int +virFileCacheLoad(virFileCachePtr cache, + const char *name, + void **data) +{ + char *file = NULL; + int ret = -1; + + if (!(file = virFileCacheGetFileName(cache, name))) + return ret; + + if (!virFileExists(file)) { + if (errno == ENOENT) { + VIR_DEBUG("No cached data '%s' for '%s'", file, name); + ret = 0; + goto cleanup; + } + virReportSystemError(errno, + _("Unable to access cache '%s' for '%s'"), + file, name); + goto cleanup; + } + + if (!(*data = cache->handlers.loadFile(file, name, cache->priv))) { + virErrorPtr err = virGetLastError(); + VIR_WARN("Failed to load cached data from '%s' for '%s': %s", + file, name, err ? NULLSTR(err->message) : "unknown error"); + virResetLastError(); + ret = 0; + goto cleanup; + } + + if (!cache->handlers.isValid(*data, cache->priv)) { + VIR_DEBUG("Outdated cached capabilities '%s' for '%s'", file, name); + ignore_value(unlink(file)); + ret = 0; + goto cleanup; + } + + VIR_DEBUG("Loaded cached data '%s' for '%s'", file, name); + + ret = 1; + + cleanup: + VIR_FREE(file); + return ret; +} + + +static int +virFileCacheSave(virFileCachePtr cache, + const char *name, + void *data) +{ + char *file = NULL; + int ret = -1; + + if (!(file = virFileCacheGetFileName(cache, name))) + return ret; + + if (cache->handlers.saveFile(data, file, cache->priv) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(file); + return ret; +} + + +static void * +virFileCacheNewData(virFileCachePtr cache, + const char *name) +{ + void *data = NULL; + int rv; + + if ((rv = virFileCacheLoad(cache, name, &data)) < 0) + goto cleanup; + + if (rv == 0) { + virObjectUnref(data); + if (!(data = cache->handlers.newData(name, cache->priv))) + goto cleanup; + + if (virFileCacheSave(cache, name, data) < 0) { + virObjectUnref(data); + data = NULL; + } + } + + cleanup: + return data; +} + + +/** + * virFileCacheNew: + * @dir: the cache directory where all the cache files will be stored + * @handlers: filled structure with all required handlers + * + * Creates a new cache object which handles caching any data to files + * stored on a filesystem. + * + * Returns new cache object or NULL on error. + */ +virFileCachePtr +virFileCacheNew(const char *dir, + virFileCacheHandlers handlers) +{ + virFileCachePtr cache; + + if (virFileCacheInitialize() < 0) + return NULL; + + if (!(cache = virObjectNew(virFileCacheClass))) + return NULL; + + if (!(cache->table = virHashCreate(10, virObjectFreeHashData))) + goto cleanup; + + if (VIR_STRDUP(cache->dir, dir) < 0) + goto cleanup; + + cache->handlers = handlers; + + return cache; + + cleanup: + virObjectUnref(cache); + return NULL; +} + + +static void +virFileCacheValidate(virFileCachePtr cache, + const char *name, + void **data) +{ + if (*data && !cache->handlers.isValid(*data, cache->priv)) { + VIR_DEBUG("Cached data '%p' no longer valid for '%s'", + *data, NULLSTR(name)); + if (name) + virHashRemoveEntry(cache->table, name); + *data = NULL; + } + + if (!*data && name) { + VIR_DEBUG("Creating data for '%s'", name); + *data = virFileCacheNewData(cache, name); + if (*data) { + VIR_DEBUG("Caching data '%p' for '%s'", *data, name); + if (virHashAddEntry(cache->table, name, *data) < 0) { + virObjectUnref(*data); + *data = NULL; + } + } + } +} + + +/** + * virFileCacheLookup: + * @cache: existing cache object + * @name: name of the data stored in a cache + * + * Lookup a data specified by name. This tries to find a file with + * cached data, if it doesn't exist or is no longer valid new data + * is created. + * + * Returns data object or NULL on error. The caller is responsible for + * unrefing the data. + */ +void * +virFileCacheLookup(virFileCachePtr cache, + const char *name) +{ + void *data = NULL; + + virObjectLock(cache); + + data = virHashLookup(cache->table, name); + virFileCacheValidate(cache, name, &data); + + virObjectRef(data); + virObjectUnlock(cache); + + return data; +} + + +/** + * virFileCacheLookupByFunc: + * @cache: existing cache object + * @iter: an iterator to identify the desired data + * @iterData: extra opaque information passed to the @iter + * + * Similar to virFileCacheLookup() except it search by @iter. + * + * Returns data object or NULL on error. The caller is responsible for + * unrefing the data. + */ +void * +virFileCacheLookupByFunc(virFileCachePtr cache, + virHashSearcher iter, + const void *iterData) +{ + void *data = NULL; + char *name = NULL; + + virObjectLock(cache); + + data = virHashSearch(cache->table, iter, iterData, (void **)&name); + virFileCacheValidate(cache, name, &data); + + virObjectRef(data); + virObjectUnlock(cache); + + VIR_FREE(name); + + return data; +} + + +/** + * virFileCacheGetPriv: + * @cache: existing cache object + * + * Returns private data used by @handlers. + */ +void * +virFileCacheGetPriv(virFileCachePtr cache) +{ + return cache->priv; +} + + +/** + * virFileCacheSetPriv: + * @cache: existing cache object + * @priv: private data to set + * + * Sets private data used by @handlers. + */ +void +virFileCacheSetPriv(virFileCachePtr cache, + void *priv) +{ + cache->priv = priv; +} + + +/** + * virFileCacheInsertData: + * @cache: existing cache object + * @name: name of the new data + * @data: the actual data object to store in cache + * + * Adds a new data into a cache but doesn't store the data into + * a file. This function should be used only by testing code. + * + * Returns 0 on success, -1 on error. + */ +int +virFileCacheInsertData(virFileCachePtr cache, + const char *name, + void *data) +{ + if (virHashUpdateEntry(cache->table, name, data) < 0) + return -1; + + return 0; +} diff --git a/src/util/virfilecache.h b/src/util/virfilecache.h new file mode 100644 index 0000000000..3447616361 --- /dev/null +++ b/src/util/virfilecache.h @@ -0,0 +1,136 @@ +/* + * virfilecache.h: file caching for data + * + * Copyright (C) 2017 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_FILE_CACHE_H__ +# define __VIR_FILE_CACHE_H__ + +# include "internal.h" + +# include "virobject.h" +# include "virhash.h" + +typedef struct _virFileCache virFileCache; +typedef virFileCache *virFileCachePtr; + +/** + * virFileCacheIsValidPtr: + * @data: data object to validate + * @priv: private data created together with cache + * + * Validates the cached data whether it needs to be refreshed + * or no. + * + * Returns *true* if it's valid or *false* if not valid. + */ +typedef bool +(*virFileCacheIsValidPtr)(void *data, + void *priv); + +/** + * virFileCacheNewDataPtr: + * @name: name of the new data + * @priv: private data created together with cache + * + * Creates a new data based on the @name. The returned data must be + * an instance of virObject. + * + * Returns data object or NULL on error. + */ +typedef void * +(*virFileCacheNewDataPtr)(const char *name, + void *priv); + +/** + * virFileCacheLoadFilePtr: + * @filename: name of a file with cached data + * @name: name of the cached data + * @priv: private data created together with cache + * + * Loads the cached data from a file @filename. + * + * Returns cached data object or NULL on error. + */ +typedef void * +(*virFileCacheLoadFilePtr)(const char *filename, + const char *name, + void *priv); + +/** + * virFileCacheSaveFilePtr: + * @data: data object to save into a file + * @filename: name of the file where to store the cached data + * @priv: private data created together with cache + * + * Stores the cached to a file @filename. + * + * Returns 0 on success, -1 on error. + */ +typedef int +(*virFileCacheSaveFilePtr)(void *data, + const char *filename, + void *priv); + +/** + * virFileCachePrivFreePtr: + * @priv: private data created together with cache + * + * This is used to free the private data when the cache object + * is removed. + */ +typedef void +(*virFileCachePrivFreePtr)(void *priv); + +typedef struct _virFileCacheHandlers virFileCacheHandlers; +typedef virFileCacheHandlers *virFileCacheHandlersPtr; +struct _virFileCacheHandlers { + virFileCacheIsValidPtr isValid; + virFileCacheNewDataPtr newData; + virFileCacheLoadFilePtr loadFile; + virFileCacheSaveFilePtr saveFile; + virFileCachePrivFreePtr privFree; +}; + +virFileCachePtr +virFileCacheNew(const char *dir, + virFileCacheHandlers handlers); + +void * +virFileCacheLookup(virFileCachePtr cache, + const char *name); + +void * +virFileCacheLookupByFunc(virFileCachePtr cache, + virHashSearcher iter, + const void *iterData); + +void * +virFileCacheGetPriv(virFileCachePtr cache); + +void +virFileCacheSetPriv(virFileCachePtr cache, + void *priv); + +int +virFileCacheInsertData(virFileCachePtr cache, + const char *name, + void *data); + +#endif /* __VIR_FILE_CACHE_H__ */ -- 2.13.2
On Mon, Jul 10, 2017 at 14:46:41 +0200, Pavel Hrdina wrote:
The new virFileCache will nicely handle the caching logic for any data that we would like to cache. For each type of data we will just need to implement few handlers that will take care of creating, validating, loading and saving the cached data.
The cached data must be an instance of virObject.
Currently we cache QEMU capabilities which will start using virFileCache.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 9 ++ src/util/virfilecache.c | 391 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfilecache.h | 136 +++++++++++++++++ 5 files changed, 538 insertions(+) create mode 100644 src/util/virfilecache.c create mode 100644 src/util/virfilecache.h
diff --git a/po/POTFILES.in b/po/POTFILES.in index 275df1f297..ae81a57220 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -209,6 +209,7 @@ src/util/vireventpoll.c src/util/virfcp.c src/util/virfdstream.c src/util/virfile.c +src/util/virfilecache.c src/util/virfirewall.c src/util/virfirmware.c src/util/virhash.c diff --git a/src/Makefile.am b/src/Makefile.am index eae32dc58a..a58118db67 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -194,6 +194,7 @@ UTIL_SOURCES = \ util/virxdrdefs.h \ util/virxml.c util/virxml.h \ util/virmdev.c util/virmdev.h \ + util/virfilecache.c util/virfilecache.h \ $(NULL)
EXTRA_DIST += \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 888412ac78..054a00c1d1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1707,6 +1707,15 @@ virFileWriteStr; virFindFileInPath;
+# util/virfilecache.h +virFileCacheGetPriv; +virFileCacheInsertData; +virFileCacheLookup; +virFileCacheLookupByFunc; +virFileCacheNew; +virFileCacheSetPriv; + + # util/virfirewall.h virFirewallAddRuleFull; virFirewallApply; diff --git a/src/util/virfilecache.c b/src/util/virfilecache.c new file mode 100644 index 0000000000..0b0e361daa --- /dev/null +++ b/src/util/virfilecache.c @@ -0,0 +1,391 @@ +/* + * virfilecache.c: file caching for data + * + * Copyright (C) 2017 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 "internal.h" + +#include "viralloc.h" +#include "vircrypto.h" +#include "virerror.h" +#include "virfile.h" +#include "virfilecache.h" +#include "virhash.h" +#include "virlog.h" +#include "virobject.h" +#include "virstring.h" + +#include <sys/stat.h> +#include <sys/types.h> +#include <unistd.h> + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.filecache") + + +struct _virFileCache { + virObjectLockable object; + + virHashTablePtr table; + + char *dir; + void *priv; + + virFileCacheHandlers handlers; +}; + + +static virClassPtr virFileCacheClass; + + +static void +virFileCacheDispose(void *obj) +{ + virFileCachePtr cache = obj; + + VIR_FREE(cache->dir); + + virHashFree(cache->table); + + if (cache->priv && cache->handlers.privFree) + cache->handlers.privFree(cache->priv); +} + + +static int +virFileCacheOnceInit(void) +{ + if (!(virFileCacheClass = virClassNew(virClassForObjectLockable(), + "virFileCache", + sizeof(virFileCache), + virFileCacheDispose))) + return -1; + + return 0; +} + + +VIR_ONCE_GLOBAL_INIT(virFileCache) + + +static char * +virFileCacheGetFileName(virFileCachePtr cache, + const char *name) +{ + char *file = NULL; + char *namehash = NULL; + + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &namehash) < 0) + goto cleanup; + + if (virAsprintf(&file, "%s/%s.cache", cache->dir, namehash) < 0) + goto cleanup;
This changes the filename used by QEMU capabilities cache from *.xml to *.cache. While it shouldn't really matter, the code would just ignore the old cache files and create new ones leaving mess in the directory. Can we change the APIs so that every user of virFileCache can decide what suffix (if any) should be used for the cache files?
+ + if (virFileMakePath(cache->dir) < 0) { + virReportSystemError(errno, + _("Unable to create directory '%s'"), + cache->dir); + VIR_FREE(file); + file = NULL; + } + + cleanup: + VIR_FREE(namehash); + return file; +} + + +static int +virFileCacheLoad(virFileCachePtr cache, + const char *name, + void **data) +{ + char *file = NULL; + int ret = -1; + + if (!(file = virFileCacheGetFileName(cache, name))) + return ret; + + if (!virFileExists(file)) { + if (errno == ENOENT) { + VIR_DEBUG("No cached data '%s' for '%s'", file, name); + ret = 0; + goto cleanup; + } + virReportSystemError(errno, + _("Unable to access cache '%s' for '%s'"), + file, name); + goto cleanup; + } + + if (!(*data = cache->handlers.loadFile(file, name, cache->priv))) { + virErrorPtr err = virGetLastError(); + VIR_WARN("Failed to load cached data from '%s' for '%s': %s", + file, name, err ? NULLSTR(err->message) : "unknown error"); + virResetLastError(); + ret = 0; + goto cleanup; + } + + if (!cache->handlers.isValid(*data, cache->priv)) { + VIR_DEBUG("Outdated cached capabilities '%s' for '%s'", file, name); + ignore_value(unlink(file)); + ret = 0; + goto cleanup; + } + + VIR_DEBUG("Loaded cached data '%s' for '%s'", file, name); + + ret = 1; + + cleanup: + VIR_FREE(file); + return ret; +} + + +static int +virFileCacheSave(virFileCachePtr cache, + const char *name, + void *data) +{ + char *file = NULL; + int ret = -1; + + if (!(file = virFileCacheGetFileName(cache, name))) + return ret; + + if (cache->handlers.saveFile(data, file, cache->priv) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(file); + return ret; +} + + +static void * +virFileCacheNewData(virFileCachePtr cache, + const char *name) +{ + void *data = NULL; + int rv; + + if ((rv = virFileCacheLoad(cache, name, &data)) < 0) + goto cleanup; + + if (rv == 0) { + virObjectUnref(data);
I think this should go inside virFileCacheLoad, i.e., the function should fill in valid data pointer only if it returns 1, otherwise the caller should get NULL in data.
+ if (!(data = cache->handlers.newData(name, cache->priv))) + goto cleanup; + + if (virFileCacheSave(cache, name, data) < 0) { + virObjectUnref(data); + data = NULL; + } + } + + cleanup: + return data; +} + + +/** + * virFileCacheNew: + * @dir: the cache directory where all the cache files will be stored + * @handlers: filled structure with all required handlers + * + * Creates a new cache object which handles caching any data to files + * stored on a filesystem. + * + * Returns new cache object or NULL on error. + */ +virFileCachePtr +virFileCacheNew(const char *dir,
The file name suffix could go here as an extra parameter.
+ virFileCacheHandlers handlers) +{ + virFileCachePtr cache; + + if (virFileCacheInitialize() < 0) + return NULL; + + if (!(cache = virObjectNew(virFileCacheClass))) + return NULL; + + if (!(cache->table = virHashCreate(10, virObjectFreeHashData))) + goto cleanup; + + if (VIR_STRDUP(cache->dir, dir) < 0) + goto cleanup; + + cache->handlers = handlers; + + return cache; + + cleanup: + virObjectUnref(cache); + return NULL; +} + + +static void +virFileCacheValidate(virFileCachePtr cache, + const char *name, + void **data) +{ + if (*data && !cache->handlers.isValid(*data, cache->priv)) { + VIR_DEBUG("Cached data '%p' no longer valid for '%s'", + *data, NULLSTR(name)); + if (name) + virHashRemoveEntry(cache->table, name); + *data = NULL; + } + + if (!*data && name) { + VIR_DEBUG("Creating data for '%s'", name); + *data = virFileCacheNewData(cache, name); + if (*data) { + VIR_DEBUG("Caching data '%p' for '%s'", *data, name); + if (virHashAddEntry(cache->table, name, *data) < 0) { + virObjectUnref(*data); + *data = NULL; + } + } + } +} + + +/** + * virFileCacheLookup: + * @cache: existing cache object + * @name: name of the data stored in a cache + * + * Lookup a data specified by name. This tries to find a file with + * cached data, if it doesn't exist or is no longer valid new data + * is created. + * + * Returns data object or NULL on error. The caller is responsible for + * unrefing the data. + */ +void * +virFileCacheLookup(virFileCachePtr cache, + const char *name) +{ + void *data = NULL; + + virObjectLock(cache); + + data = virHashLookup(cache->table, name); + virFileCacheValidate(cache, name, &data); + + virObjectRef(data); + virObjectUnlock(cache); + + return data; +} + + +/** + * virFileCacheLookupByFunc: + * @cache: existing cache object + * @iter: an iterator to identify the desired data + * @iterData: extra opaque information passed to the @iter + * + * Similar to virFileCacheLookup() except it search by @iter. + * + * Returns data object or NULL on error. The caller is responsible for + * unrefing the data. + */ +void * +virFileCacheLookupByFunc(virFileCachePtr cache, + virHashSearcher iter, + const void *iterData) +{ + void *data = NULL; + char *name = NULL; + + virObjectLock(cache); + + data = virHashSearch(cache->table, iter, iterData, (void **)&name); + virFileCacheValidate(cache, name, &data); + + virObjectRef(data); + virObjectUnlock(cache); + + VIR_FREE(name); + + return data; +} + + +/** + * virFileCacheGetPriv: + * @cache: existing cache object + * + * Returns private data used by @handlers. + */ +void * +virFileCacheGetPriv(virFileCachePtr cache) +{
No virObjectLock(cache)?
+ return cache->priv; +} + + +/** + * virFileCacheSetPriv: + * @cache: existing cache object + * @priv: private data to set + * + * Sets private data used by @handlers. + */ +void +virFileCacheSetPriv(virFileCachePtr cache, + void *priv) +{
No virObjectLock(cache)?
+ cache->priv = priv; +} + + +/** + * virFileCacheInsertData: + * @cache: existing cache object + * @name: name of the new data + * @data: the actual data object to store in cache + * + * Adds a new data into a cache but doesn't store the data into + * a file. This function should be used only by testing code. + * + * Returns 0 on success, -1 on error. + */ +int +virFileCacheInsertData(virFileCachePtr cache, + const char *name, + void *data) +{
No virObjectLock(cache)?
+ if (virHashUpdateEntry(cache->table, name, data) < 0) + return -1; + + return 0; +} diff --git a/src/util/virfilecache.h b/src/util/virfilecache.h new file mode 100644 index 0000000000..3447616361 --- /dev/null +++ b/src/util/virfilecache.h @@ -0,0 +1,136 @@ +/* + * virfilecache.h: file caching for data + * + * Copyright (C) 2017 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_FILE_CACHE_H__ +# define __VIR_FILE_CACHE_H__ + +# include "internal.h" + +# include "virobject.h" +# include "virhash.h" + +typedef struct _virFileCache virFileCache; +typedef virFileCache *virFileCachePtr; + +/** + * virFileCacheIsValidPtr: + * @data: data object to validate + * @priv: private data created together with cache + * + * Validates the cached data whether it needs to be refreshed + * or no. + * + * Returns *true* if it's valid or *false* if not valid. + */ +typedef bool +(*virFileCacheIsValidPtr)(void *data, + void *priv); + +/** + * virFileCacheNewDataPtr: + * @name: name of the new data + * @priv: private data created together with cache + * + * Creates a new data based on the @name. The returned data must be + * an instance of virObject. + * + * Returns data object or NULL on error. + */ +typedef void * +(*virFileCacheNewDataPtr)(const char *name, + void *priv); + +/** + * virFileCacheLoadFilePtr: + * @filename: name of a file with cached data + * @name: name of the cached data + * @priv: private data created together with cache + * + * Loads the cached data from a file @filename. + * + * Returns cached data object or NULL on error. + */ +typedef void * +(*virFileCacheLoadFilePtr)(const char *filename, + const char *name, + void *priv); + +/** + * virFileCacheSaveFilePtr: + * @data: data object to save into a file + * @filename: name of the file where to store the cached data + * @priv: private data created together with cache + * + * Stores the cached to a file @filename. + * + * Returns 0 on success, -1 on error. + */ +typedef int +(*virFileCacheSaveFilePtr)(void *data, + const char *filename, + void *priv); + +/** + * virFileCachePrivFreePtr: + * @priv: private data created together with cache + * + * This is used to free the private data when the cache object + * is removed. + */ +typedef void +(*virFileCachePrivFreePtr)(void *priv); + +typedef struct _virFileCacheHandlers virFileCacheHandlers; +typedef virFileCacheHandlers *virFileCacheHandlersPtr; +struct _virFileCacheHandlers { + virFileCacheIsValidPtr isValid; + virFileCacheNewDataPtr newData; + virFileCacheLoadFilePtr loadFile; + virFileCacheSaveFilePtr saveFile; + virFileCachePrivFreePtr privFree; +}; + +virFileCachePtr +virFileCacheNew(const char *dir, + virFileCacheHandlers handlers); + +void * +virFileCacheLookup(virFileCachePtr cache, + const char *name); + +void * +virFileCacheLookupByFunc(virFileCachePtr cache, + virHashSearcher iter, + const void *iterData); + +void * +virFileCacheGetPriv(virFileCachePtr cache); + +void +virFileCacheSetPriv(virFileCachePtr cache, + void *priv);
Any reason for adding this virFileCacheSetPriv API instead of passing the priv data to virFileCacheNew when creating new cache? Do you plan to use different priv data with a single cache?
+ +int +virFileCacheInsertData(virFileCachePtr cache, + const char *name, + void *data); + +#endif /* __VIR_FILE_CACHE_H__ */
Jirka
On Mon, Jul 17, 2017 at 03:19:49PM +0200, Jiri Denemark wrote:
On Mon, Jul 10, 2017 at 14:46:41 +0200, Pavel Hrdina wrote:
The new virFileCache will nicely handle the caching logic for any data that we would like to cache. For each type of data we will just need to implement few handlers that will take care of creating, validating, loading and saving the cached data.
The cached data must be an instance of virObject.
Currently we cache QEMU capabilities which will start using virFileCache.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 9 ++ src/util/virfilecache.c | 391 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfilecache.h | 136 +++++++++++++++++ 5 files changed, 538 insertions(+) create mode 100644 src/util/virfilecache.c create mode 100644 src/util/virfilecache.h
diff --git a/po/POTFILES.in b/po/POTFILES.in index 275df1f297..ae81a57220 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -209,6 +209,7 @@ src/util/vireventpoll.c src/util/virfcp.c src/util/virfdstream.c src/util/virfile.c +src/util/virfilecache.c src/util/virfirewall.c src/util/virfirmware.c src/util/virhash.c diff --git a/src/Makefile.am b/src/Makefile.am index eae32dc58a..a58118db67 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -194,6 +194,7 @@ UTIL_SOURCES = \ util/virxdrdefs.h \ util/virxml.c util/virxml.h \ util/virmdev.c util/virmdev.h \ + util/virfilecache.c util/virfilecache.h \ $(NULL)
EXTRA_DIST += \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 888412ac78..054a00c1d1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1707,6 +1707,15 @@ virFileWriteStr; virFindFileInPath;
+# util/virfilecache.h +virFileCacheGetPriv; +virFileCacheInsertData; +virFileCacheLookup; +virFileCacheLookupByFunc; +virFileCacheNew; +virFileCacheSetPriv; + + # util/virfirewall.h virFirewallAddRuleFull; virFirewallApply; diff --git a/src/util/virfilecache.c b/src/util/virfilecache.c new file mode 100644 index 0000000000..0b0e361daa --- /dev/null +++ b/src/util/virfilecache.c @@ -0,0 +1,391 @@ +/* + * virfilecache.c: file caching for data + * + * Copyright (C) 2017 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 "internal.h" + +#include "viralloc.h" +#include "vircrypto.h" +#include "virerror.h" +#include "virfile.h" +#include "virfilecache.h" +#include "virhash.h" +#include "virlog.h" +#include "virobject.h" +#include "virstring.h" + +#include <sys/stat.h> +#include <sys/types.h> +#include <unistd.h> + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.filecache") + + +struct _virFileCache { + virObjectLockable object; + + virHashTablePtr table; + + char *dir; + void *priv; + + virFileCacheHandlers handlers; +}; + + +static virClassPtr virFileCacheClass; + + +static void +virFileCacheDispose(void *obj) +{ + virFileCachePtr cache = obj; + + VIR_FREE(cache->dir); + + virHashFree(cache->table); + + if (cache->priv && cache->handlers.privFree) + cache->handlers.privFree(cache->priv); +} + + +static int +virFileCacheOnceInit(void) +{ + if (!(virFileCacheClass = virClassNew(virClassForObjectLockable(), + "virFileCache", + sizeof(virFileCache), + virFileCacheDispose))) + return -1; + + return 0; +} + + +VIR_ONCE_GLOBAL_INIT(virFileCache) + + +static char * +virFileCacheGetFileName(virFileCachePtr cache, + const char *name) +{ + char *file = NULL; + char *namehash = NULL; + + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &namehash) < 0) + goto cleanup; + + if (virAsprintf(&file, "%s/%s.cache", cache->dir, namehash) < 0) + goto cleanup;
This changes the filename used by QEMU capabilities cache from *.xml to *.cache. While it shouldn't really matter, the code would just ignore the old cache files and create new ones leaving mess in the directory. Can we change the APIs so that every user of virFileCache can decide what suffix (if any) should be used for the cache files?
I'll add it. There is no harm having that parameter.
+ + if (virFileMakePath(cache->dir) < 0) { + virReportSystemError(errno, + _("Unable to create directory '%s'"), + cache->dir); + VIR_FREE(file); + file = NULL; + } + + cleanup: + VIR_FREE(namehash); + return file; +} + + +static int +virFileCacheLoad(virFileCachePtr cache, + const char *name, + void **data) +{ + char *file = NULL; + int ret = -1; + + if (!(file = virFileCacheGetFileName(cache, name))) + return ret; + + if (!virFileExists(file)) { + if (errno == ENOENT) { + VIR_DEBUG("No cached data '%s' for '%s'", file, name); + ret = 0; + goto cleanup; + } + virReportSystemError(errno, + _("Unable to access cache '%s' for '%s'"), + file, name); + goto cleanup; + } + + if (!(*data = cache->handlers.loadFile(file, name, cache->priv))) { + virErrorPtr err = virGetLastError(); + VIR_WARN("Failed to load cached data from '%s' for '%s': %s", + file, name, err ? NULLSTR(err->message) : "unknown error"); + virResetLastError(); + ret = 0; + goto cleanup; + } + + if (!cache->handlers.isValid(*data, cache->priv)) { + VIR_DEBUG("Outdated cached capabilities '%s' for '%s'", file, name); + ignore_value(unlink(file)); + ret = 0; + goto cleanup; + } + + VIR_DEBUG("Loaded cached data '%s' for '%s'", file, name); + + ret = 1; + + cleanup: + VIR_FREE(file); + return ret; +} + + +static int +virFileCacheSave(virFileCachePtr cache, + const char *name, + void *data) +{ + char *file = NULL; + int ret = -1; + + if (!(file = virFileCacheGetFileName(cache, name))) + return ret; + + if (cache->handlers.saveFile(data, file, cache->priv) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(file); + return ret; +} + + +static void * +virFileCacheNewData(virFileCachePtr cache, + const char *name) +{ + void *data = NULL; + int rv; + + if ((rv = virFileCacheLoad(cache, name, &data)) < 0) + goto cleanup; + + if (rv == 0) { + virObjectUnref(data);
I think this should go inside virFileCacheLoad, i.e., the function should fill in valid data pointer only if it returns 1, otherwise the caller should get NULL in data.
Good point, I'll fix it.
+ if (!(data = cache->handlers.newData(name, cache->priv))) + goto cleanup; + + if (virFileCacheSave(cache, name, data) < 0) { + virObjectUnref(data); + data = NULL; + } + } + + cleanup: + return data; +} + + +/** + * virFileCacheNew: + * @dir: the cache directory where all the cache files will be stored + * @handlers: filled structure with all required handlers + * + * Creates a new cache object which handles caching any data to files + * stored on a filesystem. + * + * Returns new cache object or NULL on error. + */ +virFileCachePtr +virFileCacheNew(const char *dir,
The file name suffix could go here as an extra parameter.
+ virFileCacheHandlers handlers) +{ + virFileCachePtr cache; + + if (virFileCacheInitialize() < 0) + return NULL; + + if (!(cache = virObjectNew(virFileCacheClass))) + return NULL; + + if (!(cache->table = virHashCreate(10, virObjectFreeHashData))) + goto cleanup; + + if (VIR_STRDUP(cache->dir, dir) < 0) + goto cleanup; + + cache->handlers = handlers; + + return cache; + + cleanup: + virObjectUnref(cache); + return NULL; +} + + +static void +virFileCacheValidate(virFileCachePtr cache, + const char *name, + void **data) +{ + if (*data && !cache->handlers.isValid(*data, cache->priv)) { + VIR_DEBUG("Cached data '%p' no longer valid for '%s'", + *data, NULLSTR(name)); + if (name) + virHashRemoveEntry(cache->table, name); + *data = NULL; + } + + if (!*data && name) { + VIR_DEBUG("Creating data for '%s'", name); + *data = virFileCacheNewData(cache, name); + if (*data) { + VIR_DEBUG("Caching data '%p' for '%s'", *data, name); + if (virHashAddEntry(cache->table, name, *data) < 0) { + virObjectUnref(*data); + *data = NULL; + } + } + } +} + + +/** + * virFileCacheLookup: + * @cache: existing cache object + * @name: name of the data stored in a cache + * + * Lookup a data specified by name. This tries to find a file with + * cached data, if it doesn't exist or is no longer valid new data + * is created. + * + * Returns data object or NULL on error. The caller is responsible for + * unrefing the data. + */ +void * +virFileCacheLookup(virFileCachePtr cache, + const char *name) +{ + void *data = NULL; + + virObjectLock(cache); + + data = virHashLookup(cache->table, name); + virFileCacheValidate(cache, name, &data); + + virObjectRef(data); + virObjectUnlock(cache); + + return data; +} + + +/** + * virFileCacheLookupByFunc: + * @cache: existing cache object + * @iter: an iterator to identify the desired data + * @iterData: extra opaque information passed to the @iter + * + * Similar to virFileCacheLookup() except it search by @iter. + * + * Returns data object or NULL on error. The caller is responsible for + * unrefing the data. + */ +void * +virFileCacheLookupByFunc(virFileCachePtr cache, + virHashSearcher iter, + const void *iterData) +{ + void *data = NULL; + char *name = NULL; + + virObjectLock(cache); + + data = virHashSearch(cache->table, iter, iterData, (void **)&name); + virFileCacheValidate(cache, name, &data); + + virObjectRef(data); + virObjectUnlock(cache); + + VIR_FREE(name); + + return data; +} + + +/** + * virFileCacheGetPriv: + * @cache: existing cache object + * + * Returns private data used by @handlers. + */ +void * +virFileCacheGetPriv(virFileCachePtr cache) +{
No virObjectLock(cache)?
I'll fix it.
+ return cache->priv; +} + + +/** + * virFileCacheSetPriv: + * @cache: existing cache object + * @priv: private data to set + * + * Sets private data used by @handlers. + */ +void +virFileCacheSetPriv(virFileCachePtr cache, + void *priv) +{
No virObjectLock(cache)?
I'll fix it.
+ cache->priv = priv; +} + + +/** + * virFileCacheInsertData: + * @cache: existing cache object + * @name: name of the new data + * @data: the actual data object to store in cache + * + * Adds a new data into a cache but doesn't store the data into + * a file. This function should be used only by testing code. + * + * Returns 0 on success, -1 on error. + */ +int +virFileCacheInsertData(virFileCachePtr cache, + const char *name, + void *data) +{
No virObjectLock(cache)?
I'll fix it.
+ if (virHashUpdateEntry(cache->table, name, data) < 0) + return -1; + + return 0; +} diff --git a/src/util/virfilecache.h b/src/util/virfilecache.h new file mode 100644 index 0000000000..3447616361 --- /dev/null +++ b/src/util/virfilecache.h @@ -0,0 +1,136 @@ +/* + * virfilecache.h: file caching for data + * + * Copyright (C) 2017 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_FILE_CACHE_H__ +# define __VIR_FILE_CACHE_H__ + +# include "internal.h" + +# include "virobject.h" +# include "virhash.h" + +typedef struct _virFileCache virFileCache; +typedef virFileCache *virFileCachePtr; + +/** + * virFileCacheIsValidPtr: + * @data: data object to validate + * @priv: private data created together with cache + * + * Validates the cached data whether it needs to be refreshed + * or no. + * + * Returns *true* if it's valid or *false* if not valid. + */ +typedef bool +(*virFileCacheIsValidPtr)(void *data, + void *priv); + +/** + * virFileCacheNewDataPtr: + * @name: name of the new data + * @priv: private data created together with cache + * + * Creates a new data based on the @name. The returned data must be + * an instance of virObject. + * + * Returns data object or NULL on error. + */ +typedef void * +(*virFileCacheNewDataPtr)(const char *name, + void *priv); + +/** + * virFileCacheLoadFilePtr: + * @filename: name of a file with cached data + * @name: name of the cached data + * @priv: private data created together with cache + * + * Loads the cached data from a file @filename. + * + * Returns cached data object or NULL on error. + */ +typedef void * +(*virFileCacheLoadFilePtr)(const char *filename, + const char *name, + void *priv); + +/** + * virFileCacheSaveFilePtr: + * @data: data object to save into a file + * @filename: name of the file where to store the cached data + * @priv: private data created together with cache + * + * Stores the cached to a file @filename. + * + * Returns 0 on success, -1 on error. + */ +typedef int +(*virFileCacheSaveFilePtr)(void *data, + const char *filename, + void *priv); + +/** + * virFileCachePrivFreePtr: + * @priv: private data created together with cache + * + * This is used to free the private data when the cache object + * is removed. + */ +typedef void +(*virFileCachePrivFreePtr)(void *priv); + +typedef struct _virFileCacheHandlers virFileCacheHandlers; +typedef virFileCacheHandlers *virFileCacheHandlersPtr; +struct _virFileCacheHandlers { + virFileCacheIsValidPtr isValid; + virFileCacheNewDataPtr newData; + virFileCacheLoadFilePtr loadFile; + virFileCacheSaveFilePtr saveFile; + virFileCachePrivFreePtr privFree; +}; + +virFileCachePtr +virFileCacheNew(const char *dir, + virFileCacheHandlers handlers); + +void * +virFileCacheLookup(virFileCachePtr cache, + const char *name); + +void * +virFileCacheLookupByFunc(virFileCachePtr cache, + virHashSearcher iter, + const void *iterData); + +void * +virFileCacheGetPriv(virFileCachePtr cache); + +void +virFileCacheSetPriv(virFileCachePtr cache, + void *priv);
Any reason for adding this virFileCacheSetPriv API instead of passing the priv data to virFileCacheNew when creating new cache? Do you plan to use different priv data with a single cache?
This gives you more flexibility while creating new cache and it's not strictly required to work with the cache. For example you don't have to allocate the priv data before creating new cache or you don't have to pass NULL in case you have no use for priv data.
+ +int +virFileCacheInsertData(virFileCachePtr cache, + const char *name, + void *data); + +#endif /* __VIR_FILE_CACHE_H__ */
Jirka
Thanks, Pavel
Implements 3 test cases that covers how the cache is used. We have to mock unlink() function because the caching code unlinks files that are no longer valid and we don't want to do it in our tests. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/Makefile.am | 12 ++ ...a15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache | 1 + ...ae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache | 1 + tests/virfilecachetest.c | 238 +++++++++++++++++++++ 4 files changed, 252 insertions(+) create mode 100644 tests/virfilecachedata/5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache create mode 120000 tests/virfilecachedata/9ca150bf3119b75dcac8e8bae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache create mode 100644 tests/virfilecachetest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 19986dc992..7ec048066e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -182,6 +182,7 @@ test_programs = virshtest sockettest \ virpcitest \ virendiantest \ virfiletest \ + virfilecachetest \ virfirewalltest \ viriscsitest \ virkeycodetest \ @@ -211,6 +212,7 @@ test_libraries = libshunload.la \ virrandommock.la \ virhostcpumock.la \ domaincapsmock.la \ + virfilecachemock.la \ $(NULL) if WITH_REMOTE @@ -1138,6 +1140,12 @@ virhostcpumock_la_CFLAGS = $(AM_CFLAGS) virhostcpumock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) virhostcpumock_la_LIBADD = $(MOCKLIBS_LIBS) +virfilecachemock_la_SOURCES = \ + virfilecachemock.c +virfilecachemock_la_CFLAGS = $(AM_CFLAGS) +virfilecachemock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) +virfilecachemock_la_LIBADD = $(MOCKLIBS_LIBS) + if WITH_LINUX vircaps2xmltest_SOURCES = \ vircaps2xmltest.c testutils.h testutils.c virfilewrapper.h virfilewrapper.c @@ -1370,6 +1378,10 @@ virfiletest_SOURCES = \ virfiletest.c testutils.h testutils.c virfiletest_LDADD = $(LDADDS) +virfilecachetest_SOURCES = \ + virfilecachetest.c testutils.h testutils.c +virfilecachetest_LDADD = $(LDADDS) + virfirewalltest_SOURCES = \ virfirewalltest.c testutils.h testutils.c virfirewalltest_LDADD = $(LDADDS) $(DBUS_LIBS) diff --git a/tests/virfilecachedata/5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache b/tests/virfilecachedata/5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache new file mode 100644 index 0000000000..72943a16fb --- /dev/null +++ b/tests/virfilecachedata/5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache @@ -0,0 +1 @@ +aaa diff --git a/tests/virfilecachedata/9ca150bf3119b75dcac8e8bae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache b/tests/virfilecachedata/9ca150bf3119b75dcac8e8bae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache new file mode 120000 index 0000000000..94df26de63 --- /dev/null +++ b/tests/virfilecachedata/9ca150bf3119b75dcac8e8bae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache @@ -0,0 +1 @@ +5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache \ No newline at end of file diff --git a/tests/virfilecachetest.c b/tests/virfilecachetest.c new file mode 100644 index 0000000000..98c46f86fc --- /dev/null +++ b/tests/virfilecachetest.c @@ -0,0 +1,238 @@ +/* + * Copyright (C) 2017 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 "testutils.h" +#include "virfile.h" +#include "virfilecache.h" + + +#define VIR_FROM_THIS VIR_FROM_NONE + + +struct _testFileCacheObj { + virObject object; + char *data; +}; +typedef struct _testFileCacheObj testFileCacheObj; +typedef testFileCacheObj *testFileCacheObjPtr; + + +static virClassPtr testFileCacheObjClass; + + +static void +testFileCacheObjDispose(void *opaque) +{ + testFileCacheObjPtr obj = opaque; + VIR_FREE(obj->data); +} + + +static int +testFileCacheObjOnceInit(void) +{ + if (!(testFileCacheObjClass = virClassNew(virClassForObject(), + "testFileCacheObj", + sizeof(testFileCacheObj), + testFileCacheObjDispose))) + return -1; + + return 0; +} + + +VIR_ONCE_GLOBAL_INIT(testFileCacheObj) + + +static testFileCacheObjPtr +testFileCacheObjNew(const char *data) +{ + testFileCacheObjPtr obj; + + if (testFileCacheObjInitialize() < 0) + return NULL; + + if (!(obj = virObjectNew(testFileCacheObjClass))) + return NULL; + + if (VIR_STRDUP(obj->data, data) < 0) + goto error; + + return obj; + + error: + virObjectUnref(obj); + return NULL; +} + + +struct _testFileCachePriv { + bool dataSaved; + const char *newData; + const char *expectData; +}; +typedef struct _testFileCachePriv testFileCachePriv; +typedef testFileCachePriv *testFileCachePrivPtr; + + +static bool +testFileCacheIsValid(void *data, + void *priv) +{ + testFileCachePrivPtr testPriv = priv; + testFileCacheObjPtr obj = data; + + return STREQ(testPriv->expectData, obj->data); +} + + +static void * +testFileCacheNewData(const char *name ATTRIBUTE_UNUSED, + void *priv) +{ + testFileCachePrivPtr testPriv = priv; + + return testFileCacheObjNew(testPriv->newData); +} + + +static void * +testFileCacheLoadFile(const char *filename, + const char *name ATTRIBUTE_UNUSED, + void *priv ATTRIBUTE_UNUSED) +{ + testFileCacheObjPtr obj; + char *data; + + if (virFileReadAll(filename, 20, &data) < 0) + return NULL; + + obj = testFileCacheObjNew(data); + + VIR_FREE(data); + return obj; +} + + +static int +testFileCacheSaveFile(void *data ATTRIBUTE_UNUSED, + const char *filename ATTRIBUTE_UNUSED, + void *priv) +{ + testFileCachePrivPtr testPriv = priv; + + testPriv->dataSaved = true; + + return 0; +} + + +virFileCacheHandlers testFileCacheHandlers = { + .isValid = testFileCacheIsValid, + .newData = testFileCacheNewData, + .loadFile = testFileCacheLoadFile, + .saveFile = testFileCacheSaveFile +}; + + +struct _testFileCacheData { + virFileCachePtr cache; + const char *name; + const char *newData; + const char *expectData; + bool expectSave; +}; +typedef struct _testFileCacheData testFileCacheData; +typedef testFileCacheData *testFileCacheDataPtr; + + +static int +testFileCache(const void *opaque) +{ + int ret = -1; + const testFileCacheData *data = opaque; + testFileCacheObjPtr obj = NULL; + testFileCachePrivPtr testPriv = virFileCacheGetPriv(data->cache); + + testPriv->dataSaved = false; + testPriv->newData = data->newData; + testPriv->expectData = data->expectData; + + if (!(obj = virFileCacheLookup(data->cache, data->name))) { + fprintf(stderr, "Getting cached data failed.\n"); + goto cleanup; + } + + if (!obj->data || STRNEQ(data->expectData, obj->data)) { + fprintf(stderr, "Expect data '%s', loaded data '%s'.\n", + data->expectData, NULLSTR(obj->data)); + goto cleanup; + } + + if (data->expectSave != testPriv->dataSaved) { + fprintf(stderr, "Expect data to be saved '%s', data saved '%s'.\n", + data->expectSave ? "yes" : "no", + testPriv->dataSaved ? "yes" : "no"); + goto cleanup; + } + + ret = 0; + + cleanup: + virObjectUnref(obj); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + testFileCachePriv testPriv = {0}; + virFileCachePtr cache = NULL; + + if (!(cache = virFileCacheNew(abs_srcdir "/virfilecachedata", + testFileCacheHandlers))) + return EXIT_FAILURE; + + virFileCacheSetPriv(cache, &testPriv); + +#define TEST_RUN(name, newData, expectData, expectSave) \ + do { \ + testFileCacheData data = { \ + cache, name, newData, expectData, expectSave \ + }; \ + if (virTestRun(name, testFileCache, &data) < 0) \ + ret = -1; \ + } while (0) + + /* The cache file name is created using: + * '$ echo -n $TEST_NAME | sha256sum' */ + TEST_RUN("cacheValid", NULL, "aaa\n", false); + TEST_RUN("cacheInvalid", "bbb\n", "bbb\n", true); + TEST_RUN("cacheMissing", "ccc\n", "ccc\n", true); + + virObjectUnref(cache); + + return ret != 0 ? EXIT_FAILURE : EXIT_SUCCESS; +} + +VIR_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virfilecachemock.so") -- 2.13.2
On Mon, Jul 10, 2017 at 14:46:42 +0200, Pavel Hrdina wrote:
Implements 3 test cases that covers how the cache is used.
We have to mock unlink() function because the caching code unlinks files that are no longer valid and we don't want to do it in our tests.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/Makefile.am | 12 ++ ...a15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache | 1 + ...ae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache | 1 + tests/virfilecachetest.c | 238 +++++++++++++++++++++ 4 files changed, 252 insertions(+) create mode 100644 tests/virfilecachedata/5f3154560c130108b282a2aa15b1658aa16923e46497dd8deeb6be287ddb0ca0.cache create mode 120000 tests/virfilecachedata/9ca150bf3119b75dcac8e8bae4bc3a28e75bc3e262757001e8b953580f5e75ef.cache create mode 100644 tests/virfilecachetest.c
virfilecachemock.c is missing, otherwise the patch looks OK. Jirka
Cleanups the code a little bit and reduces amount of arguments passed throughout the functions. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 44 +++++++++++++++++++++----------------------- src/qemu/qemu_capspriv.h | 8 ++------ tests/qemucapabilitiestest.c | 4 ++-- tests/testutilsqemu.c | 5 +---- 4 files changed, 26 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6659744a34..c9c28da556 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -474,11 +474,13 @@ struct _virQEMUCaps { char *binary; time_t ctime; + time_t libvirtCtime; virBitmapPtr flags; unsigned int version; unsigned int kvmVersion; + unsigned int libvirtVersion; char *package; virArch arch; @@ -3783,9 +3785,7 @@ virQEMUCapsLoadCPUModels(virQEMUCapsPtr qemuCaps, int virQEMUCapsLoadCache(virCapsPtr caps, virQEMUCapsPtr qemuCaps, - const char *filename, - time_t *selfctime, - unsigned long *selfvers) + const char *filename) { xmlDocPtr doc = NULL; int ret = -1; @@ -3827,11 +3827,11 @@ virQEMUCapsLoadCache(virCapsPtr caps, _("missing selfctime in QEMU capabilities XML")); goto cleanup; } - *selfctime = (time_t)l; + qemuCaps->libvirtCtime = (time_t)l; - *selfvers = 0; + qemuCaps->libvirtVersion = 0; if (virXPathULong("string(./selfvers)", ctxt, &lu) == 0) - *selfvers = lu; + qemuCaps->libvirtVersion = lu; qemuCaps->usedQMP = virXPathBoolean("count(./usedQMP) > 0", ctxt) > 0; @@ -4104,9 +4104,7 @@ virQEMUCapsFormatCPUModels(virQEMUCapsPtr qemuCaps, char * -virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps, - time_t selfCTime, - unsigned long selfVersion) +virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; char *ret = NULL; @@ -4118,9 +4116,9 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps, virBufferAsprintf(&buf, "<qemuctime>%llu</qemuctime>\n", (long long) qemuCaps->ctime); virBufferAsprintf(&buf, "<selfctime>%llu</selfctime>\n", - (long long) selfCTime); + (long long) qemuCaps->libvirtCtime); virBufferAsprintf(&buf, "<selfvers>%lu</selfvers>\n", - (unsigned long) selfVersion); + (unsigned long) qemuCaps->libvirtVersion); if (qemuCaps->usedQMP) virBufferAddLit(&buf, "<usedQMP/>\n"); @@ -4195,9 +4193,7 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) char *xml = NULL; int ret = -1; - xml = virQEMUCapsFormatCache(qemuCaps, - virGetSelfLastChanged(), - LIBVIR_VERSION_NUMBER); + xml = virQEMUCapsFormatCache(qemuCaps); if (virFileWriteStr(filename, xml, 0600) < 0) { virReportSystemError(errno, @@ -4209,7 +4205,7 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) VIR_DEBUG("Saved caps '%s' for '%s' with (%lld, %lld)", filename, qemuCaps->binary, (long long)qemuCaps->ctime, - (long long)virGetSelfLastChanged()); + (long long)qemuCaps->libvirtCtime); ret = 0; cleanup: @@ -4299,8 +4295,6 @@ virQEMUCapsInitCached(virCapsPtr caps, char *binaryhash = NULL; struct stat sb; time_t qemuctime = qemuCaps->ctime; - time_t selfctime; - unsigned long selfvers; if (virAsprintf(&capsdir, "%s/capabilities", cacheDir) < 0) goto cleanup; @@ -4333,8 +4327,7 @@ virQEMUCapsInitCached(virCapsPtr caps, goto cleanup; } - if (virQEMUCapsLoadCache(caps, qemuCaps, capsfile, - &selfctime, &selfvers) < 0) { + if (virQEMUCapsLoadCache(caps, qemuCaps, capsfile) < 0) { VIR_WARN("Failed to load cached caps from '%s' for '%s': %s", capsfile, qemuCaps->binary, virGetLastErrorMessage()); virResetLastError(); @@ -4345,13 +4338,15 @@ virQEMUCapsInitCached(virCapsPtr caps, goto discard; /* Discard cache if QEMU binary or libvirtd changed */ - if (selfctime != virGetSelfLastChanged() || - selfvers != LIBVIR_VERSION_NUMBER) { + if (qemuCaps->libvirtCtime != virGetSelfLastChanged() || + qemuCaps->libvirtVersion != LIBVIR_VERSION_NUMBER) { VIR_DEBUG("Outdated capabilities for '%s': libvirt changed " "(%lld vs %lld, %lu vs %lu)", qemuCaps->binary, - (long long)selfctime, (long long)virGetSelfLastChanged(), - selfvers, (unsigned long)LIBVIR_VERSION_NUMBER); + (long long)qemuCaps->libvirtCtime, + (long long)virGetSelfLastChanged(), + (unsigned long)qemuCaps->libvirtVersion, + (unsigned long)LIBVIR_VERSION_NUMBER); goto discard; } @@ -5248,6 +5243,9 @@ virQEMUCapsNewForBinaryInternal(virCapsPtr caps, goto error; } + qemuCaps->libvirtCtime = virGetSelfLastChanged(); + qemuCaps->libvirtVersion = LIBVIR_VERSION_NUMBER; + if (cacheDir && virQEMUCapsRememberCached(qemuCaps, cacheDir) < 0) goto error; diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h index 94fa75b960..1162e0b284 100644 --- a/src/qemu/qemu_capspriv.h +++ b/src/qemu/qemu_capspriv.h @@ -50,12 +50,8 @@ virQEMUCapsNewForBinaryInternal(virCapsPtr caps, int virQEMUCapsLoadCache(virCapsPtr caps, virQEMUCapsPtr qemuCaps, - const char *filename, - time_t *selfctime, - unsigned long *selfvers); -char *virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps, - time_t selfCTime, - unsigned long selfVersion); + const char *filename); +char *virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps); int virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c index bbb683eb83..45d2a885bd 100644 --- a/tests/qemucapabilitiestest.c +++ b/tests/qemucapabilitiestest.c @@ -66,7 +66,7 @@ testQemuCaps(const void *opaque) qemuMonitorTestGetMonitor(mon)) < 0) goto cleanup; - if (!(actual = virQEMUCapsFormatCache(capsActual, 0, 0))) + if (!(actual = virQEMUCapsFormatCache(capsActual))) goto cleanup; if (virTestCompareToFile(actual, capsFile) < 0) @@ -108,7 +108,7 @@ testQemuCapsCopy(const void *opaque) if (!(copy = virQEMUCapsNewCopy(orig))) goto cleanup; - if (!(actual = virQEMUCapsFormatCache(copy, 0, 0))) + if (!(actual = virQEMUCapsFormatCache(copy))) goto cleanup; if (virTestCompareToFile(actual, capsFile) < 0) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index ee4853841c..88e11ba14a 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -568,12 +568,9 @@ qemuTestParseCapabilities(virCapsPtr caps, const char *capsFile) { virQEMUCapsPtr qemuCaps = NULL; - time_t selfctime; - unsigned long version; if (!(qemuCaps = virQEMUCapsNew()) || - virQEMUCapsLoadCache(caps, qemuCaps, capsFile, - &selfctime, &version) < 0) + virQEMUCapsLoadCache(caps, qemuCaps, capsFile) < 0) goto error; return qemuCaps; -- 2.13.2
On Mon, Jul 10, 2017 at 14:46:43 +0200, Pavel Hrdina wrote:
Cleanups the code a little bit and reduces amount of arguments passed throughout the functions.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 44 +++++++++++++++++++++----------------------- src/qemu/qemu_capspriv.h | 8 ++------ tests/qemucapabilitiestest.c | 4 ++-- tests/testutilsqemu.c | 5 +---- 4 files changed, 26 insertions(+), 35 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 112 +++++++++++++++++++++---------------------- src/qemu/qemu_capabilities.h | 5 -- 2 files changed, 56 insertions(+), 61 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c9c28da556..8a2a73dc5b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4282,6 +4282,62 @@ virQEMUCapsReset(virQEMUCapsPtr qemuCaps) } +static bool +virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, + time_t qemuctime, + uid_t runUid, + gid_t runGid) +{ + bool kvmUsable; + + if (!qemuCaps->binary) + return true; + + if (!qemuctime) { + struct stat sb; + + if (stat(qemuCaps->binary, &sb) < 0) { + char ebuf[1024]; + VIR_DEBUG("Failed to stat QEMU binary '%s': %s", + qemuCaps->binary, + virStrerror(errno, ebuf, sizeof(ebuf))); + return false; + } + qemuctime = sb.st_ctime; + } + + if (qemuctime != qemuCaps->ctime) { + VIR_DEBUG("Outdated capabilities for '%s': QEMU binary changed " + "(%lld vs %lld)", + qemuCaps->binary, + (long long) qemuctime, (long long) qemuCaps->ctime); + return false; + } + + kvmUsable = virFileAccessibleAs("/dev/kvm", R_OK | W_OK, + runUid, runGid) == 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_ENABLE_KVM) && + kvmUsable) { + VIR_DEBUG("KVM was not enabled when probing '%s', " + "but it should be usable now", + qemuCaps->binary); + return false; + } + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && + !kvmUsable) { + VIR_DEBUG("KVM was enabled when probing '%s', " + "but it is not available now", + qemuCaps->binary); + return false; + } + + return true; +} + + static int virQEMUCapsInitCached(virCapsPtr caps, virQEMUCapsPtr qemuCaps, @@ -5277,62 +5333,6 @@ virQEMUCapsNewForBinary(virCapsPtr caps, } -bool -virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, - time_t qemuctime, - uid_t runUid, - gid_t runGid) -{ - bool kvmUsable; - - if (!qemuCaps->binary) - return true; - - if (!qemuctime) { - struct stat sb; - - if (stat(qemuCaps->binary, &sb) < 0) { - char ebuf[1024]; - VIR_DEBUG("Failed to stat QEMU binary '%s': %s", - qemuCaps->binary, - virStrerror(errno, ebuf, sizeof(ebuf))); - return false; - } - qemuctime = sb.st_ctime; - } - - if (qemuctime != qemuCaps->ctime) { - VIR_DEBUG("Outdated capabilities for '%s': QEMU binary changed " - "(%lld vs %lld)", - qemuCaps->binary, - (long long) qemuctime, (long long) qemuCaps->ctime); - return false; - } - - kvmUsable = virFileAccessibleAs("/dev/kvm", R_OK | W_OK, - runUid, runGid) == 0; - - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_ENABLE_KVM) && - kvmUsable) { - VIR_DEBUG("KVM was not enabled when probing '%s', " - "but it should be usable now", - qemuCaps->binary); - return false; - } - - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && - !kvmUsable) { - VIR_DEBUG("KVM was enabled when probing '%s', " - "but it is not available now", - qemuCaps->binary); - return false; - } - - return true; -} - - struct virQEMUCapsMachineTypeFilter { const char *machineType; virQEMUCapsFlags *flags; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 6e95876e8d..7f5382df62 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -494,11 +494,6 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps, size_t *nmachines, virCapsGuestMachinePtr **machines); -bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, - time_t ctime, - uid_t runUid, - gid_t runGid); - void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps, const char *machineType); -- 2.13.2
On Mon, Jul 10, 2017 at 14:46:44 +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 112 +++++++++++++++++++++---------------------- src/qemu/qemu_capabilities.h | 5 -- 2 files changed, 56 insertions(+), 61 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 8a2a73dc5b..4d8890aaaf 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4293,6 +4293,18 @@ virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, if (!qemuCaps->binary) return true; + if (qemuCaps->libvirtCtime != virGetSelfLastChanged() || + qemuCaps->libvirtVersion != LIBVIR_VERSION_NUMBER) { + VIR_DEBUG("Outdated capabilities for '%s': libvirt changed " + "(%lld vs %lld, %lu vs %lu)", + qemuCaps->binary, + (long long)qemuCaps->libvirtCtime, + (long long)virGetSelfLastChanged(), + (unsigned long)qemuCaps->libvirtVersion, + (unsigned long)LIBVIR_VERSION_NUMBER); + return false; + } + if (!qemuctime) { struct stat sb; @@ -4393,19 +4405,6 @@ virQEMUCapsInitCached(virCapsPtr caps, if (!virQEMUCapsIsValid(qemuCaps, qemuctime, runUid, runGid)) goto discard; - /* Discard cache if QEMU binary or libvirtd changed */ - if (qemuCaps->libvirtCtime != virGetSelfLastChanged() || - qemuCaps->libvirtVersion != LIBVIR_VERSION_NUMBER) { - VIR_DEBUG("Outdated capabilities for '%s': libvirt changed " - "(%lld vs %lld, %lu vs %lu)", - qemuCaps->binary, - (long long)qemuCaps->libvirtCtime, - (long long)virGetSelfLastChanged(), - (unsigned long)qemuCaps->libvirtVersion, - (unsigned long)LIBVIR_VERSION_NUMBER); - goto discard; - } - VIR_DEBUG("Loaded '%s' for '%s' ctime %lld usedQMP=%d", capsfile, qemuCaps->binary, (long long)qemuCaps->ctime, qemuCaps->usedQMP); -- 2.13.2
On Mon, Jul 10, 2017 at 14:46:45 +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 8a2a73dc5b..4d8890aaaf 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4293,6 +4293,18 @@ virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, if (!qemuCaps->binary) return true;
+ if (qemuCaps->libvirtCtime != virGetSelfLastChanged() || + qemuCaps->libvirtVersion != LIBVIR_VERSION_NUMBER) { + VIR_DEBUG("Outdated capabilities for '%s': libvirt changed " + "(%lld vs %lld, %lu vs %lu)", + qemuCaps->binary, + (long long)qemuCaps->libvirtCtime, + (long long)virGetSelfLastChanged(), + (unsigned long)qemuCaps->libvirtVersion, + (unsigned long)LIBVIR_VERSION_NUMBER); + return false; + } +
This change causes libvirt's ctime to be compared everytime the cache is looked up (in virQEMUCapsCacheValidate). This looked fishy, but virGetSelfLastChanged returns cached ctime fetched only once when libvirtd starts. Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
It's not required and following patches will change the code. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4d8890aaaf..4d26e04fc3 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4284,11 +4284,11 @@ virQEMUCapsReset(virQEMUCapsPtr qemuCaps) static bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, - time_t qemuctime, uid_t runUid, gid_t runGid) { bool kvmUsable; + struct stat sb; if (!qemuCaps->binary) return true; @@ -4305,24 +4305,19 @@ virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, return false; } - if (!qemuctime) { - struct stat sb; - - if (stat(qemuCaps->binary, &sb) < 0) { - char ebuf[1024]; - VIR_DEBUG("Failed to stat QEMU binary '%s': %s", - qemuCaps->binary, - virStrerror(errno, ebuf, sizeof(ebuf))); - return false; - } - qemuctime = sb.st_ctime; + if (stat(qemuCaps->binary, &sb) < 0) { + char ebuf[1024]; + VIR_DEBUG("Failed to stat QEMU binary '%s': %s", + qemuCaps->binary, + virStrerror(errno, ebuf, sizeof(ebuf))); + return false; } - if (qemuctime != qemuCaps->ctime) { + if (sb.st_ctime != qemuCaps->ctime) { VIR_DEBUG("Outdated capabilities for '%s': QEMU binary changed " "(%lld vs %lld)", qemuCaps->binary, - (long long) qemuctime, (long long) qemuCaps->ctime); + (long long) sb.st_ctime, (long long) qemuCaps->ctime); return false; } @@ -4362,7 +4357,6 @@ virQEMUCapsInitCached(virCapsPtr caps, int ret = -1; char *binaryhash = NULL; struct stat sb; - time_t qemuctime = qemuCaps->ctime; if (virAsprintf(&capsdir, "%s/capabilities", cacheDir) < 0) goto cleanup; @@ -4402,7 +4396,7 @@ virQEMUCapsInitCached(virCapsPtr caps, goto discard; } - if (!virQEMUCapsIsValid(qemuCaps, qemuctime, runUid, runGid)) + if (!virQEMUCapsIsValid(qemuCaps, runUid, runGid)) goto discard; VIR_DEBUG("Loaded '%s' for '%s' ctime %lld usedQMP=%d", @@ -4411,7 +4405,6 @@ virQEMUCapsInitCached(virCapsPtr caps, ret = 1; cleanup: - qemuCaps->ctime = qemuctime; VIR_FREE(binaryhash); VIR_FREE(capsfile); VIR_FREE(capsdir); @@ -5413,7 +5406,7 @@ virQEMUCapsCacheValidate(virQEMUCapsCachePtr cache, virQEMUCapsPtr *qemuCaps) { if (*qemuCaps && - !virQEMUCapsIsValid(*qemuCaps, 0, cache->runUid, cache->runGid)) { + !virQEMUCapsIsValid(*qemuCaps, cache->runUid, cache->runGid)) { VIR_DEBUG("Cached capabilities %p no longer valid for %s", *qemuCaps, binary); virHashRemoveEntry(cache->binaries, binary); -- 2.13.2
On Mon, Jul 10, 2017 at 14:46:46 +0200, Pavel Hrdina wrote:
It's not required and following patches will change the code.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4d8890aaaf..4d26e04fc3 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4284,11 +4284,11 @@ virQEMUCapsReset(virQEMUCapsPtr qemuCaps)
static bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, - time_t qemuctime, uid_t runUid, gid_t runGid) { bool kvmUsable; + struct stat sb;
if (!qemuCaps->binary) return true; @@ -4305,24 +4305,19 @@ virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, return false; }
- if (!qemuctime) { - struct stat sb; - - if (stat(qemuCaps->binary, &sb) < 0) { - char ebuf[1024]; - VIR_DEBUG("Failed to stat QEMU binary '%s': %s", - qemuCaps->binary, - virStrerror(errno, ebuf, sizeof(ebuf))); - return false; - } - qemuctime = sb.st_ctime; + if (stat(qemuCaps->binary, &sb) < 0) { + char ebuf[1024]; + VIR_DEBUG("Failed to stat QEMU binary '%s': %s", + qemuCaps->binary, + virStrerror(errno, ebuf, sizeof(ebuf))); + return false; }
- if (qemuctime != qemuCaps->ctime) { + if (sb.st_ctime != qemuCaps->ctime) { VIR_DEBUG("Outdated capabilities for '%s': QEMU binary changed " "(%lld vs %lld)", qemuCaps->binary, - (long long) qemuctime, (long long) qemuCaps->ctime); + (long long) sb.st_ctime, (long long) qemuCaps->ctime); return false; }
@@ -4362,7 +4357,6 @@ virQEMUCapsInitCached(virCapsPtr caps, int ret = -1; char *binaryhash = NULL; struct stat sb; - time_t qemuctime = qemuCaps->ctime;
if (virAsprintf(&capsdir, "%s/capabilities", cacheDir) < 0) goto cleanup;
This hunk is wrong or you are missing some code which would handle this differently. virQEMUCapsNewForBinaryInternal sets qemuCaps->ctime and calls virQEMUCapsInitCached, which tries to load cached data in qemuCaps. Once loaded from the cache, qemuCaps->ctime is set to the cached one and if the cache is found to be invalid, the cached ctime remains stored in qemuCaps->ctime instead of the real one.
@@ -4402,7 +4396,7 @@ virQEMUCapsInitCached(virCapsPtr caps, goto discard; }
- if (!virQEMUCapsIsValid(qemuCaps, qemuctime, runUid, runGid)) + if (!virQEMUCapsIsValid(qemuCaps, runUid, runGid)) goto discard;
VIR_DEBUG("Loaded '%s' for '%s' ctime %lld usedQMP=%d", @@ -4411,7 +4405,6 @@ virQEMUCapsInitCached(virCapsPtr caps,
ret = 1; cleanup: - qemuCaps->ctime = qemuctime; VIR_FREE(binaryhash); VIR_FREE(capsfile); VIR_FREE(capsdir);
And this hunk is wrong too. However, the next patch fixes the code and these two hunks are not needed anymore. Just move the hunks from 7/11 to 8/11.
@@ -5413,7 +5406,7 @@ virQEMUCapsCacheValidate(virQEMUCapsCachePtr cache, virQEMUCapsPtr *qemuCaps) { if (*qemuCaps && - !virQEMUCapsIsValid(*qemuCaps, 0, cache->runUid, cache->runGid)) { + !virQEMUCapsIsValid(*qemuCaps, cache->runUid, cache->runGid)) { VIR_DEBUG("Cached capabilities %p no longer valid for %s", *qemuCaps, binary); virHashRemoveEntry(cache->binaries, binary);
The rest of the patch looks OK. Jirka
Preparation for switching to virFileCache where there are two callbacks, one to get a new data and second one to load a cached data. This also removes virQEMUCapsReset which is no longer required. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 141 +++++++++++++++++++------------------------ src/qemu/qemu_capspriv.h | 1 - tests/qemucapsprobe.c | 2 +- 3 files changed, 63 insertions(+), 81 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4d26e04fc3..1f3240b942 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4251,37 +4251,6 @@ virQEMUCapsRememberCached(virQEMUCapsPtr qemuCaps, const char *cacheDir) } -static void -virQEMUCapsReset(virQEMUCapsPtr qemuCaps) -{ - size_t i; - - virBitmapClearAll(qemuCaps->flags); - qemuCaps->version = qemuCaps->kvmVersion = 0; - VIR_FREE(qemuCaps->package); - qemuCaps->arch = VIR_ARCH_NONE; - qemuCaps->usedQMP = false; - - virObjectUnref(qemuCaps->kvmCPUModels); - qemuCaps->kvmCPUModels = NULL; - virObjectUnref(qemuCaps->tcgCPUModels); - qemuCaps->tcgCPUModels = NULL; - - for (i = 0; i < qemuCaps->nmachineTypes; i++) { - VIR_FREE(qemuCaps->machineTypes[i].name); - VIR_FREE(qemuCaps->machineTypes[i].alias); - } - VIR_FREE(qemuCaps->machineTypes); - qemuCaps->nmachineTypes = 0; - - VIR_FREE(qemuCaps->gicCapabilities); - qemuCaps->ngicCapabilities = 0; - - virQEMUCapsHostCPUDataClear(&qemuCaps->kvmCPU); - virQEMUCapsHostCPUDataClear(&qemuCaps->tcgCPU); -} - - static bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, uid_t runUid, @@ -4347,7 +4316,8 @@ virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, static int virQEMUCapsInitCached(virCapsPtr caps, - virQEMUCapsPtr qemuCaps, + virQEMUCapsPtr *qemuCaps, + const char *binary, const char *cacheDir, uid_t runUid, gid_t runGid) @@ -4357,13 +4327,12 @@ virQEMUCapsInitCached(virCapsPtr caps, int ret = -1; char *binaryhash = NULL; struct stat sb; + virQEMUCapsPtr qemuCapsNew = NULL; if (virAsprintf(&capsdir, "%s/capabilities", cacheDir) < 0) goto cleanup; - if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, - qemuCaps->binary, - &binaryhash) < 0) + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, binary, &binaryhash) < 0) goto cleanup; if (virAsprintf(&capsfile, "%s/%s.xml", capsdir, binaryhash) < 0) @@ -4379,31 +4348,38 @@ virQEMUCapsInitCached(virCapsPtr caps, if (stat(capsfile, &sb) < 0) { if (errno == ENOENT) { VIR_DEBUG("No cached capabilities '%s' for '%s'", - capsfile, qemuCaps->binary); + capsfile, binary); ret = 0; goto cleanup; } virReportSystemError(errno, _("Unable to access cache '%s' for '%s'"), - capsfile, qemuCaps->binary); + capsfile, binary); goto cleanup; } - if (virQEMUCapsLoadCache(caps, qemuCaps, capsfile) < 0) { + if (!(qemuCapsNew = virQEMUCapsNew())) + goto cleanup; + + if (VIR_STRDUP(qemuCapsNew->binary, binary) < 0) + goto discard; + + if (virQEMUCapsLoadCache(caps, qemuCapsNew, capsfile) < 0) { VIR_WARN("Failed to load cached caps from '%s' for '%s': %s", - capsfile, qemuCaps->binary, virGetLastErrorMessage()); + capsfile, qemuCapsNew->binary, virGetLastErrorMessage()); virResetLastError(); goto discard; } - if (!virQEMUCapsIsValid(qemuCaps, runUid, runGid)) + if (!virQEMUCapsIsValid(qemuCapsNew, runUid, runGid)) goto discard; VIR_DEBUG("Loaded '%s' for '%s' ctime %lld usedQMP=%d", - capsfile, qemuCaps->binary, - (long long)qemuCaps->ctime, qemuCaps->usedQMP); + capsfile, qemuCapsNew->binary, + (long long)qemuCapsNew->ctime, qemuCapsNew->usedQMP); ret = 1; + *qemuCaps = qemuCapsNew; cleanup: VIR_FREE(binaryhash); VIR_FREE(capsfile); @@ -4412,9 +4388,9 @@ virQEMUCapsInitCached(virCapsPtr caps, discard: VIR_DEBUG("Dropping cached capabilities '%s' for '%s'", - capsfile, qemuCaps->binary); + capsfile, qemuCapsNew->binary); ignore_value(unlink(capsfile)); - virQEMUCapsReset(qemuCaps); + virObjectUnref(qemuCapsNew); ret = 0; goto cleanup; } @@ -5230,14 +5206,12 @@ virQEMUCapsPtr virQEMUCapsNewForBinaryInternal(virCapsPtr caps, const char *binary, const char *libDir, - const char *cacheDir, uid_t runUid, gid_t runGid, bool qmpOnly) { virQEMUCapsPtr qemuCaps; struct stat sb; - int rv; char *qmperr = NULL; if (!(qemuCaps = virQEMUCapsNew())) @@ -5265,42 +5239,30 @@ virQEMUCapsNewForBinaryInternal(virCapsPtr caps, goto error; } - if (!cacheDir) - rv = 0; - else if ((rv = virQEMUCapsInitCached(caps, qemuCaps, cacheDir, - runUid, runGid)) < 0) + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid, &qmperr) < 0) { + virQEMUCapsLogProbeFailure(binary); goto error; + } - if (rv == 0) { - if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid, &qmperr) < 0) { - virQEMUCapsLogProbeFailure(binary); - goto error; - } - - if (qmpOnly && !qemuCaps->usedQMP) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to probe QEMU binary with QMP: %s"), - qmperr ? qmperr : _("unknown error")); - virQEMUCapsLogProbeFailure(binary); - goto error; - } - - if (!qemuCaps->usedQMP && - virQEMUCapsInitHelp(qemuCaps, runUid, runGid, qmperr) < 0) { - virQEMUCapsLogProbeFailure(binary); - goto error; - } + if (qmpOnly && !qemuCaps->usedQMP) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to probe QEMU binary with QMP: %s"), + qmperr ? qmperr : _("unknown error")); + virQEMUCapsLogProbeFailure(binary); + goto error; + } - qemuCaps->libvirtCtime = virGetSelfLastChanged(); - qemuCaps->libvirtVersion = LIBVIR_VERSION_NUMBER; + if (!qemuCaps->usedQMP && + virQEMUCapsInitHelp(qemuCaps, runUid, runGid, qmperr) < 0) { + virQEMUCapsLogProbeFailure(binary); + goto error; + } - if (cacheDir && - virQEMUCapsRememberCached(qemuCaps, cacheDir) < 0) - goto error; + qemuCaps->libvirtCtime = virGetSelfLastChanged(); + qemuCaps->libvirtVersion = LIBVIR_VERSION_NUMBER; - virQEMUCapsInitHostCPUModel(qemuCaps, caps, VIR_DOMAIN_VIRT_KVM); - virQEMUCapsInitHostCPUModel(qemuCaps, caps, VIR_DOMAIN_VIRT_QEMU); - } + virQEMUCapsInitHostCPUModel(qemuCaps, caps, VIR_DOMAIN_VIRT_KVM); + virQEMUCapsInitHostCPUModel(qemuCaps, caps, VIR_DOMAIN_VIRT_QEMU); cleanup: VIR_FREE(qmperr); @@ -5320,8 +5282,29 @@ virQEMUCapsNewForBinary(virCapsPtr caps, uid_t runUid, gid_t runGid) { - return virQEMUCapsNewForBinaryInternal(caps, binary, libDir, cacheDir, - runUid, runGid, false); + int rv; + virQEMUCapsPtr qemuCaps = NULL; + + if ((rv = virQEMUCapsInitCached(caps, &qemuCaps, binary, cacheDir, + runUid, runGid)) < 0) + goto error; + + if (rv == 0) { + if (!(qemuCaps = virQEMUCapsNewForBinaryInternal(caps, binary, + libDir, runUid, + runGid, false))) { + goto error; + } + + if (virQEMUCapsRememberCached(qemuCaps, cacheDir) < 0) + goto error; + } + + return qemuCaps; + + error: + virObjectUnref(qemuCaps); + return NULL; } diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h index 1162e0b284..3458fc0700 100644 --- a/src/qemu/qemu_capspriv.h +++ b/src/qemu/qemu_capspriv.h @@ -43,7 +43,6 @@ virQEMUCapsPtr virQEMUCapsNewForBinaryInternal(virCapsPtr caps, const char *binary, const char *libDir, - const char *cacheDir, uid_t runUid, gid_t runGid, bool qmpOnly); diff --git a/tests/qemucapsprobe.c b/tests/qemucapsprobe.c index 662c7036ed..581ac38465 100644 --- a/tests/qemucapsprobe.c +++ b/tests/qemucapsprobe.c @@ -70,7 +70,7 @@ main(int argc, char **argv) if (virThreadCreate(&thread, false, eventLoop, NULL) < 0) return EXIT_FAILURE; - if (!(caps = virQEMUCapsNewForBinaryInternal(NULL, argv[1], "/tmp", NULL, + if (!(caps = virQEMUCapsNewForBinaryInternal(NULL, argv[1], "/tmp", -1, -1, true))) return EXIT_FAILURE; -- 2.13.2
On Mon, Jul 10, 2017 at 14:46:47 +0200, Pavel Hrdina wrote:
Preparation for switching to virFileCache where there are two callbacks, one to get a new data and second one to load a cached data.
This also removes virQEMUCapsReset which is no longer required.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 141 +++++++++++++++++++------------------------ src/qemu/qemu_capspriv.h | 1 - tests/qemucapsprobe.c | 2 +- 3 files changed, 63 insertions(+), 81 deletions(-)
The code looks a lot better after this patch. Once you move the two hunks from the previous patch here, of course. Jirka
This will store private data that will be used by following patches when switching to virFileCache. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 51 ++++++++++++++++++++++++++------------------ src/qemu/qemu_capspriv.h | 12 ++++++++--- 2 files changed, 39 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 1f3240b942..e190cfa8b1 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3765,6 +3765,14 @@ virQEMUCapsLoadCPUModels(virQEMUCapsPtr qemuCaps, } +static void +virQEMUCapsCachePrivFree(virQEMUCapsCachePrivPtr priv) +{ + VIR_FREE(priv->libDir); + VIR_FREE(priv); +} + + /* * Parsing a doc that looks like * @@ -4253,8 +4261,7 @@ virQEMUCapsRememberCached(virQEMUCapsPtr qemuCaps, const char *cacheDir) static bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, - uid_t runUid, - gid_t runGid) + virQEMUCapsCachePrivPtr priv) { bool kvmUsable; struct stat sb; @@ -4291,7 +4298,7 @@ virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, } kvmUsable = virFileAccessibleAs("/dev/kvm", R_OK | W_OK, - runUid, runGid) == 0; + priv->runUid, priv->runGid) == 0; if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_ENABLE_KVM) && @@ -4319,8 +4326,7 @@ virQEMUCapsInitCached(virCapsPtr caps, virQEMUCapsPtr *qemuCaps, const char *binary, const char *cacheDir, - uid_t runUid, - gid_t runGid) + virQEMUCapsCachePrivPtr priv) { char *capsdir = NULL; char *capsfile = NULL; @@ -4371,7 +4377,7 @@ virQEMUCapsInitCached(virCapsPtr caps, goto discard; } - if (!virQEMUCapsIsValid(qemuCapsNew, runUid, runGid)) + if (!virQEMUCapsIsValid(qemuCapsNew, priv)) goto discard; VIR_DEBUG("Loaded '%s' for '%s' ctime %lld usedQMP=%d", @@ -5277,22 +5283,21 @@ virQEMUCapsNewForBinaryInternal(virCapsPtr caps, static virQEMUCapsPtr virQEMUCapsNewForBinary(virCapsPtr caps, const char *binary, - const char *libDir, const char *cacheDir, - uid_t runUid, - gid_t runGid) + virQEMUCapsCachePrivPtr priv) { int rv; virQEMUCapsPtr qemuCaps = NULL; - if ((rv = virQEMUCapsInitCached(caps, &qemuCaps, binary, cacheDir, - runUid, runGid)) < 0) + if ((rv = virQEMUCapsInitCached(caps, &qemuCaps, binary, cacheDir, priv)) < 0) goto error; if (rv == 0) { if (!(qemuCaps = virQEMUCapsNewForBinaryInternal(caps, binary, - libDir, runUid, - runGid, false))) { + priv->libDir, + priv->runUid, + priv->runGid, + false))) { goto error; } @@ -5366,13 +5371,17 @@ virQEMUCapsCacheNew(const char *libDir, if (!(cache->binaries = virHashCreate(10, virObjectFreeHashData))) 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; + if (VIR_ALLOC(cache->priv) < 0) + goto error; + + if (VIR_STRDUP(cache->priv->libDir, libDir) < 0) + goto error; + + cache->priv->runUid = runUid; + cache->priv->runGid = runGid; return cache; @@ -5389,7 +5398,7 @@ virQEMUCapsCacheValidate(virQEMUCapsCachePtr cache, virQEMUCapsPtr *qemuCaps) { if (*qemuCaps && - !virQEMUCapsIsValid(*qemuCaps, cache->runUid, cache->runGid)) { + !virQEMUCapsIsValid(*qemuCaps, cache->priv)) { VIR_DEBUG("Cached capabilities %p no longer valid for %s", *qemuCaps, binary); virHashRemoveEntry(cache->binaries, binary); @@ -5399,8 +5408,8 @@ virQEMUCapsCacheValidate(virQEMUCapsCachePtr cache, if (!*qemuCaps) { VIR_DEBUG("Creating capabilities for %s", binary); *qemuCaps = virQEMUCapsNewForBinary(caps, binary, - cache->libDir, cache->cacheDir, - cache->runUid, cache->runGid); + cache->cacheDir, + cache->priv); if (*qemuCaps) { VIR_DEBUG("Caching capabilities %p for %s", *qemuCaps, binary); if (virHashAddEntry(cache->binaries, binary, *qemuCaps) < 0) { @@ -5519,7 +5528,7 @@ virQEMUCapsCacheFree(virQEMUCapsCachePtr cache) if (!cache) return; - VIR_FREE(cache->libDir); + virQEMUCapsCachePrivFree(cache->priv); VIR_FREE(cache->cacheDir); virHashFree(cache->binaries); virMutexDestroy(&cache->lock); diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h index 3458fc0700..61e7c34d12 100644 --- a/src/qemu/qemu_capspriv.h +++ b/src/qemu/qemu_capspriv.h @@ -28,13 +28,19 @@ #ifndef __QEMU_CAPSPRIV_H__ # define __QEMU_CAPSPRIV_H__ +struct _virQEMUCapsCachePriv { + char *libDir; + uid_t runUid; + gid_t runGid; +}; +typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv; +typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr; + struct _virQEMUCapsCache { virMutex lock; virHashTablePtr binaries; - char *libDir; char *cacheDir; - uid_t runUid; - gid_t runGid; + virQEMUCapsCachePrivPtr priv; }; virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps); -- 2.13.2
On Mon, Jul 10, 2017 at 14:46:48 +0200, Pavel Hrdina wrote:
This will store private data that will be used by following patches when switching to virFileCache.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 51 ++++++++++++++++++++++++++------------------ src/qemu/qemu_capspriv.h | 12 ++++++++--- 2 files changed, 39 insertions(+), 24 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The switch contains considerable amount of changes: virQEMUCapsRememberCached() is removed because this is now handled by virFileCacheSave(). virQEMUCapsInitCached() is removed because this is now handled by virFileCacheLoad(). virQEMUCapsNewForBinary() is split into two functions, virQEMUCapsNewData() which creates new data if there is nothing cached and virQEMUCapsLoadFile() which loads the cached data. This is now handled by virFileCacheNewData(). virQEMUCapsCacheValidate() is removed because this is now handled by virFileCacheValidate(). virQEMUCapsCacheFree() is removed because it's no longer required. Add virCapsPtr into virQEMUCapsCachePriv because for each call of virFileCacheLookup*() we need to use current virCapsPtr. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 322 ++++++++++++------------------------------- src/qemu/qemu_capabilities.h | 17 +-- src/qemu/qemu_capspriv.h | 8 +- src/qemu/qemu_conf.h | 3 +- src/qemu/qemu_driver.c | 2 +- tests/testutilsqemu.c | 9 +- tests/testutilsqemu.h | 3 +- 7 files changed, 102 insertions(+), 262 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e190cfa8b1..c3e09616de 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -29,6 +29,7 @@ #include "virlog.h" #include "virerror.h" #include "virfile.h" +#include "virfilecache.h" #include "virpidfile.h" #include "virprocess.h" #include "cpu/cpu.h" @@ -953,7 +954,7 @@ virQEMUCapsFindBinaryForArch(virArch hostarch, static int virQEMUCapsInitGuest(virCapsPtr caps, - virQEMUCapsCachePtr cache, + virFileCachePtr cache, virArch hostarch, virArch guestarch) { @@ -1170,7 +1171,8 @@ virQEMUCapsProbeHostCPUForEmulator(virCapsPtr caps, } -virCapsPtr virQEMUCapsInit(virQEMUCapsCachePtr cache) +virCapsPtr +virQEMUCapsInit(virFileCachePtr cache) { virCapsPtr caps; size_t i; @@ -2163,7 +2165,7 @@ virQEMUCapsExtractDeviceStr(const char *qemu, int virQEMUCapsGetDefaultVersion(virCapsPtr caps, - virQEMUCapsCachePtr capsCache, + virFileCachePtr capsCache, unsigned int *version) { virQEMUCapsPtr qemucaps; @@ -3766,8 +3768,10 @@ virQEMUCapsLoadCPUModels(virQEMUCapsPtr qemuCaps, static void -virQEMUCapsCachePrivFree(virQEMUCapsCachePrivPtr priv) +virQEMUCapsCachePrivFree(void *privData) { + virQEMUCapsCachePrivPtr priv = privData; + VIR_FREE(priv->libDir); VIR_FREE(priv); } @@ -4196,8 +4200,11 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps) static int -virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) +virQEMUCapsSaveFile(void *data, + const char *filename, + void *privData ATTRIBUTE_UNUSED) { + virQEMUCapsPtr qemuCaps = data; char *xml = NULL; int ret = -1; @@ -4221,48 +4228,13 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) 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 bool -virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, - virQEMUCapsCachePrivPtr priv) +virQEMUCapsIsValid(void *data, + void *privData) { + virQEMUCapsPtr qemuCaps = data; + virQEMUCapsCachePrivPtr priv = privData; bool kvmUsable; struct stat sb; @@ -4321,87 +4293,6 @@ virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps, } -static int -virQEMUCapsInitCached(virCapsPtr caps, - virQEMUCapsPtr *qemuCaps, - const char *binary, - const char *cacheDir, - virQEMUCapsCachePrivPtr priv) -{ - char *capsdir = NULL; - char *capsfile = NULL; - int ret = -1; - char *binaryhash = NULL; - struct stat sb; - virQEMUCapsPtr qemuCapsNew = NULL; - - if (virAsprintf(&capsdir, "%s/capabilities", cacheDir) < 0) - goto cleanup; - - if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, 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, binary); - ret = 0; - goto cleanup; - } - virReportSystemError(errno, - _("Unable to access cache '%s' for '%s'"), - capsfile, binary); - goto cleanup; - } - - if (!(qemuCapsNew = virQEMUCapsNew())) - goto cleanup; - - if (VIR_STRDUP(qemuCapsNew->binary, binary) < 0) - goto discard; - - if (virQEMUCapsLoadCache(caps, qemuCapsNew, capsfile) < 0) { - VIR_WARN("Failed to load cached caps from '%s' for '%s': %s", - capsfile, qemuCapsNew->binary, virGetLastErrorMessage()); - virResetLastError(); - goto discard; - } - - if (!virQEMUCapsIsValid(qemuCapsNew, priv)) - goto discard; - - VIR_DEBUG("Loaded '%s' for '%s' ctime %lld usedQMP=%d", - capsfile, qemuCapsNew->binary, - (long long)qemuCapsNew->ctime, qemuCapsNew->usedQMP); - - ret = 1; - *qemuCaps = qemuCapsNew; - cleanup: - VIR_FREE(binaryhash); - VIR_FREE(capsfile); - VIR_FREE(capsdir); - return ret; - - discard: - VIR_DEBUG("Dropping cached capabilities '%s' for '%s'", - capsfile, qemuCapsNew->binary); - ignore_value(unlink(capsfile)); - virObjectUnref(qemuCapsNew); - ret = 0; - goto cleanup; -} - - #define QEMU_SYSTEM_PREFIX "qemu-system-" static int @@ -5280,36 +5171,45 @@ virQEMUCapsNewForBinaryInternal(virCapsPtr caps, goto cleanup; } -static virQEMUCapsPtr -virQEMUCapsNewForBinary(virCapsPtr caps, - const char *binary, - const char *cacheDir, - virQEMUCapsCachePrivPtr priv) +static void * +virQEMUCapsNewData(const char *binary, + void *privData) { - int rv; - virQEMUCapsPtr qemuCaps = NULL; + virQEMUCapsCachePrivPtr priv = privData; - if ((rv = virQEMUCapsInitCached(caps, &qemuCaps, binary, cacheDir, priv)) < 0) - goto error; + return virQEMUCapsNewForBinaryInternal(priv->caps, + binary, + priv->libDir, + priv->runUid, + priv->runGid, + false); +} + + +static void * +virQEMUCapsLoadFile(const char *filename, + const char *binary, + void *privData) +{ + virQEMUCapsPtr qemuCaps = virQEMUCapsNew(); + virQEMUCapsCachePrivPtr priv = privData; - if (rv == 0) { - if (!(qemuCaps = virQEMUCapsNewForBinaryInternal(caps, binary, - priv->libDir, - priv->runUid, - priv->runGid, - false))) { - goto error; - } + if (!qemuCaps) + return NULL; - if (virQEMUCapsRememberCached(qemuCaps, cacheDir) < 0) - goto error; - } + if (VIR_STRDUP(qemuCaps->binary, binary) < 0) + goto error; + + if (virQEMUCapsLoadCache(priv->caps, qemuCaps, filename) < 0) + goto error; + cleanup: return qemuCaps; error: virObjectUnref(qemuCaps); - return NULL; + qemuCaps = NULL; + goto cleanup; } @@ -5351,90 +5251,63 @@ virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps, } -virQEMUCapsCachePtr +virFileCacheHandlers qemuCapsCacheHandlers = { + .isValid = virQEMUCapsIsValid, + .newData = virQEMUCapsNewData, + .loadFile = virQEMUCapsLoadFile, + .saveFile = virQEMUCapsSaveFile, + .privFree = virQEMUCapsCachePrivFree, +}; + + +virFileCachePtr virQEMUCapsCacheNew(const char *libDir, const char *cacheDir, uid_t runUid, gid_t runGid) { - virQEMUCapsCachePtr cache; + char *capsCacheDir = NULL; + virFileCachePtr cache = NULL; + virQEMUCapsCachePrivPtr priv = NULL; - if (VIR_ALLOC(cache) < 0) - return NULL; - - if (virMutexInit(&cache->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to initialize mutex")); - VIR_FREE(cache); - return NULL; - } - - if (!(cache->binaries = virHashCreate(10, virObjectFreeHashData))) + if (virAsprintf(&capsCacheDir, "%s/capabilities", cacheDir) < 0) goto error; - if (VIR_STRDUP(cache->cacheDir, cacheDir) < 0) + + if (!(cache = virFileCacheNew(capsCacheDir, qemuCapsCacheHandlers))) goto error; - if (VIR_ALLOC(cache->priv) < 0) + if (VIR_ALLOC(priv) < 0) goto error; + virFileCacheSetPriv(cache, priv); - if (VIR_STRDUP(cache->priv->libDir, libDir) < 0) + if (VIR_STRDUP(priv->libDir, libDir) < 0) goto error; - cache->priv->runUid = runUid; - cache->priv->runGid = runGid; + priv->runUid = runUid; + priv->runGid = runGid; + cleanup: + VIR_FREE(capsCacheDir); return cache; error: - virQEMUCapsCacheFree(cache); - return NULL; -} - - -static void ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) -virQEMUCapsCacheValidate(virQEMUCapsCachePtr cache, - const char *binary, - virCapsPtr caps, - virQEMUCapsPtr *qemuCaps) -{ - if (*qemuCaps && - !virQEMUCapsIsValid(*qemuCaps, cache->priv)) { - VIR_DEBUG("Cached capabilities %p no longer valid for %s", - *qemuCaps, binary); - virHashRemoveEntry(cache->binaries, binary); - *qemuCaps = NULL; - } - - if (!*qemuCaps) { - VIR_DEBUG("Creating capabilities for %s", binary); - *qemuCaps = virQEMUCapsNewForBinary(caps, binary, - cache->cacheDir, - cache->priv); - if (*qemuCaps) { - VIR_DEBUG("Caching capabilities %p for %s", *qemuCaps, binary); - if (virHashAddEntry(cache->binaries, binary, *qemuCaps) < 0) { - virObjectUnref(*qemuCaps); - *qemuCaps = NULL; - } - } - } + virObjectUnref(cache); + cache = NULL; + goto cleanup; } virQEMUCapsPtr virQEMUCapsCacheLookup(virCapsPtr caps, - virQEMUCapsCachePtr cache, + virFileCachePtr cache, const char *binary) { virQEMUCapsPtr ret = NULL; + virQEMUCapsCachePrivPtr priv = virFileCacheGetPriv(cache); - virMutexLock(&cache->lock); - - ret = virHashLookup(cache->binaries, binary); - virQEMUCapsCacheValidate(cache, binary, caps, &ret); - virObjectRef(ret); - - virMutexUnlock(&cache->lock); + priv->caps = caps; + ret = virFileCacheLookup(cache, binary); + priv->caps = NULL; VIR_DEBUG("Returning caps %p for %s", ret, binary); return ret; @@ -5443,7 +5316,7 @@ virQEMUCapsCacheLookup(virCapsPtr caps, virQEMUCapsPtr virQEMUCapsCacheLookupCopy(virCapsPtr caps, - virQEMUCapsCachePtr cache, + virFileCachePtr cache, const char *binary, const char *machineType) { @@ -5478,64 +5351,39 @@ virQEMUCapsCompareArch(const void *payload, virQEMUCapsPtr virQEMUCapsCacheLookupByArch(virCapsPtr caps, - virQEMUCapsCachePtr cache, + virFileCachePtr cache, virArch arch) { virQEMUCapsPtr ret = NULL; + virQEMUCapsCachePrivPtr priv = virFileCacheGetPriv(cache); virArch target; struct virQEMUCapsSearchData data = { .arch = arch }; - virMutexLock(&cache->lock); - ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, &data, NULL); + priv->caps = caps; + ret = virFileCacheLookupByFunc(cache, virQEMUCapsCompareArch, &data); if (!ret) { /* If the first attempt at finding capabilities has failed, try * again using the QEMU target as lookup key instead */ target = virQEMUCapsFindTarget(virArchFromHost(), data.arch); if (target != data.arch) { data.arch = target; - ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, - &data, NULL); + ret = virFileCacheLookupByFunc(cache, virQEMUCapsCompareArch, &data); } } + priv->caps = NULL; - if (ret) { - char *binary; - - if (VIR_STRDUP(binary, ret->binary) < 0) { - ret = NULL; - } else { - virQEMUCapsCacheValidate(cache, binary, caps, &ret); - VIR_FREE(binary); - } - } else { + if (!ret) { virReportError(VIR_ERR_INVALID_ARG, _("unable to find any emulator to serve '%s' " "architecture"), virArchToString(arch)); } - virObjectRef(ret); - virMutexUnlock(&cache->lock); - VIR_DEBUG("Returning caps %p for arch %s", ret, virArchToString(arch)); return ret; } -void -virQEMUCapsCacheFree(virQEMUCapsCachePtr cache) -{ - if (!cache) - return; - - virQEMUCapsCachePrivFree(cache->priv); - VIR_FREE(cache->cacheDir); - virHashFree(cache->binaries); - virMutexDestroy(&cache->lock); - VIR_FREE(cache); -} - - bool virQEMUCapsSupportsChardev(const virDomainDef *def, virQEMUCapsPtr qemuCaps, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 7f5382df62..77b519d939 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -30,6 +30,7 @@ # include "qemu_monitor.h" # include "domain_capabilities.h" # include "virfirmware.h" +# include "virfilecache.h" /* * Internal flags to keep track of qemu command line capabilities @@ -423,9 +424,6 @@ typedef enum { typedef struct _virQEMUCaps virQEMUCaps; typedef virQEMUCaps *virQEMUCapsPtr; -typedef struct _virQEMUCapsCache virQEMUCapsCache; -typedef virQEMUCapsCache *virQEMUCapsCachePtr; - virQEMUCapsPtr virQEMUCapsNew(void); void virQEMUCapsSet(virQEMUCapsPtr qemuCaps, @@ -497,25 +495,24 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps, void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps, const char *machineType); -virQEMUCapsCachePtr virQEMUCapsCacheNew(const char *libDir, +virFileCachePtr virQEMUCapsCacheNew(const char *libDir, const char *cacheDir, uid_t uid, gid_t gid); virQEMUCapsPtr virQEMUCapsCacheLookup(virCapsPtr caps, - virQEMUCapsCachePtr cache, + virFileCachePtr cache, const char *binary); virQEMUCapsPtr virQEMUCapsCacheLookupCopy(virCapsPtr caps, - virQEMUCapsCachePtr cache, + virFileCachePtr cache, const char *binary, const char *machineType); virQEMUCapsPtr virQEMUCapsCacheLookupByArch(virCapsPtr caps, - virQEMUCapsCachePtr cache, + virFileCachePtr cache, virArch arch); -void virQEMUCapsCacheFree(virQEMUCapsCachePtr cache); -virCapsPtr virQEMUCapsInit(virQEMUCapsCachePtr cache); +virCapsPtr virQEMUCapsInit(virFileCachePtr cache); int virQEMUCapsGetDefaultVersion(virCapsPtr caps, - virQEMUCapsCachePtr capsCache, + virFileCachePtr capsCache, unsigned int *version); VIR_ENUM_DECL(virQEMUCaps); diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h index 61e7c34d12..77e41d753b 100644 --- a/src/qemu/qemu_capspriv.h +++ b/src/qemu/qemu_capspriv.h @@ -29,6 +29,7 @@ # define __QEMU_CAPSPRIV_H__ struct _virQEMUCapsCachePriv { + virCapsPtr caps; char *libDir; uid_t runUid; gid_t runGid; @@ -36,13 +37,6 @@ struct _virQEMUCapsCachePriv { typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv; typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr; -struct _virQEMUCapsCache { - virMutex lock; - virHashTablePtr binaries; - char *cacheDir; - virQEMUCapsCachePrivPtr priv; -}; - virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps); virQEMUCapsPtr diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 1407eef4d4..3013f24ed4 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -48,6 +48,7 @@ # include "virclosecallbacks.h" # include "virhostdev.h" # include "virfile.h" +# include "virfilecache.h" # include "virfirmware.h" # ifdef CPU_SETSIZE /* Linux */ @@ -245,7 +246,7 @@ struct _virQEMUDriver { virDomainXMLOptionPtr xmlopt; /* Immutable pointer, self-locking APIs */ - virQEMUCapsCachePtr qemuCapsCache; + virFileCachePtr qemuCapsCache; /* Immutable pointer, self-locking APIs */ virObjectEventStatePtr domainEventState; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cdb727b554..fd705645c4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1078,7 +1078,7 @@ qemuStateCleanup(void) virObjectUnref(qemu_driver->hostdevMgr); virHashFree(qemu_driver->sharedDevices); virObjectUnref(qemu_driver->caps); - virQEMUCapsCacheFree(qemu_driver->qemuCapsCache); + virObjectUnref(qemu_driver->qemuCapsCache); virObjectUnref(qemu_driver->domains); virObjectUnref(qemu_driver->remotePorts); diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 88e11ba14a..710d12dc4c 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -11,6 +11,7 @@ # define __QEMU_CAPSPRIV_H_ALLOW__ # include "qemu/qemu_capspriv.h" # include "virstring.h" +# include "virfilecache.h" # define VIR_FROM_THIS VIR_FROM_QEMU @@ -587,14 +588,14 @@ void qemuTestDriverFree(virQEMUDriver *driver) virFileDeleteTree(driver->config->stateDir); virFileDeleteTree(driver->config->configDir); } - virQEMUCapsCacheFree(driver->qemuCapsCache); + virObjectUnref(driver->qemuCapsCache); virObjectUnref(driver->xmlopt); virObjectUnref(driver->caps); virObjectUnref(driver->config); virObjectUnref(driver->securityManager); } -int qemuTestCapsCacheInsert(virQEMUCapsCachePtr cache, +int qemuTestCapsCacheInsert(virFileCachePtr cache, virQEMUCapsPtr caps) { size_t i; @@ -609,9 +610,7 @@ int qemuTestCapsCacheInsert(virQEMUCapsCachePtr cache, for (i = 0; i < ARRAY_CARDINALITY(QEMUBinList); i++) { virObjectRef(tmpCaps); - if (virHashUpdateEntry(cache->binaries, - QEMUBinList[i], - tmpCaps) < 0) { + if (virFileCacheInsertData(cache, QEMUBinList[i], tmpCaps) < 0) { virObjectUnref(tmpCaps); return -1; } diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h index 05e5651ae8..f29c6e5d62 100644 --- a/tests/testutilsqemu.h +++ b/tests/testutilsqemu.h @@ -1,6 +1,7 @@ #ifdef WITH_QEMU # include "capabilities.h" +# include "virfilecache.h" # include "domain_conf.h" # include "qemu/qemu_capabilities.h" # include "qemu/qemu_conf.h" @@ -30,7 +31,7 @@ void qemuTestSetHostCPU(virCapsPtr caps, int qemuTestDriverInit(virQEMUDriver *driver); void qemuTestDriverFree(virQEMUDriver *driver); -int qemuTestCapsCacheInsert(virQEMUCapsCachePtr cache, +int qemuTestCapsCacheInsert(virFileCachePtr cache, virQEMUCapsPtr caps); int testQemuCapsSetGIC(virQEMUCapsPtr qemuCaps, -- 2.13.2
On Mon, Jul 10, 2017 at 14:46:49 +0200, Pavel Hrdina wrote:
The switch contains considerable amount of changes:
virQEMUCapsRememberCached() is removed because this is now handled by virFileCacheSave().
virQEMUCapsInitCached() is removed because this is now handled by virFileCacheLoad().
virQEMUCapsNewForBinary() is split into two functions, virQEMUCapsNewData() which creates new data if there is nothing cached and virQEMUCapsLoadFile() which loads the cached data. This is now handled by virFileCacheNewData().
virQEMUCapsCacheValidate() is removed because this is now handled by virFileCacheValidate().
virQEMUCapsCacheFree() is removed because it's no longer required.
Add virCapsPtr into virQEMUCapsCachePriv because for each call of virFileCacheLookup*() we need to use current virCapsPtr.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 322 ++++++++++++------------------------------- src/qemu/qemu_capabilities.h | 17 +-- src/qemu/qemu_capspriv.h | 8 +- src/qemu/qemu_conf.h | 3 +- src/qemu/qemu_driver.c | 2 +- tests/testutilsqemu.c | 9 +- tests/testutilsqemu.h | 3 +- 7 files changed, 102 insertions(+), 262 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e190cfa8b1..c3e09616de 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c ... virQEMUCapsPtr virQEMUCapsCacheLookup(virCapsPtr caps, - virQEMUCapsCachePtr cache, + virFileCachePtr cache, const char *binary) { virQEMUCapsPtr ret = NULL; + virQEMUCapsCachePrivPtr priv = virFileCacheGetPriv(cache);
- virMutexLock(&cache->lock); - - ret = virHashLookup(cache->binaries, binary); - virQEMUCapsCacheValidate(cache, binary, caps, &ret); - virObjectRef(ret); - - virMutexUnlock(&cache->lock); + priv->caps = caps; + ret = virFileCacheLookup(cache, binary); + priv->caps = NULL;
This is not safe anymore since the cache is locked only inside virFileCacheLookup. I think you will need to use a new parameter for passing the capabilities to virFileCacheLookup.
VIR_DEBUG("Returning caps %p for %s", ret, binary); return ret;
...
@@ -5478,64 +5351,39 @@ virQEMUCapsCompareArch(const void *payload,
virQEMUCapsPtr virQEMUCapsCacheLookupByArch(virCapsPtr caps, - virQEMUCapsCachePtr cache, + virFileCachePtr cache, virArch arch) { virQEMUCapsPtr ret = NULL; + virQEMUCapsCachePrivPtr priv = virFileCacheGetPriv(cache); virArch target; struct virQEMUCapsSearchData data = { .arch = arch };
- virMutexLock(&cache->lock); - ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, &data, NULL); + priv->caps = caps; + ret = virFileCacheLookupByFunc(cache, virQEMUCapsCompareArch, &data); if (!ret) { /* If the first attempt at finding capabilities has failed, try * again using the QEMU target as lookup key instead */ target = virQEMUCapsFindTarget(virArchFromHost(), data.arch); if (target != data.arch) { data.arch = target; - ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, - &data, NULL); + ret = virFileCacheLookupByFunc(cache, virQEMUCapsCompareArch, &data); } } + priv->caps = NULL;
This is unsafe too.
- if (ret) { - char *binary; - - if (VIR_STRDUP(binary, ret->binary) < 0) { - ret = NULL; - } else { - virQEMUCapsCacheValidate(cache, binary, caps, &ret); - VIR_FREE(binary); - } - } else { + if (!ret) { virReportError(VIR_ERR_INVALID_ARG, _("unable to find any emulator to serve '%s' " "architecture"), virArchToString(arch)); }
- virObjectRef(ret); - virMutexUnlock(&cache->lock); - VIR_DEBUG("Returning caps %p for arch %s", ret, virArchToString(arch));
return ret; }
Jirka
On Wed, Jul 19, 2017 at 12:19:17PM +0200, Jiri Denemark wrote:
On Mon, Jul 10, 2017 at 14:46:49 +0200, Pavel Hrdina wrote:
The switch contains considerable amount of changes:
virQEMUCapsRememberCached() is removed because this is now handled by virFileCacheSave().
virQEMUCapsInitCached() is removed because this is now handled by virFileCacheLoad().
virQEMUCapsNewForBinary() is split into two functions, virQEMUCapsNewData() which creates new data if there is nothing cached and virQEMUCapsLoadFile() which loads the cached data. This is now handled by virFileCacheNewData().
virQEMUCapsCacheValidate() is removed because this is now handled by virFileCacheValidate().
virQEMUCapsCacheFree() is removed because it's no longer required.
Add virCapsPtr into virQEMUCapsCachePriv because for each call of virFileCacheLookup*() we need to use current virCapsPtr.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 322 ++++++++++++------------------------------- src/qemu/qemu_capabilities.h | 17 +-- src/qemu/qemu_capspriv.h | 8 +- src/qemu/qemu_conf.h | 3 +- src/qemu/qemu_driver.c | 2 +- tests/testutilsqemu.c | 9 +- tests/testutilsqemu.h | 3 +- 7 files changed, 102 insertions(+), 262 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e190cfa8b1..c3e09616de 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c ... virQEMUCapsPtr virQEMUCapsCacheLookup(virCapsPtr caps, - virQEMUCapsCachePtr cache, + virFileCachePtr cache, const char *binary) { virQEMUCapsPtr ret = NULL; + virQEMUCapsCachePrivPtr priv = virFileCacheGetPriv(cache);
- virMutexLock(&cache->lock); - - ret = virHashLookup(cache->binaries, binary); - virQEMUCapsCacheValidate(cache, binary, caps, &ret); - virObjectRef(ret); - - virMutexUnlock(&cache->lock); + priv->caps = caps; + ret = virFileCacheLookup(cache, binary); + priv->caps = NULL;
This is not safe anymore since the cache is locked only inside virFileCacheLookup. I think you will need to use a new parameter for passing the capabilities to virFileCacheLookup.
Right, I thought that this might be an issue, I'll fix both cases. Thanks, Pavel
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 10 ++++++++++ src/qemu/qemu_capspriv.h | 9 --------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c3e09616de..0395a397ce 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3767,6 +3767,16 @@ virQEMUCapsLoadCPUModels(virQEMUCapsPtr qemuCaps, } +struct _virQEMUCapsCachePriv { + virCapsPtr caps; + char *libDir; + uid_t runUid; + gid_t runGid; +}; +typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv; +typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr; + + static void virQEMUCapsCachePrivFree(void *privData) { diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h index 77e41d753b..9c360438db 100644 --- a/src/qemu/qemu_capspriv.h +++ b/src/qemu/qemu_capspriv.h @@ -28,15 +28,6 @@ #ifndef __QEMU_CAPSPRIV_H__ # define __QEMU_CAPSPRIV_H__ -struct _virQEMUCapsCachePriv { - virCapsPtr caps; - char *libDir; - uid_t runUid; - gid_t runGid; -}; -typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv; -typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr; - virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps); virQEMUCapsPtr -- 2.13.2
On Mon, Jul 10, 2017 at 14:46:50 +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 10 ++++++++++ src/qemu/qemu_capspriv.h | 9 --------- 2 files changed, 10 insertions(+), 9 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
participants (2)
-
Jiri Denemark -
Pavel Hrdina