10:41 p.m.
I'm almost to the point where virStorageSource can track everything
that virStorageFileMetadata was used for, so that we can use one
struct instead of two. But while working on this today, I noticed
that virStorageFileChainLookup() does not have any unit tests, so
I'll be writing those before removing the two remaining redundant
fields.
Eric Blake (5):
conf: provide details on network backing store
conf: expose probe for non-local storage
conf: delete useless backingStoreIsFile field
conf: return backing information separately from metadata
conf: delete useless backingStoreFormat field
src/conf/domain_conf.c | 2 +-
src/libvirt_private.syms | 1 +
src/storage/storage_backend_fs.c | 34 ++++----
src/storage/storage_backend_gluster.c | 14 ++--
src/util/virstoragefile.c | 146 +++++++++++++++++++++-------------
src/util/virstoragefile.h | 9 ++-
tests/virstoragetest.c | 49 ++++--------
7 files changed, 135 insertions(+), 120 deletions(-)
--
1.9.0
10:41 p.m.
New subject: [libvirt] [PATCH 1/5] conf: provide details on network backing store
So far, my work has been merely preserving the status quo of
backing file analysis. But this patch starts to tread in the
territory of making the backing chain code more powerful - we
will eventually support network storage containing non-raw
formats. Here, we expose metadata information about a network
backing store, even if that information is still hardcoded to
a raw format for now.
* src/util/virstoragefile.c (virStorageFileGetMetadataRecurse):
Also populate struct for non-file backing.
(virStorageFileGetMetadata, virStorageFileGetMetadatainternal):
Recognize non-file top image.
(virFindBackingFile): Add comment.
* tests/virstoragetest.c (mymain): Update test to reflect it.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/util/virstoragefile.c | 62 +++++++++++++++++++++++++++++++++++------------
tests/virstoragetest.c | 17 +++++++++----
2 files changed, 59 insertions(+), 20 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index f5fe8ad..7a91a01 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -569,6 +569,14 @@ static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
ATTRIBUTE_NONNULL(4)
virFindBackingFile(const char *start, const char *path,
char **directory, char **canonical)
{
+ /* FIXME - when we eventually allow non-raw network devices, we
+ * must ensure that we handle backing files the same way as qemu.
+ * For a qcow2 top file of gluster://server/vol/img, qemu treats
+ * the relative backing file 'rel' as meaning
+ * 'gluster://server/vol/rel', while the backing file '/abs' is
+ * used as a local file. But we cannot canonicalize network
+ * devices via canonicalize_file_name(), because they are not part
+ * of the local file system. */
char *combined = NULL;
int ret = -1;
@@ -874,6 +882,12 @@ virStorageFileGetMetadataInternal(const char *path,
meta->backingStoreRaw, path);
}
+ } else {
+ if (VIR_STRDUP(meta->backingStoreRaw, backing) < 0) {
+ VIR_FREE(backing);
+ goto cleanup;
+ }
+ backingFormat = VIR_STORAGE_FILE_RAW;
}
VIR_FREE(backing);
meta->backingStoreFormat = backingFormat;
@@ -1132,18 +1146,32 @@ virStorageFileGetMetadataRecurse(const char *path, const char
*canonPath,
if (virHashAddEntry(cycle, canonPath, (void *)1) < 0)
return -1;
- if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) {
- virReportSystemError(-fd, _("Failed to open file '%s'"),
path);
- return -1;
- }
+ if (virBackingStoreIsFile(path)) {
+ if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) {
+ virReportSystemError(-fd, _("Failed to open file '%s'"),
path);
+ return -1;
+ }
- ret = virStorageFileGetMetadataFromFDInternal(path, canonPath, directory,
- fd, format, meta);
+ ret = virStorageFileGetMetadataFromFDInternal(path, canonPath,
+ directory,
+ fd, format, meta);
- if (VIR_CLOSE(fd) < 0)
- VIR_WARN("could not close file %s", path);
+ if (VIR_CLOSE(fd) < 0)
+ VIR_WARN("could not close file %s", 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->path, path) < 0)
+ return -1;
+ if (VIR_STRDUP(meta->canonPath, path) < 0)
+ return -1;
+ meta->type = VIR_STORAGE_TYPE_NETWORK;
+ meta->format = VIR_STORAGE_FILE_RAW;
+ ret = 0;
+ }
- if (ret == 0 && meta->backingStoreIsFile) {
+ if (ret == 0 && meta->backingStore) {
virStorageFileMetadataPtr backing;
if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO &&
!allow_probe)
@@ -1207,12 +1235,16 @@ virStorageFileGetMetadata(const char *path, int format,
if (!cycle)
return NULL;
- if (!(canonPath = canonicalize_file_name(path))) {
- virReportSystemError(errno, _("unable to resolve '%s'"),
path);
- goto cleanup;
- }
- if (!(directory = mdir_name(path))) {
- virReportOOMError();
+ if (virBackingStoreIsFile(path)) {
+ if (!(canonPath = canonicalize_file_name(path))) {
+ virReportSystemError(errno, _("unable to resolve '%s'"),
path);
+ goto cleanup;
+ }
+ if (!(directory = mdir_name(path))) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ } else if (VIR_STRDUP(canonPath, path) < 0) {
goto cleanup;
}
if (VIR_ALLOC(meta) < 0)
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 9a9b0d8..e8409f6 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -621,17 +621,24 @@ mymain(void)
if (virCommandRun(cmd, NULL) < 0)
ret = -1;
qcow2.expBackingStore = "nbd:example.org:6000";
- qcow2.expBackingStoreRaw = NULL;
+ qcow2.expBackingStoreRaw = "nbd:example.org:6000";
qcow2.expBackingDirRel = NULL;
qcow2.expBackingDirAbs = NULL;
qcow2.expBackingFormat = VIR_STORAGE_FILE_RAW;
/* Qcow2 file with backing protocol instead of file */
+ testFileData nbd = {
+ .pathRel = "nbd:example.org:6000",
+ .pathAbs = "nbd:example.org:6000",
+ .canonPath = "nbd:example.org:6000",
+ .type = VIR_STORAGE_TYPE_NETWORK,
+ .format = VIR_STORAGE_FILE_RAW,
+ };
TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2,
- (&qcow2), EXP_PASS,
- (&qcow2), ALLOW_PROBE | EXP_PASS,
- (&qcow2), EXP_PASS,
- (&qcow2), ALLOW_PROBE | EXP_PASS);
+ (&qcow2, &nbd), EXP_PASS,
+ (&qcow2, &nbd), ALLOW_PROBE | EXP_PASS,
+ (&qcow2, &nbd), EXP_PASS,
+ (&qcow2, &nbd), ALLOW_PROBE | EXP_PASS);
/* qed file */
testFileData qed = {
--
1.9.0
4:58 a.m.
New subject: [libvirt] [PATCH 1/5] conf: provide details on network backing store
On 04/10/14 05:41, Eric Blake wrote:
So far, my work has been merely preserving the status quo of
backing file analysis. But this patch starts to tread in the
territory of making the backing chain code more powerful - we
will eventually support network storage containing non-raw
formats. Here, we expose metadata information about a network
backing store, even if that information is still hardcoded to
a raw format for now.
* src/util/virstoragefile.c (virStorageFileGetMetadataRecurse):
Also populate struct for non-file backing.
(virStorageFileGetMetadata, virStorageFileGetMetadatainternal):
Recognize non-file top image.
(virFindBackingFile): Add comment.
* tests/virstoragetest.c (mymain): Update test to reflect it.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/util/virstoragefile.c | 62 +++++++++++++++++++++++++++++++++++------------
tests/virstoragetest.c | 17 +++++++++----
2 files changed, 59 insertions(+), 20 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index f5fe8ad..7a91a01 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -874,6 +882,12 @@ virStorageFileGetMetadataInternal(const char
*path,
meta->backingStoreRaw, path);
}
+ } else {
+ if (VIR_STRDUP(meta->backingStoreRaw, backing) < 0) {
+ VIR_FREE(backing);
+ goto cleanup;
+ }
+ backingFormat = VIR_STORAGE_FILE_RAW;
}
VIR_FREE(backing);
meta->backingStoreFormat = backingFormat;
This hunk invalidates the comment in virStorageFileChainCheckBroken, as
currently backing store raw will be set always.
ACK with the comment updated.
Peter
5:36 p.m.
New subject: [libvirt] [PATCH 1/5] conf: provide details on network backing store
On 04/10/2014 03:58 AM, Peter Krempa wrote:
On 04/10/14 05:41, Eric Blake wrote:
> So far, my work has been merely preserving the status quo of
> backing file analysis. But this patch starts to tread in the
> territory of making the backing chain code more powerful - we
> will eventually support network storage containing non-raw
> formats. Here, we expose metadata information about a network
> backing store, even if that information is still hardcoded to
> a raw format for now.
> + } else {
> + if (VIR_STRDUP(meta->backingStoreRaw, backing) < 0) {
> + VIR_FREE(backing);
> + goto cleanup;
> + }
> + backingFormat = VIR_STORAGE_FILE_RAW;
> }
> VIR_FREE(backing);
> meta->backingStoreFormat = backingFormat;
This hunk invalidates the comment in virStorageFileChainCheckBroken, as
currently backing store raw will be set always.
ACK with the comment updated.
I squashed this in and pushed, along with the other ACKed patches in the
series:
diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c
index 7a91a01..8222528 100644
--- i/src/util/virstoragefile.c
+++ w/src/util/virstoragefile.c
@@ -1272,7 +1272,8 @@ virStorageFileGetMetadata(const char *path, int
format,
*
* If CHAIN is broken, set *brokenFile to the broken file name,
* otherwise set it to NULL. Caller MUST free *brokenFile after use.
- * Return 0 on success, negative on error.
+ * Return 0 on success (including when brokenFile is set), negative on
+ * error (allocation failure).
*/
int
virStorageFileChainGetBroken(virStorageFileMetadataPtr chain,
@@ -1288,8 +1289,8 @@
virStorageFileChainGetBroken(virStorageFileMetadataPtr chain,
tmp = chain;
while (tmp) {
- /* Break if no backing store, backing store is not file, or
- * other problem such as infinite loop */
+ /* Break when we hit end of chain; report error if we detected
+ * a missing backing file, infinite loop, or other error */
if (!tmp->backingStoreRaw)
break;
if (!tmp->backingStore) {
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:41 p.m.
New subject: [libvirt] [PATCH 2/5] conf: expose probe for non-local storage
Deciding if a user string represents a local file instead of a
network path is an operation worth exposing directly, particularly
since the next patch will be removing a redundant variable that
was caching the information.
* src/util/virstoragefile.h (virStorageIsFile): New declaration.
* src/util/virstoragefile.c (virBackingStoreIsFile): Rename...
(virStorageIsFile): ...export, and allow NULL input.
(virStorageFileGetMetadataInternal)
(virStorageFileGetMetadataRecurse, virStorageFileGetMetadata):
Update callers.
* src/conf/domain_conf.c (virDomainDiskDefForeachPath): Use it.
* src/storage/storage_backend_fs.c (virStorageBackendProbeTarget):
Likewise.
* src/libvirt_private.syms (virstoragefile.h): Export function.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/conf/domain_conf.c | 2 +-
src/libvirt_private.syms | 1 +
src/storage/storage_backend_fs.c | 2 +-
src/util/virstoragefile.c | 20 +++++++++++++-------
src/util/virstoragefile.h | 1 +
5 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1c4f9d2..726c8ba 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18565,7 +18565,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
goto cleanup;
tmp = disk->backingChain;
- while (tmp && tmp->backingStoreIsFile) {
+ while (tmp && virStorageIsFile(tmp->backingStore)) {
if (!ignoreOpenFailure && !tmp->backingMeta) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to visit backing chain file %s"),
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9c189dc..cd43335 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1833,6 +1833,7 @@ virStorageFileIsClusterFS;
virStorageFileProbeFormat;
virStorageFileProbeFormatFromBuf;
virStorageFileResize;
+virStorageIsFile;
virStorageNetHostDefClear;
virStorageNetHostDefCopy;
virStorageNetHostDefFree;
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index bb20385..5730e3b 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -118,7 +118,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
*backingStore = meta->backingStore;
meta->backingStore = NULL;
if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO &&
- meta->backingStoreIsFile) {
+ virStorageIsFile(*backingStore)) {
if ((ret = virStorageFileProbeFormat(*backingStore, -1, -1)) < 0) {
/* If the backing file is currently unavailable, only log an error,
* but continue. Returning -1 here would disable the whole storage
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7a91a01..cb66da4 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -688,11 +688,17 @@ virStorageFileMatchesVersion(int format,
return false;
}
-static bool
-virBackingStoreIsFile(const char *backing)
+bool
+virStorageIsFile(const char *backing)
{
- char *colon = strchr(backing, ':');
- char *slash = strchr(backing, '/');
+ char *colon;
+ char *slash;
+
+ if (!backing)
+ return false;
+
+ colon = strchr(backing, ':');
+ slash = strchr(backing, '/');
/* Reject anything that looks like a protocol (such as nbd: or
* rbd:); if someone really does want a relative file name that
@@ -866,7 +872,7 @@ virStorageFileGetMetadataInternal(const char *path,
VIR_FREE(backing);
goto cleanup;
}
- if (virBackingStoreIsFile(backing)) {
+ if (virStorageIsFile(backing)) {
meta->backingStoreIsFile = true;
meta->backingStoreRaw = meta->backingStore;
meta->backingStore = NULL;
@@ -1146,7 +1152,7 @@ virStorageFileGetMetadataRecurse(const char *path, const char
*canonPath,
if (virHashAddEntry(cycle, canonPath, (void *)1) < 0)
return -1;
- if (virBackingStoreIsFile(path)) {
+ if (virStorageIsFile(path)) {
if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) {
virReportSystemError(-fd, _("Failed to open file '%s'"),
path);
return -1;
@@ -1235,7 +1241,7 @@ virStorageFileGetMetadata(const char *path, int format,
if (!cycle)
return NULL;
- if (virBackingStoreIsFile(path)) {
+ if (virStorageIsFile(path)) {
if (!(canonPath = canonicalize_file_name(path))) {
virReportSystemError(errno, _("unable to resolve '%s'"),
path);
goto cleanup;
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 4192140..6c08c31 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -315,6 +315,7 @@ int virStorageFileResize(const char *path,
bool pre_allocate);
int virStorageFileIsClusterFS(const char *path);
+bool virStorageIsFile(const char *path);
int virStorageFileGetLVMKey(const char *path,
char **key);
--
1.9.0
5 a.m.
New subject: [libvirt] [PATCH 2/5] conf: expose probe for non-local storage
On 04/10/14 05:41, Eric Blake wrote:
Deciding if a user string represents a local file instead of a
network path is an operation worth exposing directly, particularly
since the next patch will be removing a redundant variable that
was caching the information.
* src/util/virstoragefile.h (virStorageIsFile): New declaration.
* src/util/virstoragefile.c (virBackingStoreIsFile): Rename...
(virStorageIsFile): ...export, and allow NULL input.
(virStorageFileGetMetadataInternal)
(virStorageFileGetMetadataRecurse, virStorageFileGetMetadata):
Update callers.
* src/conf/domain_conf.c (virDomainDiskDefForeachPath): Use it.
* src/storage/storage_backend_fs.c (virStorageBackendProbeTarget):
Likewise.
* src/libvirt_private.syms (virstoragefile.h): Export function.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/conf/domain_conf.c | 2 +-
src/libvirt_private.syms | 1 +
src/storage/storage_backend_fs.c | 2 +-
src/util/virstoragefile.c | 20 +++++++++++++-------
src/util/virstoragefile.h | 1 +
5 files changed, 17 insertions(+), 9 deletions(-)
ACK, although we should switch to determining whether the file is local
by inspecting other metadata than the backing string in the future.
Peter
10:41 p.m.
New subject: [libvirt] [PATCH 3/5] conf: delete useless backingStoreIsFile field
Finally starting to prune away some of the old fields that have
been made redundant by the new fields, on my way towards directly
reusing virStorageSource.
* src/util/virstoragefile.h (_virStorageFileMetadata): Drop
field.
* src/util/virstoragefile.c (virStorageFileGetMetadataInternal)
(virStorageFileChainLookup): Adjust callers.
* tests/virstoragetest.c (_testFileData, testStorageChain)
(mymain): Simplify test.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/util/virstoragefile.c | 5 +----
src/util/virstoragefile.h | 1 -
tests/virstoragetest.c | 16 +++-------------
3 files changed, 4 insertions(+), 18 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index cb66da4..fe8144a 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -866,14 +866,12 @@ virStorageFileGetMetadataInternal(const char *path,
if (store == BACKING_STORE_ERROR)
goto cleanup;
- meta->backingStoreIsFile = false;
if (backing != NULL) {
if (VIR_STRDUP(meta->backingStore, backing) < 0) {
VIR_FREE(backing);
goto cleanup;
}
if (virStorageIsFile(backing)) {
- meta->backingStoreIsFile = true;
meta->backingStoreRaw = meta->backingStore;
meta->backingStore = NULL;
if (virFindBackingFile(directory, backing,
@@ -882,7 +880,6 @@ virStorageFileGetMetadataInternal(const char *path,
/* the backing file is (currently) unavailable, treat this
* file as standalone:
* backingStoreRaw is kept to mark broken image chains */
- meta->backingStoreIsFile = false;
backingFormat = VIR_STORAGE_FILE_NONE;
VIR_WARN("Backing file '%s' of image '%s' is
missing.",
meta->backingStoreRaw, path);
@@ -1569,7 +1566,7 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const
char *start,
} else if (STREQ_NULLABLE(name, owner->backingStoreRaw) ||
STREQ(name, owner->backingStore)) {
break;
- } else if (owner->backingStoreIsFile) {
+ } else if (virStorageIsFile(owner->backingStore)) {
char *absName = NULL;
if (virFindBackingFile(owner->directory, name,
NULL, &absName) < 0)
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 6c08c31..4da160b 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -148,7 +148,6 @@ struct _virStorageFileMetadata {
char *backingStore; /* Canonical name (absolute file, or protocol). Should be same as
backingMeta->canonPath */
char *directory; /* The directory containing basename of backingStoreRaw. Should be
same as backingMeta->relDir */
int backingStoreFormat; /* enum virStorageFileFormat. Should be same as
backingMeta->format */
- bool backingStoreIsFile; /* Should be same as backingMeta->type <
VIR_STORAGE_TYPE_NETWORK */
};
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index e8409f6..846d130 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -233,7 +233,6 @@ struct _testFileData
const char *expBackingDirRel;
const char *expBackingDirAbs;
enum virStorageFileFormat expBackingFormat;
- bool expIsFile;
unsigned long long expCapacity;
bool expEncrypted;
const char *pathRel;
@@ -326,13 +325,12 @@ testStorageChain(const void *args)
expRelDir = isAbs ? data->files[i]->relDirAbs
: data->files[i]->relDirRel;
if (virAsprintf(&expect,
- "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld
%d\n"
+ "store:%s\nraw:%s\ndirectory:%s\nother:%d %lld %d\n"
"path:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n",
NULLSTR(data->files[i]->expBackingStore),
NULLSTR(data->files[i]->expBackingStoreRaw),
NULLSTR(expBackingDirectory),
data->files[i]->expBackingFormat,
- data->files[i]->expIsFile,
data->files[i]->expCapacity,
data->files[i]->expEncrypted,
NULLSTR(expPath),
@@ -341,12 +339,12 @@ testStorageChain(const void *args)
data->files[i]->type,
data->files[i]->format) < 0 ||
virAsprintf(&actual,
- "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld
%d\n"
+ "store:%s\nraw:%s\ndirectory:%s\nother:%d %lld %d\n"
"path:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n",
NULLSTR(elt->backingStore),
NULLSTR(elt->backingStoreRaw),
NULLSTR(elt->directory),
- elt->backingStoreFormat, elt->backingStoreIsFile,
+ elt->backingStoreFormat,
elt->capacity, !!elt->encryption,
NULLSTR(elt->path),
NULLSTR(elt->canonPath),
@@ -458,7 +456,6 @@ mymain(void)
.expBackingDirRel = ".",
.expBackingDirAbs = datadir,
.expBackingFormat = VIR_STORAGE_FILE_RAW,
- .expIsFile = true,
.expCapacity = 1024,
.pathRel = "qcow2",
.pathAbs = canonqcow2,
@@ -520,7 +517,6 @@ mymain(void)
.expBackingDirRel = datadir,
.expBackingDirAbs = datadir,
.expBackingFormat = VIR_STORAGE_FILE_QCOW2,
- .expIsFile = true,
.expCapacity = 1024,
.pathRel = "wrap",
.pathAbs = abswrap,
@@ -562,7 +558,6 @@ mymain(void)
.expBackingDirRel = datadir,
.expBackingDirAbs = datadir,
.expBackingFormat = VIR_STORAGE_FILE_RAW,
- .expIsFile = true,
.expCapacity = 1024,
.pathRel = "wrap",
.pathAbs = abswrap,
@@ -588,7 +583,6 @@ mymain(void)
qcow2.expBackingStore = NULL;
qcow2.expBackingStoreRaw = datadir "/bogus";
qcow2.expBackingFormat = VIR_STORAGE_FILE_NONE;
- qcow2.expIsFile = false;
qcow2.pathRel = "qcow2";
qcow2.relDirRel = ".";
@@ -647,7 +641,6 @@ mymain(void)
.expBackingDirRel = datadir,
.expBackingDirAbs = datadir,
.expBackingFormat = VIR_STORAGE_FILE_RAW,
- .expIsFile = true,
.expCapacity = 1024,
.pathRel = "qed",
.pathAbs = absqed,
@@ -714,7 +707,6 @@ mymain(void)
.expBackingDirRel = "sub/../sub/..",
.expBackingDirAbs = datadir "/sub/../sub/..",
.expBackingFormat = VIR_STORAGE_FILE_RAW,
- .expIsFile = true,
.expCapacity = 1024,
.pathRel = "../sub/link1",
.pathAbs = "../sub/link1",
@@ -730,7 +722,6 @@ mymain(void)
.expBackingDirRel = "sub/../sub",
.expBackingDirAbs = datadir "/sub/../sub",
.expBackingFormat = VIR_STORAGE_FILE_QCOW2,
- .expIsFile = true,
.expCapacity = 1024,
.pathRel = "sub/link2",
.pathAbs = abslink2,
@@ -762,7 +753,6 @@ mymain(void)
qcow2.expBackingDirRel = ".";
qcow2.expBackingDirAbs = datadir;
qcow2.expBackingFormat= VIR_STORAGE_FILE_NONE;
- qcow2.expIsFile = true;
/* Behavior of an infinite loop chain */
TEST_CHAIN(16, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2,
--
1.9.0
5 a.m.
New subject: [libvirt] [PATCH 3/5] conf: delete useless backingStoreIsFile field
On 04/10/14 05:41, Eric Blake wrote:
Finally starting to prune away some of the old fields that have
been made redundant by the new fields, on my way towards directly
reusing virStorageSource.
* src/util/virstoragefile.h (_virStorageFileMetadata): Drop
field.
* src/util/virstoragefile.c (virStorageFileGetMetadataInternal)
(virStorageFileChainLookup): Adjust callers.
* tests/virstoragetest.c (_testFileData, testStorageChain)
(mymain): Simplify test.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/util/virstoragefile.c | 5 +----
src/util/virstoragefile.h | 1 -
tests/virstoragetest.c | 16 +++-------------
3 files changed, 4 insertions(+), 18 deletions(-)
ACK,
Peter
10:41 p.m.
New subject: [libvirt] [PATCH 4/5] conf: return backing information separately from metadata
A couple pieces of virStorageFileMetadata are used only while
collecting information about the chain, and don't need to
live permanently in the struct. This patch refactors external
callers to collect the information separately, so that the
next patch can remove the fields.
* src/util/virstoragefile.h (virStorageFileGetMetadataFromBuf):
Alter signature.
* src/util/virstoragefile.c (virStorageFileGetMetadataInternal):
Likewise.
(virStorageFileGetMetadataFromFDInternal): Adjust callers.
* src/storage/storage_backend_fs.c (virStorageBackendProbeTarget):
Likewise.
* src/storage/storage_backend_gluster.c
(virStorageBackendGlusterRefreshVol): Likewise.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/storage/storage_backend_fs.c | 34 ++++++++++++++-----------------
src/storage/storage_backend_gluster.c | 14 ++++++-------
src/util/virstoragefile.c | 38 ++++++++++++++++++++---------------
src/util/virstoragefile.h | 6 +++++-
4 files changed, 48 insertions(+), 44 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 5730e3b..be963d4 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -106,7 +106,9 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
if (!(meta = virStorageFileGetMetadataFromBuf(target->path,
header, len,
- target->format))) {
+ target->format,
+ backingStore,
+ backingStoreFormat))) {
ret = -1;
goto error;
}
@@ -114,25 +116,19 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
VIR_FORCE_CLOSE(fd);
- if (meta && meta->backingStore) {
- *backingStore = meta->backingStore;
- meta->backingStore = NULL;
- if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO &&
- virStorageIsFile(*backingStore)) {
- if ((ret = virStorageFileProbeFormat(*backingStore, -1, -1)) < 0) {
- /* If the backing file is currently unavailable, only log an error,
- * but continue. Returning -1 here would disable the whole storage
- * pool, making it unavailable for even maintenance. */
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot probe backing volume format: %s"),
- *backingStore);
- ret = -3;
- } else {
- *backingStoreFormat = ret;
- ret = 0;
- }
+ if (meta && *backingStore &&
+ *backingStoreFormat == VIR_STORAGE_FILE_AUTO &&
+ virStorageIsFile(*backingStore)) {
+ if ((ret = virStorageFileProbeFormat(*backingStore, -1, -1)) < 0) {
+ /* If the backing file is currently unavailable, only log an error,
+ * but continue. Returning -1 here would disable the whole storage
+ * pool, making it unavailable for even maintenance. */
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot probe backing volume format: %s"),
+ *backingStore);
+ ret = -3;
} else {
- *backingStoreFormat = meta->backingStoreFormat;
+ *backingStoreFormat = ret;
ret = 0;
}
} else {
diff --git a/src/storage/storage_backend_gluster.c
b/src/storage/storage_backend_gluster.c
index 2593c1c..e0a25df 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -298,16 +298,14 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr
state,
len)) < 0)
goto cleanup;
if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len,
- vol->target.format)))
+ vol->target.format,
+ &vol->backingStore.path,
+ &vol->backingStore.format)))
goto cleanup;
- if (meta->backingStore) {
- vol->backingStore.path = meta->backingStore;
- meta->backingStore = NULL;
- vol->backingStore.format = meta->backingStoreFormat;
- if (vol->backingStore.format < 0)
- vol->backingStore.format = VIR_STORAGE_FILE_RAW;
- }
+ if (vol->backingStore.path &&
+ vol->backingStore.format < 0)
+ vol->backingStore.format = VIR_STORAGE_FILE_RAW;
if (meta->capacity)
vol->target.capacity = meta->capacity;
if (meta->encryption) {
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index fe8144a..c509a9a 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -790,13 +790,16 @@ qcow2GetFeatures(virBitmapPtr *features,
* information about the file and its backing store. */
static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(7)
+ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9)
virStorageFileGetMetadataInternal(const char *path,
const char *canonPath,
const char *directory,
char *buf,
size_t len,
int format,
- virStorageFileMetadataPtr meta)
+ virStorageFileMetadataPtr meta,
+ char **backingStore,
+ int *backingFormat)
{
int ret = -1;
@@ -855,10 +858,9 @@ virStorageFileGetMetadataInternal(const char *path,
}
if (fileTypeInfo[format].getBackingStore != NULL) {
- char *backing;
- int backingFormat;
+ char *backing = NULL;
int store = fileTypeInfo[format].getBackingStore(&backing,
- &backingFormat,
+ backingFormat,
buf, len);
if (store == BACKING_STORE_INVALID)
goto done;
@@ -880,23 +882,20 @@ virStorageFileGetMetadataInternal(const char *path,
/* the backing file is (currently) unavailable, treat this
* file as standalone:
* backingStoreRaw is kept to mark broken image chains */
- backingFormat = VIR_STORAGE_FILE_NONE;
+ *backingFormat = VIR_STORAGE_FILE_NONE;
VIR_WARN("Backing file '%s' of image '%s' is
missing.",
meta->backingStoreRaw, path);
}
} else {
- if (VIR_STRDUP(meta->backingStoreRaw, backing) < 0) {
- VIR_FREE(backing);
- goto cleanup;
- }
- backingFormat = VIR_STORAGE_FILE_RAW;
+ *backingStore = backing;
+ backing = NULL;
+ *backingFormat = VIR_STORAGE_FILE_RAW;
}
VIR_FREE(backing);
- meta->backingStoreFormat = backingFormat;
} else {
meta->backingStore = NULL;
- meta->backingStoreFormat = VIR_STORAGE_FILE_NONE;
+ *backingFormat = VIR_STORAGE_FILE_NONE;
}
}
@@ -980,6 +979,8 @@ virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid)
* @buf: header bytes from @path
* @len: length of @buf
* @format: expected image format
+ * @backing: output malloc'd name of backing image, if any
+ * @backingFormat: format of @backing
*
* Extract metadata about the storage volume with the specified
* image format. If image format is VIR_STORAGE_FILE_AUTO, it
@@ -989,7 +990,7 @@ virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid)
* format, since a malicious guest can turn a raw file into any
* other non-raw format at will.
*
- * If the returned meta.backingStoreFormat is VIR_STORAGE_FILE_AUTO
+ * If the returned @backingFormat is VIR_STORAGE_FILE_AUTO
* it indicates the image didn't specify an explicit format for its
* backing store. Callers are advised against probing for the
* backing store format in this case.
@@ -1000,7 +1001,9 @@ virStorageFileMetadataPtr
virStorageFileGetMetadataFromBuf(const char *path,
char *buf,
size_t len,
- int format)
+ int format,
+ char **backing,
+ int *backingFormat)
{
virStorageFileMetadataPtr ret = NULL;
char *canonPath;
@@ -1013,7 +1016,8 @@ virStorageFileGetMetadataFromBuf(const char *path,
goto cleanup;
if (virStorageFileGetMetadataInternal(path, canonPath, ".", buf, len,
- format, ret) < 0) {
+ format, ret, backing,
+ backingFormat) < 0) {
virStorageFileFreeMetadata(ret);
ret = NULL;
}
@@ -1068,7 +1072,9 @@ virStorageFileGetMetadataFromFDInternal(const char *path,
}
ret = virStorageFileGetMetadataInternal(path, canonPath, directory,
- buf, len, format, meta);
+ buf, len, format, meta,
+ &meta->backingStoreRaw,
+ &meta->backingStoreFormat);
if (ret == 0) {
if (S_ISREG(sb.st_mode))
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 4da160b..edfe9eb 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -295,7 +295,11 @@ virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char
*path,
virStorageFileMetadataPtr virStorageFileGetMetadataFromBuf(const char *path,
char *buf,
size_t len,
- int format);
+ int format,
+ char **backing,
+ int *backingFormat)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)
+ ATTRIBUTE_NONNULL(6);
int virStorageFileChainGetBroken(virStorageFileMetadataPtr chain,
char **broken_file);
--
1.9.0
5:30 a.m.
New subject: [libvirt] [PATCH 4/5] conf: return backing information separately from metadata
On 04/10/14 05:41, Eric Blake wrote:
A couple pieces of virStorageFileMetadata are used only while
collecting information about the chain, and don't need to
live permanently in the struct. This patch refactors external
callers to collect the information separately, so that the
next patch can remove the fields.
* src/util/virstoragefile.h (virStorageFileGetMetadataFromBuf):
Alter signature.
* src/util/virstoragefile.c (virStorageFileGetMetadataInternal):
Likewise.
(virStorageFileGetMetadataFromFDInternal): Adjust callers.
* src/storage/storage_backend_fs.c (virStorageBackendProbeTarget):
Likewise.
* src/storage/storage_backend_gluster.c
(virStorageBackendGlusterRefreshVol): Likewise.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/storage/storage_backend_fs.c | 34 ++++++++++++++-----------------
src/storage/storage_backend_gluster.c | 14 ++++++-------
src/util/virstoragefile.c | 38 ++++++++++++++++++++---------------
src/util/virstoragefile.h | 6 +++++-
4 files changed, 48 insertions(+), 44 deletions(-)
ACK,
Peter
10:41 p.m.
New subject: [libvirt] [PATCH 5/5] conf: delete useless backingStoreFormat field
Drop another redundant field from virStorageFileMetadata.
* src/util/virstoragefile.h (_virStorageFileMetadata): Drop
field.
* src/util/virstoragefile.c
(virStorageFileGetMetadataFromFDInternal)
(virStorageFileGetMetadataFromFD)
(virStorageFileGetMetadataRecurse): Adjust callers.
* tests/virstoragetest.c (_testFileData, testStorageChain)
(mymain): Simplify test.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/util/virstoragefile.c | 39 ++++++++++++++++++---------------------
src/util/virstoragefile.h | 1 -
tests/virstoragetest.c | 22 ++--------------------
3 files changed, 20 insertions(+), 42 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index c509a9a..3645abb 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1035,13 +1035,19 @@ virStorageFileGetMetadataFromFDInternal(const char *path,
const char *directory,
int fd,
int format,
- virStorageFileMetadataPtr meta)
+ virStorageFileMetadataPtr meta,
+ int *backingFormat)
{
char *buf = NULL;
ssize_t len = VIR_STORAGE_MAX_HEADER;
struct stat sb;
int ret = -1;
+ int dummy;
+ if (backingFormat)
+ *backingFormat = VIR_STORAGE_FILE_NONE;
+ else
+ backingFormat = &dummy;
if (fstat(fd, &sb) < 0) {
virReportSystemError(errno,
_("cannot stat file '%s'"),
@@ -1074,7 +1080,7 @@ virStorageFileGetMetadataFromFDInternal(const char *path,
ret = virStorageFileGetMetadataInternal(path, canonPath, directory,
buf, len, format, meta,
&meta->backingStoreRaw,
- &meta->backingStoreFormat);
+ backingFormat);
if (ret == 0) {
if (S_ISREG(sb.st_mode))
@@ -1099,11 +1105,6 @@ virStorageFileGetMetadataFromFDInternal(const char *path,
* format, since a malicious guest can turn a raw file into any
* other non-raw format at will.
*
- * If the returned meta.backingStoreFormat is VIR_STORAGE_FILE_AUTO
- * it indicates the image didn't specify an explicit format for its
- * backing store. Callers are advised against probing for the
- * backing store format in this case.
- *
* Caller MUST free the result after use via virStorageFileFreeMetadata.
*/
virStorageFileMetadataPtr
@@ -1121,7 +1122,7 @@ virStorageFileGetMetadataFromFD(const char *path,
if (VIR_ALLOC(ret) < 0)
goto cleanup;
if (virStorageFileGetMetadataFromFDInternal(path, canonPath, ".",
- fd, format, ret) < 0) {
+ fd, format, ret, NULL) < 0) {
virStorageFileFreeMetadata(ret);
ret = NULL;
}
@@ -1142,6 +1143,8 @@ virStorageFileGetMetadataRecurse(const char *path, const char
*canonPath,
{
int fd;
int ret = -1;
+ int backingFormat;
+
VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d",
path, canonPath, NULLSTR(directory), format,
(int)uid, (int)gid, allow_probe);
@@ -1163,7 +1166,8 @@ virStorageFileGetMetadataRecurse(const char *path, const char
*canonPath,
ret = virStorageFileGetMetadataFromFDInternal(path, canonPath,
directory,
- fd, format, meta);
+ fd, format, meta,
+ &backingFormat);
if (VIR_CLOSE(fd) < 0)
VIR_WARN("could not close file %s", path);
@@ -1183,19 +1187,17 @@ virStorageFileGetMetadataRecurse(const char *path, const char
*canonPath,
if (ret == 0 && meta->backingStore) {
virStorageFileMetadataPtr backing;
- if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO &&
!allow_probe)
- meta->backingStoreFormat = VIR_STORAGE_FILE_RAW;
- else if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE)
- meta->backingStoreFormat = VIR_STORAGE_FILE_AUTO;
- format = meta->backingStoreFormat;
+ 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,
meta->backingStore,
- meta->directory, format,
+ meta->directory, backingFormat,
uid, gid, allow_probe,
cycle, backing) < 0) {
/* If we failed to get backing data, mark the chain broken */
- meta->backingStoreFormat = VIR_STORAGE_FILE_NONE;
VIR_FREE(meta->backingStore);
virStorageFileFreeMetadata(backing);
} else {
@@ -1220,11 +1222,6 @@ virStorageFileGetMetadataRecurse(const char *path, const char
*canonPath,
* format, since a malicious guest can turn a raw file into any
* other non-raw format at will.
*
- * If the returned meta.backingStoreFormat is VIR_STORAGE_FILE_AUTO
- * it indicates the image didn't specify an explicit format for its
- * backing store. Callers are advised against using ALLOW_PROBE, as
- * it would probe the backing store format in this case.
- *
* Caller MUST free result after use via virStorageFileFreeMetadata.
*/
virStorageFileMetadataPtr
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index edfe9eb..c747f20 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -147,7 +147,6 @@ struct _virStorageFileMetadata {
* store. */
char *backingStore; /* Canonical name (absolute file, or protocol). Should be same as
backingMeta->canonPath */
char *directory; /* The directory containing basename of backingStoreRaw. Should be
same as backingMeta->relDir */
- int backingStoreFormat; /* enum virStorageFileFormat. Should be same as
backingMeta->format */
};
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 846d130..13b5032 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -232,7 +232,6 @@ struct _testFileData
const char *expBackingStoreRaw;
const char *expBackingDirRel;
const char *expBackingDirAbs;
- enum virStorageFileFormat expBackingFormat;
unsigned long long expCapacity;
bool expEncrypted;
const char *pathRel;
@@ -325,12 +324,11 @@ testStorageChain(const void *args)
expRelDir = isAbs ? data->files[i]->relDirAbs
: data->files[i]->relDirRel;
if (virAsprintf(&expect,
- "store:%s\nraw:%s\ndirectory:%s\nother:%d %lld %d\n"
+ "store:%s\nraw:%s\ndirectory:%s\nother:%lld %d\n"
"path:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n",
NULLSTR(data->files[i]->expBackingStore),
NULLSTR(data->files[i]->expBackingStoreRaw),
NULLSTR(expBackingDirectory),
- data->files[i]->expBackingFormat,
data->files[i]->expCapacity,
data->files[i]->expEncrypted,
NULLSTR(expPath),
@@ -339,12 +337,11 @@ testStorageChain(const void *args)
data->files[i]->type,
data->files[i]->format) < 0 ||
virAsprintf(&actual,
- "store:%s\nraw:%s\ndirectory:%s\nother:%d %lld %d\n"
+ "store:%s\nraw:%s\ndirectory:%s\nother:%lld %d\n"
"path:%s\ncanon:%s\nrelDir:%s\ntype:%d %d\n",
NULLSTR(elt->backingStore),
NULLSTR(elt->backingStoreRaw),
NULLSTR(elt->directory),
- elt->backingStoreFormat,
elt->capacity, !!elt->encryption,
NULLSTR(elt->path),
NULLSTR(elt->canonPath),
@@ -428,7 +425,6 @@ mymain(void)
/* Raw image, whether with right format or no specified format */
testFileData raw = {
- .expBackingFormat = VIR_STORAGE_FILE_NONE,
.pathRel = "raw",
.pathAbs = canonraw,
.canonPath = canonraw,
@@ -455,7 +451,6 @@ mymain(void)
.expBackingStoreRaw = "raw",
.expBackingDirRel = ".",
.expBackingDirAbs = datadir,
- .expBackingFormat = VIR_STORAGE_FILE_RAW,
.expCapacity = 1024,
.pathRel = "qcow2",
.pathAbs = canonqcow2,
@@ -466,7 +461,6 @@ mymain(void)
.format = VIR_STORAGE_FILE_QCOW2,
};
testFileData qcow2_as_raw = {
- .expBackingFormat = VIR_STORAGE_FILE_NONE,
.pathRel = "qcow2",
.pathAbs = canonqcow2,
.canonPath = canonqcow2,
@@ -516,7 +510,6 @@ mymain(void)
.expBackingStoreRaw = absqcow2,
.expBackingDirRel = datadir,
.expBackingDirAbs = datadir,
- .expBackingFormat = VIR_STORAGE_FILE_QCOW2,
.expCapacity = 1024,
.pathRel = "wrap",
.pathAbs = abswrap,
@@ -546,8 +539,6 @@ mymain(void)
"-b", absqcow2, "wrap", NULL);
if (virCommandRun(cmd, NULL) < 0)
ret = -1;
- wrap.expBackingFormat = VIR_STORAGE_FILE_AUTO;
- qcow2.expBackingFormat = VIR_STORAGE_FILE_AUTO;
qcow2_as_raw.pathRel = absqcow2;
qcow2_as_raw.relDirRel = datadir;
@@ -557,7 +548,6 @@ mymain(void)
.expBackingStoreRaw = absqcow2,
.expBackingDirRel = datadir,
.expBackingDirAbs = datadir,
- .expBackingFormat = VIR_STORAGE_FILE_RAW,
.expCapacity = 1024,
.pathRel = "wrap",
.pathAbs = abswrap,
@@ -582,7 +572,6 @@ mymain(void)
ret = -1;
qcow2.expBackingStore = NULL;
qcow2.expBackingStoreRaw = datadir "/bogus";
- qcow2.expBackingFormat = VIR_STORAGE_FILE_NONE;
qcow2.pathRel = "qcow2";
qcow2.relDirRel = ".";
@@ -618,7 +607,6 @@ mymain(void)
qcow2.expBackingStoreRaw = "nbd:example.org:6000";
qcow2.expBackingDirRel = NULL;
qcow2.expBackingDirAbs = NULL;
- qcow2.expBackingFormat = VIR_STORAGE_FILE_RAW;
/* Qcow2 file with backing protocol instead of file */
testFileData nbd = {
@@ -640,7 +628,6 @@ mymain(void)
.expBackingStoreRaw = absraw,
.expBackingDirRel = datadir,
.expBackingDirAbs = datadir,
- .expBackingFormat = VIR_STORAGE_FILE_RAW,
.expCapacity = 1024,
.pathRel = "qed",
.pathAbs = absqed,
@@ -651,7 +638,6 @@ mymain(void)
.format = VIR_STORAGE_FILE_QED,
};
testFileData qed_as_raw = {
- .expBackingFormat = VIR_STORAGE_FILE_NONE,
.pathRel = "qed",
.pathAbs = absqed,
.canonPath = canonqed,
@@ -706,7 +692,6 @@ mymain(void)
.expBackingStoreRaw = "../raw",
.expBackingDirRel = "sub/../sub/..",
.expBackingDirAbs = datadir "/sub/../sub/..",
- .expBackingFormat = VIR_STORAGE_FILE_RAW,
.expCapacity = 1024,
.pathRel = "../sub/link1",
.pathAbs = "../sub/link1",
@@ -721,7 +706,6 @@ mymain(void)
.expBackingStoreRaw = "../sub/link1",
.expBackingDirRel = "sub/../sub",
.expBackingDirAbs = datadir "/sub/../sub",
- .expBackingFormat = VIR_STORAGE_FILE_QCOW2,
.expCapacity = 1024,
.pathRel = "sub/link2",
.pathAbs = abslink2,
@@ -752,7 +736,6 @@ mymain(void)
qcow2.expBackingStoreRaw = "qcow2";
qcow2.expBackingDirRel = ".";
qcow2.expBackingDirAbs = datadir;
- qcow2.expBackingFormat= VIR_STORAGE_FILE_NONE;
/* Behavior of an infinite loop chain */
TEST_CHAIN(16, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2,
@@ -777,7 +760,6 @@ mymain(void)
qcow2.expBackingDirRel = datadir;
qcow2.pathRel = absqcow2;
qcow2.relDirRel = datadir;
- wrap.expBackingFormat = VIR_STORAGE_FILE_QCOW2;
/* Behavior of an infinite loop chain */
TEST_CHAIN(17, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2,
--
1.9.0
6:51 a.m.
New subject: [libvirt] [PATCH 5/5] conf: delete useless backingStoreFormat field
On 04/10/14 05:41, Eric Blake wrote:
Drop another redundant field from virStorageFileMetadata.
* src/util/virstoragefile.h (_virStorageFileMetadata): Drop
field.
* src/util/virstoragefile.c
(virStorageFileGetMetadataFromFDInternal)
(virStorageFileGetMetadataFromFD)
(virStorageFileGetMetadataRecurse): Adjust callers.
* tests/virstoragetest.c (_testFileData, testStorageChain)
(mymain): Simplify test.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/util/virstoragefile.c | 39 ++++++++++++++++++---------------------
src/util/virstoragefile.h | 1 -
tests/virstoragetest.c | 22 ++--------------------
3 files changed, 20 insertions(+), 42 deletions(-)
(Re) ACK, the previous attempt didn't go through apparently.
Peter
11:55 p.m.
On 04/09/2014 09:41 PM, Eric Blake wrote:
I'm almost to the point where virStorageSource can track
everything
that virStorageFileMetadata was used for, so that we can use one
struct instead of two. But while working on this today, I noticed
that virStorageFileChainLookup() does not have any unit tests, so
I'll be writing those before removing the two remaining redundant
fields.
It will look something like this (although I still need more test cases
before turning it into a formal commit):
diff --git i/tests/virstoragetest.c w/tests/virstoragetest.c
index 13b5032..225d18d 100644
--- i/tests/virstoragetest.c
+++ w/tests/virstoragetest.c
@@ -375,12 +375,69 @@ testStorageChain(const void *args)
return ret;
}
+struct testLookupData
+{
+ virStorageFileMetadataPtr chain;
+ const char *start;
+ const char *name;
+ const char *expResult;
+ virStorageFileMetadataPtr expMeta;
+ const char *expParent;
+};
+
+static int
+testStorageLookup(const void *args)
+{
+ const struct testLookupData *data = args;
+ int ret = 0;
+ const char *actualResult;
+ virStorageFileMetadataPtr actualMeta;
+ const char *actualParent;
+
+ /* This function is documented as giving results within chain,
+ * which means we can use pointer equality instead of STREQ. Test
+ * twice to ensure optional parameters don't cause NULL deref. */
+ actualResult = virStorageFileChainLookup(data->chain, data->start,
+ data->name, NULL, NULL);
+ if (data->expResult != actualResult) {
+ fprintf(stderr, "result 1: expected %s(%p), got %s(%p)\n",
+ NULLSTR(data->expResult), data->expResult,
+ NULLSTR(actualResult), actualResult);
+ ret = -1;
+ }
+
+ actualResult = virStorageFileChainLookup(data->chain, data->start,
+ data->name, &actualMeta,
+ &actualParent);
+ if (data->expResult != actualResult) {
+ fprintf(stderr, "result 2: expected %s(%p), got %s(%p)\n",
+ NULLSTR(data->expResult), data->expResult,
+ NULLSTR(actualResult), actualResult);
+ ret = -1;
+ }
+ if (data->expMeta != actualMeta) {
+ fprintf(stderr, "meta: expected %p, got %p\n",
+ data->expMeta, actualMeta);
+ ret = -1;
+ }
+ if (data->expParent != actualParent) {
+ fprintf(stderr, "parent: expected %s(%p), got %s(%p)\n",
+ NULLSTR(data->expParent), data->expParent,
+ NULLSTR(actualParent), actualParent);
+ ret = -1;
+ }
+
+ return ret;
+}
+
static int
mymain(void)
{
int ret;
virCommandPtr cmd = NULL;
struct testChainData data;
+ virStorageFileMetadataPtr chain = NULL;
+ const char *start = NULL;
/* Prep some files with qemu-img; if that is not found on PATH, or
* if it lacks support for qcow2 and qed, skip this test. */
@@ -768,6 +825,26 @@ mymain(void)
(&wrap, &qcow2), EXP_WARN,
(&wrap, &qcow2), ALLOW_PROBE | EXP_WARN);
+ /* Test behavior of chain lookups */
+ start = "wrap";
+ chain = virStorageFileGetMetadata("wrap", VIR_STORAGE_FILE_QCOW2,
+ -1, -1, false);
+ if (!chain)
+ return EXIT_FAILURE;
+
+#define TEST_LOOKUP(id, name, result, meta, parent) \
+ do { \
+ struct testLookupData data2 = { chain, start, name, \
+ result, meta, parent, }; \
+ if (virtTestRun("Chain lookup " #id, \
+ testStorageLookup, &data2) < 0) \
+ ret = -1; \
+ } while (0)
+
+ TEST_LOOKUP(1, "wrap", start, chain, NULL);
+ TEST_LOOKUP(2, abswrap, start, chain, NULL);
+ TEST_LOOKUP(3, "qcow2", chain->backingStore, chain->backingMeta,
start);
+
/* Final cleanup */
testCleanupImages();
virCommandFree(cmd);
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3881
days inactive
3881
days old
12 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa