[libvirt] [PATCHv2 11/16] storage: make it easier to find file within chain

[Laine Stump]

On 10/13/2012 06:00 PM, Eric Blake wrote:
> In order to temporarily label files read/write during a commit
> operation, we need to crawl the backing chain and find the absolute
> file name that needs labeling in the first place, as well as the
> name of the file that owns the backing file.
>
> * src/util/storage_file.c (virStorageFileChainLookup): New
> function.
> * src/util/storage_file.h: Declare it.
> * src/libvirt_private.syms (storage_file.h): Export it.
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/storage_file.c  | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/storage_file.h  |  7 ++++++
>  3 files changed, 71 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index fe31bbe..e40d630 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1125,6 +1125,7 @@ virStorageGenerateQcowPassphrase;
>
>
>  # storage_file.h
> +virStorageFileChainLookup;
>  virStorageFileFormatTypeFromString;
>  virStorageFileFormatTypeToString;
>  virStorageFileFreeMetadata;
> diff --git a/src/util/storage_file.c b/src/util/storage_file.c
> index 6f299c0..218891e 100644
> --- a/src/util/storage_file.c
> +++ b/src/util/storage_file.c
> @@ -1260,3 +1260,66 @@ const char *virStorageFileGetSCSIKey(const char *path)
>      return NULL;
>  }
>  #endif
> +
> +/* Given a CHAIN that starts at the named file START, return the
> + * canonical name for the backing file NAME within that chain, or pass
> + * NULL to find the base of the chain.  If *META is not NULL, set it
> + * to the point in the chain that describes NAME (or to NULL if the
> + * backing element is not a file).  If *PARENT is not NULL, set it to
> + * the canonical name of the parent (or to NULL if NAME matches
> + * START).  The results point within CHAIN, and must not be
> + * independently freed.  */
> +const char *
> +virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start,
> +                          const char *name, virStorageFileMetadataPtr *meta,
> +                          const char **parent)
> +{
> +    virStorageFileMetadataPtr owner;
> +    const char *tmp;
> +
> +    if (!parent)
> +        parent = &tmp;
> +
> +    *parent = NULL;
> +    if (name ? STREQ(start, name) || virFileLinkPointsTo(start, name) :
> +        !chain->backingStore) {

Hmm. First time I've ever seen ?: used inside an if statement (since for
me the entire purpose of ?: is to eliminate the need for an if). Maybe
this would be more quickly parseable as (the admittedly longer):

     if ((name & (STREQ(start, name) || virFileLinkPointsTo(start,
name))) ||
         (!name && !chain->backingStore)) {

Not mandatory though.

> +        if (meta)
> +            *meta = chain;
> +        return start;

Don't you need to make sure start is an absolute path?


> +    }
> +
> +    owner = chain;
> +    *parent = start;
> +    while (owner) {
> +        if (!owner->backingStore)
> +            goto error;
> +        if (!name) {
> +            if (!owner->backingMeta ||
> +                !owner->backingMeta->backingStore)
> +                break;
> +        } else if (STREQ_NULLABLE(name, owner->backingStoreRaw) ||
> +                   STREQ(name, owner->backingStore)) {
> +            break;
> +        } else if (owner->backingStoreIsFile) {
> +            char *abs = absolutePathFromBaseFile(*parent, name);
> +            if (abs && STREQ(abs, owner->backingStore)) {
> +                VIR_FREE(abs);
> +                break;
> +            }
> +            VIR_FREE(abs);
> +        }
> +        *parent = owner->backingStore;
> +        owner = owner->backingMeta;
> +    }
> +    if (!owner)
> +        goto error;
> +    if (meta)
> +        *meta = owner->backingMeta;
> +    return owner->backingStore;

and I'm recalling from the previous patch that owner->backingStore is
already stored in its canonical form, right?

> +
> +error:
> +    *parent = NULL;
> +    if (meta)
> +        *meta = NULL;
> +    return NULL;
> +}
> diff --git a/src/util/storage_file.h b/src/util/storage_file.h
> index 685fcb8..9b00dba 100644
> --- a/src/util/storage_file.h
> +++ b/src/util/storage_file.h
> @@ -78,6 +78,13 @@ virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path,
>                                                            int fd,
>                                                            int format);
>
> +const char *virStorageFileChainLookup(virStorageFileMetadataPtr chain,
> +                                      const char *start,
> +                                      const char *name,
> +                                      virStorageFileMetadataPtr *meta,
> +                                      const char **parent)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +
>  void virStorageFileFreeMetadata(virStorageFileMetadataPtr meta);
>
>  int virStorageFileResize(const char *path, unsigned long long capacity);

ACK as-is if start doesn't need to be canonicalized (i.e. if I didn't
understand :-), otherwise ACK with that change.