10:08 a.m.
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(a)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