[libvirt] [PATCH 18/18] util: storage: Invert the way recursive metadata retrieval works
Peter Krempa
pkrempa at redhat.com
Sun Apr 20 22:13:22 UTC 2014
To avoid having the root of a backing chain present twice in the list we
need to invert the working of virStorageFileGetMetadataRecurse.
Until now the recursive worker created a new backing chain element from
the name and other information passed as arguments. This required us to
pass the data of the parend in a deconstructed way and the worker
created a new entry for the parent.
This patch converts this function so that it just fills in metadata
about the parent and creates a backing chain element from those. This
removes the duplication of the first element.
To avoid breaking the test suite, virstoragetest now calls a wrapper
that creates the parent structure explicitly and pre-fills it with the
test data with same function signature as previously used.
---
src/conf/domain_conf.c | 5 +-
src/qemu/qemu_domain.c | 12 ++-
src/qemu/qemu_driver.c | 6 +-
src/security/virt-aa-helper.c | 7 +-
src/util/virstoragefile.c | 193 ++++++++++++++++++++++--------------------
src/util/virstoragefile.h | 7 +-
tests/virstoragetest.c | 47 +++++++++-
7 files changed, 158 insertions(+), 119 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 69ca5e7..99b57ee 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18564,9 +18564,8 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
if (iter(disk, path, 0, opaque) < 0)
goto cleanup;
- /* XXX: temporarily we need to select the second element of the backing
- * chain to start as the first is the copy of the disk itself. */
- tmp = disk->src.backingStore ? disk->src.backingStore->backingStore : NULL;
+
+ tmp = disk->src.backingStore;
while (tmp && virStorageIsFile(tmp->path)) {
if (!ignoreOpenFailure && tmp->backingStoreRaw && !tmp->backingStore) {
virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 45ed872..bb9cb6b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2239,10 +2239,10 @@ qemuDiskChainCheckBroken(virDomainDiskDefPtr disk)
{
char *brokenFile = NULL;
- if (!virDomainDiskGetSource(disk) || !disk->src.backingStore)
+ if (!virDomainDiskGetSource(disk))
return 0;
- if (virStorageFileChainGetBroken(disk->src.backingStore, &brokenFile) < 0)
+ if (virStorageFileChainGetBroken(&disk->src, &brokenFile) < 0)
return -1;
if (brokenFile) {
@@ -2419,11 +2419,9 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
qemuDomainGetImageIds(cfg, vm, disk, &uid, &gid);
- disk->src.backingStore = virStorageFileGetMetadata(src,
- virDomainDiskGetFormat(disk),
- uid, gid,
- cfg->allowDiskFormatProbing);
- if (!disk->src.backingStore)
+ if (virStorageFileGetMetadata(&disk->src,
+ uid, gid,
+ cfg->allowDiskFormatProbing) < 0)
ret = -1;
cleanup:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c0094c1..7dd2b86 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15134,7 +15134,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) &&
STREQ_NULLABLE(format, "raw") &&
- disk->src.backingStore->backingStore->path) {
+ disk->src.backingStore->path) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' has backing file, so raw shallow copy "
"is not possible"),
@@ -15341,8 +15341,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
if (!top) {
top_canon = disk->src.path;
- top_meta = disk->src.backingStore;
- } else if (!(top_canon = virStorageFileChainLookup(disk->src.backingStore,
+ top_meta = &disk->src;
+ } else if (!(top_canon = virStorageFileChainLookup(&disk->src,
top, &top_meta,
&top_parent))) {
goto endjob;
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 1c89815..32fc04a 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -943,18 +943,15 @@ get_files(vahControl * ctl)
for (i = 0; i < ctl->def->ndisks; i++) {
virDomainDiskDefPtr disk = ctl->def->disks[i];
- const char *src = virDomainDiskGetSource(disk);
- if (!src)
+ if (!virDomainDiskGetSource(disk))
continue;
/* XXX - if we knew the qemu user:group here we could send it in
* so that the open could be re-tried as that user:group.
*/
if (!disk->src.backingStore) {
bool probe = ctl->allowDiskFormatProbing;
- disk->src.backingStore = virStorageFileGetMetadata(src,
- virDomainDiskGetFormat(disk),
- -1, -1, probe);
+ virStorageFileGetMetadata(&disk->src, -1, -1, probe);
}
/* XXX passing ignoreOpenFailure = true to get back to the behavior
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 8d5ef2f..b2d8f62 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1104,105 +1104,112 @@ virStorageFileGetMetadataFromFD(const char *path,
/* Recursive workhorse for virStorageFileGetMetadata. */
static int
-virStorageFileGetMetadataRecurse(const char *path, const char *canonPath,
- const char *directory,
- int format, uid_t uid, gid_t gid,
- bool allow_probe, virHashTablePtr cycle,
- virStorageSourcePtr meta)
+virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
+ const char *canonPath,
+ uid_t uid, gid_t gid,
+ bool allow_probe,
+ virHashTablePtr cycle)
{
int fd;
int ret = -1;
+ virStorageSourcePtr backingStore = NULL;
int backingFormat;
- char *backingPath = NULL;
- char *backingDirectory = NULL;
VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d",
- path, canonPath, NULLSTR(directory), format,
+ src->path, canonPath, NULLSTR(src->relDir), src->format,
(int)uid, (int)gid, allow_probe);
if (virHashLookup(cycle, canonPath)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("backing store for %s is self-referential"),
- path);
+ src->path);
return -1;
}
+
if (virHashAddEntry(cycle, canonPath, (void *)1) < 0)
return -1;
- if (virStorageIsFile(path)) {
- if (VIR_STRDUP(meta->relPath, path) < 0)
- return -1;
- if (VIR_STRDUP(meta->path, canonPath) < 0)
- return -1;
- if (VIR_STRDUP(meta->relDir, directory) < 0)
+ if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) {
+ if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, uid, gid, 0)) < 0) {
+ virReportSystemError(-fd, _("Failed to open file '%s'"),
+ src->path);
return -1;
- meta->format = format;
+ }
- if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) {
- virReportSystemError(-fd, _("Failed to open file '%s'"), path);
+ if (virStorageFileGetMetadataFromFDInternal(src, fd,
+ &backingFormat) < 0) {
+ VIR_FORCE_CLOSE(fd);
return -1;
}
- ret = virStorageFileGetMetadataFromFDInternal(meta, fd, &backingFormat);
-
if (VIR_CLOSE(fd) < 0)
- VIR_WARN("could not close file %s", path);
+ VIR_WARN("could not close file %s", src->path);
} else {
- /* FIXME: when the proper storage drivers are compiled in, it
- * would be nice to read metadata from the network storage to
- * allow for non-raw images. */
- if (VIR_STRDUP(meta->relPath, path) < 0)
- return -1;
- if (VIR_STRDUP(meta->path, path) < 0)
- return -1;
- meta->type = VIR_STORAGE_TYPE_NETWORK;
- meta->format = VIR_STORAGE_FILE_RAW;
- ret = 0;
+ /* TODO: currently we call this only for local storage */
+ return 0;
}
- if (ret == 0 && meta->backingStoreRaw) {
- if (virStorageIsFile(meta->backingStoreRaw)) {
- if (virFindBackingFile(directory,
- meta->backingStoreRaw,
- &backingDirectory,
- &backingPath) < 0) {
- /* the backing file is (currently) unavailable, treat this
- * file as standalone:
- * backingStoreRaw is kept to mark broken image chains */
- VIR_WARN("Backing file '%s' of image '%s' is missing.",
- meta->backingStoreRaw, path);
-
- return 0;
- }
- } else {
- if (VIR_STRDUP(backingPath, meta->backingStoreRaw) < 0)
- return -1;
- }
+ /* check wether we need to go deeper */
+ if (!src->backingStoreRaw)
+ return 0;
- virStorageSourcePtr backing;
-
- if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe)
- backingFormat = VIR_STORAGE_FILE_RAW;
- else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE)
- backingFormat = VIR_STORAGE_FILE_AUTO;
- if (VIR_ALLOC(backing) < 0 ||
- virStorageFileGetMetadataRecurse(meta->backingStoreRaw,
- backingPath,
- backingDirectory, backingFormat,
- uid, gid, allow_probe,
- cycle, backing) < 0) {
- /* If we failed to get backing data, mark the chain broken */
- virStorageSourceFree(backing);
- } else {
- meta->backingStore = backing;
+ if (VIR_ALLOC(backingStore) < 0)
+ return -1;
+
+ if (VIR_STRDUP(backingStore->relPath, src->backingStoreRaw) < 0)
+ goto cleanup;
+
+ if (virStorageIsFile(src->backingStoreRaw)) {
+ backingStore->type = VIR_STORAGE_TYPE_FILE;
+
+ if (virFindBackingFile(src->relDir,
+ src->backingStoreRaw,
+ &backingStore->relDir,
+ &backingStore->path) < 0) {
+ /* the backing file is (currently) unavailable, treat this
+ * file as standalone:
+ * backingStoreRaw is kept to mark broken image chains */
+ VIR_WARN("Backing file '%s' of image '%s' is missing.",
+ src->backingStoreRaw, src->path);
+ ret = 0;
+ goto cleanup;
}
+ } else {
+ /* TODO: To satisfy the test case, copy the network URI as path. T
+ * his will be removed later */
+ backingStore->type = VIR_STORAGE_TYPE_NETWORK;
+
+ if (VIR_STRDUP(backingStore->path, src->backingStoreRaw) < 0)
+ goto cleanup;
}
- VIR_FREE(backingDirectory);
- VIR_FREE(backingPath);
+ if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe)
+ backingStore->format = VIR_STORAGE_FILE_RAW;
+ else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE)
+ backingStore->format = VIR_STORAGE_FILE_AUTO;
+ else
+ backingStore->format = backingFormat;
+
+ if (virStorageFileGetMetadataRecurse(backingStore,
+ backingStore->path,
+ uid, gid, allow_probe,
+ cycle) < 0) {
+ /* if we fail somewhere midway, just accept the and return a
+ * broken chain */
+ ret = 0;
+ goto cleanup;
+ }
+
+ src->backingStore = backingStore;
+ backingStore = NULL;
+ ret = 0;
+
+ cleanup:
+ virStorageSourceFree(backingStore);
return ret;
}
+
/**
* virStorageFileGetMetadata:
*
@@ -1220,51 +1227,51 @@ virStorageFileGetMetadataRecurse(const char *path, const char *canonPath,
*
* Caller MUST free result after use via virStorageSourceFree.
*/
-virStorageSourcePtr
-virStorageFileGetMetadata(const char *path, int format,
+int
+virStorageFileGetMetadata(virStorageSourcePtr src,
uid_t uid, gid_t gid,
bool allow_probe)
{
VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d",
- path, format, (int)uid, (int)gid, allow_probe);
+ src->path, src->format, (int)uid, (int)gid, allow_probe);
- virHashTablePtr cycle = virHashCreate(5, NULL);
- virStorageSourcePtr meta = NULL;
- virStorageSourcePtr ret = NULL;
+ virHashTablePtr cycle = NULL;
char *canonPath = NULL;
- char *directory = NULL;
+ int ret = -1;
- if (!cycle)
- return NULL;
+ if (!(cycle = virHashCreate(5, NULL)))
+ return -1;
- if (virStorageIsFile(path)) {
- if (!(canonPath = canonicalize_file_name(path))) {
- virReportSystemError(errno, _("unable to resolve '%s'"), path);
+ if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) {
+ if (!(canonPath = canonicalize_file_name(src->path))) {
+ virReportSystemError(errno, _("unable to resolve '%s'"),
+ src->path);
goto cleanup;
}
- if (!(directory = mdir_name(path))) {
+
+ if (!src->relPath &&
+ VIR_STRDUP(src->relPath, src->path) < 0)
+ goto cleanup;
+
+ if (!src->relDir &&
+ !(src->relDir = mdir_name(src->path))) {
virReportOOMError();
goto cleanup;
}
- } else if (VIR_STRDUP(canonPath, path) < 0) {
+ } else {
+ /* TODO: currently unimplemented for non-local storage */
+ ret = 0;
goto cleanup;
}
- if (VIR_ALLOC(meta) < 0)
- goto cleanup;
- if (format <= VIR_STORAGE_FILE_NONE)
- format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW;
- if (virStorageFileGetMetadataRecurse(path, canonPath, directory, format,
- uid, gid, allow_probe, cycle,
- meta) < 0)
- goto cleanup;
- ret = meta;
- meta = NULL;
+ if (src->format <= VIR_STORAGE_FILE_NONE)
+ src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW;
+
+ ret = virStorageFileGetMetadataRecurse(src, canonPath, uid, gid,
+ allow_probe, cycle);
cleanup:
- virStorageSourceFree(meta);
VIR_FREE(canonPath);
- VIR_FREE(directory);
virHashFree(cycle);
return ret;
}
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index e60befe..a0adb9b 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -263,10 +263,9 @@ int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid);
int virStorageFileProbeFormatFromBuf(const char *path, char *buf,
size_t buflen);
-virStorageSourcePtr virStorageFileGetMetadata(const char *path,
- int format,
- uid_t uid, gid_t gid,
- bool allow_probe)
+int virStorageFileGetMetadata(virStorageSourcePtr src,
+ uid_t uid, gid_t gid,
+ bool allow_probe)
ATTRIBUTE_NONNULL(1);
virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path,
int fd,
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 370b8c2..5320c78 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -29,6 +29,7 @@
#include "virlog.h"
#include "virstoragefile.h"
#include "virstring.h"
+#include "dirname.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -88,6 +89,44 @@ testCleanupImages(void)
virFileDeleteTree(datadir);
}
+
+static virStorageSourcePtr
+testStorageFileGetMetadata(const char *path,
+ int format,
+ uid_t uid, gid_t gid,
+ bool allow_probe)
+{
+ virStorageSourcePtr ret = NULL;
+
+ if (VIR_ALLOC(ret) < 0)
+ return NULL;
+
+ ret->type = VIR_STORAGE_TYPE_FILE;
+ ret->format = format;
+
+ if (VIR_STRDUP(ret->relPath, path) < 0)
+ goto error;
+
+ if (!(ret->relDir = mdir_name(path))) {
+ virReportOOMError();
+ goto error;
+ }
+
+ if (!(ret->path = canonicalize_file_name(path))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "failed to resolve '%s'", path);
+ goto error;
+ }
+
+ if (virStorageFileGetMetadata(ret, uid, gid, allow_probe) < 0)
+ goto error;
+
+ return ret;
+
+ error:
+ virStorageSourceFree(ret);
+ return NULL;
+}
+
static int
testPrepImages(void)
{
@@ -272,7 +311,7 @@ testStorageChain(const void *args)
char *broken = NULL;
bool isAbs = !!(data->flags & ABS_START);
- meta = virStorageFileGetMetadata(data->start, data->format, -1, -1,
+ meta = testStorageFileGetMetadata(data->start, data->format, -1, -1,
(data->flags & ALLOW_PROBE) != 0);
if (!meta) {
if (data->flags & EXP_FAIL) {
@@ -825,7 +864,7 @@ mymain(void)
ret = -1;
/* Test behavior of chain lookups, absolute backing from relative start */
- chain = virStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2,
+ chain = testStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2,
-1, -1, false);
if (!chain) {
ret = -1;
@@ -870,7 +909,7 @@ mymain(void)
/* Test behavior of chain lookups, relative backing from absolute start */
virStorageSourceFree(chain);
- chain = virStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2,
+ chain = testStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2,
-1, -1, false);
if (!chain) {
ret = -1;
@@ -900,7 +939,7 @@ mymain(void)
/* Test behavior of chain lookups, relative backing */
virStorageSourceFree(chain);
- chain = virStorageFileGetMetadata("sub/link2", VIR_STORAGE_FILE_QCOW2,
+ chain = testStorageFileGetMetadata("sub/link2", VIR_STORAGE_FILE_QCOW2,
-1, -1, false);
if (!chain) {
ret = -1;
--
1.9.1
More information about the libvir-list
mailing list