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@xxxxxxxxxx>
> ---
> 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,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list