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

Re: [libvirt] [PATCHv2 01/16] storage: list more file types



On Sat, Oct 13, 2012 at 4:59 PM, Eric Blake <eblake redhat com> wrote:
> When an image has no backing file, using VIR_STORAGE_FILE_AUTO
> for its type is a bit confusing.  Additionally, a future patch
> would like to reserve a default value for the case of no file
> type specified in the XML, but different from the current use
> of -1 to imply probing, since probing is not always safe.
>
> Also, a couple of file types were missing compared to supported
> code: libxl supports 'vhd', and qemu supports 'fat' for directories
> passed through as a file system.
>
> * src/util/storage_file.h (virStorageFileFormat): Add
> VIR_STORAGE_FILE_NONE, VIR_STORAGE_FILE_FAT, VIR_STORAGE_FILE_VHD.
> * src/util/storage_file.c (virStorageFileMatchesVersion): Match
> documentation when version probing not supported.
> (cowGetBackingStore, qcowXGetBackingStore, qcow1GetBackingStore)
> (qedGetBackingStore, virStorageFileGetMetadataFromBuf)
> (virStorageFileGetMetadataFromFD): Take NONE into account.
> * src/conf/domain_conf.c (virDomainDiskDefForeachPath): Likewise.
> * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Likewise.
> * src/conf/storage_conf.c (virStorageVolumeFormatFromString): New
> function.
> (poolTypeInfo): Use it.
> ---
>  src/conf/domain_conf.c  |  2 +-
>  src/conf/storage_conf.c | 15 ++++++++---
>  src/qemu/qemu_driver.c  |  2 +-
>  src/util/storage_file.c | 69 +++++++++++++++++++++++++++++++++++--------------
>  src/util/storage_file.h |  8 ++++--
>  5 files changed, 69 insertions(+), 27 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c87c615..460a57c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14648,7 +14648,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
>          if (STREQ(formatStr, "aio"))
>              formatStr = "raw"; /* Xen compat */
>
> -        if ((format = virStorageFileFormatTypeFromString(formatStr)) < 0) {
> +        if ((format = virStorageFileFormatTypeFromString(formatStr)) <= 0) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("unknown disk format '%s' for %s"),
>                             disk->driverType, disk->src);
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 4daf059..5d9e61a 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -135,6 +135,15 @@ struct _virStoragePoolTypeInfo {
>      virStorageVolOptions volOptions;
>  };
>
> +static int
> +virStorageVolumeFormatFromString(const char *format)
> +{
> +    int ret = virStorageFileFormatTypeFromString(format);
> +    if (ret == VIR_STORAGE_FILE_NONE)
> +        return -1;
> +    return ret;
> +}
> +
>  static virStoragePoolTypeInfo poolTypeInfo[] = {
>      { .poolType = VIR_STORAGE_POOL_LOGICAL,
>        .poolOptions = {
> @@ -148,7 +157,7 @@ static virStoragePoolTypeInfo poolTypeInfo[] = {
>      { .poolType = VIR_STORAGE_POOL_DIR,
>        .volOptions = {
>              .defaultFormat = VIR_STORAGE_FILE_RAW,
> -            .formatFromString = virStorageFileFormatTypeFromString,
> +            .formatFromString = virStorageVolumeFormatFromString,
>              .formatToString = virStorageFileFormatTypeToString,
>          },
>      },
> @@ -161,7 +170,7 @@ static virStoragePoolTypeInfo poolTypeInfo[] = {
>          },
>        .volOptions = {
>              .defaultFormat = VIR_STORAGE_FILE_RAW,
> -            .formatFromString = virStorageFileFormatTypeFromString,
> +            .formatFromString = virStorageVolumeFormatFromString,
>              .formatToString = virStorageFileFormatTypeToString,
>          },
>      },
> @@ -175,7 +184,7 @@ static virStoragePoolTypeInfo poolTypeInfo[] = {
>          },
>        .volOptions = {
>              .defaultFormat = VIR_STORAGE_FILE_RAW,
> -            .formatFromString = virStorageFileFormatTypeFromString,
> +            .formatFromString = virStorageVolumeFormatFromString,
>              .formatToString = virStorageFileFormatTypeToString,
>          },
>      },
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 0787039..0e4b2d0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -9192,7 +9192,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom,
>
>      /* Probe for magic formats */
>      if (disk->driverType) {
> -        if ((format = virStorageFileFormatTypeFromString(disk->driverType)) < 0) {
> +        if ((format = virStorageFileFormatTypeFromString(disk->driverType)) <= 0) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("unknown disk format %s for %s"),
>                             disk->driverType, disk->src);
> diff --git a/src/util/storage_file.c b/src/util/storage_file.c
> index eea760c..3d1b7cf 100644
> --- a/src/util/storage_file.c
> +++ b/src/util/storage_file.c
> @@ -1,7 +1,7 @@
>  /*
>   * storage_file.c: file utility functions for FS storage backend
>   *
> - * Copyright (C) 2007-2011 Red Hat, Inc.
> + * Copyright (C) 2007-2012 Red Hat, Inc.
>   * Copyright (C) 2007-2008 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -45,9 +45,11 @@
>
>  VIR_ENUM_IMPL(virStorageFileFormat,
>                VIR_STORAGE_FILE_LAST,
> +              "none",
>                "raw", "dir", "bochs",
>                "cloop", "cow", "dmg", "iso",
> -              "qcow", "qcow2", "qed", "vmdk", "vpc")
> +              "qcow", "qcow2", "qed", "vmdk", "vpc",
> +              "fat", "vhd")
>
>  enum lv_endian {
>      LV_LITTLE_ENDIAN = 1, /* 1234 */
> @@ -123,8 +125,12 @@ qedGetBackingStore(char **, int *, const unsigned char *, size_t);
>
>
>  static struct FileTypeInfo const fileTypeInfo[] = {
> -    [VIR_STORAGE_FILE_RAW] = { NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL },
> -    [VIR_STORAGE_FILE_DIR] = { NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, 0, 0, 0, 0, NULL },
> +    [VIR_STORAGE_FILE_NONE] = { NULL, NULL, LV_LITTLE_ENDIAN,
> +                                -1, 0, 0, 0, 0, 0, NULL },
> +    [VIR_STORAGE_FILE_RAW] = { NULL, NULL, LV_LITTLE_ENDIAN,
> +                               -1, 0, 0, 0, 0, 0, NULL },
> +    [VIR_STORAGE_FILE_DIR] = { NULL, NULL, LV_LITTLE_ENDIAN,
> +                               -1, 0, 0, 0, 0, 0, NULL },
>      [VIR_STORAGE_FILE_BOCHS] = {
>          /*"Bochs Virtual HD Image", */ /* Untested */ NULL,
>          NULL,
> @@ -180,6 +186,11 @@ static struct FileTypeInfo const fileTypeInfo[] = {
>          LV_BIG_ENDIAN, 12, 0x10000,
>          8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, -1, NULL
>      },
> +    /* Not direct file formats, but used for various drivers */
> +    [VIR_STORAGE_FILE_FAT] = { NULL, NULL, LV_LITTLE_ENDIAN,
> +                               -1, 0, 0, 0, 0, 0, NULL },
> +    [VIR_STORAGE_FILE_VHD] = { NULL, NULL, LV_LITTLE_ENDIAN,
> +                               -1, 0, 0, 0, 0, 0, NULL },
>  };
>  verify(ARRAY_CARDINALITY(fileTypeInfo) == VIR_STORAGE_FILE_LAST);
>
> @@ -195,8 +206,10 @@ cowGetBackingStore(char **res,
>
>      if (buf_size < 4+4+ COW_FILENAME_MAXLEN)
>          return BACKING_STORE_INVALID;
> -    if (buf[4+4] == '\0') /* cow_header_v2.backing_file[0] */
> +    if (buf[4+4] == '\0') { /* cow_header_v2.backing_file[0] */
> +        *format = VIR_STORAGE_FILE_NONE;
>          return BACKING_STORE_OK;
> +    }
>
>      *res = strndup ((const char*)buf + 4+4, COW_FILENAME_MAXLEN);
>      if (*res == NULL) {
> @@ -298,8 +311,11 @@ qcowXGetBackingStore(char **res,
>              | (buf[QCOWX_HDR_BACKING_FILE_SIZE+1] << 16)
>              | (buf[QCOWX_HDR_BACKING_FILE_SIZE+2] << 8)
>              | buf[QCOWX_HDR_BACKING_FILE_SIZE+3]); /* QCowHeader.backing_file_size */
> -    if (size == 0)
> +    if (size == 0) {
> +        if (format)
> +            *format = VIR_STORAGE_FILE_NONE;
>          return BACKING_STORE_OK;
> +    }
>      if (offset + size > buf_size || offset + size < offset)
>          return BACKING_STORE_INVALID;
>      if (size + 1 == 0)
> @@ -335,8 +351,12 @@ qcowXGetBackingStore(char **res,
>       * between the end of the header (QCOW2_HDR_TOTAL_SIZE)
>       * and the start of the backingStoreName (offset)
>       */
> -    if (isQCow2 && format)
> -        qcow2GetBackingStoreFormat(format, buf, buf_size, QCOW2_HDR_TOTAL_SIZE, offset);
> +    if (isQCow2 && format) {
> +        qcow2GetBackingStoreFormat(format, buf, buf_size, QCOW2_HDR_TOTAL_SIZE,
> +                                   offset);
> +        if (*format <= VIR_STORAGE_FILE_NONE)
> +            return BACKING_STORE_INVALID;
> +    }
>
>      return BACKING_STORE_OK;
>  }
> @@ -348,10 +368,15 @@ qcow1GetBackingStore(char **res,
>                       const unsigned char *buf,
>                       size_t buf_size)
>  {
> +    int ret;
> +
>      /* QCow1 doesn't have the extensions capability
>       * used to store backing format */
>      *format = VIR_STORAGE_FILE_AUTO;
> -    return qcowXGetBackingStore(res, NULL, buf, buf_size, false);
> +    ret = qcowXGetBackingStore(res, NULL, buf, buf_size, false);
> +    if (ret == 0 && *buf == '\0')
> +        *format = VIR_STORAGE_FILE_NONE;
> +    return ret;
>  }
>
>  static int
> @@ -401,6 +426,7 @@ vmdk4GetBackingStore(char **res,
>      desc[len] = '\0';
>      start = strstr(desc, prefix);
>      if (start == NULL) {
> +        *format = VIR_STORAGE_FILE_NONE;
>          ret = BACKING_STORE_OK;
>          goto cleanup;
>      }
> @@ -411,6 +437,7 @@ vmdk4GetBackingStore(char **res,
>          goto cleanup;
>      }
>      if (end == start) {
> +        *format = VIR_STORAGE_FILE_NONE;
>          ret = BACKING_STORE_OK;
>          goto cleanup;
>      }
> @@ -464,8 +491,10 @@ qedGetBackingStore(char **res,
>      if (buf_size < QED_HDR_FEATURES_OFFSET+8)
>          return BACKING_STORE_INVALID;
>      flags = qedGetHeaderULL(buf + QED_HDR_FEATURES_OFFSET);
> -    if (!(flags & QED_F_BACKING_FILE))
> +    if (!(flags & QED_F_BACKING_FILE)) {
> +        *format = VIR_STORAGE_FILE_NONE;
>          return BACKING_STORE_OK;
> +    }
>
>      /* Parse the backing file */
>      if (buf_size < QED_HDR_BACKING_FILE_OFFSET+8)
> @@ -485,12 +514,10 @@ qedGetBackingStore(char **res,
>      memcpy(*res, buf + offset, size);
>      (*res)[size] = '\0';
>
> -    if (format) {
> -        if (flags & QED_F_BACKING_FORMAT_NO_PROBE)
> -            *format = virStorageFileFormatTypeFromString("raw");
> -        else
> -            *format = VIR_STORAGE_FILE_AUTO_SAFE;
> -    }
> +    if (flags & QED_F_BACKING_FORMAT_NO_PROBE)
> +        *format = virStorageFileFormatTypeFromString("raw");
> +    else
> +        *format = VIR_STORAGE_FILE_AUTO_SAFE;
>
>      return BACKING_STORE_OK;
>  }
> @@ -564,7 +591,7 @@ virStorageFileMatchesVersion(int format,
>
>      /* Validate version number info */
>      if (fileTypeInfo[format].versionOffset == -1)
> -        return true;
> +        return false;
>
>      if ((fileTypeInfo[format].versionOffset + 4) > buflen)
>          return false;
> @@ -607,7 +634,9 @@ virStorageFileGetMetadataFromBuf(int format,
>      /* XXX we should consider moving virStorageBackendUpdateVolInfo
>       * code into this method, for non-magic files
>       */
> -    if (!fileTypeInfo[format].magic) {
> +    if (format <= VIR_STORAGE_FILE_NONE ||
> +        format >= VIR_STORAGE_FILE_LAST ||
> +        !fileTypeInfo[format].magic) {
>          return 0;
>      }
>
> @@ -682,7 +711,7 @@ virStorageFileGetMetadataFromBuf(int format,
>              meta->backingStoreFormat = backingFormat;
>          } else {
>              meta->backingStore = NULL;
> -            meta->backingStoreFormat = VIR_STORAGE_FILE_AUTO;
> +            meta->backingStoreFormat = VIR_STORAGE_FILE_NONE;
>          }
>      }
>
> @@ -867,7 +896,7 @@ virStorageFileGetMetadataFromFD(const char *path,
>      if (format == VIR_STORAGE_FILE_AUTO)
>          format = virStorageFileProbeFormatFromBuf(path, head, len);
>
> -    if (format < 0 ||
> +    if (format <= VIR_STORAGE_FILE_NONE ||
>          format >= VIR_STORAGE_FILE_LAST) {
>          virReportSystemError(EINVAL, _("unknown storage file format %d"),
>                               format);
> diff --git a/src/util/storage_file.h b/src/util/storage_file.h
> index 2e76b1b..683ec73 100644
> --- a/src/util/storage_file.h
> +++ b/src/util/storage_file.h
> @@ -1,7 +1,7 @@
>  /*
>   * storage_file.c: file utility functions for FS storage backend
>   *
> - * Copyright (C) 2007-2009 Red Hat, Inc.
> + * Copyright (C) 2007-2009, 2012 Red Hat, Inc.
>   * Copyright (C) 2007-2008 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -29,7 +29,8 @@
>  enum virStorageFileFormat {
>      VIR_STORAGE_FILE_AUTO_SAFE = -2,
>      VIR_STORAGE_FILE_AUTO = -1,
> -    VIR_STORAGE_FILE_RAW = 0,
> +    VIR_STORAGE_FILE_NONE = 0,
> +    VIR_STORAGE_FILE_RAW,
>      VIR_STORAGE_FILE_DIR,
>      VIR_STORAGE_FILE_BOCHS,
>      VIR_STORAGE_FILE_CLOOP,
> @@ -41,6 +42,9 @@ enum virStorageFileFormat {
>      VIR_STORAGE_FILE_QED,
>      VIR_STORAGE_FILE_VMDK,
>      VIR_STORAGE_FILE_VPC,
> +    VIR_STORAGE_FILE_FAT,
> +    VIR_STORAGE_FILE_VHD,
> +
>      VIR_STORAGE_FILE_LAST,
>  };
>
> --

Visual ACK. I'll try to apply and build it later.

-- 
Doug Goldstein


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