[libvirt] [PATCH 5/6] conf: tweak chain lookup internals

[John Ferlan]

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 at 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,
>