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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list