[libvirt] [PATCH 4/5] conf: track more fields in backing chain metadata

[Peter Krempa]

On 04/04/14 06:32, Eric Blake wrote:
> The current use of virStorageFileMetadata is awkward; to learn
> some of the information about a child node, you have to read
> fields in the parent node.  This does not lend itself well to
> modifying backing chains (whether inserting a new node in the
> chain, or consolidating existing nodes); better would be to
> learn about a child node directly in that node.  This patch
> sets up some new fields which contain redundant information,
> although not necessarily in the final desired state for the
> new fields (see the next patch for actual tests of what is there
> now).  Then later patches will do any refactoring necessary to
> get the fields to their desired states, and update clients to
> get the information from the new fields, so we can finally
> delete the fields that are tracking information about the wrong
> node.
> 
> * src/util/virstoragefile.h (_virStorageFileMetadata): Add
> path, canonName, relDir, type, and format fields.  Reorder
> existing fields, and add lots of comments.
> * src/util/virstoragefile.c (virStorageFileFreeMetadata): Clean
> new fields.
> (virStorageFileGetMetadataInternal)
> (virStorageFileGetMetadataFromFDInternal): Start populating new
> fields.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/util/virstoragefile.c | 30 ++++++++++++++++++++++++++++--
>  src/util/virstoragefile.h | 35 ++++++++++++++++++++++++++++++-----
>  2 files changed, 58 insertions(+), 7 deletions(-)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 0e1461d..d741fb7 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c

...

>      return ret;
> @@ -1205,6 +1227,10 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta)
>      if (!meta)
>          return;
> 
> +    VIR_FREE(meta->path);
> +    VIR_FREE(meta->canonName);
> +    VIR_FREE(meta->relDir);
> +
>      virStorageFileFreeMetadata(meta->backingMeta);
>      VIR_FREE(meta->backingStore);
>      VIR_FREE(meta->backingStoreRaw);
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 3807285..a284e37 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -112,17 +112,42 @@ struct _virStorageTimestamps {
>  typedef struct _virStorageFileMetadata virStorageFileMetadata;
>  typedef virStorageFileMetadata *virStorageFileMetadataPtr;
>  struct _virStorageFileMetadata {
> -    char *backingStore; /* Canonical name (absolute file, or protocol) */
> -    char *backingStoreRaw; /* If file, original name, possibly relative */
> -    char *directory; /* The directory containing basename of backingStoreRaw */
> -    int backingStoreFormat; /* enum virStorageFileFormat */
> -    bool backingStoreIsFile;
> +    /* Name of this file as spelled by the user (top level) or
> +     * metadata of the parent (if this is a backing store).  */
> +    char *path;
> +    /* Canonical name of this file, used to detect loops in the
> +     * backing store chain.  */
> +    char *canonName;
> +    /* Directory to start from if backingStoreRaw is a relative file
> +     * name */
> +    char *relDir;
> +    /* Name of the backing store recorded in metadata of the parent */
> +    char *backingStoreRaw;

Hmm, this field seems pretty redundant to me, IIUC it's the same data as
in "path".

OTOH, the "path" field should contain the canonical path as the target
is to convert it to the new storage file struct. In that case the
"canonName" will be redundant and backingStoreRaw needs to be kept to
track the original name.

In any case ... one of them seems to be duplicate.

> +
> +    /* Backing chain.  backingMeta->origName should match
> +     * backingStoreRaw; but the fields are duplicated in the common
> +     * case to make it possible to detect missing backing files, as
> +     * follows: if backingStoreRaw is NULL, this should be NULL.  If
> +     * this is NULL and backingStoreRaw is non-NULL, there was an
> +     * error following the chain (such as missing file).  Otherwise,
> +     * information about the backing store is here.  */
>      virStorageFileMetadataPtr backingMeta;
> 
> +    /* Details about the current image */
> +    int type; /* enum virStorageType */
> +    int format; /* enum virStorageFileFormat */
>      virStorageEncryptionPtr encryption;
>      unsigned long long capacity;
>      virBitmapPtr features; /* bits described by enum virStorageFileFeature */
>      char *compat;
> +
> +    /* Fields I'm trying to delete, because it is confusing to have to
> +     * query the parent metadata for details about the backing
> +     * store.  */
> +    char *backingStore; /* Canonical name (absolute file, or protocol). Should be same as backingMeta->canon */
> +    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 */
>  };
> 
> 

Peter


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140404/6525209d/attachment-0001.sig>