[libvirt] [PATCH 2/7] util: add support for qcow2v3 image detection

[Martin Kletzander]

On 06/10/2013 02:10 PM, Ján Tomko wrote:
> Detect qcow2 images with version 3 in the image header as
> VIR_STORAGE_FILE_QCOW2.
> 
> These images have a feature bitfield, with just one feature supported
> so far: lazy_refcounts.
> 
> The header length changed too, moving the location of the backing
> format name.
> ---
>  src/util/virstoragefile.c | 164 +++++++++++++++++++++++++++++++++++-----------
>  src/util/virstoragefile.h |  12 ++++
>  2 files changed, 139 insertions(+), 37 deletions(-)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index a391738..f30324e 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -59,6 +59,11 @@ VIR_ENUM_IMPL(virStorageFileFormat,
>                "qcow", "qcow2", "qed", "vmdk", "vpc",
>                "fat", "vhd", "vdi")
>  
> +VIR_ENUM_IMPL(virStorageFileFeature,
> +              VIR_STORAGE_FILE_FEATURE_LAST,
> +              "lazy_refcounts",
> +              )
> +
>  enum lv_endian {
>      LV_LITTLE_ENDIAN = 1, /* 1234 */
>      LV_BIG_ENDIAN         /* 4321 */
> @@ -81,7 +86,7 @@ struct FileTypeInfo {
>                             * where we find version number,
>                             * -1 to always fail the version test,
>                             * -2 to always pass the version test */
> -    int versionNumber;    /* Version number to validate */
> +    int versionNumbers[3]; /* Version numbers to validate, terminated by 0 */

There could be a constant for this, which will be increased in case of
need and will appear on some more readable place.

>      int sizeOffset;       /* Byte offset from start of file
>                             * where we find capacity info,
>                             * -1 to use st_size as capacity */
> @@ -95,6 +100,8 @@ struct FileTypeInfo {
>                             * -1 if encryption is not used */
>      int (*getBackingStore)(char **res, int *format,
>                             const unsigned char *buf, size_t buf_size);
> +    int (*getFeatures)(virBitmapPtr *features, int format,
> +                       unsigned char *buf, ssize_t len);
>  };
>  
>  static int cowGetBackingStore(char **, int *,
> @@ -103,6 +110,8 @@ static int qcow1GetBackingStore(char **, int *,
>                                  const unsigned char *, size_t);
>  static int qcow2GetBackingStore(char **, int *,
>                                  const unsigned char *, size_t);
> +static int qcow2GetFeatures(virBitmapPtr *features, int format,
> +                            unsigned char *buf, ssize_t len);
>  static int vmdk4GetBackingStore(char **, int *,
>                                  const unsigned char *, size_t);
>  static int
> @@ -122,6 +131,13 @@ qedGetBackingStore(char **, int *, const unsigned char *, size_t);
>  #define QCOW2_HDR_EXTENSION_END 0
>  #define QCOW2_HDR_EXTENSION_BACKING_FORMAT 0xE2792ACA
>  
> +#define QCOW2v3_HDR_FEATURES_INCOMPATIBLE (QCOW2_HDR_TOTAL_SIZE)
> +#define QCOW2v3_HDR_FEATURES_COMPATIBLE (QCOW2v3_HDR_FEATURES_INCOMPATIBLE+8)
> +#define QCOW2v3_HDR_FEATURES_AUTOCLEAR (QCOW2v3_HDR_FEATURES_COMPATIBLE+8)
> +
> +/* The location of the header size [4 bytes] */
> +#define QCOW2v3_HDR_SIZE       (QCOW2_HDR_TOTAL_SIZE+8+8+8+4)
> +

I must admit I'm not very fond of the naming (qcow2v3), but since this
is an implementation detail and makes sense, let's go with it.

>  #define QED_HDR_FEATURES_OFFSET (4+4+4+4)
>  #define QED_HDR_IMAGE_SIZE (QED_HDR_FEATURES_OFFSET+8+8+8+8)
>  #define QED_HDR_BACKING_FILE_OFFSET (QED_HDR_IMAGE_SIZE+8)
> @@ -137,16 +153,16 @@ qedGetBackingStore(char **, int *, const unsigned char *, size_t);
>  
>  static struct FileTypeInfo const fileTypeInfo[] = {
>      [VIR_STORAGE_FILE_NONE] = { 0, NULL, NULL, LV_LITTLE_ENDIAN,
> -                                -1, 0, 0, 0, 0, 0, NULL },
> +                                -1, {0}, 0, 0, 0, 0, NULL, NULL },
>      [VIR_STORAGE_FILE_RAW] = { 0, NULL, NULL, LV_LITTLE_ENDIAN,
> -                               -1, 0, 0, 0, 0, 0, NULL },
> +                               -1, {0}, 0, 0, 0, 0, NULL, NULL },
>      [VIR_STORAGE_FILE_DIR] = { 0, NULL, NULL, LV_LITTLE_ENDIAN,
> -                               -1, 0, 0, 0, 0, 0, NULL },
> +                               -1, {0}, 0, 0, 0, 0, NULL, NULL },
>      [VIR_STORAGE_FILE_BOCHS] = {
>          /*"Bochs Virtual HD Image", */ /* Untested */
>          0, NULL, NULL,
> -        LV_LITTLE_ENDIAN, 64, 0x20000,
> -        32+16+16+4+4+4+4+4, 8, 1, -1, NULL
> +        LV_LITTLE_ENDIAN, 64, {0x20000, 0},

Good you added the trailing zero, in case some format goes to 3 version
numbers, compiler will shout and we won't get to segfauls'n'stuff.  Took
me a while to appreciate this ;)  But...

[...]
>  
> +/* qcow2 compatible features in the order they appear on-disk */
> +enum qcow2CompatibleFeature {
> +    QCOW2_COMPATIBLE_FEATURE_LAZY_REFCOUNTS = 0,
> +
> +    QCOW2_COMPATIBLE_FEATURE_LAST
> +};
> +
> +/* conversion to virStorageFeatures */

Did you mean virStorageFileFeature?

> +static int qcow2CompatibleFeatureArray[] = {

s/int/const int/ maybe ?

[...]
> @@ -611,12 +653,14 @@ virStorageFileMatchesVersion(int format,
>      else
>          version = virReadBufInt32BE(buf + fileTypeInfo[format].versionOffset);
>  
> -    VIR_DEBUG("Compare detected version %d vs expected version %d",
> -              version, fileTypeInfo[format].versionNumber);
> -    if (version != fileTypeInfo[format].versionNumber)
> -        return false;
> +    for (i = 0; fileTypeInfo[format].versionNumbers[i] != 0; i++) {
> +        VIR_DEBUG("Compare detected version %d vs expected version %d",

Changing the 'expected version' to something like 'one of the expected
versions' could make users less confused when going through the logs,
but not required.

> +                  version, fileTypeInfo[format].versionNumbers[i]);
> +        if (version == fileTypeInfo[format].versionNumbers[i])
> +            return true;
> +    }
>  

... the constant mentioned earlier could be checked upon in here as well
and that would make the format specification 1) safer 2) shorter (not
much, though).

[...]
> @@ -669,6 +713,42 @@ cleanup:
>  }
>  
>  
> +static int
> +qcow2GetFeatures(virBitmapPtr *features,
> +                int format,
> +                unsigned char *buf,
> +                ssize_t len)

indentation

[...]

ACK with:
 - the constant used instead of magic '3'
 - const int
 - indentation

Martin