[libvirt] [PATCHv1 00/13] Support qcow2 features in external snapshots
https://bugzilla.redhat.com/show_bug.cgi?id=980327 First ten patches contain no functional change. Patches 11 and 12 potentially change the default qcow2 compatibility level. The last patch contains the proposed XML. Ján Tomko (13): Visually separate snapshot disk subelements Remove feature formating funcs from pool-specific options Separate out virStorageFeatureParse Separate virStorageFeatureFormat Split out storage format 'compat' attribute sanity check Use XPath when parsing snapshot disk definition Rename virStorageBackendCreateQemuImgCmd Introduce struct _virStorageBackendQemuImgInfo Split out qemu-img command generation Remove overengineered loop Use qemu-img to precreate the qcow2 disks Rework qemu-img command generation for inactive external snapshots Add qcow2 features to snapshot XML docs/formatsnapshot.html.in | 50 ++- docs/schemas/domainsnapshot.rng | 10 +- po/POTFILES.in | 1 + src/Makefile.am | 5 + src/conf/snapshot_conf.c | 58 ++-- src/conf/storage_conf.c | 83 +---- src/conf/storage_feature_conf.c | 87 +++++ src/conf/storage_feature_conf.h | 25 ++ src/qemu/qemu_driver.c | 57 ++-- src/storage/storage_backend.c | 376 +++++++++++++-------- src/storage/storage_backend.h | 19 +- src/storage/storage_driver.c | 27 ++ src/storage/storage_driver.h | 4 + src/util/virstoragefile.c | 29 ++ src/util/virstoragefile.h | 1 + .../disk_snapshot_features.xml | 30 ++ .../disk_snapshot_features.xml | 30 ++ tests/domainsnapshotxml2xmltest.c | 2 + tests/storagevolxml2argvtest.c | 5 +- 19 files changed, 589 insertions(+), 310 deletions(-) create mode 100644 src/conf/storage_feature_conf.c create mode 100644 src/conf/storage_feature_conf.h create mode 100644 tests/domainsnapshotxml2xmlin/disk_snapshot_features.xml create mode 100644 tests/domainsnapshotxml2xmlout/disk_snapshot_features.xml -- 2.0.5
Create four smaller blocks of texts instead of one large one. Smaller blocks are user-friendlier and could potentially make one's life happier. --- docs/formatsnapshot.html.in | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 4f7b7b2..c3ab516 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -156,22 +156,31 @@ require that if specified, the snapshot mode must not override any snapshot mode attached to the corresponding domain disk, while others like qemu allow this field to - override the domain default. If the snapshot mode is - external (whether specified or inherited), then there is - an optional sub-element <code>source</code>, with an - attribute <code>file</code> giving the name, and an - optional sub-element <code>driver</code>, with an - attribute <code>type</code> giving the driver type (such - as qcow2), of the new file created by the external - snapshot of the new file. If <code>source</code> is not - given and the disk is backed by a local image file (not - a block device or remote storage), a file name is - generated that consists of the existing file name - with anything after the trailing dot replaced by the - snapshot name. Remember that with external - 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. + override the domain default. + + <dl> + <dt><code>source</code></dt> + <dd>If the snapshot mode is external (whether specified + or inherited), then there is an optional sub-element + <code>source</code>, with an attribute <code>file</code> + giving the name of the new file. + If <code>source</code> is not + given and the disk is backed by a local image file (not + a block device or remote storage), a file name is + generated that consists of the existing file name + with anything after the trailing dot replaced by the + snapshot name. Remember that with external + 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. + </dd> + <dt><code>driver</code></dt> + <dd>An optional sub-element <code>driver</code>, + with an attribute <code>type</code> giving the driver type (such + as qcow2), of the new file created by the external + snapshot of the new file. + </dd> + </dl> <span class="since">Since 1.2.2</span> the <code>disk</code> element supports an optional attribute <code>type</code> if the -- 2.0.5
On Fri, Apr 10, 2015 at 14:58:53 +0200, Ján Tomko wrote:
Create four smaller blocks of texts instead of one large one. Smaller blocks are user-friendlier and could potentially make one's life happier.
I'd drop the last sentence before pushing.
--- docs/formatsnapshot.html.in | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 4f7b7b2..c3ab516 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -156,22 +156,31 @@ require that if specified, the snapshot mode must not override any snapshot mode attached to the corresponding domain disk, while others like qemu allow this field to - override the domain default. If the snapshot mode is - external (whether specified or inherited), then there is - an optional sub-element <code>source</code>, with an - attribute <code>file</code> giving the name, and an - optional sub-element <code>driver</code>, with an - attribute <code>type</code> giving the driver type (such - as qcow2), of the new file created by the external - snapshot of the new file. If <code>source</code> is not - given and the disk is backed by a local image file (not - a block device or remote storage), a file name is - generated that consists of the existing file name - with anything after the trailing dot replaced by the - snapshot name. Remember that with external - 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. + override the domain default. + + <dl> + <dt><code>source</code></dt> + <dd>If the snapshot mode is external (whether specified + or inherited), then there is an optional sub-element + <code>source</code>, with an attribute <code>file</code> + giving the name of the new file. + If <code>source</code> is not + given and the disk is backed by a local image file (not + a block device or remote storage), a file name is + generated that consists of the existing file name + with anything after the trailing dot replaced by the + snapshot name. Remember that with external + 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.
Hmmm, I should fix this one later. We now support a full disk source subXML here along with the "file" attribute. I somehow forgot to document that :/
+ </dd> + <dt><code>driver</code></dt> + <dd>An optional sub-element <code>driver</code>, + with an attribute <code>type</code> giving the driver type (such + as qcow2), of the new file created by the external + snapshot of the new file. + </dd> + </dl>
<span class="since">Since 1.2.2</span> the <code>disk</code> element supports an optional attribute <code>type</code> if the
ACK Peter
We only have one formatting function for the features. --- src/conf/storage_conf.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 987bb73..339f08f 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -104,8 +104,6 @@ VIR_ENUM_IMPL(virStoragePoolSourceAdapter, 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); @@ -116,8 +114,6 @@ struct _virStorageVolOptions { int defaultFormat; virStorageVolFormatToString formatToString; virStorageVolFormatFromString formatFromString; - virStorageVolFeatureToString featureToString; - virStorageVolFeatureFromString featureFromString; }; /* Flags to indicate mandatory components in the pool source */ @@ -172,8 +168,6 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { .defaultFormat = VIR_STORAGE_FILE_RAW, .formatFromString = virStorageVolumeFormatFromString, .formatToString = virStorageFileFormatTypeToString, - .featureFromString = virStorageFileFeatureTypeFromString, - .featureToString = virStorageFileFeatureTypeToString, }, }, {.poolType = VIR_STORAGE_POOL_FS, @@ -187,8 +181,6 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { .defaultFormat = VIR_STORAGE_FILE_RAW, .formatFromString = virStorageVolumeFormatFromString, .formatToString = virStorageFileFormatTypeToString, - .featureFromString = virStorageFileFeatureTypeFromString, - .featureToString = virStorageFileFeatureTypeToString, }, }, {.poolType = VIR_STORAGE_POOL_NETFS, @@ -203,8 +195,6 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { .defaultFormat = VIR_STORAGE_FILE_RAW, .formatFromString = virStorageVolumeFormatFromString, .formatToString = virStorageFileFormatTypeToString, - .featureFromString = virStorageFileFeatureTypeFromString, - .featureToString = virStorageFileFeatureTypeToString, }, }, {.poolType = VIR_STORAGE_POOL_ISCSI, @@ -258,8 +248,6 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { .defaultFormat = VIR_STORAGE_FILE_RAW, .formatToString = virStorageFileFormatTypeToString, .formatFromString = virStorageVolumeFormatFromString, - .featureFromString = virStorageFileFeatureTypeFromString, - .featureToString = virStorageFileFeatureTypeToString, } }, {.poolType = VIR_STORAGE_POOL_MPATH, @@ -1404,7 +1392,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (virXPathNode("./target/nocow", ctxt)) ret->target.nocow = true; - if (options->featureFromString && virXPathNode("./target/features", ctxt)) { + if (virXPathNode("./target/features", ctxt)) { if ((n = virXPathNodeSet("./target/features/*", ctxt, &nodes)) < 0) goto error; @@ -1415,7 +1403,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, goto error; for (i = 0; i < n; i++) { - int f = options->featureFromString((const char*)nodes[i]->name); + int f = virStorageFileFeatureTypeFromString((const char*)nodes[i]->name); if (f < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported feature %s"), @@ -1576,7 +1564,7 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferEscapeString(buf, "<compat>%s</compat>\n", def->compat); - if (options->featureToString && def->features) { + if (def->features) { size_t i; bool empty = virBitmapIsAllClear(def->features); @@ -1590,7 +1578,7 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, for (i = 0; i < VIR_STORAGE_FILE_FEATURE_LAST; i++) { if (virBitmapIsBitSet(def->features, i)) virBufferAsprintf(buf, "<%s/>\n", - options->featureToString(i)); + virStorageFileFeatureTypeToString(i)); } if (!empty) { virBufferAdjustIndent(buf, -2); -- 2.0.5
On Fri, Apr 10, 2015 at 14:58:54 +0200, Ján Tomko wrote:
We only have one formatting function for the features. --- src/conf/storage_conf.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-)
ACK. I don't really see where we would have completely different feature sets for different pool types. Also if that would happen we can have a checker that rejects the unsupported ones for the given type and they can be in a single enum. Peter
To allow sharing the function between snapshot_conf and storage_conf. --- po/POTFILES.in | 1 + src/Makefile.am | 5 ++++ src/conf/storage_conf.c | 32 ++++----------------- src/conf/storage_feature_conf.c | 62 +++++++++++++++++++++++++++++++++++++++++ src/conf/storage_feature_conf.h | 22 +++++++++++++++ 5 files changed, 96 insertions(+), 26 deletions(-) create mode 100644 src/conf/storage_feature_conf.c create mode 100644 src/conf/storage_feature_conf.h diff --git a/po/POTFILES.in b/po/POTFILES.in index dd06ab3..09dcd99 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -34,6 +34,7 @@ src/conf/object_event.c src/conf/secret_conf.c src/conf/snapshot_conf.c src/conf/storage_conf.c +src/conf/storage_feature_conf.c src/conf/virchrdev.c src/cpu/cpu.c src/cpu/cpu_generic.c diff --git a/src/Makefile.am b/src/Makefile.am index 91a4c17..4e8679a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -309,6 +309,10 @@ NWFILTER_CONF_SOURCES = \ STORAGE_CONF_SOURCES = \ conf/storage_conf.h conf/storage_conf.c +# Storage driver generic impl APIs +STORAGE_FEATURE_CONF_SOURCES = \ + conf/storage_feature_conf.h conf/storage_feature_conf.c + # Interface driver generic impl APIs INTERFACE_CONF_SOURCES = \ conf/interface_conf.c conf/interface_conf.h @@ -342,6 +346,7 @@ CONF_SOURCES = \ $(NWFILTER_CONF_SOURCES) \ $(NODE_DEVICE_CONF_SOURCES) \ $(STORAGE_CONF_SOURCES) \ + $(STORAGE_FEATURE_CONF_SOURCES) \ $(INTERFACE_CONF_SOURCES) \ $(SECRET_CONF_SOURCES) \ $(CPU_CONF_SOURCES) \ diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 339f08f..a5b5c1b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -36,6 +36,7 @@ #include "virerror.h" #include "datatypes.h" #include "storage_conf.h" +#include "storage_feature_conf.h" #include "virstoragefile.h" #include "virxml.h" @@ -1255,9 +1256,6 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, char *unit = NULL; char *backingStore = NULL; xmlNodePtr node; - xmlNodePtr *nodes = NULL; - size_t i; - int n; virCheckFlags(VIR_VOL_XML_PARSE_NO_CAPACITY | VIR_VOL_XML_PARSE_OPT_CAPACITY, NULL); @@ -1392,31 +1390,13 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (virXPathNode("./target/nocow", ctxt)) ret->target.nocow = true; - if (virXPathNode("./target/features", ctxt)) { - if ((n = virXPathNodeSet("./target/features/*", ctxt, &nodes)) < 0) - goto error; - - if (!ret->target.compat && VIR_STRDUP(ret->target.compat, "1.1") < 0) - goto error; - - if (!(ret->target.features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST))) - goto error; - - for (i = 0; i < n; i++) { - int f = virStorageFileFeatureTypeFromString((const char*)nodes[i]->name); - - if (f < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported feature %s"), - (const char*)nodes[i]->name); - goto error; - } - ignore_value(virBitmapSetBit(ret->target.features, f)); - } - VIR_FREE(nodes); - } + if (virStorageFeaturesParse(ctxt, + "./target/features", + &ret->target.compat, + &ret->target.features) < 0) + goto error; cleanup: - VIR_FREE(nodes); VIR_FREE(allocation); VIR_FREE(capacity); VIR_FREE(unit); diff --git a/src/conf/storage_feature_conf.c b/src/conf/storage_feature_conf.c new file mode 100644 index 0000000..77e6406 --- /dev/null +++ b/src/conf/storage_feature_conf.c @@ -0,0 +1,62 @@ +/* + * storage_feature_conf.c: config handling for storage file features + * + * Copyright: Red Hat, Inc + * + * LGLPv2.1+ + */ + +#include <config.h> + +#include "storage_feature_conf.h" +#include "viralloc.h" +#include "virerror.h" +#include "virstoragefile.h" +#include "virstring.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +int virStorageFeaturesParse(xmlXPathContextPtr ctxt, + const char *xpath, + char **compat, + virBitmapPtr *features) +{ + xmlNodePtr *nodes = NULL; + char *feat_xpath = NULL; + size_t i; + int n; + int ret = -1; + + if (!virXPathNode(xpath, ctxt)) + return 0; + + if (!*compat && VIR_STRDUP(*compat, "1.1") < 0) + return -1; + + if (virAsprintf(&feat_xpath, "%s/*", xpath) < 0) + goto cleanup; + + if ((n = virXPathNodeSet(feat_xpath, ctxt, &nodes)) < 0) + goto cleanup; + + if (!(*features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST))) + goto cleanup; + + for (i = 0; i < n; i++) { + int f = virStorageFileFeatureTypeFromString((const char*)nodes[i]->name); + + if (f < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported feature %s"), + (const char*)nodes[i]->name); + goto cleanup; + } + ignore_value(virBitmapSetBit(*features, f)); + } + + ret = 0; + + cleanup: + VIR_FREE(nodes); + VIR_FREE(feat_xpath); + return ret; +} diff --git a/src/conf/storage_feature_conf.h b/src/conf/storage_feature_conf.h new file mode 100644 index 0000000..a411b66 --- /dev/null +++ b/src/conf/storage_feature_conf.h @@ -0,0 +1,22 @@ +/* + * storage_feature_conf.h: config handling for storage file features + * + * Copyright: Red Hat, Inc + * + * LGLPv2.1+ + */ + +#ifndef __VIR_STORAGE_FEATURE_CONF_H__ +# define __VIR_STORAGE_FEATURE_CONF_H__ + +# include "internal.h" + +# include "virbitmap.h" +# include "virxml.h" + +int virStorageFeaturesParse(xmlXPathContextPtr ctxt, + const char *xpath, + char **compat, + virBitmapPtr *features); + +#endif /* __VIR_STORAGE_FEATURE_CONF_H__ */ -- 2.0.5
On Fri, Apr 10, 2015 at 14:58:55 +0200, Ján Tomko wrote:
To allow sharing the function between snapshot_conf and storage_conf. --- po/POTFILES.in | 1 + src/Makefile.am | 5 ++++ src/conf/storage_conf.c | 32 ++++----------------- src/conf/storage_feature_conf.c | 62 +++++++++++++++++++++++++++++++++++++++++ src/conf/storage_feature_conf.h | 22 +++++++++++++++ 5 files changed, 96 insertions(+), 26 deletions(-) create mode 100644 src/conf/storage_feature_conf.c create mode 100644 src/conf/storage_feature_conf.h
...
diff --git a/src/Makefile.am b/src/Makefile.am index 91a4c17..4e8679a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -309,6 +309,10 @@ NWFILTER_CONF_SOURCES = \
conf/interface_conf.c conf/interface_conf.h @@ -342,6 +346,7 @@ CONF_SOURCES = \ $(NWFILTER_CONF_SOURCES) \ $(NODE_DEVICE_CONF_SOURCES) \ $(STORAGE_CONF_SOURCES) \ + $(STORAGE_FEATURE_CONF_SOURCES) \
The backslash at the end is not indented with tabs thus it's improperly aligned when viewed with the git tool that formats 8 spaces for tabs.
$(INTERFACE_CONF_SOURCES) \ $(SECRET_CONF_SOURCES) \ $(CPU_CONF_SOURCES) \ diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 339f08f..a5b5c1b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -36,6 +36,7 @@ #include "virerror.h" #include "datatypes.h" #include "storage_conf.h" +#include "storage_feature_conf.h" #include "virstoragefile.h"
#include "virxml.h" @@ -1255,9 +1256,6 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, char *unit = NULL; char *backingStore = NULL; xmlNodePtr node; - xmlNodePtr *nodes = NULL; - size_t i; - int n;
virCheckFlags(VIR_VOL_XML_PARSE_NO_CAPACITY | VIR_VOL_XML_PARSE_OPT_CAPACITY, NULL); @@ -1392,31 +1390,13 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (virXPathNode("./target/nocow", ctxt)) ret->target.nocow = true;
- if (virXPathNode("./target/features", ctxt)) { - if ((n = virXPathNodeSet("./target/features/*", ctxt, &nodes)) < 0) - goto error; - - if (!ret->target.compat && VIR_STRDUP(ret->target.compat, "1.1") < 0) - goto error; - - if (!(ret->target.features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST))) - goto error; - - for (i = 0; i < n; i++) { - int f = virStorageFileFeatureTypeFromString((const char*)nodes[i]->name); - - if (f < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported feature %s"), - (const char*)nodes[i]->name); - goto error; - } - ignore_value(virBitmapSetBit(ret->target.features, f)); - } - VIR_FREE(nodes); - } + if (virStorageFeaturesParse(ctxt, + "./target/features", + &ret->target.compat, + &ret->target.features) < 0) + goto error;
cleanup: - VIR_FREE(nodes); VIR_FREE(allocation); VIR_FREE(capacity); VIR_FREE(unit); diff --git a/src/conf/storage_feature_conf.c b/src/conf/storage_feature_conf.c new file mode 100644 index 0000000..77e6406 --- /dev/null +++ b/src/conf/storage_feature_conf.c @@ -0,0 +1,62 @@ +/* + * storage_feature_conf.c: config handling for storage file features + * + * Copyright: Red Hat, Inc + * + * LGLPv2.1+ + */
I like this compact header, but I'm not sure if the rest of upstream has the same opinion.
+ +#include <config.h> + +#include "storage_feature_conf.h" +#include "viralloc.h" +#include "virerror.h" +#include "virstoragefile.h" +#include "virstring.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +int virStorageFeaturesParse(xmlXPathContextPtr ctxt,
return type on the same line
+ const char *xpath, + char **compat, + virBitmapPtr *features) +{ + xmlNodePtr *nodes = NULL; + char *feat_xpath = NULL; + size_t i; + int n; + int ret = -1; + + if (!virXPathNode(xpath, ctxt)) + return 0; + + if (!*compat && VIR_STRDUP(*compat, "1.1") < 0) + return -1;
Is there a specific reason that you check whether the compat string is not assigned previously?
+ + if (virAsprintf(&feat_xpath, "%s/*", xpath) < 0) + goto cleanup; + + if ((n = virXPathNodeSet(feat_xpath, ctxt, &nodes)) < 0) + goto cleanup; + + if (!(*features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST))) + goto cleanup; + + for (i = 0; i < n; i++) { + int f = virStorageFileFeatureTypeFromString((const char*)nodes[i]->name); + + if (f < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported feature %s"), + (const char*)nodes[i]->name); + goto cleanup; + } + ignore_value(virBitmapSetBit(*features, f)); + } + + ret = 0; + + cleanup: + VIR_FREE(nodes); + VIR_FREE(feat_xpath); + return ret; +} diff --git a/src/conf/storage_feature_conf.h b/src/conf/storage_feature_conf.h new file mode 100644 index 0000000..a411b66 --- /dev/null +++ b/src/conf/storage_feature_conf.h @@ -0,0 +1,22 @@ +/* + * storage_feature_conf.h: config handling for storage file features + * + * Copyright: Red Hat, Inc + * + * LGLPv2.1+ + */ + +#ifndef __VIR_STORAGE_FEATURE_CONF_H__ +# define __VIR_STORAGE_FEATURE_CONF_H__ + +# include "internal.h" + +# include "virbitmap.h" +# include "virxml.h" + +int virStorageFeaturesParse(xmlXPathContextPtr ctxt, + const char *xpath, + char **compat, + virBitmapPtr *features);
All four arguments are ATTRIBUTE_NONNULL.
+ +#endif /* __VIR_STORAGE_FEATURE_CONF_H__ */
The code looks okay, but I'm not entirely sure about the licence section, so I'd rather have another opinion. Peter
On Mon, Apr 13, 2015 at 07:41:12AM +0200, Peter Krempa wrote:
On Fri, Apr 10, 2015 at 14:58:55 +0200, Ján Tomko wrote:
diff --git a/src/conf/storage_feature_conf.c b/src/conf/storage_feature_conf.c new file mode 100644 index 0000000..77e6406 --- /dev/null +++ b/src/conf/storage_feature_conf.c @@ -0,0 +1,62 @@ +/* + * storage_feature_conf.c: config handling for storage file features + * + * Copyright: Red Hat, Inc + * + * LGLPv2.1+ + */
I like this compact header, but I'm not sure if the rest of upstream has the same opinion.
It was just a placeholder and I forgot to update it before sending the series. If we want to switch to a more compact header, I think it should be done consistently all over the codebase. I have squashed in a copy of the header from another conf file.
+ const char *xpath, + char **compat, + virBitmapPtr *features) +{ + xmlNodePtr *nodes = NULL; + char *feat_xpath = NULL; + size_t i; + int n; + int ret = -1; + + if (!virXPathNode(xpath, ctxt)) + return 0; + + if (!*compat && VIR_STRDUP(*compat, "1.1") < 0) + return -1;
Is there a specific reason that you check whether the compat string is not assigned previously?
Yes, the user might have specified a different compat level. Jan
For reuse in snapshot_conf. --- src/conf/storage_conf.c | 23 +---------------------- src/conf/storage_feature_conf.c | 25 +++++++++++++++++++++++++ src/conf/storage_feature_conf.h | 3 +++ 3 files changed, 29 insertions(+), 22 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index a5b5c1b..ee3019a 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1544,28 +1544,7 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferEscapeString(buf, "<compat>%s</compat>\n", def->compat); - if (def->features) { - size_t i; - bool empty = virBitmapIsAllClear(def->features); - - if (empty) { - virBufferAddLit(buf, "<features/>\n"); - } else { - virBufferAddLit(buf, "<features>\n"); - virBufferAdjustIndent(buf, 2); - } - - for (i = 0; i < VIR_STORAGE_FILE_FEATURE_LAST; i++) { - if (virBitmapIsBitSet(def->features, i)) - virBufferAsprintf(buf, "<%s/>\n", - virStorageFileFeatureTypeToString(i)); - } - if (!empty) { - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</features>\n"); - } - } - + virStorageFeaturesFormat(buf, def->features); virBufferAdjustIndent(buf, -2); virBufferAsprintf(buf, "</%s>\n", type); return 0; diff --git a/src/conf/storage_feature_conf.c b/src/conf/storage_feature_conf.c index 77e6406..2a4b3df 100644 --- a/src/conf/storage_feature_conf.c +++ b/src/conf/storage_feature_conf.c @@ -60,3 +60,28 @@ int virStorageFeaturesParse(xmlXPathContextPtr ctxt, VIR_FREE(feat_xpath); return ret; } + +void virStorageFeaturesFormat(virBufferPtr buf, + virBitmapPtr features) +{ + size_t i; + + if (!features) + return; + + if (virBitmapIsAllClear(features)) { + virBufferAddLit(buf, "<features/>\n"); + return; + } + virBufferAddLit(buf, "<features>\n"); + virBufferAdjustIndent(buf, 2); + + for (i = 0; i < VIR_STORAGE_FILE_FEATURE_LAST; i++) { + if (virBitmapIsBitSet(features, i)) + virBufferAsprintf(buf, "<%s/>\n", + virStorageFileFeatureTypeToString(i)); + } + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</features>\n"); +} diff --git a/src/conf/storage_feature_conf.h b/src/conf/storage_feature_conf.h index a411b66..14de20e 100644 --- a/src/conf/storage_feature_conf.h +++ b/src/conf/storage_feature_conf.h @@ -12,11 +12,14 @@ # include "internal.h" # include "virbitmap.h" +# include "virbuffer.h" # include "virxml.h" int virStorageFeaturesParse(xmlXPathContextPtr ctxt, const char *xpath, char **compat, virBitmapPtr *features); +void virStorageFeaturesFormat(virBufferPtr buf, + virBitmapPtr features); #endif /* __VIR_STORAGE_FEATURE_CONF_H__ */ -- 2.0.5
On Fri, Apr 10, 2015 at 14:58:56 +0200, Ján Tomko wrote:
For reuse in snapshot_conf. --- src/conf/storage_conf.c | 23 +---------------------- src/conf/storage_feature_conf.c | 25 +++++++++++++++++++++++++ src/conf/storage_feature_conf.h | 3 +++ 3 files changed, 29 insertions(+), 22 deletions(-)
...
diff --git a/src/conf/storage_feature_conf.c b/src/conf/storage_feature_conf.c index 77e6406..2a4b3df 100644 --- a/src/conf/storage_feature_conf.c +++ b/src/conf/storage_feature_conf.c @@ -60,3 +60,28 @@ int virStorageFeaturesParse(xmlXPathContextPtr ctxt, VIR_FREE(feat_xpath); return ret; } + +void virStorageFeaturesFormat(virBufferPtr buf, + virBitmapPtr features)
<coding_style_bike_shed> there's only one line between functions and the return type is not on a new line. </coding_style_bike_shed>
+{ + size_t i; + + if (!features) + return; + + if (virBitmapIsAllClear(features)) { + virBufferAddLit(buf, "<features/>\n"); + return;
I like this control flow change.
+ } + virBufferAddLit(buf, "<features>\n"); + virBufferAdjustIndent(buf, 2); + + for (i = 0; i < VIR_STORAGE_FILE_FEATURE_LAST; i++) { + if (virBitmapIsBitSet(features, i)) + virBufferAsprintf(buf, "<%s/>\n", + virStorageFileFeatureTypeToString(i)); + } + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</features>\n"); +}
ACK, Peter
For future reuse in the snapshot XML. --- src/conf/storage_conf.c | 16 ++-------------- src/util/virstoragefile.c | 29 +++++++++++++++++++++++++++++ src/util/virstoragefile.h | 1 + 3 files changed, 32 insertions(+), 14 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index ee3019a..2c34394 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1372,20 +1372,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, } ret->target.compat = virXPathString("string(./target/compat)", ctxt); - if (ret->target.compat) { - char **version = virStringSplit(ret->target.compat, ".", 2); - unsigned int result; - - if (!version || !version[1] || - virStrToLong_ui(version[0], NULL, 10, &result) < 0 || - virStrToLong_ui(version[1], NULL, 10, &result) < 0) { - virStringFreeList(version); - virReportError(VIR_ERR_XML_ERROR, "%s", - _("forbidden characters in 'compat' attribute")); - goto error; - } - virStringFreeList(version); - } + if (!virStorageFileCheckCompat(ret->target.compat)) + goto error; if (virXPathNode("./target/nocow", ctxt)) ret->target.nocow = true; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 96be02e..c1246e7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2873,3 +2873,32 @@ virStorageFileGetRelativeBackingPath(virStorageSourcePtr top, VIR_FREE(tmp); return ret; } + + +/* + * virStorageFileCheckCompat + */ +bool +virStorageFileCheckCompat(const char *compat) +{ + char **version; + unsigned int result; + bool ret; + + if (!compat) + return true; + + version = virStringSplit(compat, ".", 2); + if (!version || !version[1] || + virStrToLong_ui(version[0], NULL, 10, &result) < 0 || + virStrToLong_ui(version[1], NULL, 10, &result) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("forbidden characters in 'compat' attribute")); + goto cleanup; + } + ret = true; + + cleanup: + virStringFreeList(version); + return ret; +} diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b4c3808..ed90774 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -384,4 +384,5 @@ int virStorageFileGetRelativeBackingPath(virStorageSourcePtr from, char **relpath) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +bool virStorageFileCheckCompat(const char *compat); #endif /* __VIR_STORAGE_FILE_H__ */ -- 2.0.5
On Fri, Apr 10, 2015 at 14:58:57 +0200, Ján Tomko wrote:
For future reuse in the snapshot XML. --- src/conf/storage_conf.c | 16 ++-------------- src/util/virstoragefile.c | 29 +++++++++++++++++++++++++++++ src/util/virstoragefile.h | 1 + 3 files changed, 32 insertions(+), 14 deletions(-)
...
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 96be02e..c1246e7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2873,3 +2873,32 @@ virStorageFileGetRelativeBackingPath(virStorageSourcePtr top, VIR_FREE(tmp); return ret; } + + +/* + * virStorageFileCheckCompat + */ +bool +virStorageFileCheckCompat(const char *compat) +{ + char **version; + unsigned int result; + bool ret; + + if (!compat) + return true; + + version = virStringSplit(compat, ".", 2); + if (!version || !version[1] || + virStrToLong_ui(version[0], NULL, 10, &result) < 0 || + virStrToLong_ui(version[1], NULL, 10, &result) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("forbidden characters in 'compat' attribute")); + goto cleanup;
Functions that set errors usually return an integer despite only returning two possible values in total.
+ } + ret = true; + + cleanup: + virStringFreeList(version); + return ret; +}
ACK if you change the return type to 'int' or add a documentation comment explaining when the error is set. Peter
Instead of going through XML nodes in a loop and having to check if they are duplicate. --- src/conf/snapshot_conf.c | 46 ++++++++++++++++++++-------------------------- 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index dc5436f..ca6796a 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -113,7 +113,11 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, int ret = -1; char *snapshot = NULL; char *type = NULL; + char *driver = NULL; xmlNodePtr cur; + xmlNodePtr saved = ctxt->node; + + ctxt->node = node; if (VIR_ALLOC(def->src) < 0) goto cleanup; @@ -148,33 +152,20 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, def->src->type = VIR_STORAGE_TYPE_FILE; } - for (cur = node->children; cur; cur = cur->next) { - if (cur->type != XML_ELEMENT_NODE) - continue; - - if (!def->src->path && - xmlStrEqual(cur->name, BAD_CAST "source")) { - - if (virDomainDiskSourceParse(cur, ctxt, def->src) < 0) - goto cleanup; + if ((cur = virXPathNode("./source", ctxt)) && + virDomainDiskSourceParse(cur, ctxt, def->src) < 0) + goto cleanup; - } else if (!def->src->format && - xmlStrEqual(cur->name, BAD_CAST "driver")) { - char *driver = virXMLPropString(cur, "type"); - if (driver) { - def->src->format = virStorageFileFormatTypeFromString(driver); - if (def->src->format < VIR_STORAGE_FILE_BACKING) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - def->src->format <= 0 - ? _("unknown disk snapshot driver '%s'") - : _("disk format '%s' lacks backing file " - "support"), - driver); - VIR_FREE(driver); - goto cleanup; - } - VIR_FREE(driver); - } + if ((driver = virXPathString("string(./driver/@type)", ctxt))) { + def->src->format = virStorageFileFormatTypeFromString(driver); + if (def->src->format < VIR_STORAGE_FILE_BACKING) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + def->src->format <= 0 + ? _("unknown disk snapshot driver '%s'") + : _("disk format '%s' lacks backing file " + "support"), + driver); + goto cleanup; } } @@ -193,6 +184,9 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, ret = 0; cleanup: + ctxt->node = saved; + + VIR_FREE(driver); VIR_FREE(snapshot); VIR_FREE(type); if (ret < 0) -- 2.0.5
On Fri, Apr 10, 2015 at 14:58:58 +0200, Ján Tomko wrote:
Instead of going through XML nodes in a loop and having to check if they are duplicate. --- src/conf/snapshot_conf.c | 46 ++++++++++++++++++++-------------------------- 1 file changed, 20 insertions(+), 26 deletions(-)
ACK, Peter
Add FromVol at the end. This function will create the qemu-img command line from volume definitions and check them. --- src/storage/storage_backend.c | 21 ++++++++++++--------- src/storage/storage_backend.h | 14 +++++++------- tests/storagevolxml2argvtest.c | 5 +++-- 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 9322571..fdda0dc 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -858,14 +858,17 @@ virStorageBackendCreateQemuImgOpts(char **opts, return -1; } +/* Create a qemu-img virCommand from the supplied binary path, + * volume definitions and imgformat + */ virCommandPtr -virStorageBackendCreateQemuImgCmd(virConnectPtr conn, - virStoragePoolObjPtr pool, - virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol, - unsigned int flags, - const char *create_tool, - int imgformat) +virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol, + unsigned int flags, + const char *create_tool, + int imgformat) { virCommandPtr cmd = NULL; bool do_encryption = (vol->target.encryption != NULL); @@ -1094,8 +1097,8 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, if (imgformat < 0) goto cleanup; - cmd = virStorageBackendCreateQemuImgCmd(conn, pool, vol, inputvol, flags, - create_tool, imgformat); + cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, pool, vol, inputvol, + flags, create_tool, imgformat); if (!cmd) goto cleanup; diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index fd2451c..bb1e8d8 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -192,13 +192,13 @@ char *virStorageBackendStablePath(virStoragePoolObjPtr pool, bool loop); virCommandPtr -virStorageBackendCreateQemuImgCmd(virConnectPtr conn, - virStoragePoolObjPtr pool, - virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol, - unsigned int flags, - const char *create_tool, - int imgformat); +virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol, + unsigned int flags, + const char *create_tool, + int imgformat); /* ------- virStorageFile backends ------------ */ typedef struct _virStorageFileBackend virStorageFileBackend; diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index 696659c..f5f86c8 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -98,8 +98,9 @@ testCompareXMLToArgvFiles(bool shouldFail, testSetVolumeType(vol, pool); testSetVolumeType(inputvol, inputpool); - cmd = virStorageBackendCreateQemuImgCmd(conn, &poolobj, vol, inputvol, - flags, create_tool, imgformat); + cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, &poolobj, vol, + inputvol, flags, + create_tool, imgformat); if (!cmd) { if (shouldFail) { virResetLastError(); -- 2.0.5
On Fri, Apr 10, 2015 at 14:58:59 +0200, Ján Tomko wrote:
Add FromVol at the end. This function will create the qemu-img command line from volume definitions and check them. --- src/storage/storage_backend.c | 21 ++++++++++++--------- src/storage/storage_backend.h | 14 +++++++------- tests/storagevolxml2argvtest.c | 5 +++-- 3 files changed, 22 insertions(+), 18 deletions(-)
ACK, Peter
This will contain the data required for creating the qemu-img command line without having access to the volume definition. --- src/storage/storage_backend.c | 165 ++++++++++++++++++++++-------------------- 1 file changed, 86 insertions(+), 79 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index fdda0dc..f5b95ec 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -796,41 +796,53 @@ virStorageBackendQEMUImgBackingFormat(const char *qemuimg) return ret; } +struct _virStorageBackendQemuImgInfo { + int format; + const char *path; + unsigned long long size_arg; + bool encryption; + bool preallocate; + const char *compat; + virBitmapPtr features; + bool nocow; + + const char *backingPath; + int backingFormat; + + const char *inputPath; + int inputFormat; +}; + static int virStorageBackendCreateQemuImgOpts(char **opts, - const char *backingType, - bool encryption, - bool preallocate, - int format, - const char *compat, - bool nocow, - virBitmapPtr features) + struct _virStorageBackendQemuImgInfo info) { virBuffer buf = VIR_BUFFER_INITIALIZER; size_t i; - if (backingType) - virBufferAsprintf(&buf, "backing_fmt=%s,", backingType); - if (encryption) + if (info.backingPath) + virBufferAsprintf(&buf, "backing_fmt=%s,", + virStorageFileFormatTypeToString(info.backingFormat)); + if (info.encryption) virBufferAddLit(&buf, "encryption=on,"); - if (preallocate) + if (info.preallocate) virBufferAddLit(&buf, "preallocation=metadata,"); - if (nocow) + if (info.nocow) virBufferAddLit(&buf, "nocow=on,"); - if (compat) - virBufferAsprintf(&buf, "compat=%s,", compat); - if (features && format == VIR_STORAGE_FILE_QCOW2) { + if (info.compat) + virBufferAsprintf(&buf, "compat=%s,", info.compat); + if (info.features && info.format == VIR_STORAGE_FILE_QCOW2) { for (i = 0; i < VIR_STORAGE_FILE_FEATURE_LAST; i++) { - if (virBitmapIsBitSet(features, i)) { + if (virBitmapIsBitSet(info.features, i)) { switch ((virStorageFileFeature) i) { case VIR_STORAGE_FILE_FEATURE_LAZY_REFCOUNTS: - if (STREQ_NULLABLE(compat, "0.10")) { + if (STREQ_NULLABLE(info.compat, "0.10")) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Feature %s not supported with compat" " level %s"), virStorageFileFeatureTypeToString(i), - compat); + info.compat); goto error; } break; @@ -871,75 +883,75 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, int imgformat) { virCommandPtr cmd = NULL; - bool do_encryption = (vol->target.encryption != NULL); - unsigned long long int size_arg; - bool preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA); const char *type; const char *backingType = NULL; - const char *inputPath = NULL; const char *inputType = NULL; - const char *compat = vol->target.compat; char *opts = NULL; - bool convert = false; - bool backing = false; + struct _virStorageBackendQemuImgInfo info = { + .format = vol->target.format, + .path = vol->target.path, + .encryption = vol->target.encryption != NULL, + .preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA), + .compat = vol->target.compat, + .features = vol->target.features, + .nocow = vol->target.nocow, + }; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); /* Treat output block devices as 'raw' format */ - type = virStorageFileFormatTypeToString(vol->type == VIR_STORAGE_VOL_BLOCK ? - VIR_STORAGE_FILE_RAW : - vol->target.format); + if (vol->type == VIR_STORAGE_VOL_BLOCK) + info.format = VIR_STORAGE_FILE_RAW; - if (!type) { + if (!(type = virStorageFileFormatTypeToString(info.format))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown storage vol type %d"), - vol->target.format); + info.format); return NULL; } - if (preallocate && vol->target.format != VIR_STORAGE_FILE_QCOW2) { + if (info.preallocate && info.format != VIR_STORAGE_FILE_QCOW2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("metadata preallocation only available with qcow2")); return NULL; } - if (vol->target.compat && vol->target.format != VIR_STORAGE_FILE_QCOW2) { + if (info.compat && info.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) { + if (info.features && info.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)) { + if (!(info.inputPath = inputvol->target.path)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("missing input volume target path")); return NULL; } - inputType = virStorageFileFormatTypeToString(inputvol->type == VIR_STORAGE_VOL_BLOCK ? - VIR_STORAGE_FILE_RAW : - inputvol->target.format); - - if (!inputType) { + info.inputFormat = inputvol->target.format; + if (inputvol->type == VIR_STORAGE_VOL_BLOCK) + info.inputFormat = VIR_STORAGE_FILE_RAW; + if (!(inputType = virStorageFileFormatTypeToString(info.inputFormat))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown storage vol type %d"), - inputvol->target.format); + info.inputFormat); return NULL; } - } if (vol->target.backingStore) { int accessRetCode = -1; char *absolutePath = NULL; - backingType = virStorageFileFormatTypeToString(vol->target.backingStore->format); + info.backingFormat = vol->target.backingStore->format; + info.backingPath = vol->target.backingStore->path; - if (preallocate) { + if (info.preallocate) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("metadata preallocation conflicts with backing" " store")); @@ -951,43 +963,41 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, * may cause issues with lvm. Untested essentially. */ if (inputvol && inputvol->target.backingStore && - STRNEQ_NULLABLE(inputvol->target.backingStore->path, - vol->target.backingStore->path)) { + STRNEQ_NULLABLE(inputvol->target.backingStore->path, info.backingPath)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("a different backing store cannot be specified.")); return NULL; } - if (backingType == NULL) { + if (!(backingType = virStorageFileFormatTypeToString(info.backingFormat))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown storage vol backing store type %d"), - vol->target.backingStore->format); + info.backingFormat); return NULL; } /* Convert relative backing store paths to absolute paths for access * validation. */ - if ('/' != *(vol->target.backingStore->path) && + if ('/' != *(info.backingPath) && virAsprintf(&absolutePath, "%s/%s", pool->def->target.path, - vol->target.backingStore->path) < 0) + info.backingPath) < 0) return NULL; - accessRetCode = access(absolutePath ? absolutePath - : vol->target.backingStore->path, R_OK); + accessRetCode = access(absolutePath ? absolutePath : info.backingPath, R_OK); VIR_FREE(absolutePath); if (accessRetCode != 0) { virReportSystemError(errno, _("inaccessible backing store volume %s"), - vol->target.backingStore->path); + info.backingPath); return NULL; } } - if (do_encryption) { + if (info.encryption) { virStorageEncryptionPtr enc; - if (vol->target.format != VIR_STORAGE_FILE_QCOW && - vol->target.format != VIR_STORAGE_FILE_QCOW2) { + if (info.format != VIR_STORAGE_FILE_QCOW && + info.format != VIR_STORAGE_FILE_QCOW2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("qcow volume encryption unsupported with " "volume format %s"), type); @@ -1014,33 +1024,30 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, } /* Size in KB */ - size_arg = VIR_DIV_UP(vol->target.capacity, 1024); + info.size_arg = VIR_DIV_UP(vol->target.capacity, 1024); cmd = virCommandNew(create_tool); - convert = !!inputvol; - backing = !inputvol && vol->target.backingStore; + /* ignore the backing volume when we're converting a volume */ + if (info.inputPath) { + info.backingPath = NULL; + backingType = NULL; + } - if (convert) + if (info.inputPath) virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); else virCommandAddArgList(cmd, "create", "-f", type, NULL); - if (backing) - virCommandAddArgList(cmd, "-b", vol->target.backingStore->path, NULL); + if (info.backingPath) + virCommandAddArgList(cmd, "-b", info.backingPath, NULL); if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS) { - if (vol->target.format == VIR_STORAGE_FILE_QCOW2 && !compat && + if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat && imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT) - compat = "0.10"; - - if (virStorageBackendCreateQemuImgOpts(&opts, - backing ? backingType : NULL, - do_encryption, preallocate, - vol->target.format, - compat, - vol->target.nocow, - vol->target.features) < 0) { + info.compat = "0.10"; + + if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) { virCommandFree(cmd); return NULL; } @@ -1048,22 +1055,22 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, virCommandAddArgList(cmd, "-o", opts, NULL); VIR_FREE(opts); } else { - if (backing) { + if (info.backingPath) { 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); + info.path, create_tool); } - if (do_encryption) + if (info.encryption) virCommandAddArg(cmd, "-e"); } - if (convert) - virCommandAddArg(cmd, inputPath); - virCommandAddArg(cmd, vol->target.path); - if (!convert && size_arg) - virCommandAddArgFormat(cmd, "%lluK", size_arg); + if (info.inputPath) + virCommandAddArg(cmd, info.inputPath); + virCommandAddArg(cmd, info.path); + if (!info.inputPath && info.size_arg) + virCommandAddArgFormat(cmd, "%lluK", info.size_arg); return cmd; } -- 2.0.5
On Fri, Apr 10, 2015 at 14:59:00 +0200, Ján Tomko wrote:
This will contain the data required for creating the qemu-img command line without having access to the volume definition. --- src/storage/storage_backend.c | 165 ++++++++++++++++++++++-------------------- 1 file changed, 86 insertions(+), 79 deletions(-)
ACK, Peter
Do not require the volume definition. This will allow code reuse when creating snapshots. --- src/storage/storage_backend.c | 203 +++++++++++++++++++++++------------------- 1 file changed, 109 insertions(+), 94 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f5b95ec..bfbc193 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -870,38 +870,16 @@ virStorageBackendCreateQemuImgOpts(char **opts, return -1; } -/* Create a qemu-img virCommand from the supplied binary path, - * volume definitions and imgformat - */ -virCommandPtr -virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, - virStoragePoolObjPtr pool, - virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol, - unsigned int flags, - const char *create_tool, - int imgformat) +static virCommandPtr +virStorageBackendCreateQemuImgCmd(const char *create_tool, + int imgformat, + struct _virStorageBackendQemuImgInfo info) { virCommandPtr cmd = NULL; - const char *type; + const char *type = NULL; const char *backingType = NULL; const char *inputType = NULL; char *opts = NULL; - struct _virStorageBackendQemuImgInfo info = { - .format = vol->target.format, - .path = vol->target.path, - .encryption = vol->target.encryption != NULL, - .preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA), - .compat = vol->target.compat, - .features = vol->target.features, - .nocow = vol->target.nocow, - }; - - virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); - - /* Treat output block devices as 'raw' format */ - if (vol->type == VIR_STORAGE_VOL_BLOCK) - info.format = VIR_STORAGE_FILE_RAW; if (!(type = virStorageFileFormatTypeToString(info.format))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -926,6 +904,107 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, return NULL; } + if (info.inputPath && + !(inputType = virStorageFileFormatTypeToString(info.inputFormat))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown storage vol type %d"), + info.inputFormat); + return NULL; + } + + if (info.backingPath) { + if (info.preallocate) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("metadata preallocation conflicts with backing" + " store")); + return NULL; + } + + if (!(backingType = virStorageFileFormatTypeToString(info.backingFormat))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown storage vol backing store type %d"), + info.backingFormat); + return NULL; + } + } + + /* ignore the backing volume when we're converting a volume */ + if (info.inputPath) { + info.backingPath = NULL; + backingType = NULL; + } + + cmd = virCommandNew(create_tool); + + if (info.inputPath) + virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); + else + virCommandAddArgList(cmd, "create", "-f", type, NULL); + + if (info.backingPath) + virCommandAddArgList(cmd, "-b", info.backingPath, NULL); + + if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS) { + if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat && + imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT) + info.compat = "0.10"; + + if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) { + virCommandFree(cmd); + return NULL; + } + if (opts) + virCommandAddArgList(cmd, "-o", opts, NULL); + VIR_FREE(opts); + } else { + if (info.backingPath) { + 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", + info.path, create_tool); + } + if (info.encryption) + virCommandAddArg(cmd, "-e"); + } + + if (info.inputPath) + virCommandAddArg(cmd, info.inputPath); + virCommandAddArg(cmd, info.path); + if (!info.inputPath && info.size_arg) + virCommandAddArgFormat(cmd, "%lluK", info.size_arg); + return cmd; +} + +/* Create a qemu-img virCommand from the supplied binary path, + * volume definitions and imgformat + */ +virCommandPtr +virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol, + unsigned int flags, + const char *create_tool, + int imgformat) +{ + virCommandPtr cmd = NULL; + struct _virStorageBackendQemuImgInfo info = { + .format = vol->target.format, + .path = vol->target.path, + .encryption = vol->target.encryption != NULL, + .preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA), + .compat = vol->target.compat, + .features = vol->target.features, + .nocow = vol->target.nocow, + }; + + virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); + + /* Treat output block devices as 'raw' format */ + if (vol->type == VIR_STORAGE_VOL_BLOCK) + info.format = VIR_STORAGE_FILE_RAW; + if (inputvol) { if (!(info.inputPath = inputvol->target.path)) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -936,12 +1015,6 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, info.inputFormat = inputvol->target.format; if (inputvol->type == VIR_STORAGE_VOL_BLOCK) info.inputFormat = VIR_STORAGE_FILE_RAW; - if (!(inputType = virStorageFileFormatTypeToString(info.inputFormat))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown storage vol type %d"), - info.inputFormat); - return NULL; - } } if (vol->target.backingStore) { @@ -951,13 +1024,6 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, info.backingFormat = vol->target.backingStore->format; info.backingPath = vol->target.backingStore->path; - if (info.preallocate) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("metadata preallocation conflicts with backing" - " store")); - return NULL; - } - /* XXX: Not strictly required: qemu-img has an option a different * backing store, not really sure what use it serves though, and it * may cause issues with lvm. Untested essentially. @@ -969,13 +1035,6 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, return NULL; } - if (!(backingType = virStorageFileFormatTypeToString(info.backingFormat))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown storage vol backing store type %d"), - info.backingFormat); - return NULL; - } - /* Convert relative backing store paths to absolute paths for access * validation. */ @@ -1000,7 +1059,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, info.format != VIR_STORAGE_FILE_QCOW2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("qcow volume encryption unsupported with " - "volume format %s"), type); + "volume format %s"), + virStorageFileFormatTypeToString(info.format)); return NULL; } enc = vol->target.encryption; @@ -1026,52 +1086,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, /* Size in KB */ info.size_arg = VIR_DIV_UP(vol->target.capacity, 1024); - cmd = virCommandNew(create_tool); - - /* ignore the backing volume when we're converting a volume */ - if (info.inputPath) { - info.backingPath = NULL; - backingType = NULL; - } - - if (info.inputPath) - virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); - else - virCommandAddArgList(cmd, "create", "-f", type, NULL); - - if (info.backingPath) - virCommandAddArgList(cmd, "-b", info.backingPath, NULL); - - if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS) { - if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat && - imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT) - info.compat = "0.10"; - - if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) { - virCommandFree(cmd); - return NULL; - } - if (opts) - virCommandAddArgList(cmd, "-o", opts, NULL); - VIR_FREE(opts); - } else { - if (info.backingPath) { - 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", - info.path, create_tool); - } - if (info.encryption) - virCommandAddArg(cmd, "-e"); - } - - if (info.inputPath) - virCommandAddArg(cmd, info.inputPath); - virCommandAddArg(cmd, info.path); - if (!info.inputPath && info.size_arg) - virCommandAddArgFormat(cmd, "%lluK", info.size_arg); - + cmd = virStorageBackendCreateQemuImgCmd(create_tool, imgformat, info); return cmd; } -- 2.0.5
On Fri, Apr 10, 2015 at 14:59:01 +0200, Ján Tomko wrote:
Do not require the volume definition. This will allow code reuse when creating snapshots. --- src/storage/storage_backend.c | 203 +++++++++++++++++++++++------------------- 1 file changed, 109 insertions(+), 94 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f5b95ec..bfbc193 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -870,38 +870,16 @@ virStorageBackendCreateQemuImgOpts(char **opts, return -1; }
-/* Create a qemu-img virCommand from the supplied binary path, - * volume definitions and imgformat - */ -virCommandPtr -virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, - virStoragePoolObjPtr pool, - virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol, - unsigned int flags, - const char *create_tool, - int imgformat) +static virCommandPtr +virStorageBackendCreateQemuImgCmd(const char *create_tool, + int imgformat, + struct _virStorageBackendQemuImgInfo info) { virCommandPtr cmd = NULL; - const char *type; + const char *type = NULL; const char *backingType = NULL; const char *inputType = NULL; char *opts = NULL; - struct _virStorageBackendQemuImgInfo info = { - .format = vol->target.format, - .path = vol->target.path, - .encryption = vol->target.encryption != NULL, - .preallocate = !!(flags & VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA), - .compat = vol->target.compat, - .features = vol->target.features, - .nocow = vol->target.nocow, - }; - - virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); - - /* Treat output block devices as 'raw' format */ - if (vol->type == VIR_STORAGE_VOL_BLOCK) - info.format = VIR_STORAGE_FILE_RAW;
if (!(type = virStorageFileFormatTypeToString(info.format))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -926,6 +904,107 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, return NULL; }
+ if (info.inputPath && + !(inputType = virStorageFileFormatTypeToString(info.inputFormat))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown storage vol type %d"), + info.inputFormat); + return NULL; + } + + if (info.backingPath) { + if (info.preallocate) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("metadata preallocation conflicts with backing" + " store")); + return NULL; + } + + if (!(backingType = virStorageFileFormatTypeToString(info.backingFormat))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown storage vol backing store type %d"), + info.backingFormat); + return NULL; + } + } + + /* ignore the backing volume when we're converting a volume */ + if (info.inputPath) { + info.backingPath = NULL; + backingType = NULL; + }
Shouldn't this go before you bother to check if the backing info is correct?
+ + cmd = virCommandNew(create_tool); + + if (info.inputPath) + virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL); + else + virCommandAddArgList(cmd, "create", "-f", type, NULL); + + if (info.backingPath) + virCommandAddArgList(cmd, "-b", info.backingPath, NULL); + + if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS) { + if (info.format == VIR_STORAGE_FILE_QCOW2 && !info.compat && + imgformat == QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT) + info.compat = "0.10"; + + if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) { + virCommandFree(cmd); + return NULL; + } + if (opts) + virCommandAddArgList(cmd, "-o", opts, NULL); + VIR_FREE(opts); + } else { + if (info.backingPath) { + 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", + info.path, create_tool); + } + if (info.encryption) + virCommandAddArg(cmd, "-e"); + } + + if (info.inputPath) + virCommandAddArg(cmd, info.inputPath); + virCommandAddArg(cmd, info.path); + if (!info.inputPath && info.size_arg) + virCommandAddArgFormat(cmd, "%lluK", info.size_arg); + return cmd; +}
Looks good otherwise. Peter
Do not loop over enum with one value. --- src/storage/storage_backend.c | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index bfbc193..4ecea88 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -818,7 +818,6 @@ virStorageBackendCreateQemuImgOpts(char **opts, struct _virStorageBackendQemuImgInfo info) { virBuffer buf = VIR_BUFFER_INITIALIZER; - size_t i; if (info.backingPath) virBufferAsprintf(&buf, "backing_fmt=%s,", @@ -832,28 +831,18 @@ virStorageBackendCreateQemuImgOpts(char **opts, if (info.compat) virBufferAsprintf(&buf, "compat=%s,", info.compat); + if (info.features && info.format == VIR_STORAGE_FILE_QCOW2) { - for (i = 0; i < VIR_STORAGE_FILE_FEATURE_LAST; i++) { - if (virBitmapIsBitSet(info.features, i)) { - switch ((virStorageFileFeature) i) { - case VIR_STORAGE_FILE_FEATURE_LAZY_REFCOUNTS: - if (STREQ_NULLABLE(info.compat, "0.10")) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Feature %s not supported with compat" - " level %s"), - virStorageFileFeatureTypeToString(i), - info.compat); - goto error; - } - break; - - /* coverity[dead_error_begin] */ - case VIR_STORAGE_FILE_FEATURE_LAST: - ; - } - virBufferAsprintf(&buf, "%s,", - virStorageFileFeatureTypeToString(i)); + if (virBitmapIsBitSet(info.features, + VIR_STORAGE_FILE_FEATURE_LAZY_REFCOUNTS)) { + if (STREQ_NULLABLE(info.compat, "0.10")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("lazy_refcounts not supported with compat" + " level %s"), + info.compat); + goto error; } + virBufferAddLit(&buf, "lazy_refcounts,"); } } -- 2.0.5
On Fri, Apr 10, 2015 at 14:59:02 +0200, Ján Tomko wrote:
Do not loop over enum with one value. --- src/storage/storage_backend.c | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-)
ACK. Peter
On Mon, Apr 13, 2015 at 07:51:22AM +0200, Peter Krempa wrote:
On Fri, Apr 10, 2015 at 14:59:02 +0200, Ján Tomko wrote:
Do not loop over enum with one value. --- src/storage/storage_backend.c | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-)
ACK.
Thanks, I have pushed all the ACKed patches up to here (that is without patches 3, 4 and 9). Jan
The blockdev-snapshot-sync command only takes a format but does not allow specifying the compat level or other features that should be used. Pre-create the qcow2 file ourselves and tell qemu to reuse it. Note: the default compat level for qemu (and thus external snapshot creation) is now 1.10 but libvirt's storage driver still uses 0.10. After this patch, 0.10 will be the default for both. --- src/qemu/qemu_driver.c | 11 ++++++++ src/storage/storage_backend.c | 66 +++++++++++++++++++++++++++++++++++++++++++ src/storage/storage_backend.h | 5 ++++ src/storage/storage_driver.c | 27 ++++++++++++++++++ src/storage/storage_driver.h | 4 +++ 5 files changed, 113 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 921417c..4f14546 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14073,6 +14073,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, char *device = NULL; char *source = NULL; const char *formatStr = NULL; + const char *qemuImgPath = NULL; int ret = -1, rc; bool need_unlink = false; @@ -14082,6 +14083,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, return -1; } + if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) + goto cleanup; + if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0) goto cleanup; @@ -14114,6 +14118,13 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, goto cleanup; } need_unlink = true; + if (newDiskSrc->format == VIR_STORAGE_FILE_QCOW2) { + rc = virStorageFileCreateWithFormat(newDiskSrc, + source, + disk->src, + qemuImgPath); + reuse = true; + } } /* set correct security, cgroup and locking options on the new image */ diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 4ecea88..9ffbc6e 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1079,6 +1079,72 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, return cmd; } + +/* Create a qemu-img virCommand from the supplied binary path, + * StorageSource and path (translated for network drives + * supported by qemu-img) + */ +static virCommandPtr +virStorageBackendCreateQemuImgCmdFromSource(virStorageSourcePtr src, + const char *path, + virStorageSourcePtr backingSrc, + const char *create_tool) +{ + virCommandPtr cmd = NULL; + struct _virStorageBackendQemuImgInfo info = { + .format = src->format, + .path = path, + .encryption = NULL, + .preallocate = false, + .compat = src->compat, + .features = src->features, + .nocow = src->nocow, + }; + char *tmpstr = NULL; + + info.backingFormat = backingSrc->format; + info.backingPath = backingSrc->path; + + if (!(tmpstr = virBitmapFormat(info.features))) + return NULL; + + VIR_DEBUG("creating file via qemu_img: format %s path %s compat %s features %s", + virStorageFileFormatTypeToString(info.format), + info.path, info.compat, tmpstr); + VIR_DEBUG("... backing format %s backing path %s", + virStorageFileFormatTypeToString(info.backingFormat), + info.backingPath); + + cmd = virStorageBackendCreateQemuImgCmd(create_tool, + QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT, + info); + return cmd; +} + + +int +virStorageBackendCreateQemuImgFileFromSource(virStorageSourcePtr src, + const char *path, + virStorageSourcePtr backingSrc, + const char *create_tool) +{ + int ret = -1; + virCommandPtr cmd = NULL; + + cmd = virStorageBackendCreateQemuImgCmdFromSource(src, path, backingSrc, + create_tool); + if (!cmd) + goto cleanup; + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; + cleanup: + return ret; +} + + static int virStorageBackendCreateQemuImg(virConnectPtr conn, virStoragePoolObjPtr pool, diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index bb1e8d8..c591845 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -249,6 +249,11 @@ virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol); virStorageFileBackendPtr virStorageFileBackendForTypeInternal(int type, int protocol, bool report); +int +virStorageBackendCreateQemuImgFileFromSource(virStorageSourcePtr src, + const char *path, + virStorageSourcePtr backingSrc, + const char *create_tool); struct _virStorageFileBackend { diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 551a0ca..162e065 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2725,6 +2725,33 @@ virStorageFileCreate(virStorageSourcePtr src) return ret; } +/** + * virStorageFileCreate: Creates a storage file stub with the specified + * format + * + * @src: file structure pointing to the file + * @path: path to the file + * @backingSrc: file structure pointing to the backing file + * @create_tool: path to qemu-img + * + * Returns 0 on success, -2 if the function isn't supported by the backend, + * -1 on other failure. Errno is set in case of failure. + */ +int virStorageFileCreateWithFormat(virStorageSourcePtr src, + const char *path, + virStorageSourcePtr backingSrc, + const char *create_tool) +{ + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileCreate) { + errno = ENOSYS; + return -2; + } + + return virStorageBackendCreateQemuImgFileFromSource(src, path, backingSrc, + create_tool); +} + /** * virStorageFileUnlink: Unlink storage file via storage driver diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 4f28dc1..c9d943a 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -36,6 +36,10 @@ int virStorageFileInitAs(virStorageSourcePtr src, void virStorageFileDeinit(virStorageSourcePtr src); int virStorageFileCreate(virStorageSourcePtr src); +int virStorageFileCreateWithFormat(virStorageSourcePtr src, + const char *path, + virStorageSourcePtr backingSrc, + const char *create_tool); int virStorageFileUnlink(virStorageSourcePtr src); int virStorageFileStat(virStorageSourcePtr src, struct stat *stat); -- 2.0.5
On Fri, Apr 10, 2015 at 14:59:03 +0200, Ján Tomko wrote:
The blockdev-snapshot-sync command only takes a format but does not allow specifying the compat level or other features that should be used.
Pre-create the qcow2 file ourselves and tell qemu to reuse it.
Note: the default compat level for qemu (and thus external snapshot creation) is now 1.10 but libvirt's storage driver still uses 0.10. After this patch, 0.10 will be the default for both. --- src/qemu/qemu_driver.c | 11 ++++++++ src/storage/storage_backend.c | 66 +++++++++++++++++++++++++++++++++++++++++++ src/storage/storage_backend.h | 5 ++++ src/storage/storage_driver.c | 27 ++++++++++++++++++ src/storage/storage_driver.h | 4 +++ 5 files changed, 113 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 921417c..4f14546 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14073,6 +14073,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, char *device = NULL; char *source = NULL; const char *formatStr = NULL; + const char *qemuImgPath = NULL; int ret = -1, rc; bool need_unlink = false;
@@ -14082,6 +14083,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, return -1; }
+ if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) + goto cleanup; + if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0) goto cleanup;
@@ -14114,6 +14118,13 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, goto cleanup; } need_unlink = true; + if (newDiskSrc->format == VIR_STORAGE_FILE_QCOW2) { + rc = virStorageFileCreateWithFormat(newDiskSrc, + source, + disk->src, + qemuImgPath); + reuse = true; + }
Since this step is way more prone to fail compared to the existing pre-creation step (that is used just to allow labeling the file before passing it to qemu) I'd rather see this happen prior to actually starting to take the snapshot (at this point, the memory was already snapshotted and the VM is possibly paused, so if this takes a long time or fails we would waste the memory snapshot).
}
/* set correct security, cgroup and locking options on the new image */ diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 4ecea88..9ffbc6e 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1079,6 +1079,72 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, return cmd; }
+ +/* Create a qemu-img virCommand from the supplied binary path, + * StorageSource and path (translated for network drives + * supported by qemu-img) + */ +static virCommandPtr +virStorageBackendCreateQemuImgCmdFromSource(virStorageSourcePtr src, + const char *path, + virStorageSourcePtr backingSrc, + const char *create_tool) +{ + virCommandPtr cmd = NULL; + struct _virStorageBackendQemuImgInfo info = { + .format = src->format, + .path = path, + .encryption = NULL, + .preallocate = false, + .compat = src->compat, + .features = src->features, + .nocow = src->nocow, + }; + char *tmpstr = NULL; + + info.backingFormat = backingSrc->format; + info.backingPath = backingSrc->path;
This definitely is not enough to pass the backing source path. Once you have a network path you need a lot of the fields virStorageSource structure to format the path since it needs to format the qemu-compatible backing string. A better way will be to format the string from backingSrc right at this point to the qemu source string and use that. Additionally, you'll also need
+ + if (!(tmpstr = virBitmapFormat(info.features))) + return NULL; + + VIR_DEBUG("creating file via qemu_img: format %s path %s compat %s features %s", + virStorageFileFormatTypeToString(info.format), + info.path, info.compat, tmpstr); + VIR_DEBUG("... backing format %s backing path %s", + virStorageFileFormatTypeToString(info.backingFormat), + info.backingPath); + + cmd = virStorageBackendCreateQemuImgCmd(create_tool, + QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT, + info); + return cmd; +} + + +int +virStorageBackendCreateQemuImgFileFromSource(virStorageSourcePtr src, + const char *path,
Since you already are passing @src, there should be no need to use @path. Additionally since @path contains authentication data for some protocols we should not pass it to commands that would show in the process list with the arguments. (We already do that when starting the VM. I have it on my todo list)
+ virStorageSourcePtr backingSrc, + const char *create_tool) +{ + int ret = -1; + virCommandPtr cmd = NULL; + + cmd = virStorageBackendCreateQemuImgCmdFromSource(src, path, backingSrc, + create_tool); + if (!cmd) + goto cleanup; + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; + cleanup: + return ret; +} + + static int virStorageBackendCreateQemuImg(virConnectPtr conn, virStoragePoolObjPtr pool, diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index bb1e8d8..c591845 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -249,6 +249,11 @@ virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol); virStorageFileBackendPtr virStorageFileBackendForTypeInternal(int type, int protocol, bool report); +int +virStorageBackendCreateQemuImgFileFromSource(virStorageSourcePtr src, + const char *path, + virStorageSourcePtr backingSrc, + const char *create_tool);
struct _virStorageFileBackend { diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 551a0ca..162e065 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2725,6 +2725,33 @@ virStorageFileCreate(virStorageSourcePtr src) return ret; }
+/** + * virStorageFileCreate: Creates a storage file stub with the specified + * format + * + * @src: file structure pointing to the file + * @path: path to the file + * @backingSrc: file structure pointing to the backing file + * @create_tool: path to qemu-img + * + * Returns 0 on success, -2 if the function isn't supported by the backend, + * -1 on other failure. Errno is set in case of failure. + */ +int virStorageFileCreateWithFormat(virStorageSourcePtr src, + const char *path, + virStorageSourcePtr backingSrc, + const char *create_tool) +{ + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileCreate) { + errno = ENOSYS;
This check doesn't make sense since storageFileCreate is not used. The function that calls qemu-img should make sure that qemu-img actually is able to create the file.
+ return -2; + } + + return virStorageBackendCreateQemuImgFileFromSource(src, path, backingSrc, + create_tool); +} +
/** * virStorageFileUnlink: Unlink storage file via storage driver
Peter
Reuse the code from storage backend. This also fixes the backing_fmd typo by removing it. --- src/qemu/qemu_driver.c | 46 ++++++++++--------------------------------- src/storage/storage_backend.c | 2 +- 2 files changed, 11 insertions(+), 37 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4f14546..3ea42f2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13468,7 +13468,6 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, size_t i; virDomainSnapshotDiskDefPtr snapdisk; virDomainDiskDefPtr defdisk; - virCommandPtr cmd = NULL; const char *qemuImgPath; virBitmapPtr created = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -13489,48 +13488,25 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue; - if (!snapdisk->src->format) - snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; - - /* creates cmd line args: qemu-img create -f qcow2 -o */ - if (!(cmd = virCommandNewArgList(qemuImgPath, - "create", - "-f", - virStorageFileFormatTypeToString(snapdisk->src->format), - "-o", - NULL))) + if (defdisk->src->format == VIR_STORAGE_FILE_NONE && + !cfg->allowDiskFormatProbing) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown image format of '%s' and " + "format probing is disabled"), + defdisk->src->path); goto cleanup; - - if (defdisk->src->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->path, - virStorageFileFormatTypeToString(defdisk->src->format)); - } else { - if (!cfg->allowDiskFormatProbing) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown image format of '%s' and " - "format probing is disabled"), - defdisk->src->path); - goto cleanup; - } - - /* adds cmd line arg: backing_file=/path/to/backing/file */ - virCommandAddArgFormat(cmd, "backing_file=%s", defdisk->src->path); } - /* adds cmd line args: /path/to/target/file */ - virCommandAddArg(cmd, snapdisk->src->path); + if (!snapdisk->src->format) + snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; /* If the target does not exist, we're going to create it possibly */ if (!virFileExists(snapdisk->src->path)) ignore_value(virBitmapSetBit(created, i)); - if (virCommandRun(cmd, NULL) < 0) + if (virStorageFileCreateWithFormat(snapdisk->src, snapdisk->src->path, + defdisk->src, qemuImgPath) < 0) goto cleanup; - - virCommandFree(cmd); - cmd = NULL; } /* update disk definitions */ @@ -13554,8 +13530,6 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, ret = 0; cleanup: - virCommandFree(cmd); - /* unlink images if creation has failed */ if (ret < 0 && created) { ssize_t bit = -1; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 9ffbc6e..ec8b7b5 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -819,7 +819,7 @@ virStorageBackendCreateQemuImgOpts(char **opts, { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (info.backingPath) + if (info.backingPath && info.backingFormat) virBufferAsprintf(&buf, "backing_fmt=%s,", virStorageFileFormatTypeToString(info.backingFormat)); if (info.encryption) -- 2.0.5
On Fri, Apr 10, 2015 at 14:59:04 +0200, Ján Tomko wrote:
Reuse the code from storage backend.
This also fixes the backing_fmd typo by removing it. --- src/qemu/qemu_driver.c | 46 ++++++++++--------------------------------- src/storage/storage_backend.c | 2 +- 2 files changed, 11 insertions(+), 37 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4f14546..3ea42f2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13468,7 +13468,6 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, size_t i; virDomainSnapshotDiskDefPtr snapdisk; virDomainDiskDefPtr defdisk; - virCommandPtr cmd = NULL; const char *qemuImgPath; virBitmapPtr created = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -13489,48 +13488,25 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue;
- if (!snapdisk->src->format) - snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; - - /* creates cmd line args: qemu-img create -f qcow2 -o */ - if (!(cmd = virCommandNewArgList(qemuImgPath, - "create", - "-f", - virStorageFileFormatTypeToString(snapdisk->src->format), - "-o", - NULL))) + if (defdisk->src->format == VIR_STORAGE_FILE_NONE && + !cfg->allowDiskFormatProbing) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown image format of '%s' and " + "format probing is disabled"), + defdisk->src->path); goto cleanup; - - if (defdisk->src->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->path, - virStorageFileFormatTypeToString(defdisk->src->format)); - } else { - if (!cfg->allowDiskFormatProbing) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown image format of '%s' and " - "format probing is disabled"), - defdisk->src->path); - goto cleanup; - } - - /* adds cmd line arg: backing_file=/path/to/backing/file */ - virCommandAddArgFormat(cmd, "backing_file=%s", defdisk->src->path); }
- /* adds cmd line args: /path/to/target/file */ - virCommandAddArg(cmd, snapdisk->src->path); + if (!snapdisk->src->format) + snapdisk->src->format = VIR_STORAGE_FILE_QCOW2;
/* If the target does not exist, we're going to create it possibly */ if (!virFileExists(snapdisk->src->path)) ignore_value(virBitmapSetBit(created, i));
- if (virCommandRun(cmd, NULL) < 0) + if (virStorageFileCreateWithFormat(snapdisk->src, snapdisk->src->path, + defdisk->src, qemuImgPath) < 0)
This part will need adjusting after my review to the previous patch.
goto cleanup; - - virCommandFree(cmd); - cmd = NULL; }
/* update disk definitions */ @@ -13554,8 +13530,6 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, ret = 0;
cleanup: - virCommandFree(cmd); - /* unlink images if creation has failed */ if (ret < 0 && created) { ssize_t bit = -1; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 9ffbc6e..ec8b7b5 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -819,7 +819,7 @@ virStorageBackendCreateQemuImgOpts(char **opts, { virBuffer buf = VIR_BUFFER_INITIALIZER;
- if (info.backingPath) + if (info.backingPath && info.backingFormat)
In retrospect I think we should forbid snapshots without specifying the format so that we don't create even more broken configurations.
virBufferAsprintf(&buf, "backing_fmt=%s,", virStorageFileFormatTypeToString(info.backingFormat)); if (info.encryption)
Peter
This allows creating an external qcow2 snapshot with qcow2 features, e.g: <disk name='hda' snapshot='external' type='file'> <source file='/path/to/file'/> <compat>1.1</compat> <features> <lazy_refcounts/> </features> </disk> https://bugzilla.redhat.com/show_bug.cgi?id=980327 --- docs/formatsnapshot.html.in | 9 +++++++ docs/schemas/domainsnapshot.rng | 10 +++++++- src/conf/snapshot_conf.c | 12 +++++++++ .../disk_snapshot_features.xml | 30 ++++++++++++++++++++++ .../disk_snapshot_features.xml | 30 ++++++++++++++++++++++ tests/domainsnapshotxml2xmltest.c | 2 ++ 6 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 tests/domainsnapshotxml2xmlin/disk_snapshot_features.xml create mode 100644 tests/domainsnapshotxml2xmlout/disk_snapshot_features.xml diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index c3ab516..569dd24 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -180,6 +180,15 @@ as qcow2), of the new file created by the external snapshot of the new file. </dd> + <dt><code>compat</code></dt> + <dd>Optional. Allows specifying the compatibility level for qcow2 volumes. + So far, this is only used for type='qcow2' volumes. Valid values are 0.10 and 1.1, + specifying QEMU version the images should be compatible with. + If the feature element is present, 1.1 is used. If omitted, 0.10 is used. + <span class="since">Since 1.2.15</span></dd> + <dt><code>features</code></dt> + <dd>Format-specific features. See the features element in + <a href="formatstorage.html">volume target elements</a> for valid features</dd> </dl> <span class="since">Since 1.2.2</span> the <code>disk</code> element diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 4ab1b82..a6d0202 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -109,7 +109,15 @@ <define name='storageSourceExtra' combine='choice'> <!-- overrides the no-op version in storagecommon.rng --> - <ref name='disksnapshotdriver'/> + <interleave> + <ref name='disksnapshotdriver'/> + <optional> + <ref name='compat'/> + </optional> + <optional> + <ref name='fileFormatFeatures'/> + </optional> + </interleave> </define> <define name='disksnapshot'> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index ca6796a..c2b36e2 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -41,6 +41,7 @@ #include "nwfilter_conf.h" #include "secret_conf.h" #include "snapshot_conf.h" +#include "storage_feature_conf.h" #include "virstoragefile.h" #include "viruuid.h" #include "virfile.h" @@ -169,6 +170,15 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, } } + def->src->compat = virXPathString("string(./compat)", ctxt); + if (!virStorageFileCheckCompat(def->src->compat)) + goto cleanup; + + if (virStorageFeaturesParse(ctxt, "./features", + &def->src->compat, + &def->src->features) < 0) + goto cleanup; + /* validate that the passed path is absolute */ if (virStorageSourceIsLocalStorage(def->src) && def->src->path && @@ -656,6 +666,8 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<driver type='%s'/>\n", virStorageFileFormatTypeToString(disk->src->format)); virDomainDiskSourceFormat(buf, disk->src, 0, 0); + virBufferEscapeString(buf, "<compat>%s</compat>\n", disk->src->compat); + virStorageFeaturesFormat(buf, disk->src->features); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</disk>\n"); diff --git a/tests/domainsnapshotxml2xmlin/disk_snapshot_features.xml b/tests/domainsnapshotxml2xmlin/disk_snapshot_features.xml new file mode 100644 index 0000000..499c28f --- /dev/null +++ b/tests/domainsnapshotxml2xmlin/disk_snapshot_features.xml @@ -0,0 +1,30 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <disks> + <disk name='hda' snapshot='external' type='file'> + <driver type='qcow2'/> + <source file='/path/to/new'/> + <compat>0.10</compat> + </disk> + <disk name='hdb' snapshot='external' type='file'> + <source file='/path/to/new2'/> + <compat>1.1</compat> + <features> + <lazy_refcounts/> + </features> + </disk> + <disk name='hdc' snapshot='external' type='file'> + <source file='/path/to/new2'/> + <compat>1.1</compat> + <features/> + </disk> + <disk name='hdd' snapshot='external' type='file'> + <source file='/path/to/new2'/> + <compat>1.1</compat> + <features> + <lazy_refcounts/> + </features> + </disk> + </disks> +</domainsnapshot> diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot_features.xml b/tests/domainsnapshotxml2xmlout/disk_snapshot_features.xml new file mode 100644 index 0000000..499c28f --- /dev/null +++ b/tests/domainsnapshotxml2xmlout/disk_snapshot_features.xml @@ -0,0 +1,30 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <disks> + <disk name='hda' snapshot='external' type='file'> + <driver type='qcow2'/> + <source file='/path/to/new'/> + <compat>0.10</compat> + </disk> + <disk name='hdb' snapshot='external' type='file'> + <source file='/path/to/new2'/> + <compat>1.1</compat> + <features> + <lazy_refcounts/> + </features> + </disk> + <disk name='hdc' snapshot='external' type='file'> + <source file='/path/to/new2'/> + <compat>1.1</compat> + <features/> + </disk> + <disk name='hdd' snapshot='external' type='file'> + <source file='/path/to/new2'/> + <compat>1.1</compat> + <features> + <lazy_refcounts/> + </features> + </disk> + </disks> +</domainsnapshot> diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index 845d52f..a25871b 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -219,10 +219,12 @@ mymain(void) DO_TEST_INOUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", false, false); DO_TEST_INOUT("disk_snapshot", NULL, false, false); DO_TEST_INOUT("disk_driver_name_null", NULL, false, false); + DO_TEST_INOUT("disk_snapshot_features", NULL, false, false); DO_TEST_IN("name_and_description", NULL); DO_TEST_IN("description_only", NULL); DO_TEST_IN("name_only", NULL); + DO_TEST_IN("disk_snapshot_features", NULL); cleanup: if (testSnapshotXMLVariableLineRegex) -- 2.0.5
On Fri, Apr 10, 2015 at 14:59:05 +0200, Ján Tomko wrote:
This allows creating an external qcow2 snapshot with qcow2 features, e.g:
<disk name='hda' snapshot='external' type='file'> <source file='/path/to/file'/> <compat>1.1</compat> <features> <lazy_refcounts/> </features> </disk>
https://bugzilla.redhat.com/show_bug.cgi?id=980327 --- docs/formatsnapshot.html.in | 9 +++++++ docs/schemas/domainsnapshot.rng | 10 +++++++- src/conf/snapshot_conf.c | 12 +++++++++ .../disk_snapshot_features.xml | 30 ++++++++++++++++++++++ .../disk_snapshot_features.xml | 30 ++++++++++++++++++++++ tests/domainsnapshotxml2xmltest.c | 2 ++ 6 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 tests/domainsnapshotxml2xmlin/disk_snapshot_features.xml create mode 100644 tests/domainsnapshotxml2xmlout/disk_snapshot_features.xml
diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index c3ab516..569dd24 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -180,6 +180,15 @@ as qcow2), of the new file created by the external snapshot of the new file. </dd> + <dt><code>compat</code></dt> + <dd>Optional. Allows specifying the compatibility level for qcow2 volumes. + So far, this is only used for type='qcow2' volumes. Valid values are 0.10 and 1.1, + specifying QEMU version the images should be compatible with. + If the feature element is present, 1.1 is used. If omitted, 0.10 is used.
For this particular case I think we should drop the last sentence. If the <compat> element is not present, regardless of the <feature> element we should use qemu default type. If a user wants to explicitly use a image type he should explicitly specify it.
+ <span class="since">Since 1.2.15</span></dd> + <dt><code>features</code></dt> + <dd>Format-specific features. See the features element in + <a href="formatstorage.html">volume target elements</a> for valid features</dd> </dl>
<span class="since">Since 1.2.2</span> the <code>disk</code> element
Peter
participants (2)
-
Ján Tomko -
Peter Krempa