[libvirt] Re: [PATCH 3/6] Introduce virStorageFileMetadata structure

[Daniel P. Berrange]

On Tue, Sep 29, 2009 at 09:56:46AM +0100, Mark McLoughlin wrote:
> Introduce a metadata structure and make virStorageGetMetadataFromFD()
> fill it in.
> 
> * src/util/storage_file.h: add virStorageFileMetadata
> 
> * src/backend/storage_backend_fs.c: virStorageGetMetadataFromFD() now
>   fills in the virStorageFileMetadata structure
> ---
>  src/storage/storage_backend_fs.c |   65 ++++++++++++++++++-------------------
>  src/util/storage_file.h          |    8 +++++
>  2 files changed, 40 insertions(+), 33 deletions(-)
> 
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index 87c30cd..7db73bf 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -287,24 +287,13 @@ static int
>  virStorageGetMetadataFromFD(virConnectPtr conn,
>                              const char *path,
>                              int fd,
> -                            int *format,
> -                            bool *encrypted,
> -                            char **backingStore,
> -                            unsigned long long *capacity)
> +                            virStorageFileMetadata *meta)
>  {
>      unsigned char head[20*512]; /* vmdk4GetBackingStore needs this much. */
>      int len, i;
>  
> -    if (format) /* If all else fails, call it a raw file */
> -        *format = VIR_STORAGE_FILE_RAW;
> -    if (encrypted)
> -        *encrypted = false;
> -    if (backingStore)
> -        *backingStore = NULL;
> -    /* Do not overwrite capacity
> -     * if (capacity)
> -     *     *capacity = 0;
> -     */
> +    /* If all else fails, call it a raw file */
> +    meta->format = VIR_STORAGE_FILE_RAW;
>  
>      if ((len = read(fd, head, sizeof(head))) < 0) {
>          virReportSystemError(conn, errno, _("cannot read header '%s'"), path);
> @@ -345,9 +334,9 @@ virStorageGetMetadataFromFD(virConnectPtr conn,
>          }
>  
>          /* Optionally extract capacity from file */
> -        if (fileTypeInfo[i].sizeOffset != -1 && capacity) {
> +        if (fileTypeInfo[i].sizeOffset != -1) {
>              if (fileTypeInfo[i].endian == LV_LITTLE_ENDIAN) {
> -                *capacity =
> +                meta->capacity =
>                      ((unsigned long long)head[fileTypeInfo[i].sizeOffset+7] << 56) |
>                      ((unsigned long long)head[fileTypeInfo[i].sizeOffset+6] << 48) |
>                      ((unsigned long long)head[fileTypeInfo[i].sizeOffset+5] << 40) |
> @@ -357,7 +346,7 @@ virStorageGetMetadataFromFD(virConnectPtr conn,
>                      ((unsigned long long)head[fileTypeInfo[i].sizeOffset+1] << 8) |
>                      ((unsigned long long)head[fileTypeInfo[i].sizeOffset]);
>              } else {
> -                *capacity =
> +                meta->capacity =
>                      ((unsigned long long)head[fileTypeInfo[i].sizeOffset] << 56) |
>                      ((unsigned long long)head[fileTypeInfo[i].sizeOffset+1] << 48) |
>                      ((unsigned long long)head[fileTypeInfo[i].sizeOffset+2] << 40) |
> @@ -368,25 +357,24 @@ virStorageGetMetadataFromFD(virConnectPtr conn,
>                      ((unsigned long long)head[fileTypeInfo[i].sizeOffset+7]);
>              }
>              /* Avoid unlikely, but theoretically possible overflow */
> -            if (*capacity > (ULLONG_MAX / fileTypeInfo[i].sizeMultiplier))
> +            if (meta->capacity > (ULLONG_MAX / fileTypeInfo[i].sizeMultiplier))
>                  continue;
> -            *capacity *= fileTypeInfo[i].sizeMultiplier;
> +            meta->capacity *= fileTypeInfo[i].sizeMultiplier;
>          }
>  
> -        if (fileTypeInfo[i].qcowCryptOffset != -1 && encrypted) {
> +        if (fileTypeInfo[i].qcowCryptOffset != -1) {
>              int crypt_format;
>  
>              crypt_format = (head[fileTypeInfo[i].qcowCryptOffset] << 24) |
>                  (head[fileTypeInfo[i].qcowCryptOffset+1] << 16) |
>                  (head[fileTypeInfo[i].qcowCryptOffset+2] << 8) |
>                  head[fileTypeInfo[i].qcowCryptOffset+3];
> -            *encrypted = crypt_format != 0;
> +            meta->encrypted = crypt_format != 0;
>          }
>  
>          /* Validation passed, we know the file format now */
> -        if (format)
> -            *format = fileTypeInfo[i].type;
> -        if (fileTypeInfo[i].getBackingStore != NULL && backingStore) {
> +        meta->format = fileTypeInfo[i].type;
> +        if (fileTypeInfo[i].getBackingStore != NULL) {
>              char *base;
>  
>              switch (fileTypeInfo[i].getBackingStore(conn, &base, head, len)) {
> @@ -400,9 +388,9 @@ virStorageGetMetadataFromFD(virConnectPtr conn,
>                  return -1;
>              }
>              if (base != NULL) {
> -                *backingStore = absolutePathFromBaseFile(path, base);
> +                meta->backingStore = absolutePathFromBaseFile(path, base);
>                  VIR_FREE(base);
> -                if (*backingStore == NULL) {
> +                if (meta->backingStore == NULL) {
>                      virReportOOMError(conn);
>                      return -1;
>                  }
> @@ -419,8 +407,7 @@ virStorageGetMetadataFromFD(virConnectPtr conn,
>          if (!virFileHasSuffix(path, fileTypeInfo[i].extension))
>              continue;
>  
> -        if (format)
> -            *format = fileTypeInfo[i].type;
> +        meta->format = fileTypeInfo[i].type;
>          return 0;
>      }
>  
> @@ -436,7 +423,7 @@ virStorageBackendProbeTarget(virConnectPtr conn,
>                               virStorageEncryptionPtr *encryption)
>  {
>      int fd, ret;
> -    bool encrypted;
> +    virStorageFileMetadata meta;
>  
>      if (encryption)
>          *encryption = NULL;
> @@ -455,16 +442,28 @@ virStorageBackendProbeTarget(virConnectPtr conn,
>          return ret; /* Take care to propagate ret, it is not always -1 */
>      }
>  
> -    if (virStorageGetMetadataFromFD(conn, target->path, fd,
> -                                    &target->format, &encrypted,
> -                                    backingStore, capacity) < 0) {
> +    memset(&meta, 0, sizeof(meta));
> +
> +    if (virStorageGetMetadataFromFD(conn, target->path, fd, &meta) < 0) {
>          close(fd);
>          return -1;
>      }
>  
>      close(fd);
>  
> -    if (encryption != NULL && encrypted) {
> +    target->format = meta.format;
> +
> +    if (backingStore) {
> +        *backingStore = meta.backingStore;
> +        meta.backingStore = NULL;
> +    }
> +
> +    VIR_FREE(meta.backingStore);
> +
> +    if (capacity && meta.capacity)
> +        *capacity = meta.capacity;
> +
> +    if (encryption != NULL && meta.encrypted) {
>          if (VIR_ALLOC(*encryption) < 0) {
>              virReportOOMError(conn);
>              if (backingStore)
> diff --git a/src/util/storage_file.h b/src/util/storage_file.h
> index 7bccbe4..b458c0e 100644
> --- a/src/util/storage_file.h
> +++ b/src/util/storage_file.h
> @@ -25,6 +25,7 @@
>  #define __VIR_STORAGE_FILE_H__
>  
>  #include "util.h"
> +#include <stdbool.h>
>  
>  enum virStorageFileFormat {
>      VIR_STORAGE_FILE_RAW = 0,
> @@ -43,4 +44,11 @@ enum virStorageFileFormat {
>  
>  VIR_ENUM_DECL(virStorageFileFormat);
>  
> +typedef struct _virStorageFileMetadata {
> +    int format;
> +    char *backingStore;
> +    unsigned long long capacity;
> +    bool encrypted;
> +} virStorageFileMetadata;
> +
>  #endif /* __VIR_STORAGE_FILE_H__ */

ACK

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|