[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] Re: [PATCH 2/6] Split virStorageGetMetadataFromFD() from virStorageBackendProbeTarget()



On Tue, Sep 29, 2009 at 09:56:45AM +0100, Mark McLoughlin wrote:
> Prepare the code probing a file's format and associated metadata for
> moving into libvirt_util.
> 
> * src/storage/storage_backend_fs.c: re-factor the format and metadata
>   probing code in preparation for moving it
> ---
>  src/storage/storage_backend_fs.c |  148 +++++++++++++++++++++++---------------
>  1 files changed, 91 insertions(+), 57 deletions(-)
> 
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index e7a3ca9..87c30cd 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -283,49 +283,37 @@ static char *absolutePathFromBaseFile(const char *base_file, const char *path)
>   * Probe the header of a file to determine what type of disk image
>   * it is, and info about its capacity if available.
>   */
> -static int virStorageBackendProbeTarget(virConnectPtr conn,
> -                                        virStorageVolTargetPtr target,
> -                                        char **backingStore,
> -                                        unsigned long long *allocation,
> -                                        unsigned long long *capacity,
> -                                        virStorageEncryptionPtr *encryption) {
> -    int fd;
> +static int
> +virStorageGetMetadataFromFD(virConnectPtr conn,
> +                            const char *path,
> +                            int fd,
> +                            int *format,
> +                            bool *encrypted,
> +                            char **backingStore,
> +                            unsigned long long *capacity)
> +{
>      unsigned char head[20*512]; /* vmdk4GetBackingStore needs this much. */
> -    int len, i, ret;
> +    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;
> -    if (encryption)
> -        *encryption = NULL;
> -
> -    if ((fd = open(target->path, O_RDONLY)) < 0) {
> -        virReportSystemError(conn, errno,
> -                             _("cannot open volume '%s'"),
> -                             target->path);
> -        return -1;
> -    }
> -
> -    if ((ret = virStorageBackendUpdateVolTargetInfoFD(conn, target, fd,
> -                                                      allocation,
> -                                                      capacity)) < 0) {
> -        close(fd);
> -        return ret; /* Take care to propagate ret, it is not always -1 */
> -    }
> +    /* Do not overwrite capacity
> +     * if (capacity)
> +     *     *capacity = 0;
> +     */
>  
>      if ((len = read(fd, head, sizeof(head))) < 0) {
> -        virReportSystemError(conn, errno,
> -                             _("cannot read header '%s'"),
> -                             target->path);
> -        close(fd);
> +        virReportSystemError(conn, errno, _("cannot read header '%s'"), path);
>          return -1;
>      }
>  
> -    close(fd);
> -
>      /* First check file magic */
>      for (i = 0 ; i < ARRAY_CARDINALITY(fileTypeInfo) ; i++) {
>          int mlen;
> -        bool encrypted_qcow = false;
>  
>          if (fileTypeInfo[i].magic == NULL)
>              continue;
> @@ -385,18 +373,19 @@ static int virStorageBackendProbeTarget(virConnectPtr conn,
>              *capacity *= fileTypeInfo[i].sizeMultiplier;
>          }
>  
> -        if (fileTypeInfo[i].qcowCryptOffset != -1) {
> +        if (fileTypeInfo[i].qcowCryptOffset != -1 && encrypted) {
>              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_qcow = crypt_format != 0;
> +            *encrypted = crypt_format != 0;
>          }
>  
>          /* Validation passed, we know the file format now */
> -        target->format = fileTypeInfo[i].type;
> +        if (format)
> +            *format = fileTypeInfo[i].type;
>          if (fileTypeInfo[i].getBackingStore != NULL && backingStore) {
>              char *base;
>  
> @@ -411,8 +400,7 @@ static int virStorageBackendProbeTarget(virConnectPtr conn,
>                  return -1;
>              }
>              if (base != NULL) {
> -                *backingStore
> -                    = absolutePathFromBaseFile(target->path, base);
> +                *backingStore = absolutePathFromBaseFile(path, base);
>                  VIR_FREE(base);
>                  if (*backingStore == NULL) {
>                      virReportOOMError(conn);
> @@ -420,23 +408,6 @@ static int virStorageBackendProbeTarget(virConnectPtr conn,
>                  }
>              }
>          }
> -        if (encryption != NULL && encrypted_qcow) {
> -            virStorageEncryptionPtr enc;
> -
> -            if (VIR_ALLOC(enc) < 0) {
> -                virReportOOMError(conn);
> -                if (backingStore)
> -                    VIR_FREE(*backingStore);
> -                return -1;
> -            }
> -            enc->format = VIR_STORAGE_ENCRYPTION_FORMAT_QCOW;
> -            *encryption = enc;
> -            /* XXX ideally we'd fill in secret UUID here
> -             * but we cannot guarentee 'conn' is non-NULL
> -             * at this point in time :-(  So we only fill
> -             * in secrets when someone first queries a vol
> -             */
> -        }
>          return 0;
>      }
>  
> @@ -445,15 +416,78 @@ static int virStorageBackendProbeTarget(virConnectPtr conn,
>          if (fileTypeInfo[i].extension == NULL)
>              continue;
>  
> -        if (!virFileHasSuffix(target->path, fileTypeInfo[i].extension))
> +        if (!virFileHasSuffix(path, fileTypeInfo[i].extension))
>              continue;
>  
> -        target->format = fileTypeInfo[i].type;
> +        if (format)
> +            *format = fileTypeInfo[i].type;
>          return 0;
>      }
>  
> -    /* All fails, so call it a raw file */
> -    target->format = VIR_STORAGE_FILE_RAW;
> +    return 0;
> +}
> +
> +static int
> +virStorageBackendProbeTarget(virConnectPtr conn,
> +                             virStorageVolTargetPtr target,
> +                             char **backingStore,
> +                             unsigned long long *allocation,
> +                             unsigned long long *capacity,
> +                             virStorageEncryptionPtr *encryption)
> +{
> +    int fd, ret;
> +    bool encrypted;
> +
> +    if (encryption)
> +        *encryption = NULL;
> +
> +    if ((fd = open(target->path, O_RDONLY)) < 0) {
> +        virReportSystemError(conn, errno,
> +                             _("cannot open volume '%s'"),
> +                             target->path);
> +        return -1;
> +    }
> +
> +    if ((ret = virStorageBackendUpdateVolTargetInfoFD(conn, target, fd,
> +                                                      allocation,
> +                                                      capacity)) < 0) {
> +        close(fd);
> +        return ret; /* Take care to propagate ret, it is not always -1 */
> +    }
> +
> +    if (virStorageGetMetadataFromFD(conn, target->path, fd,
> +                                    &target->format, &encrypted,
> +                                    backingStore, capacity) < 0) {
> +        close(fd);
> +        return -1;
> +    }
> +
> +    close(fd);
> +
> +    if (encryption != NULL && encrypted) {
> +        if (VIR_ALLOC(*encryption) < 0) {
> +            virReportOOMError(conn);
> +            if (backingStore)
> +                VIR_FREE(*backingStore);
> +            return -1;
> +        }
> +
> +        switch (target->format) {
> +        case VIR_STORAGE_FILE_QCOW:
> +        case VIR_STORAGE_FILE_QCOW2:
> +            (*encryption)->format = VIR_STORAGE_ENCRYPTION_FORMAT_QCOW;
> +            break;
> +        default:
> +            break;
> +        }
> +
> +        /* XXX ideally we'd fill in secret UUID here
> +         * but we cannot guarentee 'conn' is non-NULL
> +         * at this point in time :-(  So we only fill
> +         * in secrets when someone first queries a vol
> +         */
> +    }
> +
>      return 0;
>  }

ACK, this looks a lot nicer refactored

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]