7:58 a.m.
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
7:58 a.m.
New subject: [libvirt] [PATCHv1 01/13] Visually separate snapshot disk subelements
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
12:16 a.m.
New subject: [libvirt] [PATCHv1 01/13] Visually separate snapshot disk subelements
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
7:58 a.m.
New subject: [libvirt] [PATCHv1 02/13] Remove feature formating funcs from pool-specific options
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
12:43 a.m.
New subject: [libvirt] [PATCHv1 02/13] Remove feature formating funcs from pool-specific options
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
7:58 a.m.
New subject: [libvirt] [PATCHv1 03/13] Separate out virStorageFeatureParse
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
12:41 a.m.
New subject: [libvirt] [PATCHv1 03/13] Separate out virStorageFeatureParse
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
8:36 a.m.
New subject: [libvirt] [PATCHv1 03/13] Separate out virStorageFeatureParse
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
7:58 a.m.
New subject: [libvirt] [PATCHv1 04/13] Separate virStorageFeatureFormat
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
12:30 a.m.
New subject: [libvirt] [PATCHv1 04/13] Separate virStorageFeatureFormat
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
7:58 a.m.
New subject: [libvirt] [PATCHv1 05/13] Split out storage format 'compat' attribute sanity check
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
12:28 a.m.
New subject: [libvirt] [PATCHv1 05/13] Split out storage format 'compat' attribute sanity check
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
7:58 a.m.
New subject: [libvirt] [PATCHv1 06/13] Use XPath when parsing snapshot disk definition
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
12:23 a.m.
New subject: [libvirt] [PATCHv1 06/13] Use XPath when parsing snapshot disk definition
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
7:58 a.m.
New subject: [libvirt] [PATCHv1 07/13] Rename virStorageBackendCreateQemuImgCmd
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
1:11 a.m.
New subject: [libvirt] [PATCHv1 07/13] Rename virStorageBackendCreateQemuImgCmd
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
7:59 a.m.
New subject: [libvirt] [PATCHv1 08/13] Introduce struct _virStorageBackendQemuImgInfo
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
1:55 a.m.
New subject: [libvirt] [PATCHv1 08/13] Introduce struct _virStorageBackendQemuImgInfo
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
7:59 a.m.
New subject: [libvirt] [PATCHv1 09/13] Split out qemu-img command generation
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
2:03 a.m.
New subject: [libvirt] [PATCHv1 09/13] Split out qemu-img command generation
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
7:59 a.m.
New subject: [libvirt] [PATCHv1 10/13] Remove overengineered loop
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
12:51 a.m.
New subject: [libvirt] [PATCHv1 10/13] Remove overengineered loop
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
9:06 a.m.
New subject: [libvirt] [PATCHv1 10/13] Remove overengineered loop
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
7:59 a.m.
New subject: [libvirt] [PATCHv1 11/13] Use qemu-img to precreate the qcow2 disks
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
1:10 a.m.
New subject: [libvirt] [PATCHv1 11/13] Use qemu-img to precreate the qcow2 disks
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
7:59 a.m.
New subject: [libvirt] [PATCHv1 12/13] Rework qemu-img command generation for inactive external snapshots
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
2:14 a.m.
New subject: [libvirt] [PATCHv1 12/13] Rework qemu-img command generation for inactive external snapshots
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
7:59 a.m.
New subject: [libvirt] [PATCHv1 13/13] Add qcow2 features to snapshot XML
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
1:16 a.m.
New subject: [libvirt] [PATCHv1 13/13] Add qcow2 features to snapshot XML
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
3511
days inactive
3514
days old
28 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa