On 04/11/2014 12:21 AM, Eric Blake wrote:
Thanks to the testsuite, I feel quite confident that this rewrite
still gives the same results for all cases except for one, and
I can make the argument that _that_ case was a pre-existing bug.
When looking up relative names, the lookup is supposed to be
pegged to the directory that contains the parent qcow2 file.
* src/util/virstoragefile.c (virStorageFileChainLookup): Depend on
new rather than old fields.
* tests/virstoragetest.c (mymain): Adjust test to match fix.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/util/virstoragefile.c | 49 +++++++++++++++++++----------------------------
tests/virstoragetest.c | 3 +--
2 files changed, 21 insertions(+), 31 deletions(-)
ACK
Although I'll admit I don't know enough about all the backing chain code
to fully understand the "pre-existing" bug comment. I suppose the only
odd part I found was the comparison < VIR_STORAGE_TYPE_DIR - leaving
currently DIR, NETWORK, and VOLUME out of the comparison. My thoughts
went to what if something new comes along and what on earth was being
compared beforehand...
John
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index dcde9e5..e33f6ef 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1544,48 +1544,39 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain,
const char **parent)
{
const char *start = chain->canonPath;
- virStorageFileMetadataPtr owner;
const char *tmp;
+ const char *parentDir = ".";
+ bool nameIsFile = virStorageIsFile(name);
if (!parent)
parent = &tmp;
*parent = NULL;
- if (name ? STREQ(start, name) || virFileLinkPointsTo(start, name) :
- !chain->backingStore) {
- if (meta)
- *meta = chain;
- return start;
- }
-
- owner = chain;
- *parent = start;
- while (owner) {
- if (!owner->backingStore)
- goto error;
+ while (chain) {
if (!name) {
- if (!owner->backingMeta ||
- !owner->backingMeta->backingStore)
+ if (!chain->backingMeta)
break;
- } else if (STREQ_NULLABLE(name, owner->backingStoreRaw) ||
- STREQ(name, owner->backingStore)) {
- break;
- } else if (virStorageIsFile(owner->backingStore)) {
- int result = virFileRelLinkPointsTo(owner->directory, name,
- owner->backingStore);
- if (result < 0)
- goto error;
- if (result > 0)
+ } else {
+ if (STREQ(name, chain->path))
break;
+ if (nameIsFile && chain->type < VIR_STORAGE_TYPE_DIR) {
+ int result = virFileRelLinkPointsTo(parentDir, name,
+ chain->canonPath);
+ if (result < 0)
+ goto error;
+ if (result > 0)
+ break;
+ }
}
- *parent = owner->backingStore;
- owner = owner->backingMeta;
+ *parent = chain->canonPath;
+ parentDir = chain->relDir;
+ chain = chain->backingMeta;
}
- if (!owner)
+ if (!chain)
goto error;
if (meta)
- *meta = owner->backingMeta;
- return owner->backingStore;
+ *meta = chain;
+ return chain->canonPath;
error:
virReportError(VIR_ERR_INVALID_ARG,
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 73bcb0b..37f41bd 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -927,8 +927,7 @@ mymain(void)
TEST_LOOKUP(19, abswrap, chain->canonPath, chain, NULL);
TEST_LOOKUP(20, "../qcow2", chain->backingStore,
chain->backingMeta,
chain->canonPath);
- TEST_LOOKUP(21, "qcow2", chain->backingStore, chain->backingMeta,
- chain->canonPath);
+ TEST_LOOKUP(21, "qcow2", NULL, NULL, NULL);
TEST_LOOKUP(22, absqcow2, chain->backingStore, chain->backingMeta,
chain->canonPath);
TEST_LOOKUP(23, "raw", chain->backingMeta->backingStore,