[libvirt] [PATCH v4 0/7] Add qcow2 extensions support
Since QEMU 1.1 qcow2 supports a features bitfield, allowing to implement additional features without breaking compatibility with older QEMU versions. The version header on-disk is bumped to 3, however the format name is still 'qcow2', not 'qcow3' (which is what I used in v2). v4: rebased v3: https://www.redhat.com/archives/libvir-list/2013-May/msg01179.html v2: https://www.redhat.com/archives/libvir-list/2013-February/msg00212.html qcow2 vs. qcow3 discussion: https://www.redhat.com/archives/libvir-list/2013-March/msg00094.html Ján Tomko (7): storage: rework qemu-img command line generation util: add support for qcow2v3 image detection conf: add features to volume target XML storage: add support for creating qcow2 images with extensions conf: split out snapshot disk XML formatting Add qcow2 features support to snapshots qemu: add qcow2 extension support to inactive external snapshots docs/formatsnapshot.html.in | 5 + docs/formatstorage.html.in | 19 +++ docs/schemas/Makefile.am | 1 + docs/schemas/domainsnapshot.rng | 7 + docs/schemas/storagefilefeatures.rng | 24 ++++ docs/schemas/storagevol.rng | 7 + libvirt.spec.in | 1 + mingw-libvirt.spec.in | 2 + src/conf/snapshot_conf.c | 127 ++++++++++++++---- src/conf/snapshot_conf.h | 2 + src/conf/storage_conf.c | 74 ++++++++++ src/conf/storage_conf.h | 7 +- src/libvirt_private.syms | 2 + src/qemu/qemu_driver.c | 72 +++++++--- src/storage/storage_backend.c | 143 +++++++++++++------- src/storage/storage_backend_fs.c | 10 ++ src/util/virstoragefile.c | 164 ++++++++++++++++++----- src/util/virstoragefile.h | 12 ++ tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 5 + tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 4 + tests/storagevolxml2argvdata/qcow2-1.1.argv | 1 + tests/storagevolxml2argvdata/qcow2-lazy.argv | 1 + tests/storagevolxml2argvdata/vol-qcow2-1.1.xml | 32 +++++ tests/storagevolxml2argvdata/vol-qcow2-lazy.xml | 35 +++++ tests/storagevolxml2argvtest.c | 2 + tests/storagevolxml2xmlin/vol-qcow2-1.1.xml | 32 +++++ tests/storagevolxml2xmlin/vol-qcow2-lazy.xml | 35 +++++ tests/storagevolxml2xmlout/vol-qcow2-1.1.xml | 33 +++++ tests/storagevolxml2xmlout/vol-qcow2-lazy.xml | 35 +++++ tests/storagevolxml2xmltest.c | 2 + 30 files changed, 766 insertions(+), 130 deletions(-) create mode 100644 docs/schemas/storagefilefeatures.rng create mode 100644 tests/storagevolxml2argvdata/qcow2-1.1.argv create mode 100644 tests/storagevolxml2argvdata/qcow2-lazy.argv create mode 100644 tests/storagevolxml2argvdata/vol-qcow2-1.1.xml create mode 100644 tests/storagevolxml2argvdata/vol-qcow2-lazy.xml create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-1.1.xml create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-lazy.xml create mode 100644 tests/storagevolxml2xmlout/vol-qcow2-1.1.xml create mode 100644 tests/storagevolxml2xmlout/vol-qcow2-lazy.xml -- 1.8.1.5
Split out option string generation to make adding new options easier and simplify the code. --- src/storage/storage_backend.c | 111 ++++++++++++++++++++++-------------------- 1 file changed, 59 insertions(+), 52 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e5c9209..0abc3f3 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -651,6 +651,32 @@ cleanup: return ret; } +static int +virStorageBackendCreateQemuImgOpts(char **opts, + const char *backingType, + bool encryption, + bool preallocate) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + if (backingType) + virBufferAsprintf(&buf, "backing_fmt=%s,", backingType); + if (encryption) + virBufferAddLit(&buf, "encryption=on,"); + if (preallocate) + virBufferAddLit(&buf, "preallocation=metadata,"); + + virBufferTrim(&buf, ",", -1); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + + *opts = virBufferContentAndReset(&buf); + return 0; +} + virCommandPtr virStorageBackendCreateQemuImgCmd(virConnectPtr conn, virStoragePoolObjPtr pool, @@ -668,6 +694,9 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, const char *backingType = NULL; const char *inputPath = NULL; const char *inputType = NULL; + char *opts = NULL; + bool convert = false; + bool backing = false; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); @@ -795,65 +824,43 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, cmd = virCommandNew(create_tool); - if (inputvol) { - virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); + convert = !!inputvol; + backing = !inputvol && vol->backingStore.path; - if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS && - (do_encryption || preallocate)) { - virCommandAddArg(cmd, "-o"); - virCommandAddArgFormat(cmd, "%s%s%s", do_encryption ? "encryption=on" : "", - (do_encryption && preallocate) ? "," : "", - preallocate ? "preallocation=metadata" : ""); - } else if (do_encryption) { - virCommandAddArg(cmd, "-e"); - } - virCommandAddArgList(cmd, inputPath, vol->target.path, NULL); - } else if (vol->backingStore.path) { - virCommandAddArgList(cmd, "create", "-f", type, - "-b", vol->backingStore.path, NULL); - - switch (imgformat) { - case QEMU_IMG_BACKING_FORMAT_FLAG: - virCommandAddArgList(cmd, "-F", backingType, NULL); - if (do_encryption) - virCommandAddArg(cmd, "-e"); - virCommandAddArg(cmd, vol->target.path); - virCommandAddArgFormat(cmd, "%lluK", size_arg); - break; - - case QEMU_IMG_BACKING_FORMAT_OPTIONS: - virCommandAddArg(cmd, "-o"); - virCommandAddArgFormat(cmd, "backing_fmt=%s%s", backingType, - do_encryption ? ",encryption=on" : ""); - virCommandAddArg(cmd, vol->target.path); - virCommandAddArgFormat(cmd, "%lluK", size_arg); - break; + if (convert) + virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); + else + virCommandAddArgList(cmd, "create", "-f", type, NULL); - default: - VIR_DEBUG("Unable to set backing store format for %s with %s", - vol->target.path, create_tool); + if (backing) + virCommandAddArgList(cmd, "-b", vol->backingStore.path, NULL); - if (do_encryption) - virCommandAddArg(cmd, "-e"); - virCommandAddArg(cmd, vol->target.path); - virCommandAddArgFormat(cmd, "%lluK", size_arg); - } + if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS) { + if (virStorageBackendCreateQemuImgOpts(&opts, + backing ? backingType : NULL, + do_encryption, preallocate)) + return NULL; + if (opts) + virCommandAddArgList(cmd, "-o", opts, NULL); + VIR_FREE(opts); } else { - virCommandAddArgList(cmd, "create", "-f", type, NULL); - - if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS && - (do_encryption || preallocate)) { - virCommandAddArg(cmd, "-o"); - virCommandAddArgFormat(cmd, "%s%s%s", do_encryption ? "encryption=on" : "", - (do_encryption && preallocate) ? "," : "", - preallocate ? "preallocation=metadata" : ""); - } else if (do_encryption) { - virCommandAddArg(cmd, "-e"); + if (backing) { + if (imgformat == QEMU_IMG_BACKING_FORMAT_FLAG) + virCommandAddArgList(cmd, "-F", backingType, NULL); + else + VIR_DEBUG("Unable to set backing store format for %s with %s", + vol->target.path, create_tool); } - virCommandAddArg(cmd, vol->target.path); - virCommandAddArgFormat(cmd, "%lluK", size_arg); + if (do_encryption) + virCommandAddArg(cmd, "-e"); } + if (convert) + virCommandAddArg(cmd, inputPath); + virCommandAddArg(cmd, vol->target.path); + if (!convert) + virCommandAddArgFormat(cmd, "%lluK", size_arg); + return cmd; } -- 1.8.1.5
On 06/10/2013 02:10 PM, Ján Tomko wrote:
Split out option string generation to make adding new options easier and simplify the code. --- src/storage/storage_backend.c | 111 ++++++++++++++++++++++-------------------- 1 file changed, 59 insertions(+), 52 deletions(-)
[...]
@@ -795,65 +824,43 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn,
cmd = virCommandNew(create_tool);
- if (inputvol) { - virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); + convert = !!inputvol; + backing = !inputvol && vol->backingStore.path;
- if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS && - (do_encryption || preallocate)) { - virCommandAddArg(cmd, "-o"); - virCommandAddArgFormat(cmd, "%s%s%s", do_encryption ? "encryption=on" : "", - (do_encryption && preallocate) ? "," : "", - preallocate ? "preallocation=metadata" : ""); - } else if (do_encryption) { - virCommandAddArg(cmd, "-e"); - } - virCommandAddArgList(cmd, inputPath, vol->target.path, NULL); - } else if (vol->backingStore.path) { - virCommandAddArgList(cmd, "create", "-f", type, - "-b", vol->backingStore.path, NULL); - - switch (imgformat) { - case QEMU_IMG_BACKING_FORMAT_FLAG: - virCommandAddArgList(cmd, "-F", backingType, NULL); - if (do_encryption) - virCommandAddArg(cmd, "-e"); - virCommandAddArg(cmd, vol->target.path); - virCommandAddArgFormat(cmd, "%lluK", size_arg); - break; - - case QEMU_IMG_BACKING_FORMAT_OPTIONS: - virCommandAddArg(cmd, "-o"); - virCommandAddArgFormat(cmd, "backing_fmt=%s%s", backingType, - do_encryption ? ",encryption=on" : ""); - virCommandAddArg(cmd, vol->target.path); - virCommandAddArgFormat(cmd, "%lluK", size_arg); - break; + if (convert) + virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); + else + virCommandAddArgList(cmd, "create", "-f", type, NULL);
- default: - VIR_DEBUG("Unable to set backing store format for %s with %s", - vol->target.path, create_tool); + if (backing) + virCommandAddArgList(cmd, "-b", vol->backingStore.path, NULL);
- if (do_encryption) - virCommandAddArg(cmd, "-e"); - virCommandAddArg(cmd, vol->target.path); - virCommandAddArgFormat(cmd, "%lluK", size_arg); - } + if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS) { + if (virStorageBackendCreateQemuImgOpts(&opts, + backing ? backingType : NULL, + do_encryption, preallocate))
I guess we should use '< 0' here to unify the looks of it. Other than that it's way more readable than the previous version, so ACK with that fixed. Martin
On 06/10/2013 04:12 PM, Martin Kletzander wrote:
On 06/10/2013 02:10 PM, Ján Tomko wrote:
Split out option string generation to make adding new options easier and simplify the code. --- src/storage/storage_backend.c | 111 ++++++++++++++++++++++-------------------- 1 file changed, 59 insertions(+), 52 deletions(-)
+ if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS) { + if (virStorageBackendCreateQemuImgOpts(&opts, + backing ? backingType : NULL, + do_encryption, preallocate))
I guess we should use '< 0' here to unify the looks of it.
Other than that it's way more readable than the previous version, so ACK with that fixed.
Martin
Thanks, I've squashed this in to prevent Coverity from complaining about unused virBufferTrim's return value: diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e7930ee..aa47871 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -643,9 +643,7 @@ virStorageBackendCreateQemuImgOpts(char **opts, if (preallocate) virBufferAddLit(&buf, "preallocation=metadata,"); - virBufferTrim(&buf, ",", -1); - - if (virBufferError(&buf)) { + if (virBufferTrim(&buf, ",", -1) < 0) { virBufferFreeAndReset(&buf); virReportOOMError(); return -1; @@ -816,7 +814,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS) { if (virStorageBackendCreateQemuImgOpts(&opts, backing ? backingType : NULL, - do_encryption, preallocate)) + do_encryption, preallocate) < 0) return NULL; if (opts) virCommandAddArgList(cmd, "-o", opts, NULL);
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 */ 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) + #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}, + 32+16+16+4+4+4+4+4, 8, 1, -1, NULL, NULL }, [VIR_STORAGE_FILE_CLOOP] = { /* #!/bin/sh @@ -154,67 +170,82 @@ static struct FileTypeInfo const fileTypeInfo[] = { modprobe cloop file=$0 && mount -r -t iso9660 /dev/cloop $1 */ /* Untested */ 0, NULL, NULL, - LV_LITTLE_ENDIAN, -1, 0, - -1, 0, 0, -1, NULL + LV_LITTLE_ENDIAN, -1, {0}, + -1, 0, 0, -1, NULL, NULL }, [VIR_STORAGE_FILE_COW] = { 0, "OOOM", NULL, - LV_BIG_ENDIAN, 4, 2, - 4+4+1024+4, 8, 1, -1, cowGetBackingStore + LV_BIG_ENDIAN, 4, {2, 0}, + 4+4+1024+4, 8, 1, -1, cowGetBackingStore, NULL }, [VIR_STORAGE_FILE_DMG] = { /* XXX QEMU says there's no magic for dmg, * /usr/share/misc/magic lists double magic (both offsets * would have to match) but then disables that check. */ 0, NULL, ".dmg", - 0, -1, 0, - -1, 0, 0, -1, NULL + 0, -1, {0}, + -1, 0, 0, -1, NULL, NULL }, [VIR_STORAGE_FILE_ISO] = { 32769, "CD001", ".iso", - LV_LITTLE_ENDIAN, -2, 0, - -1, 0, 0, -1, NULL + LV_LITTLE_ENDIAN, -2, {0}, + -1, 0, 0, -1, NULL, NULL }, [VIR_STORAGE_FILE_QCOW] = { 0, "QFI", NULL, - LV_BIG_ENDIAN, 4, 1, - QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW1_HDR_CRYPT, qcow1GetBackingStore, + LV_BIG_ENDIAN, 4, {1, 0}, + QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW1_HDR_CRYPT, qcow1GetBackingStore, NULL }, [VIR_STORAGE_FILE_QCOW2] = { 0, "QFI", NULL, - LV_BIG_ENDIAN, 4, 2, + LV_BIG_ENDIAN, 4, {2, 3}, QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW2_HDR_CRYPT, qcow2GetBackingStore, + qcow2GetFeatures }, [VIR_STORAGE_FILE_QED] = { /* http://wiki.qemu.org/Features/QED */ 0, "QED", NULL, - LV_LITTLE_ENDIAN, -2, -1, - QED_HDR_IMAGE_SIZE, 8, 1, -1, qedGetBackingStore, + LV_LITTLE_ENDIAN, -2, {0}, + QED_HDR_IMAGE_SIZE, 8, 1, -1, qedGetBackingStore, NULL }, [VIR_STORAGE_FILE_VMDK] = { 0, "KDMV", NULL, - LV_LITTLE_ENDIAN, 4, 1, - 4+4+4, 8, 512, -1, vmdk4GetBackingStore + LV_LITTLE_ENDIAN, 4, {1, 0}, + 4+4+4, 8, 512, -1, vmdk4GetBackingStore, NULL }, [VIR_STORAGE_FILE_VPC] = { 0, "conectix", NULL, - LV_BIG_ENDIAN, 12, 0x10000, - 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, -1, NULL + LV_BIG_ENDIAN, 12, {0x10000, 0}, + 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, -1, NULL, NULL }, /* TODO: add getBackingStore function */ [VIR_STORAGE_FILE_VDI] = { 64, "\x7f\x10\xda\xbe", ".vdi", - LV_LITTLE_ENDIAN, 68, 0x00010001, - 64 + 5 * 4 + 256 + 7 * 4, 8, 1, -1, NULL}, + LV_LITTLE_ENDIAN, 68, {0x00010001, 0}, + 64 + 5 * 4 + 256 + 7 * 4, 8, 1, -1, NULL, NULL}, /* Not direct file formats, but used for various drivers */ [VIR_STORAGE_FILE_FAT] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, 0, 0, 0, 0, 0, NULL }, + -1, {0}, 0, 0, 0, 0, NULL, NULL }, [VIR_STORAGE_FILE_VHD] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, 0, 0, 0, 0, 0, NULL }, + -1, {0}, 0, 0, 0, 0, NULL, NULL }, }; verify(ARRAY_CARDINALITY(fileTypeInfo) == VIR_STORAGE_FILE_LAST); +/* 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 */ +static int qcow2CompatibleFeatureArray[] = { + VIR_STORAGE_FILE_FEATURE_LAZY_REFCOUNTS, +}; +verify(ARRAY_CARDINALITY(qcow2CompatibleFeatureArray) == + QCOW2_COMPATIBLE_FEATURE_LAST); + static int cowGetBackingStore(char **res, int *format, @@ -301,6 +332,8 @@ qcowXGetBackingStore(char **res, { unsigned long long offset; unsigned int size; + unsigned long long start; + int version; *res = NULL; if (format) @@ -351,11 +384,20 @@ qcowXGetBackingStore(char **res, * Thus the file region to search for extensions is * between the end of the header (QCOW2_HDR_TOTAL_SIZE) * and the start of the backingStoreName (offset) + * + * for qcow2 v3 images, the length of the header + * is stored at QCOW2v3_HDR_SIZE */ - if (isQCow2 && format && - qcow2GetBackingStoreFormat(format, buf, buf_size, QCOW2_HDR_TOTAL_SIZE, - offset) < 0) - return BACKING_STORE_INVALID; + if (isQCow2 && format) { + version = virReadBufInt32BE(buf + QCOWX_HDR_VERSION); + if (version == 2) + start = QCOW2_HDR_TOTAL_SIZE; + else + start = virReadBufInt32BE(buf + QCOW2v3_HDR_SIZE); + if (qcow2GetBackingStoreFormat(format, buf, buf_size, + start, offset) < 0) + return BACKING_STORE_INVALID; + } return BACKING_STORE_OK; } @@ -593,7 +635,7 @@ virStorageFileMatchesVersion(int format, unsigned char *buf, size_t buflen) { - int version; + int version, i; /* Validate version number info */ if (fileTypeInfo[format].versionOffset == -1) @@ -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", + version, fileTypeInfo[format].versionNumbers[i]); + if (version == fileTypeInfo[format].versionNumbers[i]) + return true; + } - return true; + return false; } static bool @@ -669,6 +713,42 @@ cleanup: } +static int +qcow2GetFeatures(virBitmapPtr *features, + int format, + unsigned char *buf, + ssize_t len) +{ + int version = -1; + virBitmapPtr feat = NULL; + uint64_t bits; + int i; + + version = virReadBufInt32BE(buf + fileTypeInfo[format].versionOffset); + + if (version == 2) + return 0; + + if (len < QCOW2v3_HDR_SIZE) + return -1; + + if (!(feat = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST))) { + virReportOOMError(); + return -1; + } + + /* todo: check for incompatible or autoclear features? */ + bits = virReadBufInt64BE(buf + QCOW2v3_HDR_FEATURES_COMPATIBLE); + for (i = 0; i < QCOW2_COMPATIBLE_FEATURE_LAST; i++) { + if (bits & ((uint64_t) 1 << i)) + ignore_value(virBitmapSetBit(feat, qcow2CompatibleFeatureArray[i])); + } + + *features = feat; + return 0; +} + + /* Given a file descriptor FD open on PATH, and optionally opened from * a given DIRECTORY, return metadata about that file, assuming it has * the given FORMAT. */ @@ -803,6 +883,14 @@ virStorageFileGetMetadataInternal(const char *path, } } + if (fileTypeInfo[format].getFeatures != NULL && + fileTypeInfo[format].getFeatures(&meta->features, format, buf, len) < 0) + goto cleanup; + + if (format == VIR_STORAGE_FILE_QCOW2 && meta->features && + VIR_STRDUP(meta->compat, "1.1") < 0) + goto cleanup; + done: ret = meta; meta = NULL; @@ -1032,7 +1120,9 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) virStorageFileFreeMetadata(meta->backingMeta); VIR_FREE(meta->backingStore); VIR_FREE(meta->backingStoreRaw); + VIR_FREE(meta->compat); VIR_FREE(meta->directory); + virBitmapFree(meta->features); VIR_FREE(meta); } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index c195c9a..a848fd0 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -24,6 +24,7 @@ #ifndef __VIR_STORAGE_FILE_H__ # define __VIR_STORAGE_FILE_H__ +# include "virbitmap.h" # include "virutil.h" enum virStorageFileFormat { @@ -51,6 +52,15 @@ enum virStorageFileFormat { VIR_ENUM_DECL(virStorageFileFormat); +enum virStorageFileFeature { + VIR_STORAGE_FILE_FEATURE_NONE = -1, + VIR_STORAGE_FILE_FEATURE_LAZY_REFCOUNTS, + + VIR_STORAGE_FILE_FEATURE_LAST +}; + +VIR_ENUM_DECL(virStorageFileFeature); + typedef struct _virStorageFileMetadata virStorageFileMetadata; typedef virStorageFileMetadata *virStorageFileMetadataPtr; struct _virStorageFileMetadata { @@ -62,6 +72,8 @@ struct _virStorageFileMetadata { virStorageFileMetadataPtr backingMeta; unsigned long long capacity; bool encrypted; + virBitmapPtr features; /* bits described by enum virStorageFileFeature */ + char *compat; }; # ifndef DEV_BSIZE -- 1.8.1.5
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
Add <features> and <compat> elements to volume target XML. <compat> is a string which for qcow2 represents the QEMU version it should be compatible with. Valid values are 0.10 and 1.1. 1.1 is implicit if the <features> element is present, otherwise qemu-img default is used. 0.10 can be specified to explicitly create older images after the qemu-img default changes. <features> contains optional features, so far <lazy_refcounts/> is available, which enables caching of reference counters, improving performance for snapshots. --- docs/formatstorage.html.in | 19 +++++++ docs/schemas/Makefile.am | 1 + docs/schemas/storagefilefeatures.rng | 24 +++++++++ docs/schemas/storagevol.rng | 7 +++ libvirt.spec.in | 1 + mingw-libvirt.spec.in | 2 + src/conf/storage_conf.c | 74 +++++++++++++++++++++++++++ src/conf/storage_conf.h | 7 ++- src/libvirt_private.syms | 2 + src/storage/storage_backend_fs.c | 10 ++++ tests/storagevolxml2xmlin/vol-qcow2-1.1.xml | 32 ++++++++++++ tests/storagevolxml2xmlin/vol-qcow2-lazy.xml | 35 +++++++++++++ tests/storagevolxml2xmlout/vol-qcow2-1.1.xml | 33 ++++++++++++ tests/storagevolxml2xmlout/vol-qcow2-lazy.xml | 35 +++++++++++++ tests/storagevolxml2xmltest.c | 2 + 15 files changed, 282 insertions(+), 2 deletions(-) create mode 100644 docs/schemas/storagefilefeatures.rng create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-1.1.xml create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-lazy.xml create mode 100644 tests/storagevolxml2xmlout/vol-qcow2-1.1.xml create mode 100644 tests/storagevolxml2xmlout/vol-qcow2-lazy.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 1a45915..30a07f4 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -334,6 +334,10 @@ <mode>0744</mode> <label>virt_image_t</label> </permissions> + <compat>1.1</compat> + <features> + <lazy_refcounts/> + </features> </target></pre> <dl> @@ -362,6 +366,21 @@ contains the MAC (eg SELinux) label string. <span class="since">Since 0.4.1</span> </dd> + <dt><code>compat</code></dt> + <dd>Specify compatibility level. So far, this is only used for + <code>type='qcow2'</code> volumes. Valid values are <code>0.10</code> + and <code>1.1</code> so far, specifying QEMU version the images should + be compatible with. If the <code>feature</code> element is present, + 1.1 is used. If omitted, qemu-img default is used. + <span class="since">Since 1.0.6</span> + </dd> + <dt><code>features</code></dt> + <dd>Format-specific features. Only used for <code>qcow2</code> now. + Valid sub-elements are: + <code><lazy_refcounts/></code> - allow delayed reference + counter updates. + <span class="since">Since 1.0.6</span> + </dd> </dl> <h3><a name="StorageVolBacking">Backing store elements</a></h3> diff --git a/docs/schemas/Makefile.am b/docs/schemas/Makefile.am index 8da2c67..47d1941 100644 --- a/docs/schemas/Makefile.am +++ b/docs/schemas/Makefile.am @@ -28,6 +28,7 @@ schema_DATA = \ nwfilter.rng \ secret.rng \ storageencryption.rng \ + storagefilefeatures.rng \ storagepool.rng \ storagevol.rng diff --git a/docs/schemas/storagefilefeatures.rng b/docs/schemas/storagefilefeatures.rng new file mode 100644 index 0000000..96a1829 --- /dev/null +++ b/docs/schemas/storagefilefeatures.rng @@ -0,0 +1,24 @@ +<?xml version="1.0"?> +<!-- A Relax NG schema for the libvirt volume features XML format --> +<grammar xmlns="http://relaxng.org/ns/structure/1.0" + datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> + + <define name='compat'> + <element name='compat'> + <data type='string'> + <param name='pattern'>[^,]+</param> + </data> + </element> + </define> + <define name='fileFormatFeatures'> + <element name='features'> + <interleave> + <optional> + <element name='lazy_refcounts'> + <empty/> + </element> + </optional> + </interleave> + </element> + </define> +</grammar> diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index 4649d91..8bc5907 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -8,6 +8,7 @@ </start> <include href='storageencryption.rng'/> + <include href='storagefilefeatures.rng'/> <define name='vol'> @@ -113,6 +114,12 @@ <optional> <ref name='encryption'/> </optional> + <optional> + <ref name='compat'/> + </optional> + <optional> + <ref name='fileFormatFeatures'/> + </optional> </element> </define> diff --git a/libvirt.spec.in b/libvirt.spec.in index 8d43e6d..0bfda11 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -2002,6 +2002,7 @@ fi %{_datadir}/libvirt/schemas/nwfilter.rng %{_datadir}/libvirt/schemas/secret.rng %{_datadir}/libvirt/schemas/storageencryption.rng +%{_datadir}/libvirt/schemas/storagefilefeatures.rng %{_datadir}/libvirt/schemas/storagepool.rng %{_datadir}/libvirt/schemas/storagevol.rng diff --git a/mingw-libvirt.spec.in b/mingw-libvirt.spec.in index 9208329..aa39231 100644 --- a/mingw-libvirt.spec.in +++ b/mingw-libvirt.spec.in @@ -213,6 +213,7 @@ rm -rf $RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh %{mingw32_datadir}/libvirt/schemas/nwfilter.rng %{mingw32_datadir}/libvirt/schemas/secret.rng %{mingw32_datadir}/libvirt/schemas/storageencryption.rng +%{mingw32_datadir}/libvirt/schemas/storagefilefeatures.rng %{mingw32_datadir}/libvirt/schemas/storagepool.rng %{mingw32_datadir}/libvirt/schemas/storagevol.rng %dir %{mingw32_datadir}/libvirt/api/ @@ -273,6 +274,7 @@ rm -rf $RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh %{mingw64_datadir}/libvirt/schemas/nwfilter.rng %{mingw64_datadir}/libvirt/schemas/secret.rng %{mingw64_datadir}/libvirt/schemas/storageencryption.rng +%{mingw64_datadir}/libvirt/schemas/storagefilefeatures.rng %{mingw64_datadir}/libvirt/schemas/storagepool.rng %{mingw64_datadir}/libvirt/schemas/storagevol.rng %dir %{mingw64_datadir}/libvirt/api/ diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index c58b728..35ad502 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -97,6 +97,8 @@ VIR_ENUM_IMPL(virStoragePoolSourceAdapterType, typedef const char *(*virStorageVolFormatToString)(int format); typedef int (*virStorageVolFormatFromString)(const char *format); +typedef const char *(*virStorageVolFeatureToString)(int feature); +typedef int (*virStorageVolFeatureFromString)(const char *feature); typedef const char *(*virStoragePoolFormatToString)(int format); typedef int (*virStoragePoolFormatFromString)(const char *format); @@ -107,6 +109,9 @@ struct _virStorageVolOptions { int defaultFormat; virStorageVolFormatToString formatToString; virStorageVolFormatFromString formatFromString; + virStorageVolFeatureToString featureToString; + virStorageVolFeatureFromString featureFromString; + int featureLast; }; /* Flags to indicate mandatory components in the pool source */ @@ -161,6 +166,9 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { .defaultFormat = VIR_STORAGE_FILE_RAW, .formatFromString = virStorageVolumeFormatFromString, .formatToString = virStorageFileFormatTypeToString, + .featureFromString = virStorageFileFeatureTypeFromString, + .featureToString = virStorageFileFeatureTypeToString, + .featureLast = VIR_STORAGE_FILE_FEATURE_LAST, }, }, {.poolType = VIR_STORAGE_POOL_FS, @@ -174,6 +182,9 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { .defaultFormat = VIR_STORAGE_FILE_RAW, .formatFromString = virStorageVolumeFormatFromString, .formatToString = virStorageFileFormatTypeToString, + .featureFromString = virStorageFileFeatureTypeFromString, + .featureToString = virStorageFileFeatureTypeToString, + .featureLast = VIR_STORAGE_FILE_FEATURE_LAST, }, }, {.poolType = VIR_STORAGE_POOL_NETFS, @@ -188,6 +199,9 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { .defaultFormat = VIR_STORAGE_FILE_RAW, .formatFromString = virStorageVolumeFormatFromString, .formatToString = virStorageFileFormatTypeToString, + .featureFromString = virStorageFileFeatureTypeFromString, + .featureToString = virStorageFileFeatureTypeToString, + .featureLast = VIR_STORAGE_FILE_FEATURE_LAST, }, }, {.poolType = VIR_STORAGE_POOL_ISCSI, @@ -299,6 +313,8 @@ virStorageVolDefFree(virStorageVolDefPtr def) } VIR_FREE(def->source.extents); + VIR_FREE(def->target.compat); + virBitmapFree(def->target.features); VIR_FREE(def->target.path); VIR_FREE(def->target.perms.label); VIR_FREE(def->target.timestamps); @@ -1248,6 +1264,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, char *capacity = NULL; char *unit = NULL; xmlNodePtr node; + xmlNodePtr *nodes = NULL; + int i, n; options = virStorageVolOptionsForPoolType(pool->type); if (options == NULL) @@ -1335,12 +1353,46 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, VIR_FREE(format); } + ret->target.compat = virXPathString("string(./target/compat)", ctxt); + if (ret->target.compat && strchr(ret->target.compat, ',')) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("forbidden characters in 'compat' attribute")); + goto cleanup; + } + + if (virXPathNode("./target/features", ctxt) && options->featureLast) { + if ((n = virXPathNodeSet("./target/features/*", ctxt, &nodes)) < 0) + goto cleanup; + + if (!ret->target.compat && VIR_STRDUP(ret->target.compat, "1.1") < 0) + goto cleanup; + + if (!(ret->target.features = virBitmapNew(options->featureLast))) + goto no_memory; + + for (i = 0; i < n; i++) { + int f = options->featureFromString((const char*)nodes[i]->name); + + if (f < 0) { + virReportError(VIR_ERR_XML_ERROR, _("unsupported feature %s"), + (const char*)nodes[i]->name); + goto cleanup; + } + ignore_value(virBitmapSetBit(ret->target.features, f)); + } + VIR_FREE(nodes); + } + if (virStorageDefParsePerms(ctxt, &ret->backingStore.perms, "./backingStore/permissions", DEFAULT_VOL_PERM_MODE) < 0) goto error; +no_memory: + virReportOOMError(); + cleanup: + VIR_FREE(nodes); VIR_FREE(allocation); VIR_FREE(capacity); VIR_FREE(unit); @@ -1476,6 +1528,28 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAdjustIndent(buf, -4); } + virBufferEscapeString(buf, " <compat>%s</compat>\n", def->compat); + + if (options->featureLast > 0 && def->features) { + int i; + bool b; + bool empty = virBitmapIsAllClear(def->features); + + if (empty) + virBufferAddLit(buf, " <features/>\n"); + else + virBufferAddLit(buf, " <features>\n"); + + for (i = 0; i < options->featureLast; i++) { + ignore_value(virBitmapGetBit(def->features, i, &b)); + if (b) + virBufferAsprintf(buf, " <%s/>\n", + options->featureToString(i)); + } + if (!empty) + virBufferAddLit(buf, " </features>\n"); + } + virBufferAsprintf(buf, " </%s>\n", type); return 0; diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 8e739ff..3af59df 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -26,6 +26,7 @@ # include "internal.h" # include "storage_encryption_conf.h" +# include "virbitmap.h" # include "virthread.h" # include <libxml/tree.h> @@ -84,9 +85,11 @@ struct _virStorageVolTarget { virStoragePerms perms; virStorageTimestampsPtr timestamps; int type; /* only used by disk backend for partition type */ - - /* only used in vol->target, not in vol->backingstore. */ + /* The next three are currently only used in vol->target, + * not in vol->backingStore. */ virStorageEncryptionPtr encryption; + virBitmapPtr features; + char *compat; }; typedef struct _virStorageVolDef virStorageVolDef; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1ea7467..333a00a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1795,6 +1795,8 @@ virSocketAddrSetPort; # util/virstoragefile.h virStorageFileChainLookup; +virStorageFileFeatureTypeFromString; +virStorageFileFeatureTypeToString; virStorageFileFormatTypeFromString; virStorageFileFormatTypeToString; virStorageFileFreeMetadata; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index b1efa50..3598d83 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -150,6 +150,16 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, */ } + virBitmapFree(target->features); + target->features = meta->features; + meta->features = NULL; + + if (meta->compat) { + VIR_FREE(target->compat); + target->compat = meta->compat; + meta->compat = NULL; + } + virStorageFileFreeMetadata(meta); return ret; diff --git a/tests/storagevolxml2xmlin/vol-qcow2-1.1.xml b/tests/storagevolxml2xmlin/vol-qcow2-1.1.xml new file mode 100644 index 0000000..e8df8b3 --- /dev/null +++ b/tests/storagevolxml2xmlin/vol-qcow2-1.1.xml @@ -0,0 +1,32 @@ +<volume> + <name>OtherDemo.img</name> + <key>/var/lib/libvirt/images/OtherDemo.img</key> + <source> + </source> + <capacity unit="G">5</capacity> + <allocation>294912</allocation> + <target> + <path>/var/lib/libvirt/images/OtherDemo.img</path> + <format type='qcow2'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='qcow'> + <secret type='passphrase' uuid='e78d4b51-a2af-485f-b0f5-afca709a80f4'/> + </encryption> + <features/> + </target> + <backingStore> + <path>/var/lib/libvirt/images/BaseDemo.img</path> + <format type='raw'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + </backingStore> +</volume> diff --git a/tests/storagevolxml2xmlin/vol-qcow2-lazy.xml b/tests/storagevolxml2xmlin/vol-qcow2-lazy.xml new file mode 100644 index 0000000..336342a --- /dev/null +++ b/tests/storagevolxml2xmlin/vol-qcow2-lazy.xml @@ -0,0 +1,35 @@ +<volume> + <name>OtherDemo.img</name> + <key>/var/lib/libvirt/images/OtherDemo.img</key> + <source> + </source> + <capacity unit="G">5</capacity> + <allocation>294912</allocation> + <target> + <path>/var/lib/libvirt/images/OtherDemo.img</path> + <format type='qcow2'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='qcow'> + <secret type='passphrase' uuid='e78d4b51-a2af-485f-b0f5-afca709a80f4'/> + </encryption> + <compat>1.1</compat> + <features> + <lazy_refcounts/> + </features> + </target> + <backingStore> + <path>/var/lib/libvirt/images/BaseDemo.img</path> + <format type='raw'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + </backingStore> +</volume> diff --git a/tests/storagevolxml2xmlout/vol-qcow2-1.1.xml b/tests/storagevolxml2xmlout/vol-qcow2-1.1.xml new file mode 100644 index 0000000..454ac11 --- /dev/null +++ b/tests/storagevolxml2xmlout/vol-qcow2-1.1.xml @@ -0,0 +1,33 @@ +<volume> + <name>OtherDemo.img</name> + <key>(null)</key> + <source> + </source> + <capacity unit='bytes'>5368709120</capacity> + <allocation unit='bytes'>294912</allocation> + <target> + <path>/var/lib/libvirt/images/OtherDemo.img</path> + <format type='qcow2'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='qcow'> + <secret type='passphrase' uuid='e78d4b51-a2af-485f-b0f5-afca709a80f4'/> + </encryption> + <compat>1.1</compat> + <features/> + </target> + <backingStore> + <path>/var/lib/libvirt/images/BaseDemo.img</path> + <format type='raw'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + </backingStore> +</volume> diff --git a/tests/storagevolxml2xmlout/vol-qcow2-lazy.xml b/tests/storagevolxml2xmlout/vol-qcow2-lazy.xml new file mode 100644 index 0000000..4e30ede --- /dev/null +++ b/tests/storagevolxml2xmlout/vol-qcow2-lazy.xml @@ -0,0 +1,35 @@ +<volume> + <name>OtherDemo.img</name> + <key>(null)</key> + <source> + </source> + <capacity unit='bytes'>5368709120</capacity> + <allocation unit='bytes'>294912</allocation> + <target> + <path>/var/lib/libvirt/images/OtherDemo.img</path> + <format type='qcow2'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='qcow'> + <secret type='passphrase' uuid='e78d4b51-a2af-485f-b0f5-afca709a80f4'/> + </encryption> + <compat>1.1</compat> + <features> + <lazy_refcounts/> + </features> + </target> + <backingStore> + <path>/var/lib/libvirt/images/BaseDemo.img</path> + <format type='raw'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + </backingStore> +</volume> diff --git a/tests/storagevolxml2xmltest.c b/tests/storagevolxml2xmltest.c index 07c79c1..e87b016 100644 --- a/tests/storagevolxml2xmltest.c +++ b/tests/storagevolxml2xmltest.c @@ -110,6 +110,8 @@ mymain(void) DO_TEST("pool-dir", "vol-file"); DO_TEST("pool-dir", "vol-file-backing"); DO_TEST("pool-dir", "vol-qcow2"); + DO_TEST("pool-dir", "vol-qcow2-1.1"); + DO_TEST("pool-dir", "vol-qcow2-lazy"); DO_TEST("pool-disk", "vol-partition"); DO_TEST("pool-logical", "vol-logical"); DO_TEST("pool-logical", "vol-logical-backing"); -- 1.8.1.5
On 06/10/2013 02:10 PM, Ján Tomko wrote:
Add <features> and <compat> elements to volume target XML.
<compat> is a string which for qcow2 represents the QEMU version it should be compatible with. Valid values are 0.10 and 1.1. 1.1 is implicit if the <features> element is present, otherwise qemu-img default is used. 0.10 can be specified to explicitly create older images after the qemu-img default changes.
Do I understand it correctly that in case there will be next format, we'll use the correct one depending on what features are requested?
<features> contains optional features, so far <lazy_refcounts/> is available, which enables caching of reference counters, improving performance for snapshots. --- docs/formatstorage.html.in | 19 +++++++ docs/schemas/Makefile.am | 1 + docs/schemas/storagefilefeatures.rng | 24 +++++++++ docs/schemas/storagevol.rng | 7 +++ libvirt.spec.in | 1 + mingw-libvirt.spec.in | 2 + src/conf/storage_conf.c | 74 +++++++++++++++++++++++++++ src/conf/storage_conf.h | 7 ++- src/libvirt_private.syms | 2 + src/storage/storage_backend_fs.c | 10 ++++ tests/storagevolxml2xmlin/vol-qcow2-1.1.xml | 32 ++++++++++++ tests/storagevolxml2xmlin/vol-qcow2-lazy.xml | 35 +++++++++++++ tests/storagevolxml2xmlout/vol-qcow2-1.1.xml | 33 ++++++++++++ tests/storagevolxml2xmlout/vol-qcow2-lazy.xml | 35 +++++++++++++ tests/storagevolxml2xmltest.c | 2 + 15 files changed, 282 insertions(+), 2 deletions(-) create mode 100644 docs/schemas/storagefilefeatures.rng create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-1.1.xml create mode 100644 tests/storagevolxml2xmlin/vol-qcow2-lazy.xml create mode 100644 tests/storagevolxml2xmlout/vol-qcow2-1.1.xml create mode 100644 tests/storagevolxml2xmlout/vol-qcow2-lazy.xml
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 1a45915..30a07f4 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -334,6 +334,10 @@ <mode>0744</mode> <label>virt_image_t</label> </permissions> + <compat>1.1</compat> + <features> + <lazy_refcounts/> + </features> </target></pre>
<dl> @@ -362,6 +366,21 @@ contains the MAC (eg SELinux) label string. <span class="since">Since 0.4.1</span> </dd> + <dt><code>compat</code></dt> + <dd>Specify compatibility level. So far, this is only used for + <code>type='qcow2'</code> volumes. Valid values are <code>0.10</code> + and <code>1.1</code> so far, specifying QEMU version the images should + be compatible with. If the <code>feature</code> element is present, + 1.1 is used. If omitted, qemu-img default is used.
If my previous presumption is correct, then it might be worth putting it here, but leaving it like this until next feature comes is OK too.
+ <span class="since">Since 1.0.6</span>
s/6/7/
+ </dd> + <dt><code>features</code></dt> + <dd>Format-specific features. Only used for <code>qcow2</code> now. + Valid sub-elements are: + <code><lazy_refcounts/></code> - allow delayed reference + counter updates.
This would look better if a list were used.
+ <span class="since">Since 1.0.6</span>
s/6/7/
+ </dd> </dl>
<h3><a name="StorageVolBacking">Backing store elements</a></h3> diff --git a/docs/schemas/Makefile.am b/docs/schemas/Makefile.am index 8da2c67..47d1941 100644 --- a/docs/schemas/Makefile.am +++ b/docs/schemas/Makefile.am @@ -28,6 +28,7 @@ schema_DATA = \ nwfilter.rng \ secret.rng \ storageencryption.rng \ + storagefilefeatures.rng \ storagepool.rng \ storagevol.rng
diff --git a/docs/schemas/storagefilefeatures.rng b/docs/schemas/storagefilefeatures.rng new file mode 100644 index 0000000..96a1829 --- /dev/null +++ b/docs/schemas/storagefilefeatures.rng @@ -0,0 +1,24 @@ +<?xml version="1.0"?> +<!-- A Relax NG schema for the libvirt volume features XML format --> +<grammar xmlns="http://relaxng.org/ns/structure/1.0" + datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> + + <define name='compat'> + <element name='compat'> + <data type='string'> + <param name='pattern'>[^,]+</param>
I'd change this to "[0-9]+\.[0-9]+"
+ </data> + </element> + </define> + <define name='fileFormatFeatures'> + <element name='features'> + <interleave> + <optional> + <element name='lazy_refcounts'> + <empty/> + </element> + </optional> + </interleave> + </element> + </define> +</grammar>
[...]
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index c58b728..35ad502 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -97,6 +97,8 @@ VIR_ENUM_IMPL(virStoragePoolSourceAdapterType,
typedef const char *(*virStorageVolFormatToString)(int format); typedef int (*virStorageVolFormatFromString)(const char *format); +typedef const char *(*virStorageVolFeatureToString)(int feature); +typedef int (*virStorageVolFeatureFromString)(const char *feature);
typedef const char *(*virStoragePoolFormatToString)(int format); typedef int (*virStoragePoolFormatFromString)(const char *format); @@ -107,6 +109,9 @@ struct _virStorageVolOptions { int defaultFormat; virStorageVolFormatToString formatToString; virStorageVolFormatFromString formatFromString; + virStorageVolFeatureToString featureToString; + virStorageVolFeatureFromString featureFromString; + int featureLast;
Will there be anything else than VIR_STORAGE_FILE_FEATURE_LAST ???
};
/* Flags to indicate mandatory components in the pool source */
[...]
@@ -1335,12 +1353,46 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, VIR_FREE(format); }
+ ret->target.compat = virXPathString("string(./target/compat)", ctxt); + if (ret->target.compat && strchr(ret->target.compat, ',')) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("forbidden characters in 'compat' attribute"));
We should check for: - available (known) compat values (very strict, not forward-compatible) - the same pattern as in RelaxNG scheme (strict, forward-compatible, ugly code) - or allow use of any string, but escape it properly when handed to qemu, for example like this: qemu-img create -f qcow2 -o compat=0.10,,preallocation=metadata \ delete.me 1M Formatting 'delete.me', fmt=qcow2 size=1048576 compat='0.10,preallocation=metadata' encryption=off cluster_size=65536 lazy_refcounts=off Invalid compatibility level: '0.10,preallocation=metadata' qemu-img: delete.me: error while creating qcow2: Invalid argument Which would make it forward-compatible, but since we have to make sure we report potential qemu-img errors back properly, I don't see a reason why there should be a problem with that. Personally, I'm for the second option.
+ goto cleanup; + } + + if (virXPathNode("./target/features", ctxt) && options->featureLast) { + if ((n = virXPathNodeSet("./target/features/*", ctxt, &nodes)) < 0) + goto cleanup; + + if (!ret->target.compat && VIR_STRDUP(ret->target.compat, "1.1") < 0) + goto cleanup; + + if (!(ret->target.features = virBitmapNew(options->featureLast))) + goto no_memory; +
Can't wait for Michal's unified OOM reporting to hit upstream, this looks weird (cleanup/no_memory).
+ for (i = 0; i < n; i++) { + int f = options->featureFromString((const char*)nodes[i]->name); + + if (f < 0) { + virReportError(VIR_ERR_XML_ERROR, _("unsupported feature %s"), + (const char*)nodes[i]->name); + goto cleanup; + } + ignore_value(virBitmapSetBit(ret->target.features, f)); + } + VIR_FREE(nodes); + } +
All the "goto cleanup;" statements in this hunk should be "goto error;".
if (virStorageDefParsePerms(ctxt, &ret->backingStore.perms, "./backingStore/permissions", DEFAULT_VOL_PERM_MODE) < 0) goto error;
+no_memory: + virReportOOMError(); +
You probably don't want this to be reported even when everything went OK, you probably wanted this before the "error:" label.
cleanup: + VIR_FREE(nodes); VIR_FREE(allocation); VIR_FREE(capacity); VIR_FREE(unit);
You should also check whether the features specified are correctly supported with the compat level (if specified), so we report nicer error than "internal error". No need to check what qemu-img supports, since we know what compat level is needed for what features. Other than these, this looks fine, but seeing the fixes in another version would be better. Martin
Add -o compat= and -o lazy_refcounts options for qemu-img. --- src/storage/storage_backend.c | 36 +++++++++++++++++++++++-- tests/storagevolxml2argvdata/qcow2-1.1.argv | 1 + tests/storagevolxml2argvdata/qcow2-lazy.argv | 1 + tests/storagevolxml2argvdata/vol-qcow2-1.1.xml | 32 ++++++++++++++++++++++ tests/storagevolxml2argvdata/vol-qcow2-lazy.xml | 35 ++++++++++++++++++++++++ tests/storagevolxml2argvtest.c | 2 ++ 6 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 tests/storagevolxml2argvdata/qcow2-1.1.argv create mode 100644 tests/storagevolxml2argvdata/qcow2-lazy.argv create mode 100644 tests/storagevolxml2argvdata/vol-qcow2-1.1.xml create mode 100644 tests/storagevolxml2argvdata/vol-qcow2-lazy.xml diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 0abc3f3..22cc051 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -655,9 +655,15 @@ static int virStorageBackendCreateQemuImgOpts(char **opts, const char *backingType, bool encryption, - bool preallocate) + bool preallocate, + int format, + const char *compat, + virBitmapPtr features) { virBuffer buf = VIR_BUFFER_INITIALIZER; + bool b; + int i; + if (backingType) virBufferAsprintf(&buf, "backing_fmt=%s,", backingType); if (encryption) @@ -665,6 +671,19 @@ virStorageBackendCreateQemuImgOpts(char **opts, if (preallocate) virBufferAddLit(&buf, "preallocation=metadata,"); + if (compat) + virBufferAsprintf(&buf, "compat=%s,", compat); + if (features && format == VIR_STORAGE_FILE_QCOW2) { + if (!compat) + virBufferAddLit(&buf, "compat=1.1,"); + for (i = 0; i < VIR_STORAGE_FILE_FEATURE_LAST; i++) { + ignore_value(virBitmapGetBit(features, i, &b)); + if (b) + virBufferAsprintf(&buf, "%s,", + virStorageFileFeatureTypeToString(i)); + } + } + virBufferTrim(&buf, ",", -1); if (virBufferError(&buf)) { @@ -717,6 +736,16 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, _("metadata preallocation only available with qcow2")); return NULL; } + if (vol->target.compat && vol->target.format != VIR_STORAGE_FILE_QCOW2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("compatibility option only available with qcow2")); + return NULL; + } + if (vol->target.features && vol->target.format != VIR_STORAGE_FILE_QCOW2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("format features only available with qcow2")); + return NULL; + } if (inputvol) { if (!(inputPath = inputvol->target.path)) { @@ -838,7 +867,10 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, if (imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS) { if (virStorageBackendCreateQemuImgOpts(&opts, backing ? backingType : NULL, - do_encryption, preallocate)) + do_encryption, preallocate, + vol->target.format, + vol->target.compat, + vol->target.features)) return NULL; if (opts) virCommandAddArgList(cmd, "-o", opts, NULL); diff --git a/tests/storagevolxml2argvdata/qcow2-1.1.argv b/tests/storagevolxml2argvdata/qcow2-1.1.argv new file mode 100644 index 0000000..797499f --- /dev/null +++ b/tests/storagevolxml2argvdata/qcow2-1.1.argv @@ -0,0 +1 @@ +qemu-img create -f qcow2 -b /dev/null -o backing_fmt=raw,encryption=on,compat=1.1 /var/lib/libvirt/images/OtherDemo.img 5242880K diff --git a/tests/storagevolxml2argvdata/qcow2-lazy.argv b/tests/storagevolxml2argvdata/qcow2-lazy.argv new file mode 100644 index 0000000..9160d47 --- /dev/null +++ b/tests/storagevolxml2argvdata/qcow2-lazy.argv @@ -0,0 +1 @@ +qemu-img create -f qcow2 -b /dev/null -o backing_fmt=raw,encryption=on,compat=1.1,lazy_refcounts /var/lib/libvirt/images/OtherDemo.img 5242880K diff --git a/tests/storagevolxml2argvdata/vol-qcow2-1.1.xml b/tests/storagevolxml2argvdata/vol-qcow2-1.1.xml new file mode 100644 index 0000000..696e1e0 --- /dev/null +++ b/tests/storagevolxml2argvdata/vol-qcow2-1.1.xml @@ -0,0 +1,32 @@ +<volume> + <name>OtherDemo.img</name> + <key>/var/lib/libvirt/images/OtherDemo.img</key> + <source> + </source> + <capacity unit="G">5</capacity> + <allocation>294912</allocation> + <target> + <path>/var/lib/libvirt/images/OtherDemo.img</path> + <format type='qcow2'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='qcow'> + <secret type='passphrase' uuid='e78d4b51-a2af-485f-b0f5-afca709a80f4'/> + </encryption> + <features/> + </target> + <backingStore> + <path>/dev/null</path> + <format type='raw'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + </backingStore> +</volume> diff --git a/tests/storagevolxml2argvdata/vol-qcow2-lazy.xml b/tests/storagevolxml2argvdata/vol-qcow2-lazy.xml new file mode 100644 index 0000000..c1d7875 --- /dev/null +++ b/tests/storagevolxml2argvdata/vol-qcow2-lazy.xml @@ -0,0 +1,35 @@ +<volume> + <name>OtherDemo.img</name> + <key>/var/lib/libvirt/images/OtherDemo.img</key> + <source> + </source> + <capacity unit="G">5</capacity> + <allocation>294912</allocation> + <target> + <path>/var/lib/libvirt/images/OtherDemo.img</path> + <format type='qcow2'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + <encryption format='qcow'> + <secret type='passphrase' uuid='e78d4b51-a2af-485f-b0f5-afca709a80f4'/> + </encryption> + <compat>1.1</compat> + <features> + <lazy_refcounts/> + </features> + </target> + <backingStore> + <path>/dev/null</path> + <format type='raw'/> + <permissions> + <mode>0644</mode> + <owner>0</owner> + <group>0</group> + <label>unconfined_u:object_r:virt_image_t:s0</label> + </permissions> + </backingStore> +</volume> diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index 92ab2f2..25ff5a7 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -188,6 +188,8 @@ mymain(void) "qcow2-nobacking-none", 0, FMT_NONE); DO_TEST(false, "pool-dir", "vol-qcow2-nobacking", "vol-file", "qcow2-nobacking-convert-none", 0, FMT_NONE); + DO_TEST(false, "pool-dir", "vol-qcow2-lazy", NULL, "qcow2-lazy", 0, FMT_OPTIONS); + DO_TEST(false, "pool-dir", "vol-qcow2-1.1", NULL, "qcow2-1.1", 0, FMT_OPTIONS); return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.8.1.5
On 06/10/2013 02:10 PM, Ján Tomko wrote:
Add -o compat= and -o lazy_refcounts options for qemu-img. --- src/storage/storage_backend.c | 36 +++++++++++++++++++++++-- tests/storagevolxml2argvdata/qcow2-1.1.argv | 1 + tests/storagevolxml2argvdata/qcow2-lazy.argv | 1 + tests/storagevolxml2argvdata/vol-qcow2-1.1.xml | 32 ++++++++++++++++++++++ tests/storagevolxml2argvdata/vol-qcow2-lazy.xml | 35 ++++++++++++++++++++++++ tests/storagevolxml2argvtest.c | 2 ++ 6 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 tests/storagevolxml2argvdata/qcow2-1.1.argv create mode 100644 tests/storagevolxml2argvdata/qcow2-lazy.argv create mode 100644 tests/storagevolxml2argvdata/vol-qcow2-1.1.xml create mode 100644 tests/storagevolxml2argvdata/vol-qcow2-lazy.xml
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 0abc3f3..22cc051 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -655,9 +655,15 @@ static int virStorageBackendCreateQemuImgOpts(char **opts, const char *backingType, bool encryption, - bool preallocate) + bool preallocate, + int format, + const char *compat, + virBitmapPtr features) { virBuffer buf = VIR_BUFFER_INITIALIZER; + bool b; + int i; + if (backingType) virBufferAsprintf(&buf, "backing_fmt=%s,", backingType); if (encryption) @@ -665,6 +671,19 @@ virStorageBackendCreateQemuImgOpts(char **opts, if (preallocate) virBufferAddLit(&buf, "preallocation=metadata,");
+ if (compat) + virBufferAsprintf(&buf, "compat=%s,", compat); + if (features && format == VIR_STORAGE_FILE_QCOW2) { + if (!compat) + virBufferAddLit(&buf, "compat=1.1,");
This is not needed, or is it? Not looking at the tests until 3/7 is refactored, but other that that, this one looks OK. Martin
Just to reduce the indentation levels. Remove the unneeded NULL check for disk->file, as virBufferEscapeString doesn't print anything with NULL arguments. --- src/conf/snapshot_conf.c | 53 ++++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index c6b97d6..b500111 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -548,6 +548,33 @@ cleanup: return ret; } +static void +virDomainSnapshotDiskDefFormat(virBufferPtr buf, + virDomainSnapshotDiskDefPtr disk) +{ + if (!disk->name) + return; + + virBufferEscapeString(buf, " <disk name='%s'", disk->name); + if (disk->snapshot > 0) + virBufferAsprintf(buf, " snapshot='%s'", + virDomainSnapshotLocationTypeToString(disk->snapshot)); + if (!disk->file && disk->format == 0) { + virBufferAddLit(buf, "/>\n"); + return; + } + + virBufferAddLit(buf, ">\n"); + + virBufferAdjustIndent(buf, 6); + if (disk->format > 0) + virBufferEscapeString(buf, "<driver type='%s'/>\n", + virStorageFileFormatTypeToString(disk->format)); + virBufferEscapeString(buf, "<source file='%s'/>\n", disk->file); + virBufferAdjustIndent(buf, -6); + virBufferAddLit(buf, " </disk>\n"); +} + char *virDomainSnapshotDefFormat(const char *domain_uuid, virDomainSnapshotDefPtr def, unsigned int flags, @@ -583,30 +610,8 @@ char *virDomainSnapshotDefFormat(const char *domain_uuid, } if (def->ndisks) { virBufferAddLit(&buf, " <disks>\n"); - for (i = 0; i < def->ndisks; i++) { - virDomainSnapshotDiskDefPtr disk = &def->disks[i]; - - if (!disk->name) - continue; - - virBufferEscapeString(&buf, " <disk name='%s'", disk->name); - if (disk->snapshot) - virBufferAsprintf(&buf, " snapshot='%s'", - virDomainSnapshotLocationTypeToString(disk->snapshot)); - if (disk->file || disk->format > 0) { - virBufferAddLit(&buf, ">\n"); - if (disk->format > 0) - virBufferEscapeString(&buf, " <driver type='%s'/>\n", - virStorageFileFormatTypeToString( - disk->format)); - if (disk->file) - virBufferEscapeString(&buf, " <source file='%s'/>\n", - disk->file); - virBufferAddLit(&buf, " </disk>\n"); - } else { - virBufferAddLit(&buf, "/>\n"); - } - } + for (i = 0; i < def->ndisks; i++) + virDomainSnapshotDiskDefFormat(&buf, &def->disks[i]); virBufferAddLit(&buf, " </disks>\n"); } if (def->dom) { -- 1.8.1.5
On 06/10/2013 02:10 PM, Ján Tomko wrote:
Just to reduce the indentation levels. Remove the unneeded NULL check for disk->file, as virBufferEscapeString doesn't print anything with NULL arguments. --- src/conf/snapshot_conf.c | 53 ++++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 24 deletions(-)
ACK, Martin
On 06/14/2013 11:38 AM, Martin Kletzander wrote:
On 06/10/2013 02:10 PM, Ján Tomko wrote:
Just to reduce the indentation levels. Remove the unneeded NULL check for disk->file, as virBufferEscapeString doesn't print anything with NULL arguments. --- src/conf/snapshot_conf.c | 53 ++++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 24 deletions(-)
ACK,
Martin
Thanks, I've pushed it now along with 1/7. Jan
--- docs/schemas/domainsnapshot.rng | 7 +++ src/conf/snapshot_conf.c | 74 ++++++++++++++++++++++++ src/conf/snapshot_conf.h | 2 + tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 5 ++ tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 4 ++ 5 files changed, 92 insertions(+) diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 7b46df1..63c8511 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -6,6 +6,7 @@ </start> <include href='domaincommon.rng'/> + <include href='storagefilefeatures.rng'/> <define name='domainsnapshot'> <element name='domainsnapshot'> @@ -144,6 +145,12 @@ <empty/> </element> </optional> + <optional> + <ref name='compat'/> + </optional> + <optional> + <ref name='fileFormatFeatures'/> + </optional> </interleave> </group> </choice> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index b500111..927315a 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -82,6 +82,9 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) { VIR_FREE(disk->name); VIR_FREE(disk->file); + VIR_FREE(disk->compat); + virBitmapFree(disk->features); + disk->features = NULL; } void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) @@ -103,6 +106,38 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) } static int +virDomainSnapshotDiskFeaturesDefParse(virDomainSnapshotDiskDefPtr def, + xmlNodePtr node) +{ + xmlNodePtr cur; + int f, n = 0; + + if (!(def->features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST))) { + virReportOOMError(); + return -1; + } + + for (cur = node->children; cur; cur = cur->next) { + if (cur->type != XML_ELEMENT_NODE) + continue; + + f = virStorageFileFeatureTypeFromString((const char*) cur->name); + + if (f < 0) { + virReportError(VIR_ERR_XML_ERROR, _("unsupported feature %s"), + (const char*) cur->name); + return -1; + } + ignore_value(virBitmapSetBit(def->features, f)); + n++; + + } + if (n > 0 && !def->compat && VIR_STRDUP(def->compat, "1.1") < 0) + return -1; + return 0; +} + +static int virDomainSnapshotDiskDefParseXML(xmlNodePtr node, virDomainSnapshotDiskDefPtr def) { @@ -146,6 +181,17 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, goto cleanup; } VIR_FREE(driver); + } else if (xmlStrEqual(cur->name, BAD_CAST "compat")) { + if (VIR_STRDUP(def->compat, (const char*)cur->content) < 0) + goto cleanup; + if (def->compat && strchr(def->compat, ',')) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("forbidden characters in 'compat' attribute")); + goto cleanup; + } + } else if (xmlStrEqual(cur->name, BAD_CAST "features")) { + if (virDomainSnapshotDiskFeaturesDefParse(def, cur) < 0) + goto cleanup; } } cur = cur->next; @@ -549,6 +595,29 @@ cleanup: } static void +virDomainSnapshotDiskFeaturesDefFormat(virBufferPtr buf, + virBitmapPtr features) +{ + int i; + + if (virBitmapIsAllClear(features)) { + virBufferAddLit(buf, "<features/>\n"); + return; + } + + virBufferAddLit(buf, "<features>\n"); + for (i = 0; i < VIR_STORAGE_FILE_FEATURE_LAST; i++) { + bool b; + + ignore_value(virBitmapGetBit(features, i, &b)); + if (b) + virBufferAsprintf(buf, " <%s/>\n", + virStorageFileFeatureTypeToString(i)); + } + virBufferAddLit(buf, "</features>\n"); +} + +static void virDomainSnapshotDiskDefFormat(virBufferPtr buf, virDomainSnapshotDiskDefPtr disk) { @@ -571,6 +640,11 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<driver type='%s'/>\n", virStorageFileFormatTypeToString(disk->format)); virBufferEscapeString(buf, "<source file='%s'/>\n", disk->file); + virBufferEscapeString(buf, "<compat>%s</compat>\n", disk->compat); + + if (disk->features) + virDomainSnapshotDiskFeaturesDefFormat(buf, disk->features); + virBufferAdjustIndent(buf, -6); virBufferAddLit(buf, " </disk>\n"); } diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 6d625a7..2be5319 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -53,6 +53,8 @@ struct _virDomainSnapshotDiskDef { int snapshot; /* enum virDomainSnapshotLocation */ char *file; /* new source file when snapshot is external */ int format; /* enum virStorageFileFormat */ + char *compat; + virBitmapPtr features; }; /* Stores the complete snapshot metadata */ diff --git a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml index ee6b46a..79895d7 100644 --- a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml @@ -11,6 +11,11 @@ </disk> <disk name='hde' snapshot='external'> <source file='/path/to/new'/> + <driver type='qcow2'/> + <compat>1.1</compat> + <features> + <lazy_refcounts/> + </features> </disk> </disks> </domainsnapshot> diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml index 5f42bf5..4c3f8ad 100644 --- a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml @@ -18,6 +18,10 @@ <disk name='hde' snapshot='external'> <driver type='qcow2'/> <source file='/path/to/new'/> + <compat>1.1</compat> + <features> + <lazy_refcounts/> + </features> </disk> <disk name='hdf' snapshot='external'> <driver type='qcow2'/> -- 1.8.1.5
For live external snapshots, the image creation is done by QEMU and we can't pass the needed options. --- docs/formatsnapshot.html.in | 5 ++++ src/qemu/qemu_driver.c | 72 +++++++++++++++++++++++++++++++++++---------- 2 files changed, 62 insertions(+), 15 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 76689cb..e160455 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -170,6 +170,11 @@ snapshots, the original file name becomes the read-only snapshot, and the new file name contains the read-write delta of all disk changes since the snapshot. + + An optional <code>compat</code> and <code>features</code> + elements may be specified to control format-specific + features. So far only for qcow2 and only for non-live + external snapshots. </dd> </dl> </dd> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 51952c9..09805da 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11008,6 +11008,44 @@ qemuDomainSnapshotCreateInactiveInternal(virQEMUDriverPtr driver, return qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-c", false); } +static int +qemuDomainSnapshotCreateQemuImgOpts(char **opts, + const char *backing_file, + const char *backing_fmt, + const char *compat, + int format, + virBitmapPtr features) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + bool b; + int i; + + virBufferEscapeString(&buf, "backing_file=%s,", backing_file); + virBufferEscapeString(&buf, "backing_fmt=%s,", backing_fmt); + virBufferEscapeString(&buf, "compat=%s,", compat); + if (features && format == VIR_STORAGE_FILE_QCOW2) { + if (!compat) + virBufferAddLit(&buf, "compat=1.1,"); + for (i = 0; i < VIR_STORAGE_FILE_FEATURE_LAST; i++) { + ignore_value(virBitmapGetBit(features, i, &b)); + if (b) + virBufferAsprintf(&buf, "%s,", + virStorageFileFeatureTypeToString(i)); + } + } + + virBufferTrim(&buf, ",", -1); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + + *opts = virBufferContentAndReset(&buf); + return 0; +} + /* The domain is expected to be locked and inactive. */ static int qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, @@ -11023,6 +11061,8 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, virBitmapPtr created = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; + char *opts = NULL; + const char *backing_fmt = NULL; if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) goto cleanup; @@ -11053,23 +11093,25 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, NULL))) goto cleanup; - if (defdisk->format > 0) { - /* adds cmd line arg: backing_file=/path/to/backing/file,backing_fmd=format */ - virCommandAddArgFormat(cmd, "backing_file=%s,backing_fmt=%s", - defdisk->src, - virStorageFileFormatTypeToString(defdisk->format)); - } else { - if (!cfg->allowDiskFormatProbing) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown image format of '%s' and " - "format probing is disabled"), - defdisk->src); - goto cleanup; - } - /* adds cmd line arg: backing_file=/path/to/backing/file */ - virCommandAddArgFormat(cmd, "backing_file=%s", defdisk->src); + if (defdisk->format > 0) { + backing_fmt = virStorageFileFormatTypeToString(defdisk->format); + } else if (!cfg->allowDiskFormatProbing) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown image format of '%s' and " + "format probing is disabled"), + defdisk->src); + goto cleanup; } + if (qemuDomainSnapshotCreateQemuImgOpts(&opts, defdisk->src, + backing_fmt, + snapdisk->compat, + snapdisk->format, + snapdisk->features) < 0) + goto cleanup; + + virCommandAddArg(cmd, opts); + VIR_FREE(opts); /* adds cmd line args: /path/to/target/file */ virCommandAddArg(cmd, snapdisk->file); -- 1.8.1.5
participants (2)
-
Ján Tomko -
Martin Kletzander