10:48 a.m.
This version was rebased on top of Eric's gluster pool series and a few
small mistakes were fixed. As nobody actually reviewed the original posting
I did not bother putting the differences in the patches.
Peter Krempa (27):
conf: Export virStorageVolType enum helper functions
test: Implement fake storage pool driver in qemuxml2argv test
qemuxml2argv: Add test to verify correct usage of disk type="volume"
qemuxml2argv: Add test for disk type='volume' with iSCSI pools
qemu: Split out formatting of network disk source URI
qemu: Simplify call pattern of qemuBuildDriveURIString
qemu: Use qemuBuildNetworkDriveURI to handle http/ftp and friends
qemu: Migrate sheepdog source generation into common function
qemu: Split out NBD command generation
qemu: Unify formatting of RBD sources
qemu: Refactor disk source string formatting
conf: Support disk source formatting without needing a
virDomainDiskDefPtr
conf: Clean up virDomainDiskSourceDefFormatInternal
conf: Split out seclabel formating code for disk source
conf: Export disk source formatter and parser
snapshot: conf: Use common parsing and formatting functions for source
snapshot: conf: Fix NULL dereference when <driver> element is empty
conf: Add functions to copy and free network disk source definitions
qemu: snapshot: Detect internal snapshots also for sheepdog and RBD
conf: Add helper do clear disk source authentication struct
qemu: Clear old translated pool source
qemu: snapshot: Touch up error message
qemu: snapshot: Add functions similar to disk source pool translation
qemu: snapshots: Declare supported and unsupported snapshot configs
RFC: snapshot: Add support for specifying snapshot disk backing type
RFC: conf: snapshot: Parse more snapshot information
RFC: qemu: snapshot: Add support for external active snapshots on
gluster
src/conf/domain_conf.c | 261 ++++++---
src/conf/domain_conf.h | 25 +
src/conf/snapshot_conf.c | 69 ++-
src/conf/snapshot_conf.h | 14 +-
src/libvirt_private.syms | 6 +
src/qemu/qemu_command.c | 652 +++++++++++----------
src/qemu/qemu_command.h | 15 +
src/qemu/qemu_conf.c | 156 +++--
src/qemu/qemu_conf.h | 8 +
src/qemu/qemu_driver.c | 429 ++++++++++++--
.../qemuxml2argv-disk-source-pool-mode.args | 10 +
.../qemuxml2argv-disk-source-pool-mode.xml | 4 +-
.../qemuxml2argv-disk-source-pool.args | 8 +
.../qemuxml2argv-disk-source-pool.xml | 2 +-
tests/qemuxml2argvtest.c | 166 ++++++
15 files changed, 1298 insertions(+), 527 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args
--
1.8.4.3
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 01/27] conf: Export virStorageVolType enum helper functions
Export string conversion from and to the virStorageVolType enum.
---
src/libvirt_private.syms | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a705c56..205fe56 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -701,6 +701,8 @@ virStorageVolDefFree;
virStorageVolDefParseFile;
virStorageVolDefParseNode;
virStorageVolDefParseString;
+virStorageVolTypeFromString;
+virStorageVolTypeToString;
# conf/storage_encryption_conf.h
--
1.8.4.3
12:31 p.m.
New subject: [libvirt] [PATCHv1.5 01/27] conf: Export virStorageVolType enum helper functions
On 26.11.2013 17:48, Peter Krempa wrote:
Export string conversion from and to the virStorageVolType enum.
---
src/libvirt_private.syms | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a705c56..205fe56 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -701,6 +701,8 @@ virStorageVolDefFree;
virStorageVolDefParseFile;
virStorageVolDefParseNode;
virStorageVolDefParseString;
+virStorageVolTypeFromString;
+virStorageVolTypeToString;
# conf/storage_encryption_conf.h
ACK, although it can be squashed into 02/27 where the functions are
first used outside storage driver.
Michal
8:27 a.m.
New subject: [libvirt] [PATCHv1.5 01/27] conf: Export virStorageVolType enum helper functions
On 11/26/13 19:31, Michal Privoznik wrote:
On 26.11.2013 17:48, Peter Krempa wrote:
> Export string conversion from and to the virStorageVolType enum.
> ---
> src/libvirt_private.syms | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a705c56..205fe56 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -701,6 +701,8 @@ virStorageVolDefFree;
> virStorageVolDefParseFile;
> virStorageVolDefParseNode;
> virStorageVolDefParseString;
> +virStorageVolTypeFromString;
> +virStorageVolTypeToString;
>
>
> # conf/storage_encryption_conf.h
>
ACK, although it can be squashed into 02/27 where the functions are
first used outside storage driver.
I'd like to keep this one separate as I'll need to backport it
separately from 02/27. I've gone ahead and pushed it.
Peter
Michal
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 02/27] test: Implement fake storage pool driver in qemuxml2argv test
To support testing of "volume" disk backing, we need to implement a few
disk driver backend functions.
The fake storage driver uses files in storagepoolxml2xmlout/POOLNAME.xml
as XML files for pool definitions and volume names are in format
"VOL_TYPE+VOL_PATH". By default type "block" is assumed (for iSCSI
test
compatibility).
The choice of this approach along with implemented functions was made so
that <disk type='volume'> can be tested in the xml2argv test.
---
tests/qemuxml2argvtest.c | 162 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 162 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a290062..a4cef84 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -18,6 +18,7 @@
# include "qemu/qemu_command.h"
# include "qemu/qemu_domain.h"
# include "datatypes.h"
+# include "conf/storage_conf.h"
# include "cpu/cpu_map.h"
# include "virstring.h"
@@ -75,6 +76,161 @@ static virSecretDriver fakeSecretDriver = {
.secretUndefine = NULL,
};
+
+# define STORAGE_POOL_XML_PATH "storagepoolxml2xmlout/"
+static const unsigned char fakeUUID[VIR_UUID_BUFLEN] = "fakeuuid";
+
+static virStoragePoolPtr
+fakeStoragePoolLookupByName(virConnectPtr conn,
+ const char *name)
+{
+ char *xmlpath = NULL;
+ virStoragePoolPtr ret = NULL;
+
+ if (STRNEQ(name, "inactive")) {
+ if (virAsprintf(&xmlpath, "%s%s.xml",
+ STORAGE_POOL_XML_PATH,
+ name) < 0)
+ return NULL;
+
+ if (!virFileExists(xmlpath)) {
+ virReportError(VIR_ERR_NO_STORAGE_POOL,
+ "File '%s' not found", xmlpath);
+ goto cleanup;
+ }
+ }
+
+ ret = virGetStoragePool(conn, name, fakeUUID, NULL, NULL);
+
+cleanup:
+ VIR_FREE(xmlpath);
+ return ret;
+}
+
+
+static virStorageVolPtr
+fakeStorageVolLookupByName(virStoragePoolPtr pool,
+ const char *name)
+{
+ char **volinfo = NULL;
+ virStorageVolPtr ret = NULL;
+
+ if (STREQ(pool->name, "inactive")) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "storage pool '%s' is not active",
pool->name);
+ return NULL;
+ }
+
+ if (STREQ(name, "nonexistent")) {
+ virReportError(VIR_ERR_NO_STORAGE_VOL,
+ "no storage vol with matching name '%s'",
name);
+ return NULL;
+ }
+
+ if (!strchr(name, '+'))
+ goto fallback;
+
+ if (!(volinfo = virStringSplit(name, "+", 2)))
+ return NULL;
+
+ if (!volinfo[1])
+ goto fallback;
+
+ ret = virGetStorageVol(pool->conn, pool->name, volinfo[1], volinfo[0],
+ NULL, NULL);
+
+cleanup:
+ virStringFreeList(volinfo);
+ return ret;
+
+fallback:
+ ret = virGetStorageVol(pool->conn, pool->name, name, "block", NULL,
NULL);
+ goto cleanup;
+}
+
+static int
+fakeStorageVolGetInfo(virStorageVolPtr vol,
+ virStorageVolInfoPtr info)
+{
+ memset(info, 0, sizeof(*info));
+
+ info->type = virStorageVolTypeFromString(vol->key);
+
+ if (info->type < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "Invalid volume type '%s'", vol->key);
+ return -1;
+ }
+
+ return 0;
+}
+
+
+static char *
+fakeStorageVolGetPath(virStorageVolPtr vol)
+{
+ char *ret = NULL;
+
+ ignore_value(virAsprintf(&ret, "/some/%s/device/%s", vol->key,
vol->name));
+
+ return ret;
+}
+
+
+static char *
+fakeStoragePoolDumpXML(virStoragePoolPtr pool,
+ unsigned int flags_unused ATTRIBUTE_UNUSED)
+{
+ char *xmlpath = NULL;
+ char *xmlbuf = NULL;
+
+ if (STREQ(pool->name, "inactive")) {
+ virReportError(VIR_ERR_NO_STORAGE_POOL, NULL);
+ return NULL;
+ }
+
+ if (virAsprintf(&xmlpath, "%s%s.xml",
+ STORAGE_POOL_XML_PATH,
+ pool->name) < 0)
+ return NULL;
+
+ if (virtTestLoadFile(xmlpath, &xmlbuf) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "failed to load XML file '%s'",
+ xmlpath);
+ goto cleanup;
+ }
+
+cleanup:
+ VIR_FREE(xmlpath);
+
+ return xmlbuf;
+}
+
+static int
+fakeStorageClose(virConnectPtr conn ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+
+static int
+fakeStoragePoolIsActive(virStoragePoolPtr pool ATTRIBUTE_UNUSED)
+{
+ return 1;
+}
+
+
+static virStorageDriver fakeStorageDriver = {
+ .name = "fake_storage",
+ .storageClose = fakeStorageClose,
+ .storagePoolLookupByName = fakeStoragePoolLookupByName,
+ .storageVolLookupByName = fakeStorageVolLookupByName,
+ .storagePoolGetXMLDesc = fakeStoragePoolDumpXML,
+ .storageVolGetPath = fakeStorageVolGetPath,
+ .storageVolGetInfo = fakeStorageVolGetInfo,
+ .storagePoolIsActive = fakeStoragePoolIsActive,
+};
+
typedef enum {
FLAG_EXPECT_ERROR = 1 << 0,
FLAG_EXPECT_FAILURE = 1 << 1,
@@ -103,6 +259,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
if (!(conn = virGetConnect()))
goto out;
conn->secretDriver = &fakeSecretDriver;
+ conn->storageDriver = &fakeStorageDriver;
if (!(vmdef = virDomainDefParseFile(xml, driver.caps, driver.xmlopt,
QEMU_EXPECTED_VIRT_TYPES,
@@ -165,6 +322,11 @@ static int testCompareXMLToArgvFiles(const char *xml,
}
}
+ for (i = 0; i < vmdef->ndisks; i++) {
+ if (qemuTranslateDiskSourcePool(conn, vmdef->disks[i]) < 0)
+ goto out;
+ }
+
if (!(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr,
(flags & FLAG_JSON), extraFlags,
migrateFrom, migrateFd, NULL,
--
1.8.4.3
12:31 p.m.
New subject: [libvirt] [PATCHv1.5 02/27] test: Implement fake storage pool driver in qemuxml2argv test
On 26.11.2013 17:48, Peter Krempa wrote:
To support testing of "volume" disk backing, we need to
implement a few
disk driver backend functions.
The fake storage driver uses files in storagepoolxml2xmlout/POOLNAME.xml
as XML files for pool definitions and volume names are in format
"VOL_TYPE+VOL_PATH". By default type "block" is assumed (for iSCSI
test
compatibility).
The choice of this approach along with implemented functions was made so
that <disk type='volume'> can be tested in the xml2argv test.
---
tests/qemuxml2argvtest.c | 162 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 162 insertions(+)
ACK
Michal
1:47 a.m.
New subject: [libvirt] [PATCHv1.5 02/27] test: Implement fake storage pool driver in qemuxml2argv test
On 27/11/13 00:48, Peter Krempa wrote:
To support testing of "volume" disk backing, we need to
implement a few
disk driver backend functions.
The fake storage driver uses files in storagepoolxml2xmlout/POOLNAME.xml
as XML files for pool definitions and volume names are in format
"VOL_TYPE+VOL_PATH". By default type "block" is assumed (for iSCSI
test
compatibility).
The choice of this approach along with implemented functions was made so
that <disk type='volume'> can be tested in the xml2argv test.
---
tests/qemuxml2argvtest.c | 162 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 162 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a290062..a4cef84 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -18,6 +18,7 @@
# include "qemu/qemu_command.h"
# include "qemu/qemu_domain.h"
# include "datatypes.h"
+# include "conf/storage_conf.h"
# include "cpu/cpu_map.h"
# include "virstring.h"
@@ -75,6 +76,161 @@ static virSecretDriver fakeSecretDriver = {
.secretUndefine = NULL,
};
+
+# define STORAGE_POOL_XML_PATH "storagepoolxml2xmlout/"
This will cause build failure when building with VPATH.
+static const unsigned char fakeUUID[VIR_UUID_BUFLEN] =
"fakeuuid";
+
+static virStoragePoolPtr
+fakeStoragePoolLookupByName(virConnectPtr conn,
+ const char *name)
+{
+ char *xmlpath = NULL;
+ virStoragePoolPtr ret = NULL;
+
+ if (STRNEQ(name, "inactive")) {
+ if (virAsprintf(&xmlpath, "%s%s.xml",
+ STORAGE_POOL_XML_PATH,
+ name) < 0)
+ return NULL;
+
+ if (!virFileExists(xmlpath)) {
+ virReportError(VIR_ERR_NO_STORAGE_POOL,
+ "File '%s' not found", xmlpath);
+ goto cleanup;
+ }
+ }
+
+ ret = virGetStoragePool(conn, name, fakeUUID, NULL, NULL);
Looks like "fakeUUID" is only used here, so generating a random UUID
might work.
+
+cleanup:
+ VIR_FREE(xmlpath);
+ return ret;
+}
+
+
+static virStorageVolPtr
+fakeStorageVolLookupByName(virStoragePoolPtr pool,
+ const char *name)
+{
+ char **volinfo = NULL;
+ virStorageVolPtr ret = NULL;
+
+ if (STREQ(pool->name, "inactive")) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "storage pool '%s' is not active",
pool->name);
+ return NULL;
+ }
+
+ if (STREQ(name, "nonexistent")) {
It will be better if it has document what these magic strings mean.
+ virReportError(VIR_ERR_NO_STORAGE_VOL,
+ "no storage vol with matching name '%s'",
name);
+ return NULL;
+ }
+
+ if (!strchr(name, '+'))
+ goto fallback;
+
+ if (!(volinfo = virStringSplit(name, "+", 2)))
+ return NULL;
+
+ if (!volinfo[1])
+ goto fallback;
+
+ ret = virGetStorageVol(pool->conn, pool->name, volinfo[1], volinfo[0],
+ NULL, NULL);
+
+cleanup:
+ virStringFreeList(volinfo);
+ return ret;
+
+fallback:
+ ret = virGetStorageVol(pool->conn, pool->name, name, "block", NULL,
NULL);
+ goto cleanup;
+}
+
+static int
+fakeStorageVolGetInfo(virStorageVolPtr vol,
+ virStorageVolInfoPtr info)
+{
+ memset(info, 0, sizeof(*info));
+
+ info->type = virStorageVolTypeFromString(vol->key);
+
+ if (info->type < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "Invalid volume type '%s'", vol->key);
+ return -1;
+ }
+
+ return 0;
+}
+
+
+static char *
+fakeStorageVolGetPath(virStorageVolPtr vol)
+{
+ char *ret = NULL;
+
+ ignore_value(virAsprintf(&ret, "/some/%s/device/%s", vol->key,
vol->name));
+
+ return ret;
+}
+
+
+static char *
+fakeStoragePoolDumpXML(virStoragePoolPtr pool,
Better to rename it as fakeStoragePoolGetXMLDesc to keep consistent.
"DumpXML" is used in virsh.
Osier
3:58 a.m.
New subject: [libvirt] [PATCHv1.5 02/27] test: Implement fake storage pool driver in qemuxml2argv test
On 11/27/13 08:47, Osier Yang wrote:
On 27/11/13 00:48, Peter Krempa wrote:
> To support testing of "volume" disk backing, we need to implement a few
> disk driver backend functions.
>
> The fake storage driver uses files in storagepoolxml2xmlout/POOLNAME.xml
> as XML files for pool definitions and volume names are in format
> "VOL_TYPE+VOL_PATH". By default type "block" is assumed (for
iSCSI test
> compatibility).
>
> The choice of this approach along with implemented functions was made so
> that <disk type='volume'> can be tested in the xml2argv test.
> ---
> tests/qemuxml2argvtest.c | 162
> +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 162 insertions(+)
>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index a290062..a4cef84 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -18,6 +18,7 @@
> # include "qemu/qemu_command.h"
> # include "qemu/qemu_domain.h"
> # include "datatypes.h"
> +# include "conf/storage_conf.h"
> # include "cpu/cpu_map.h"
> # include "virstring.h"
>
> @@ -75,6 +76,161 @@ static virSecretDriver fakeSecretDriver = {
> .secretUndefine = NULL,
> };
>
> +
> +# define STORAGE_POOL_XML_PATH "storagepoolxml2xmlout/"
This will cause build failure when building with VPATH.
Hmmm, I'll look into it.
> +static const unsigned char fakeUUID[VIR_UUID_BUFLEN] = "fakeuuid";
> +
> +static virStoragePoolPtr
> +fakeStoragePoolLookupByName(virConnectPtr conn,
> + const char *name)
> +{
> + char *xmlpath = NULL;
> + virStoragePoolPtr ret = NULL;
> +
> + if (STRNEQ(name, "inactive")) {
> + if (virAsprintf(&xmlpath, "%s%s.xml",
> + STORAGE_POOL_XML_PATH,
> + name) < 0)
> + return NULL;
> +
> + if (!virFileExists(xmlpath)) {
> + virReportError(VIR_ERR_NO_STORAGE_POOL,
> + "File '%s' not found", xmlpath);
> + goto cleanup;
> + }virGetStoragePool
> + }
> +
> + ret = virGetStoragePool(conn, name, fakeUUID, NULL, NULL);
Looks like "fakeUUID" is only used here, so generating a random UUID
might work.
Random? Why bother? This static one is used so that virGetStoragePool()
doesn't crash and isn't used anywhere else.
> +
> +cleanup:
> + VIR_FREE(xmlpath);
> + return ret;
> +}
> +
> +
> +static virStorageVolPtr
> +fakeStorageVolLookupByName(virStoragePoolPtr pool,
> + const char *name)
> +{
> + char **volinfo = NULL;
> + virStorageVolPtr ret = NULL;
> +
> + if (STREQ(pool->name, "inactive")) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + "storage pool '%s' is not active",
pool->name);
> + return NULL;
> + }
> +
> + if (STREQ(name, "nonexistent")) {
It will be better if it has document what these magic strings mean.
Yeah, I forgot this one ...
> + virReportError(VIR_ERR_NO_STORAGE_VOL,
> + "no storage vol with matching name '%s'",
name);
> + return NULL;
> + }
> +
> + if (!strchr(name, '+'))
> + goto fallback;
> +
> + if (!(volinfo = virStringSplit(name, "+", 2)))
> + return NULL;
> +
> + if (!volinfo[1])
> + goto fallback;
> +
> + ret = virGetStorageVol(pool->conn, pool->name, volinfo[1],
> volinfo[0],
> + NULL, NULL);
> +
> +cleanup:
> + virStringFreeList(volinfo);
> + return ret;
> +
> +fallback:
> + ret = virGetStorageVol(pool->conn, pool->name, name, "block",
> NULL, NULL);
> + goto cleanup;
> +}
> +
> +static int
> +fakeStorageVolGetInfo(virStorageVolPtr vol,
> + virStorageVolInfoPtr info)
> +{
> + memset(info, 0, sizeof(*info));
> +
> + info->type = virStorageVolTypeFromString(vol->key);
> +
> + if (info->type < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "Invalid volume type '%s'", vol->key);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +
> +static char *
> +fakeStorageVolGetPath(virStorageVolPtr vol)
> +{
> + char *ret = NULL;
> +
> + ignore_value(virAsprintf(&ret, "/some/%s/device/%s", vol->key,
> vol->name));
> +
> + return ret;
> +}
> +
> +
> +static char *
> +fakeStoragePoolDumpXML(virStoragePoolPtr pool,
Better to rename it as fakeStoragePoolGetXMLDesc to keep consistent.
"DumpXML" is used in virsh.
Right.
Osier
Peter
2:13 a.m.
New subject: [libvirt] [PATCHv1.5 02/27] test: Implement fake storage pool driver in qemuxml2argv test
On 27/11/13 17:58, Peter Krempa wrote:
On 11/27/13 08:47, Osier Yang wrote:
> On 27/11/13 00:48, Peter Krempa wrote:
>> To support testing of "volume" disk backing, we need to implement a
few
>> disk driver backend functions.
>>
>> The fake storage driver uses files in storagepoolxml2xmlout/POOLNAME.xml
>> as XML files for pool definitions and volume names are in format
>> "VOL_TYPE+VOL_PATH". By default type "block" is assumed (for
iSCSI test
>> compatibility).
>>
>> The choice of this approach along with implemented functions was made so
>> that <disk type='volume'> can be tested in the xml2argv test.
>> ---
>> tests/qemuxml2argvtest.c | 162
>> +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 162 insertions(+)
>>
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index a290062..a4cef84 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -18,6 +18,7 @@
>> # include "qemu/qemu_command.h"
>> # include "qemu/qemu_domain.h"
>> # include "datatypes.h"
>> +# include "conf/storage_conf.h"
>> # include "cpu/cpu_map.h"
>> # include "virstring.h"
>>
>> @@ -75,6 +76,161 @@ static virSecretDriver fakeSecretDriver = {
>> .secretUndefine = NULL,
>> };
>>
>> +
>> +# define STORAGE_POOL_XML_PATH "storagepoolxml2xmlout/"
> This will cause build failure when building with VPATH.
Hmmm, I'll look into it.
FYI, You can use "abs_srcdir" directly to construct the path now, after
Eric's patch is pushed:
https://www.redhat.com/archives/libvir-list/2013-November/msg01265.html
Osier
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 03/27] qemuxml2argv: Add test to verify correct usage of disk type="volume"
Tweak the existing file to test command line generator too.
---
tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args | 8 ++++++++
tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml | 2 +-
tests/qemuxml2argvtest.c | 2 ++
3 files changed, 11 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args
new file mode 100644
index 0000000..da87ad9
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args
@@ -0,0 +1,8 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
+/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
+-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive \
+file=/some/block/device/cdrom,if=none,media=cdrom,id=drive-ide0-0-1 -device \
+ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 -drive \
+file=/tmp/idedisk.img,if=none,id=drive-ide0-0-2 -device \
+ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2 -device \
+virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
index 465a539..6ca5cf7 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
@@ -15,7 +15,7 @@
<devices>
<emulator>/usr/bin/qemu</emulator>
<disk type='volume' device='cdrom'>
- <source pool='blk-pool0' volume='blk-pool0-vol0'
startupPolicy='optional'>
+ <source pool='pool-disk' volume='block+cdrom'>
<seclabel model='selinux' relabel='yes'>
<label>system_u:system_r:public_content_t:s0</label>
</seclabel>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a4cef84..1c50732 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -770,6 +770,8 @@ mymain(void)
DO_TEST("disk-aio",
QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_AIO,
QEMU_CAPS_DRIVE_CACHE_V2, QEMU_CAPS_DRIVE_FORMAT);
+ DO_TEST("disk-source-pool",
+ QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
DO_TEST("disk-ioeventfd",
QEMU_CAPS_DRIVE, QEMU_CAPS_VIRTIO_IOEVENTFD,
QEMU_CAPS_VIRTIO_TX_ALG, QEMU_CAPS_DEVICE,
--
1.8.4.3
12:30 p.m.
New subject: [libvirt] [PATCHv1.5 03/27] qemuxml2argv: Add test to verify correct usage of disk type="volume"
On 26.11.2013 17:48, Peter Krempa wrote:
Tweak the existing file to test command line generator too.
---
tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args | 8 ++++++++
tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml | 2 +-
tests/qemuxml2argvtest.c | 2 ++
3 files changed, 11 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args
ACK
Michal
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 04/27] qemuxml2argv: Add test for disk type='volume' with iSCSI pools
Tweak the existing file so that it can be tested for command line
corectness.
---
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 76 +-----------
src/qemu/qemu_conf.c | 129 ++++++++++++++-------
src/qemu/qemu_conf.h | 2 +
.../qemuxml2argv-disk-source-pool-mode.args | 10 ++
.../qemuxml2argv-disk-source-pool-mode.xml | 4 +-
tests/qemuxml2argvtest.c | 2 +
8 files changed, 112 insertions(+), 113 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 4561ccc..a5ef2ca 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -686,6 +686,7 @@ struct _virDomainDiskSourcePoolDef {
char *volume; /* volume name */
int voltype; /* enum virStorageVolType, internal only */
int pooltype; /* enum virStoragePoolType, internal only */
+ int actualtype; /* enum virDomainDiskType, internal only */
int mode; /* enum virDomainDiskSourcePoolMode */
};
typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 205fe56..aeb3568 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -693,6 +693,7 @@ virStoragePoolSourceFree;
virStoragePoolSourceListFormat;
virStoragePoolSourceListNewSource;
virStoragePoolTypeFromString;
+virStoragePoolTypeToString;
virStorageVolDefFindByKey;
virStorageVolDefFindByName;
virStorageVolDefFindByPath;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 763417f..b8129a3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3775,69 +3775,6 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk,
virBufferPtr op
return 0;
}
-static int
-qemuBuildVolumeString(virConnectPtr conn,
- virDomainDiskDefPtr disk,
- virBufferPtr opt)
-{
- int ret = -1;
-
- switch ((virStorageVolType) disk->srcpool->voltype) {
- case VIR_STORAGE_VOL_DIR:
- if (!disk->readonly) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot create virtual FAT disks in read-write
mode"));
- goto cleanup;
- }
- if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
- virBufferEscape(opt, ',', ",",
"file=fat:floppy:%s,", disk->src);
- else
- virBufferEscape(opt, ',', ",", "file=fat:%s,",
disk->src);
- break;
- case VIR_STORAGE_VOL_BLOCK:
- if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("tray status 'open' is invalid for "
- "block type volume"));
- goto cleanup;
- }
- if (disk->srcpool->pooltype == VIR_STORAGE_POOL_ISCSI) {
- if (disk->srcpool->mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) {
- if (qemuBuildISCSIString(conn, disk, opt) < 0)
- goto cleanup;
- virBufferAddChar(opt, ',');
- } else if (disk->srcpool->mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) {
- virBufferEscape(opt, ',', ",", "file=%s,",
disk->src);
- }
- } else {
- virBufferEscape(opt, ',', ",", "file=%s,",
disk->src);
- }
- break;
- case VIR_STORAGE_VOL_FILE:
- if (disk->auth.username) {
- if (qemuBuildISCSIString(conn, disk, opt) < 0)
- goto cleanup;
- virBufferAddChar(opt, ',');
- } else {
- virBufferEscape(opt, ',', ",", "file=%s,",
disk->src);
- }
- break;
- case VIR_STORAGE_VOL_NETWORK:
- case VIR_STORAGE_VOL_NETDIR:
- case VIR_STORAGE_VOL_LAST:
- /* Keep the compiler quiet, qemuTranslateDiskSourcePool already
- * reported the unsupported error.
- */
- goto cleanup;
- }
-
- ret = 0;
-
-cleanup:
- return ret;
-}
char *
qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
@@ -3851,6 +3788,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virDomainDiskGeometryTransTypeToString(disk->geometry.trans);
int idx = virDiskNameToIndex(disk->dst);
int busid = -1, unitid = -1;
+ int actualType = qemuDiskGetActualType(disk);
if (idx < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -3934,12 +3872,13 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
/* disk->src is NULL when we use nbd disks */
if ((disk->src ||
- (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
+ (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK &&
disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_NBD)) &&
!((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
- if (disk->type == VIR_DOMAIN_DISK_TYPE_DIR) {
+
+ if (actualType == VIR_DOMAIN_DISK_TYPE_DIR) {
/* QEMU only supports magic FAT format for now */
if (disk->format > 0 && disk->format !=
VIR_STORAGE_FILE_FAT) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -3957,7 +3896,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
disk->src);
else
virBufferEscape(&opt, ',', ",",
"file=fat:%s,", disk->src);
- } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) {
+ } else if (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK) {
switch (disk->protocol) {
case VIR_DOMAIN_DISK_PROTOCOL_NBD:
if (qemuBuildNBDString(conn, disk, &opt) < 0)
@@ -4025,11 +3964,8 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virBufferEscape(&opt, ',', ",", "%s,",
disk->src);
break;
}
- } else if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
- if (qemuBuildVolumeString(conn, disk, &opt) < 0)
- goto error;
} else {
- if ((disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
+ if ((actualType == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
(disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("tray status 'open' is invalid for "
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 77df370..58a0500 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1298,6 +1298,17 @@ cleanup:
return ret;
}
+
+int
+qemuDiskGetActualType(virDomainDiskDefPtr def)
+{
+ if (def->type == VIR_DOMAIN_DISK_TYPE_VOLUME)
+ return def->srcpool->actualtype;
+
+ return def->type;
+}
+
+
int
qemuTranslateDiskSourcePool(virConnectPtr conn,
virDomainDiskDefPtr def)
@@ -1319,72 +1330,108 @@ qemuTranslateDiskSourcePool(virConnectPtr conn,
if (!(pool = virStoragePoolLookupByName(conn, def->srcpool->pool)))
return -1;
+ if (virStoragePoolIsActive(pool) != 1) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("storage pool '%s' containing volume '%s'
"
+ "is not active"),
+ def->srcpool->pool, def->srcpool->volume);
+ goto cleanup;
+ }
+
if (!(vol = virStorageVolLookupByName(pool, def->srcpool->volume)))
goto cleanup;
if (virStorageVolGetInfo(vol, &info) < 0)
goto cleanup;
- if (def->startupPolicy &&
- info.type != VIR_STORAGE_VOL_FILE) {
+ if (!(poolxml = virStoragePoolGetXMLDesc(pool, 0)))
+ goto cleanup;
+
+ if (!(pooldef = virStoragePoolDefParseString(poolxml)))
+ goto cleanup;
+
+ def->srcpool->pooltype = pooldef->type;
+ def->srcpool->voltype = info.type;
+
+ if (def->srcpool->mode && pooldef->type != VIR_STORAGE_POOL_ISCSI)
{
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("'startupPolicy' is only valid for 'file'
type volume"));
+ _("disk source mode is only valid when "
+ "storage pool is of iscsi type"));
goto cleanup;
}
- switch ((virStorageVolType) info.type) {
- case VIR_STORAGE_VOL_FILE:
- case VIR_STORAGE_VOL_DIR:
+ switch ((enum virStoragePoolType) pooldef->type) {
+ case VIR_STORAGE_POOL_DIR:
+ case VIR_STORAGE_POOL_FS:
+ case VIR_STORAGE_POOL_NETFS:
+ case VIR_STORAGE_POOL_LOGICAL:
+ case VIR_STORAGE_POOL_DISK:
+ case VIR_STORAGE_POOL_SCSI:
if (!(def->src = virStorageVolGetPath(vol)))
goto cleanup;
- break;
- case VIR_STORAGE_VOL_BLOCK:
- if (!(poolxml = virStoragePoolGetXMLDesc(pool, 0)))
- goto cleanup;
-
- if (!(pooldef = virStoragePoolDefParseString(poolxml)))
- goto cleanup;
- if (def->srcpool->mode && pooldef->type !=
VIR_STORAGE_POOL_ISCSI) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("disk source mode is only valid when "
- "storage pool is of iscsi type"));
+ switch (info.type) {
+ case VIR_STORAGE_VOL_FILE:
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_FILE;
+ break;
+
+ case VIR_STORAGE_VOL_DIR:
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_DIR;
+ break;
+
+ case VIR_STORAGE_VOL_BLOCK:
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_BLOCK;
+ break;
+
+ case VIR_STORAGE_VOL_NETWORK:
+ case VIR_STORAGE_VOL_NETDIR:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected storage volume type '%s' "
+ "for storage pool type '%s'"),
+ virStorageVolTypeToString(info.type),
+ virStoragePoolTypeToString(pooldef->type));
goto cleanup;
}
- def->srcpool->pooltype = pooldef->type;
- if (pooldef->type == VIR_STORAGE_POOL_ISCSI) {
- /* Default to use the LUN's path on host */
- if (!def->srcpool->mode)
- def->srcpool->mode = VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST;
-
- if (def->srcpool->mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) {
- if (qemuAddISCSIPoolSourceHost(def, pooldef) < 0)
- goto cleanup;
- } else if (def->srcpool->mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) {
- if (!(def->src = virStorageVolGetPath(vol)))
- goto cleanup;
- }
+ break;
+
+ case VIR_STORAGE_POOL_ISCSI:
+ switch (def->srcpool->mode) {
+ case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DEFAULT:
+ case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_LAST:
+ def->srcpool->mode = VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST;
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_BLOCK;
+ /* fallthrough */
+ case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST:
+ if (!(def->src = virStorageVolGetPath(vol)))
+ goto cleanup;
+ break;
+
+ case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT:
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_NETWORK;
+ def->protocol = VIR_DOMAIN_DISK_PROTOCOL_ISCSI;
if (qemuTranslateDiskSourcePoolAuth(def, pooldef) < 0)
goto cleanup;
- } else {
- if (!(def->src = virStorageVolGetPath(vol)))
+
+ if (qemuAddISCSIPoolSourceHost(def, pooldef) < 0)
goto cleanup;
+ break;
}
-
break;
- case VIR_STORAGE_VOL_NETWORK:
- case VIR_STORAGE_VOL_NETDIR:
- case VIR_STORAGE_VOL_LAST:
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Using network volume as disk source is not
supported"));
+
+ case VIR_STORAGE_POOL_MPATH:
+ case VIR_STORAGE_POOL_RBD:
+ case VIR_STORAGE_POOL_SHEEPDOG:
+ case VIR_STORAGE_POOL_GLUSTER:
+ case VIR_STORAGE_POOL_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("using '%s' pools for backing 'volume'
disks "
+ "isn't yet supported"),
+ virStoragePoolTypeToString(pooldef->type));
goto cleanup;
}
- def->srcpool->voltype = info.type;
ret = 0;
cleanup:
if (ret < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index f9ff7af..b9786b1 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -304,6 +304,8 @@ int qemuSetUnprivSGIO(virDomainDeviceDefPtr dev);
int qemuDriverAllocateID(virQEMUDriverPtr driver);
virDomainXMLOptionPtr virQEMUDriverCreateXMLConf(virQEMUDriverPtr driver);
+int qemuDiskGetActualType(virDomainDiskDefPtr def);
+
int qemuTranslateDiskSourcePool(virConnectPtr conn,
virDomainDiskDefPtr def);
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
new file mode 100644
index 0000000..8f6a3dd
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
@@ -0,0 +1,10 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
+/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
+-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive \
+file=/some/block/device/unit:0:0:1,if=none,media=cdrom,id=drive-ide0-0-1 -device \
+ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 -drive \
+file=iscsi://iscsi.example.com:3260/demo-target/2,if=none,media=cdrom,id=drive-ide0-0-2
\
+-device ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2 -drive \
+file=/tmp/idedisk.img,if=none,id=drive-ide0-0-3 -device \
+ide-drive,bus=ide.0,unit=3,drive=drive-ide0-0-3,id=ide0-0-3 -device \
+virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
index b907633..9f90293 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
@@ -15,7 +15,7 @@
<devices>
<emulator>/usr/bin/qemu</emulator>
<disk type='volume' device='cdrom'>
- <source pool='blk-pool0' volume='blk-pool0-vol0'
mode='host' startupPolicy='optional'>
+ <source pool='pool-iscsi-auth' volume='unit:0:0:1'
mode='host'>
<seclabel model='selinux' relabel='yes'>
<label>system_u:system_r:public_content_t:s0</label>
</seclabel>
@@ -25,7 +25,7 @@
<address type='drive' controller='0' bus='0'
target='0' unit='1'/>
</disk>
<disk type='volume' device='cdrom'>
- <source pool='blk-pool0' volume='blk-pool0-vol1'
mode='direct' startupPolicy='optional'>
+ <source pool='pool-iscsi' volume='unit:0:0:2'
mode='direct'>
<seclabel model='selinux' relabel='yes'>
<label>system_u:system_r:public_content_t:s0</label>
</seclabel>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1c50732..49b0bd9 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -772,6 +772,8 @@ mymain(void)
QEMU_CAPS_DRIVE_CACHE_V2, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-source-pool",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
+ DO_TEST("disk-source-pool-mode",
+ QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
DO_TEST("disk-ioeventfd",
QEMU_CAPS_DRIVE, QEMU_CAPS_VIRTIO_IOEVENTFD,
QEMU_CAPS_VIRTIO_TX_ALG, QEMU_CAPS_DEVICE,
--
1.8.4.3
12:30 p.m.
New subject: [libvirt] [PATCHv1.5 04/27] qemuxml2argv: Add test for disk type='volume' with iSCSI pools
On 26.11.2013 17:48, Peter Krempa wrote:
Tweak the existing file so that it can be tested for command line
corectness.
---
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 76 +-----------
src/qemu/qemu_conf.c | 129 ++++++++++++++-------
src/qemu/qemu_conf.h | 2 +
.../qemuxml2argv-disk-source-pool-mode.args | 10 ++
.../qemuxml2argv-disk-source-pool-mode.xml | 4 +-
tests/qemuxml2argvtest.c | 2 +
8 files changed, 112 insertions(+), 113 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 4561ccc..a5ef2ca 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -686,6 +686,7 @@ struct _virDomainDiskSourcePoolDef {
char *volume; /* volume name */
int voltype; /* enum virStorageVolType, internal only */
int pooltype; /* enum virStoragePoolType, internal only */
+ int actualtype; /* enum virDomainDiskType, internal only */
int mode; /* enum virDomainDiskSourcePoolMode */
};
typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 205fe56..aeb3568 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -693,6 +693,7 @@ virStoragePoolSourceFree;
virStoragePoolSourceListFormat;
virStoragePoolSourceListNewSource;
virStoragePoolTypeFromString;
+virStoragePoolTypeToString;
virStorageVolDefFindByKey;
virStorageVolDefFindByName;
virStorageVolDefFindByPath;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 763417f..b8129a3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3775,69 +3775,6 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk,
virBufferPtr op
return 0;
}
-static int
-qemuBuildVolumeString(virConnectPtr conn,
- virDomainDiskDefPtr disk,
- virBufferPtr opt)
-{
- int ret = -1;
-
- switch ((virStorageVolType) disk->srcpool->voltype) {
- case VIR_STORAGE_VOL_DIR:
- if (!disk->readonly) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot create virtual FAT disks in read-write
mode"));
- goto cleanup;
- }
- if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
- virBufferEscape(opt, ',', ",",
"file=fat:floppy:%s,", disk->src);
- else
- virBufferEscape(opt, ',', ",", "file=fat:%s,",
disk->src);
- break;
- case VIR_STORAGE_VOL_BLOCK:
- if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("tray status 'open' is invalid for "
- "block type volume"));
- goto cleanup;
- }
- if (disk->srcpool->pooltype == VIR_STORAGE_POOL_ISCSI) {
- if (disk->srcpool->mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) {
- if (qemuBuildISCSIString(conn, disk, opt) < 0)
- goto cleanup;
- virBufferAddChar(opt, ',');
- } else if (disk->srcpool->mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) {
- virBufferEscape(opt, ',', ",", "file=%s,",
disk->src);
- }
- } else {
- virBufferEscape(opt, ',', ",", "file=%s,",
disk->src);
- }
- break;
- case VIR_STORAGE_VOL_FILE:
- if (disk->auth.username) {
- if (qemuBuildISCSIString(conn, disk, opt) < 0)
- goto cleanup;
- virBufferAddChar(opt, ',');
- } else {
- virBufferEscape(opt, ',', ",", "file=%s,",
disk->src);
- }
- break;
- case VIR_STORAGE_VOL_NETWORK:
- case VIR_STORAGE_VOL_NETDIR:
- case VIR_STORAGE_VOL_LAST:
- /* Keep the compiler quiet, qemuTranslateDiskSourcePool already
- * reported the unsupported error.
- */
- goto cleanup;
- }
-
- ret = 0;
-
-cleanup:
- return ret;
-}
char *
qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
@@ -3851,6 +3788,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virDomainDiskGeometryTransTypeToString(disk->geometry.trans);
int idx = virDiskNameToIndex(disk->dst);
int busid = -1, unitid = -1;
+ int actualType = qemuDiskGetActualType(disk);
if (idx < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -3934,12 +3872,13 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
/* disk->src is NULL when we use nbd disks */
if ((disk->src ||
- (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
+ (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK &&
disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_NBD)) &&
!((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
- if (disk->type == VIR_DOMAIN_DISK_TYPE_DIR) {
+
+ if (actualType == VIR_DOMAIN_DISK_TYPE_DIR) {
/* QEMU only supports magic FAT format for now */
if (disk->format > 0 && disk->format !=
VIR_STORAGE_FILE_FAT) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -3957,7 +3896,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
disk->src);
else
virBufferEscape(&opt, ',', ",",
"file=fat:%s,", disk->src);
- } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) {
+ } else if (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK) {
switch (disk->protocol) {
case VIR_DOMAIN_DISK_PROTOCOL_NBD:
if (qemuBuildNBDString(conn, disk, &opt) < 0)
@@ -4025,11 +3964,8 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virBufferEscape(&opt, ',', ",", "%s,",
disk->src);
break;
}
- } else if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
- if (qemuBuildVolumeString(conn, disk, &opt) < 0)
- goto error;
} else {
- if ((disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
+ if ((actualType == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
(disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("tray status 'open' is invalid for "
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 77df370..58a0500 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1298,6 +1298,17 @@ cleanup:
return ret;
}
+
+int
+qemuDiskGetActualType(virDomainDiskDefPtr def)
+{
+ if (def->type == VIR_DOMAIN_DISK_TYPE_VOLUME)
+ return def->srcpool->actualtype;
+
+ return def->type;
+}
+
+
int
qemuTranslateDiskSourcePool(virConnectPtr conn,
virDomainDiskDefPtr def)
@@ -1319,72 +1330,108 @@ qemuTranslateDiskSourcePool(virConnectPtr conn,
if (!(pool = virStoragePoolLookupByName(conn, def->srcpool->pool)))
return -1;
+ if (virStoragePoolIsActive(pool) != 1) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("storage pool '%s' containing volume '%s'
"
+ "is not active"),
+ def->srcpool->pool, def->srcpool->volume);
+ goto cleanup;
+ }
+
if (!(vol = virStorageVolLookupByName(pool, def->srcpool->volume)))
goto cleanup;
if (virStorageVolGetInfo(vol, &info) < 0)
goto cleanup;
- if (def->startupPolicy &&
- info.type != VIR_STORAGE_VOL_FILE) {
+ if (!(poolxml = virStoragePoolGetXMLDesc(pool, 0)))
+ goto cleanup;
+
+ if (!(pooldef = virStoragePoolDefParseString(poolxml)))
+ goto cleanup;
+
+ def->srcpool->pooltype = pooldef->type;
+ def->srcpool->voltype = info.type;
+
+ if (def->srcpool->mode && pooldef->type != VIR_STORAGE_POOL_ISCSI)
{
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("'startupPolicy' is only valid for 'file'
type volume"));
+ _("disk source mode is only valid when "
+ "storage pool is of iscsi type"));
goto cleanup;
}
- switch ((virStorageVolType) info.type) {
- case VIR_STORAGE_VOL_FILE:
- case VIR_STORAGE_VOL_DIR:
+ switch ((enum virStoragePoolType) pooldef->type) {
+ case VIR_STORAGE_POOL_DIR:
+ case VIR_STORAGE_POOL_FS:
+ case VIR_STORAGE_POOL_NETFS:
+ case VIR_STORAGE_POOL_LOGICAL:
+ case VIR_STORAGE_POOL_DISK:
+ case VIR_STORAGE_POOL_SCSI:
if (!(def->src = virStorageVolGetPath(vol)))
goto cleanup;
- break;
- case VIR_STORAGE_VOL_BLOCK:
- if (!(poolxml = virStoragePoolGetXMLDesc(pool, 0)))
- goto cleanup;
-
- if (!(pooldef = virStoragePoolDefParseString(poolxml)))
- goto cleanup;
- if (def->srcpool->mode && pooldef->type !=
VIR_STORAGE_POOL_ISCSI) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("disk source mode is only valid when "
- "storage pool is of iscsi type"));
+ switch (info.type) {
+ case VIR_STORAGE_VOL_FILE:
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_FILE;
+ break;
+
+ case VIR_STORAGE_VOL_DIR:
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_DIR;
+ break;
+
+ case VIR_STORAGE_VOL_BLOCK:
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_BLOCK;
+ break;
+
+ case VIR_STORAGE_VOL_NETWORK:
+ case VIR_STORAGE_VOL_NETDIR:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected storage volume type '%s' "
+ "for storage pool type '%s'"),
+ virStorageVolTypeToString(info.type),
+ virStoragePoolTypeToString(pooldef->type));
goto cleanup;
}
- def->srcpool->pooltype = pooldef->type;
- if (pooldef->type == VIR_STORAGE_POOL_ISCSI) {
- /* Default to use the LUN's path on host */
- if (!def->srcpool->mode)
- def->srcpool->mode = VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST;
-
- if (def->srcpool->mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) {
- if (qemuAddISCSIPoolSourceHost(def, pooldef) < 0)
- goto cleanup;
- } else if (def->srcpool->mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) {
- if (!(def->src = virStorageVolGetPath(vol)))
- goto cleanup;
- }
+ break;
+
+ case VIR_STORAGE_POOL_ISCSI:
+ switch (def->srcpool->mode) {
+ case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DEFAULT:
+ case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_LAST:
+ def->srcpool->mode = VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST;
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_BLOCK;
+ /* fallthrough */
+ case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST:
+ if (!(def->src = virStorageVolGetPath(vol)))
+ goto cleanup;
+ break;
+
+ case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT:
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_NETWORK;
+ def->protocol = VIR_DOMAIN_DISK_PROTOCOL_ISCSI;
if (qemuTranslateDiskSourcePoolAuth(def, pooldef) < 0)
goto cleanup;
- } else {
- if (!(def->src = virStorageVolGetPath(vol)))
+
+ if (qemuAddISCSIPoolSourceHost(def, pooldef) < 0)
goto cleanup;
+ break;
}
-
break;
- case VIR_STORAGE_VOL_NETWORK:
- case VIR_STORAGE_VOL_NETDIR:
- case VIR_STORAGE_VOL_LAST:
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Using network volume as disk source is not
supported"));
+
+ case VIR_STORAGE_POOL_MPATH:
+ case VIR_STORAGE_POOL_RBD:
+ case VIR_STORAGE_POOL_SHEEPDOG:
+ case VIR_STORAGE_POOL_GLUSTER:
+ case VIR_STORAGE_POOL_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("using '%s' pools for backing 'volume'
disks "
+ "isn't yet supported"),
+ virStoragePoolTypeToString(pooldef->type));
goto cleanup;
}
- def->srcpool->voltype = info.type;
ret = 0;
cleanup:
if (ret < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index f9ff7af..b9786b1 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -304,6 +304,8 @@ int qemuSetUnprivSGIO(virDomainDeviceDefPtr dev);
int qemuDriverAllocateID(virQEMUDriverPtr driver);
virDomainXMLOptionPtr virQEMUDriverCreateXMLConf(virQEMUDriverPtr driver);
+int qemuDiskGetActualType(virDomainDiskDefPtr def);
+
int qemuTranslateDiskSourcePool(virConnectPtr conn,
virDomainDiskDefPtr def);
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
new file mode 100644
index 0000000..8f6a3dd
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
@@ -0,0 +1,10 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
+/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
+-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive \
+file=/some/block/device/unit:0:0:1,if=none,media=cdrom,id=drive-ide0-0-1 -device \
+ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 -drive \
+file=iscsi://iscsi.example.com:3260/demo-target/2,if=none,media=cdrom,id=drive-ide0-0-2
\
+-device ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2 -drive \
+file=/tmp/idedisk.img,if=none,id=drive-ide0-0-3 -device \
+ide-drive,bus=ide.0,unit=3,drive=drive-ide0-0-3,id=ide0-0-3 -device \
+virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
index b907633..9f90293 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
@@ -15,7 +15,7 @@
<devices>
<emulator>/usr/bin/qemu</emulator>
<disk type='volume' device='cdrom'>
- <source pool='blk-pool0' volume='blk-pool0-vol0'
mode='host' startupPolicy='optional'>
+ <source pool='pool-iscsi-auth' volume='unit:0:0:1'
mode='host'>
<seclabel model='selinux' relabel='yes'>
<label>system_u:system_r:public_content_t:s0</label>
</seclabel>
@@ -25,7 +25,7 @@
<address type='drive' controller='0' bus='0'
target='0' unit='1'/>
</disk>
<disk type='volume' device='cdrom'>
- <source pool='blk-pool0' volume='blk-pool0-vol1'
mode='direct' startupPolicy='optional'>
Just curious, I see you're removing startupPolicy here and in other
patches. Would it hurt to leave it there?
At any rate, ACK.
Michal
2:29 a.m.
New subject: [libvirt] [PATCHv1.5 04/27] qemuxml2argv: Add test for disk type='volume' with iSCSI pools
On 27/11/13 00:48, Peter Krempa wrote:
Tweak the existing file so that it can be tested for command line
corectness.
It's actually lots of change, should provide detailed commit log.
---
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 76 +-----------
src/qemu/qemu_conf.c | 129 ++++++++++++++-------
src/qemu/qemu_conf.h | 2 +
.../qemuxml2argv-disk-source-pool-mode.args | 10 ++
.../qemuxml2argv-disk-source-pool-mode.xml | 4 +-
tests/qemuxml2argvtest.c | 2 +
8 files changed, 112 insertions(+), 113 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 4561ccc..a5ef2ca 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -686,6 +686,7 @@ struct _virDomainDiskSourcePoolDef {
char *volume; /* volume name */
int voltype; /* enum virStorageVolType, internal only */
int pooltype; /* enum virStoragePoolType, internal only */
+ int actualtype; /* enum virDomainDiskType, internal only */
int mode; /* enum virDomainDiskSourcePoolMode */
};
typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 205fe56..aeb3568 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -693,6 +693,7 @@ virStoragePoolSourceFree;
virStoragePoolSourceListFormat;
virStoragePoolSourceListNewSource;
virStoragePoolTypeFromString;
+virStoragePoolTypeToString;
virStorageVolDefFindByKey;
virStorageVolDefFindByName;
virStorageVolDefFindByPath;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 763417f..b8129a3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3775,69 +3775,6 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk,
virBufferPtr op
return 0;
}
-static int
-qemuBuildVolumeString(virConnectPtr conn,
- virDomainDiskDefPtr disk,
- virBufferPtr opt)
Wondering why this is completely removed, before going ahead...
-{
- int ret = -1;
-
- switch ((virStorageVolType) disk->srcpool->voltype) {
- case VIR_STORAGE_VOL_DIR:
- if (!disk->readonly) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("cannot create virtual FAT disks in read-write
mode"));
- goto cleanup;
- }
- if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
- virBufferEscape(opt, ',', ",",
"file=fat:floppy:%s,", disk->src);
- else
- virBufferEscape(opt, ',', ",", "file=fat:%s,",
disk->src);
- break;
- case VIR_STORAGE_VOL_BLOCK:
- if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("tray status 'open' is invalid for "
- "block type volume"));
With this removed, I guess one will see different error like:
_("tray status 'open' is invalid for block type disk"));
I'm doubting it's what we want for a "volume" type disk.
- goto cleanup;
- }
- if (disk->srcpool->pooltype == VIR_STORAGE_POOL_ISCSI) {
- if (disk->srcpool->mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) {
- if (qemuBuildISCSIString(conn, disk, opt) < 0)
- goto cleanup;
- virBufferAddChar(opt, ',');
- } else if (disk->srcpool->mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) {
- virBufferEscape(opt, ',', ",", "file=%s,",
disk->src);
Especially here, I'm wondering if the "mode" attribute for iscsi pool
disk still works.
- }
- } else {
- virBufferEscape(opt, ',', ",", "file=%s,",
disk->src);
- }
- break;
- case VIR_STORAGE_VOL_FILE:
- if (disk->auth.username) {
- if (qemuBuildISCSIString(conn, disk, opt) < 0)
- goto cleanup;
- virBufferAddChar(opt, ',');
- } else {
- virBufferEscape(opt, ',', ",", "file=%s,",
disk->src);
- }
- break;
- case VIR_STORAGE_VOL_NETWORK:
- case VIR_STORAGE_VOL_NETDIR:
- case VIR_STORAGE_VOL_LAST:
- /* Keep the compiler quiet, qemuTranslateDiskSourcePool already
- * reported the unsupported error.
- */
- goto cleanup;
- }
-
- ret = 0;
-
-cleanup:
- return ret;
-}
char *
qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
@@ -3851,6 +3788,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virDomainDiskGeometryTransTypeToString(disk->geometry.trans);
int idx = virDiskNameToIndex(disk->dst);
int busid = -1, unitid = -1;
+ int actualType = qemuDiskGetActualType(disk);
if (idx < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -3934,12 +3872,13 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
/* disk->src is NULL when we use nbd disks */
if ((disk->src ||
- (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
+ (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK &&
disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_NBD)) &&
!((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
- if (disk->type == VIR_DOMAIN_DISK_TYPE_DIR) {
+
+ if (actualType == VIR_DOMAIN_DISK_TYPE_DIR) {
/* QEMU only supports magic FAT format for now */
if (disk->format > 0 && disk->format !=
VIR_STORAGE_FILE_FAT) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -3957,7 +3896,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
disk->src);
else
virBufferEscape(&opt, ',', ",",
"file=fat:%s,", disk->src);
- } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) {
+ } else if (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK) {
switch (disk->protocol) {
case VIR_DOMAIN_DISK_PROTOCOL_NBD:
if (qemuBuildNBDString(conn, disk, &opt) < 0)
@@ -4025,11 +3964,8 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virBufferEscape(&opt, ',', ",", "%s,",
disk->src);
break;
}
- } else if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
- if (qemuBuildVolumeString(conn, disk, &opt) < 0)
- goto error;
} else {
- if ((disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
+ if ((actualType == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
(disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("tray status 'open' is invalid for
"
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 77df370..58a0500 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1298,6 +1298,17 @@ cleanup:
return ret;
}
+
+int
+qemuDiskGetActualType(virDomainDiskDefPtr def)
+{
+ if (def->type == VIR_DOMAIN_DISK_TYPE_VOLUME)
+ return def->srcpool->actualtype;
+
+ return def->type;
+}
+
+
int
qemuTranslateDiskSourcePool(virConnectPtr conn,
virDomainDiskDefPtr def)
@@ -1319,72 +1330,108 @@ qemuTranslateDiskSourcePool(virConnectPtr conn,
if (!(pool = virStoragePoolLookupByName(conn, def->srcpool->pool)))
return -1;
+ if (virStoragePoolIsActive(pool) != 1) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
Not sure if it's wise to use "VIR_ERR_CONFIG_UNSUPPORTED" here.
+ _("storage pool '%s' containing
volume '%s' "
+ "is not active"),
+ def->srcpool->pool, def->srcpool->volume);
+ goto cleanup;
+ }
+
if (!(vol = virStorageVolLookupByName(pool, def->srcpool->volume)))
goto cleanup;
if (virStorageVolGetInfo(vol, &info) < 0)
goto cleanup;
- if (def->startupPolicy &&
- info.type != VIR_STORAGE_VOL_FILE) {
Why this check is removed?
+ if (!(poolxml = virStoragePoolGetXMLDesc(pool, 0)))
+ goto cleanup;
+
+ if (!(pooldef = virStoragePoolDefParseString(poolxml)))
+ goto cleanup;
+
+ def->srcpool->pooltype = pooldef->type;
+ def->srcpool->voltype = info.type;
+
+ if (def->srcpool->mode && pooldef->type != VIR_STORAGE_POOL_ISCSI)
{
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("'startupPolicy' is only valid for 'file'
type volume"));
+ _("disk source mode is only valid when "
+ "storage pool is of iscsi type"));
goto cleanup;
}
- switch ((virStorageVolType) info.type) {
- case VIR_STORAGE_VOL_FILE:
- case VIR_STORAGE_VOL_DIR:
+ switch ((enum virStoragePoolType) pooldef->type) {
+ case VIR_STORAGE_POOL_DIR:
+ case VIR_STORAGE_POOL_FS:
+ case VIR_STORAGE_POOL_NETFS:
+ case VIR_STORAGE_POOL_LOGICAL:
+ case VIR_STORAGE_POOL_DISK:
+ case VIR_STORAGE_POOL_SCSI:
if (!(def->src = virStorageVolGetPath(vol)))
goto cleanup;
- break;
- case VIR_STORAGE_VOL_BLOCK:
- if (!(poolxml = virStoragePoolGetXMLDesc(pool, 0)))
- goto cleanup;
-
- if (!(pooldef = virStoragePoolDefParseString(poolxml)))
- goto cleanup;
- if (def->srcpool->mode && pooldef->type !=
VIR_STORAGE_POOL_ISCSI) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("disk source mode is only valid when "
- "storage pool is of iscsi type"));
+ switch (info.type) {
+ case VIR_STORAGE_VOL_FILE:
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_FILE;
+ break;
+
+ case VIR_STORAGE_VOL_DIR:
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_DIR;
+ break;
+
+ case VIR_STORAGE_VOL_BLOCK:
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_BLOCK;
+ break;
+
+ case VIR_STORAGE_VOL_NETWORK:
+ case VIR_STORAGE_VOL_NETDIR:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected storage volume type '%s' "
+ "for storage pool type '%s'"),
+ virStorageVolTypeToString(info.type),
+ virStoragePoolTypeToString(pooldef->type));
goto cleanup;
}
- def->srcpool->pooltype = pooldef->type;
- if (pooldef->type == VIR_STORAGE_POOL_ISCSI) {
- /* Default to use the LUN's path on host */
- if (!def->srcpool->mode)
- def->srcpool->mode = VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST;
-
- if (def->srcpool->mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) {
- if (qemuAddISCSIPoolSourceHost(def, pooldef) < 0)
- goto cleanup;
- } else if (def->srcpool->mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) {
- if (!(def->src = virStorageVolGetPath(vol)))
- goto cleanup;
- }
+ break;
+
+ case VIR_STORAGE_POOL_ISCSI:
+ switch (def->srcpool->mode) {
+ case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DEFAULT:
+ case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_LAST:
+ def->srcpool->mode = VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST;
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_BLOCK;
+ /* fallthrough */
I'd prefer the old "if...else" statements instead of a
"fallthrough", which
explicitly sets the "mode" as VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST
before going ahead.
+ case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST:
+ if (!(def->src = virStorageVolGetPath(vol)))
+ goto cleanup;
+ break;
+
+ case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT:
+ def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_NETWORK;
+ def->protocol = VIR_DOMAIN_DISK_PROTOCOL_ISCSI;
if (qemuTranslateDiskSourcePoolAuth(def, pooldef) < 0)
goto cleanup;
- } else {
- if (!(def->src = virStorageVolGetPath(vol)))
+
+ if (qemuAddISCSIPoolSourceHost(def, pooldef) < 0)
goto cleanup;
+ break;
}
-
break;
- case VIR_STORAGE_VOL_NETWORK:
- case VIR_STORAGE_VOL_NETDIR:
- case VIR_STORAGE_VOL_LAST:
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Using network volume as disk source is not
supported"));
+
+ case VIR_STORAGE_POOL_MPATH:
+ case VIR_STORAGE_POOL_RBD:
+ case VIR_STORAGE_POOL_SHEEPDOG:
+ case VIR_STORAGE_POOL_GLUSTER:
+ case VIR_STORAGE_POOL_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("using '%s' pools for backing 'volume'
disks "
+ "isn't yet supported"),
+ virStoragePoolTypeToString(pooldef->type));
goto cleanup;
}
- def->srcpool->voltype = info.type;
ret = 0;
cleanup:
if (ret < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index f9ff7af..b9786b1 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -304,6 +304,8 @@ int qemuSetUnprivSGIO(virDomainDeviceDefPtr dev);
int qemuDriverAllocateID(virQEMUDriverPtr driver);
virDomainXMLOptionPtr virQEMUDriverCreateXMLConf(virQEMUDriverPtr driver);
+int qemuDiskGetActualType(virDomainDiskDefPtr def);
+
int qemuTranslateDiskSourcePool(virConnectPtr conn,
virDomainDiskDefPtr def);
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
new file mode 100644
index 0000000..8f6a3dd
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
@@ -0,0 +1,10 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
+/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
+-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive \
+file=/some/block/device/unit:0:0:1,if=none,media=cdrom,id=drive-ide0-0-1 -device \
+ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 -drive \
+file=iscsi://iscsi.example.com:3260/demo-target/2,if=none,media=cdrom,id=drive-ide0-0-2
\
+-device ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2 -drive \
+file=/tmp/idedisk.img,if=none,id=drive-ide0-0-3 -device \
+ide-drive,bus=ide.0,unit=3,drive=drive-ide0-0-3,id=ide0-0-3 -device \
+virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
index b907633..9f90293 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
@@ -15,7 +15,7 @@
<devices>
<emulator>/usr/bin/qemu</emulator>
<disk type='volume' device='cdrom'>
- <source pool='blk-pool0' volume='blk-pool0-vol0'
mode='host' startupPolicy='optional'>
+ <source pool='pool-iscsi-auth' volume='unit:0:0:1'
mode='host'>
Same question as above.
It seems this patch destroyed the "mode" attr for iscsi pool disk completely
with this patch. It again asks for the detailed commit log, since the
changes
looks rather aggressive, which may cause regression.
Osier
3:54 a.m.
New subject: [libvirt] [PATCHv1.5 04/27] qemuxml2argv: Add test for disk type='volume' with iSCSI pools
On 11/27/13 09:29, Osier Yang wrote:
On 27/11/13 00:48, Peter Krempa wrote:
> Tweak the existing file so that it can be tested for command line
> corectness.
It's actually lots of change, should provide detailed commit log.
Hmmm, this patch actually should only touch the test files, but I
somehow accidentally squashed patch named:
qemu: Refactor qemuTranslatePool source (seen in v1 posting of this series)
with a more verbose error message. I'll split them again in next posting.
> ---
> src/conf/domain_conf.h | 1 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_command.c | 76 +-----------
> src/qemu/qemu_conf.c | 129
> ++++++++++++++-------
> src/qemu/qemu_conf.h | 2 +
> .../qemuxml2argv-disk-source-pool-mode.args | 10 ++
> .../qemuxml2argv-disk-source-pool-mode.xml | 4 +-
> tests/qemuxml2argvtest.c | 2 +
> 8 files changed, 112 insertions(+), 113 deletions(-)
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 4561ccc..a5ef2ca 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -686,6 +686,7 @@ struct _virDomainDiskSourcePoolDef {
> char *volume; /* volume name */
> int voltype; /* enum virStorageVolType, internal only */
> int pooltype; /* enum virStoragePoolType, internal only */
> + int actualtype; /* enum virDomainDiskType, internal only */
> int mode; /* enum virDomainDiskSourcePoolMode */
> };
> typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr;
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 205fe56..aeb3568 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -693,6 +693,7 @@ virStoragePoolSourceFree;
> virStoragePoolSourceListFormat;
> virStoragePoolSourceListNewSource;
> virStoragePoolTypeFromString;
> +virStoragePoolTypeToString;
> virStorageVolDefFindByKey;
> virStorageVolDefFindByName;
> virStorageVolDefFindByPath;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 763417f..b8129a3 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3775,69 +3775,6 @@ qemuBuildNBDString(virConnectPtr conn,
> virDomainDiskDefPtr disk, virBufferPtr op
> return 0;
> }
>
> -static int
> -qemuBuildVolumeString(virConnectPtr conn,
> - virDomainDiskDefPtr disk,
> - virBufferPtr opt)
Wondering why this is completely removed, before going ahead...
Well a function like this should never be necessary. The translation
function has to populate the fields in such way, that the regular
generator then does the same thing without any need for ugly hacks like
this.
> -{
> - int ret = -1;
> -
> - switch ((virStorageVolType) disk->srcpool->voltype) {
> - case VIR_STORAGE_VOL_DIR:
> - if (!disk->readonly) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("cannot create virtual FAT disks in
> read-write mode"));
> - goto cleanup;
> - }
> - if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
> - virBufferEscape(opt, ',', ",",
"file=fat:floppy:%s,",
> disk->src);
> - else
> - virBufferEscape(opt, ',', ",",
"file=fat:%s,", disk->src);
> - break;
> - case VIR_STORAGE_VOL_BLOCK:
> - if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("tray status 'open' is invalid for "
> - "block type volume"));
With this removed, I guess one will see different error like:
_("tray status 'open' is invalid for block type disk"));
I'm doubting it's what we want for a "volume" type disk.
I don't think this is a serious problem. If you insist I can change the
error message in the normal command line generator, but in this case I
think we would be gilding the lily here [1].
> - goto cleanup;
> - }
> - if (disk->srcpool->pooltype == VIR_STORAGE_POOL_ISCSI) {
> - if (disk->srcpool->mode ==
> - VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) {
> - if (qemuBuildISCSIString(conn, disk, opt) < 0)
> - goto cleanup;
> - virBufferAddChar(opt, ',');
> - } else if (disk->srcpool->mode ==
> - VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) {
> - virBufferEscape(opt, ',', ",",
"file=%s,", disk->src);
Especially here, I'm wondering if the "mode" attribute for iscsi pool
disk still works.
Why do it here if we can while translating the volume from the pool? see
below.
Also ... the test file that is touched in this patch tests it. It's not
that apparent, but these changes should be after the file was used to
test command line generation and after changing this piece of code the
test still passes.
> - }
> - } else {
> - virBufferEscape(opt, ',', ",", "file=%s,",
disk->src);
> - }
> - break;
> - case VIR_STORAGE_VOL_FILE:
> - if (disk->auth.username) {
> - if (qemuBuildISCSIString(conn, disk, opt) < 0)
> - goto cleanup;
> - virBufferAddChar(opt, ',');
> - } else {
> - virBufferEscape(opt, ',', ",", "file=%s,",
disk->src);
> - }
> - break;
> - case VIR_STORAGE_VOL_NETWORK:
> - case VIR_STORAGE_VOL_NETDIR:
> - case VIR_STORAGE_VOL_LAST:
> - /* Keep the compiler quiet, qemuTranslateDiskSourcePool already
> - * reported the unsupported error.
> - */
> - goto cleanup;
> - }
> -
> - ret = 0;
> -
> -cleanup:
> - return ret;
> -}
>
> char *
> qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
> @@ -3851,6 +3788,7 @@ qemuBuildDriveStr(virConnectPtr conn
> ATTRIBUTE_UNUSED,
> virDomainDiskGeometryTransTypeToString(disk->geometry.trans);
> int idx = virDiskNameToIndex(disk->dst);
> int busid = -1, unitid = -1;
> + int actualType = qemuDiskGetActualType(disk);
>
> if (idx < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -3934,12 +3872,13 @@ qemuBuildDriveStr(virConnectPtr conn
> ATTRIBUTE_UNUSED,
>
> /* disk->src is NULL when we use nbd disks */
> if ((disk->src ||
> - (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
> + (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK &&
> disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_NBD)) &&
> !((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
> disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
> disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
> - if (disk->type == VIR_DOMAIN_DISK_TYPE_DIR) {
> +
> + if (actualType == VIR_DOMAIN_DISK_TYPE_DIR) {
> /* QEMU only supports magic FAT format for now */
> if (disk->format > 0 && disk->format !=
> VIR_STORAGE_FILE_FAT) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -3957,7 +3896,7 @@ qemuBuildDriveStr(virConnectPtr conn
> ATTRIBUTE_UNUSED,
> disk->src);
> else
> virBufferEscape(&opt, ',', ",",
"file=fat:%s,",
> disk->src);
> - } else if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) {
> + } else if (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK) {
> switch (disk->protocol) {
> case VIR_DOMAIN_DISK_PROTOCOL_NBD:
> if (qemuBuildNBDString(conn, disk, &opt) < 0)
> @@ -4025,11 +3964,8 @@ qemuBuildDriveStr(virConnectPtr conn
> ATTRIBUTE_UNUSED,
> virBufferEscape(&opt, ',', ",",
"%s,", disk->src);
> break;
> }
> - } else if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
> - if (qemuBuildVolumeString(conn, disk, &opt) < 0)
> - goto error;
> } else {
> - if ((disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
> + if ((actualType == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
> (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("tray status 'open' is invalid for
"
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 77df370..58a0500 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1298,6 +1298,17 @@ cleanup:
> return ret;
> }
>
> +
> +int
> +qemuDiskGetActualType(virDomainDiskDefPtr def)
> +{
> + if (def->type == VIR_DOMAIN_DISK_TYPE_VOLUME)
> + return def->srcpool->actualtype;
> +
> + return def->type;
> +}
> +
> +
> int
> qemuTranslateDiskSourcePool(virConnectPtr conn,
> virDomainDiskDefPtr def)
> @@ -1319,72 +1330,108 @@ qemuTranslateDiskSourcePool(virConnectPtr conn,
> if (!(pool = virStoragePoolLookupByName(conn, def->srcpool->pool)))
> return -1;
>
> + if (virStoragePoolIsActive(pool) != 1) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
Not sure if it's wise to use "VIR_ERR_CONFIG_UNSUPPORTED" here.
Do we have a special one for this reason? Or is it worth inventing a new
one?
> + _("storage pool '%s' containing volume
'%s' "
> + "is not active"),
> + def->srcpool->pool, def->srcpool->volume);
> + goto cleanup;
> + }
> +
> if (!(vol = virStorageVolLookupByName(pool, def->srcpool->volume)))
> goto cleanup;
>
> if (virStorageVolGetInfo(vol, &info) < 0)
> goto cleanup;
>
> - if (def->startupPolicy &&
> - info.type != VIR_STORAGE_VOL_FILE) {
Why this check is removed?
Shouldn't this actually be checked in the function that generates the
commandline? It seemed to me as duplicated code.
> + if (!(poolxml = virStoragePoolGetXMLDesc(pool, 0)))
> + goto cleanup;
> +
> + if (!(pooldef = virStoragePoolDefParseString(poolxml)))
> + goto cleanup;
> +
> + def->srcpool->pooltype = pooldef->type;
> + def->srcpool->voltype = info.type;
> +
> + if (def->srcpool->mode && pooldef->type !=
VIR_STORAGE_POOL_ISCSI) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("'startupPolicy' is only valid for
'file'
> type volume"));
> + _("disk source mode is only valid when "
> + "storage pool is of iscsi type"));
> goto cleanup;
> }
>
> - switch ((virStorageVolType) info.type) {
> - case VIR_STORAGE_VOL_FILE:
> - case VIR_STORAGE_VOL_DIR:
> + switch ((enum virStoragePoolType) pooldef->type) {
> + case VIR_STORAGE_POOL_DIR:
> + case VIR_STORAGE_POOL_FS:
> + case VIR_STORAGE_POOL_NETFS:
> + case VIR_STORAGE_POOL_LOGICAL:
> + case VIR_STORAGE_POOL_DISK:
> + case VIR_STORAGE_POOL_SCSI:
> if (!(def->src = virStorageVolGetPath(vol)))
> goto cleanup;
> - break;
> - case VIR_STORAGE_VOL_BLOCK:
> - if (!(poolxml = virStoragePoolGetXMLDesc(pool, 0)))
> - goto cleanup;
> -
> - if (!(pooldef = virStoragePoolDefParseString(poolxml)))
> - goto cleanup;
>
> - if (def->srcpool->mode && pooldef->type !=
> VIR_STORAGE_POOL_ISCSI) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("disk source mode is only valid when "
> - "storage pool is of iscsi type"));
> + switch (info.type) {
> + case VIR_STORAGE_VOL_FILE:
> + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_FILE;
> + break;
> +
> + case VIR_STORAGE_VOL_DIR:
> + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_DIR;
> + break;
> +
> + case VIR_STORAGE_VOL_BLOCK:
> + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_BLOCK;
> + break;
> +
> + case VIR_STORAGE_VOL_NETWORK:
> + case VIR_STORAGE_VOL_NETDIR:
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unexpected storage volume type '%s'
"
> + "for storage pool type '%s'"),
> + virStorageVolTypeToString(info.type),
> + virStoragePoolTypeToString(pooldef->type));
> goto cleanup;
> }
>
> - def->srcpool->pooltype = pooldef->type;
> - if (pooldef->type == VIR_STORAGE_POOL_ISCSI) {
> - /* Default to use the LUN's path on host */
> - if (!def->srcpool->mode)
> - def->srcpool->mode =
> VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST;
> -
> - if (def->srcpool->mode ==
> - VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) {
> - if (qemuAddISCSIPoolSourceHost(def, pooldef) < 0)
> - goto cleanup;
> - } else if (def->srcpool->mode ==
> - VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) {
> - if (!(def->src = virStorageVolGetPath(vol)))
> - goto cleanup;
> - }
> + break;
> +
> + case VIR_STORAGE_POOL_ISCSI:
> + switch (def->srcpool->mode) {
> + case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DEFAULT:
> + case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_LAST:
> + def->srcpool->mode = VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST;
> + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_BLOCK;
> + /* fallthrough */
I'd prefer the old "if...else" statements instead of a
"fallthrough", which
explicitly sets the "mode" as VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST
before going ahead.
Well this is functionally same and less messy when attempting to add new
stuff.
> + case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST:
> + if (!(def->src = virStorageVolGetPath(vol)))
> + goto cleanup;
> + break;
> +
> + case VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT:
> + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_NETWORK;
> + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_ISCSI;
>
> if (qemuTranslateDiskSourcePoolAuth(def, pooldef) < 0)
> goto cleanup;
> - } else {
> - if (!(def->src = virStorageVolGetPath(vol)))
> +
> + if (qemuAddISCSIPoolSourceHost(def, pooldef) < 0)
> goto cleanup;
> + break;
> }
> -
> break;
> - case VIR_STORAGE_VOL_NETWORK:
> - case VIR_STORAGE_VOL_NETDIR:
> - case VIR_STORAGE_VOL_LAST:
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("Using network volume as disk source is not
> supported"));
> +
> + case VIR_STORAGE_POOL_MPATH:
> + case VIR_STORAGE_POOL_RBD:
> + case VIR_STORAGE_POOL_SHEEPDOG:
> + case VIR_STORAGE_POOL_GLUSTER:
> + case VIR_STORAGE_POOL_LAST:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("using '%s' pools for backing 'volume'
disks "
> + "isn't yet supported"),
> + virStoragePoolTypeToString(pooldef->type));
> goto cleanup;
> }
>
> - def->srcpool->voltype = info.type;
> ret = 0;
> cleanup:
> if (ret < 0)
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index f9ff7af..b9786b1 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -304,6 +304,8 @@ int qemuSetUnprivSGIO(virDomainDeviceDefPtr dev);
> int qemuDriverAllocateID(virQEMUDriverPtr driver);
> virDomainXMLOptionPtr virQEMUDriverCreateXMLConf(virQEMUDriverPtr
> driver);
>
> +int qemuDiskGetActualType(virDomainDiskDefPtr def);
> +
> int qemuTranslateDiskSourcePool(virConnectPtr conn,
> virDomainDiskDefPtr def);
>
> diff --git
> a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
> b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
> new file mode 100644
> index 0000000..8f6a3dd
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
> @@ -0,0 +1,10 @@
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
> QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig
> -nodefaults \
> +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb
> -drive \
> +file=/some/block/device/unit:0:0:1,if=none,media=cdrom,id=drive-ide0-0-1
> -device \
> +ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 -drive \
>
+file=iscsi://iscsi.example.com:3260/demo-target/2,if=none,media=cdrom,id=drive-ide0-0-2
> \
> +-device ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2
> -drive \
> +file=/tmp/idedisk.img,if=none,id=drive-ide0-0-3 -device \
> +ide-drive,bus=ide.0,unit=3,drive=drive-ide0-0-3,id=ide0-0-3 -device \
> +virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
> diff --git
> a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
> b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
> index b907633..9f90293 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
> @@ -15,7 +15,7 @@
> <devices>
> <emulator>/usr/bin/qemu</emulator>
> <disk type='volume' device='cdrom'>
> - <source pool='blk-pool0' volume='blk-pool0-vol0'
mode='host'
> startupPolicy='optional'>
> + <source pool='pool-iscsi-auth' volume='unit:0:0:1'
mode='host'>
Same question as above.
It seems this patch destroyed the "mode" attr for iscsi pool disk
completely
If you think so, please provide a test case that proves this that would
be applied before this patch that would show the problem. Otherwise I'm
pretty confident that it works as it's supposed.
with this patch. It again asks for the detailed commit log, since
the
changes
looks rather aggressive, which may cause regression.
I'll split those patches again as they should be.
Osier
Peter
[1]: gild the lily
* 1. To adorn unnecessarily something already beautiful.
* 2. To make superfluous additions to what is already complete.
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 05/27] qemu: Split out formatting of network disk source URI
The snapshot code will need to use qemu-style formatted URIs of network
disks. Split out the code to avoid duplication.
---
src/qemu/qemu_command.c | 146 ++++++++++++++++++++++++++++--------------------
src/qemu/qemu_command.h | 6 ++
2 files changed, 91 insertions(+), 61 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b8129a3..36bdc15 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3626,106 +3626,125 @@ error:
return -1;
}
-static int
-qemuBuildDriveURIString(virConnectPtr conn,
- virDomainDiskDefPtr disk, virBufferPtr opt,
- const char *scheme, virSecretUsageType secretUsageType)
-{
- int ret = -1;
- int port = 0;
- char *secret = NULL;
- char *tmpscheme = NULL;
- char *volimg = NULL;
- char *sock = NULL;
- char *user = NULL;
- char *builturi = NULL;
- const char *transp = NULL;
- virURI uri = {
- .port = port /* just to clear rest of bits */
- };
+char *
+qemuBuildNetworkDriveURI(int protocol,
+ const char *src,
+ size_t nhosts,
+ virDomainDiskHostDefPtr hosts,
+ const char *username,
+ const char *secret)
+{
+ char *ret = NULL;
+ virURIPtr uri = NULL;
- if (disk->nhosts != 1) {
+ if (nhosts != 1) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("%s accepts only one host"), scheme);
- return -1;
+ _("protocol '%s' accepts only one host"),
+ virDomainDiskProtocolTypeToString(protocol));
+ return NULL;
}
- virBufferAddLit(opt, "file=");
- transp = virDomainDiskProtocolTransportTypeToString(disk->hosts->transport);
+ if (VIR_ALLOC(uri) < 0)
+ return NULL;
- if (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP) {
- if (VIR_STRDUP(tmpscheme, scheme) < 0)
+ if (hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP) {
+ if (VIR_STRDUP(uri->scheme, virDomainDiskProtocolTypeToString(protocol)) <
0)
goto cleanup;
} else {
- if (virAsprintf(&tmpscheme, "%s+%s", scheme, transp) < 0)
+ if (virAsprintf(&uri->scheme, "%s+%s",
+ virDomainDiskProtocolTypeToString(protocol),
+ virDomainDiskProtocolTransportTypeToString(hosts->transport))
< 0)
goto cleanup;
}
- if (disk->src && virAsprintf(&volimg, "/%s", disk->src)
< 0)
+ if (src &&
+ virAsprintf(&uri->path, "/%s", src) < 0)
+ goto cleanup;
+
+ if (hosts->port &&
+ virStrToLong_i(hosts->port, NULL, 10, &uri->port) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse port number '%s'"),
+ hosts->port);
+ goto cleanup;
+ }
+
+ if (hosts->socket &&
+ virAsprintf(&uri->query, "socket=%s", hosts->socket) < 0)
goto cleanup;
- if (disk->hosts->port) {
- port = atoi(disk->hosts->port);
+ if (username) {
+ if (secret) {
+ if (virAsprintf(&uri->user, "%s:%s", username, secret) <
0)
+ goto cleanup;
+ } else {
+ if (VIR_STRDUP(uri->user, username) < 0)
+ goto cleanup;
+ }
}
- if (disk->hosts->socket &&
- virAsprintf(&sock, "socket=%s", disk->hosts->socket) < 0)
+ if (VIR_STRDUP(uri->server, hosts->name) < 0)
goto cleanup;
+ ret = virURIFormat(uri);
+
+cleanup:
+ virURIFree(uri);
+
+ return ret;
+}
+
+
+static int
+qemuBuildDriveURIString(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ virBufferPtr opt,
+ int protocol,
+ virSecretUsageType secretUsageType)
+{
+ char *secret = NULL;
+ char *builturi = NULL;
+ int ret = -1;
+
+ virBufferAddLit(opt, "file=");
+
if (disk->auth.username && secretUsageType != VIR_SECRET_USAGE_TYPE_NONE)
{
/* Get the secret string using the virDomainDiskDef */
- if (!(secret = qemuGetSecretString(conn, scheme, false,
+ if (!(secret = qemuGetSecretString(conn,
+ virDomainDiskProtocolTypeToString(protocol),
+ false,
disk->auth.secretType,
disk->auth.username,
disk->auth.secret.uuid,
disk->auth.secret.usage,
secretUsageType)))
goto cleanup;
- if (virAsprintf(&user, "%s:%s", disk->auth.username, secret)
< 0)
- goto cleanup;
}
- uri.scheme = tmpscheme; /* gluster+<transport> */
- uri.server = disk->hosts->name;
- uri.user = user;
- uri.port = port;
- uri.path = volimg;
- uri.query = sock;
+ if (!(builturi = qemuBuildNetworkDriveURI(protocol,
+ disk->src,
+ disk->nhosts,
+ disk->hosts,
+ disk->auth.username,
+ secret)))
+ goto cleanup;
- builturi = virURIFormat(&uri);
virBufferEscape(opt, ',', ",", "%s", builturi);
ret = 0;
cleanup:
- VIR_FREE(builturi);
- VIR_FREE(tmpscheme);
- VIR_FREE(volimg);
- VIR_FREE(sock);
VIR_FREE(secret);
- VIR_FREE(user);
+ VIR_FREE(builturi);
return ret;
}
-static int
-qemuBuildGlusterString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr opt)
-{
- return qemuBuildDriveURIString(conn, disk, opt, "gluster",
- VIR_SECRET_USAGE_TYPE_NONE);
-}
#define QEMU_DEFAULT_NBD_PORT "10809"
static int
-qemuBuildISCSIString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr opt)
-{
- return qemuBuildDriveURIString(conn, disk, opt, "iscsi",
- VIR_SECRET_USAGE_TYPE_ISCSI);
-}
-
-static int
qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr opt)
{
const char *transp;
@@ -3741,7 +3760,8 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk,
virBufferPtr op
&& !disk->hosts->name)
|| (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX
&& disk->hosts->socket && disk->hosts->socket[0]
!= '/'))
- return qemuBuildDriveURIString(conn, disk, opt, "nbd",
+ return qemuBuildDriveURIString(conn, disk, opt,
+ VIR_DOMAIN_DISK_PROTOCOL_NBD,
VIR_SECRET_USAGE_TYPE_NONE);
virBufferAddLit(opt, "file=nbd:");
@@ -3910,12 +3930,16 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virBufferAddChar(&opt, ',');
break;
case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
- if (qemuBuildGlusterString(conn, disk, &opt) < 0)
+ if (qemuBuildDriveURIString(conn, disk, &opt,
+ VIR_DOMAIN_DISK_PROTOCOL_GLUSTER,
+ VIR_SECRET_USAGE_TYPE_NONE) < 0)
goto error;
virBufferAddChar(&opt, ',');
break;
case VIR_DOMAIN_DISK_PROTOCOL_ISCSI:
- if (qemuBuildISCSIString(conn, disk, &opt) < 0)
+ if (qemuBuildDriveURIString(conn, disk, &opt,
+ VIR_DOMAIN_DISK_PROTOCOL_ISCSI,
+ VIR_SECRET_USAGE_TYPE_ISCSI) < 0)
goto error;
virBufferAddChar(&opt, ',');
break;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 3277ba4..66c23cc 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -180,6 +180,12 @@ char * qemuBuildHubDevStr(virDomainDefPtr def,
char * qemuBuildRedirdevDevStr(virDomainDefPtr def,
virDomainRedirdevDefPtr dev,
virQEMUCapsPtr qemuCaps);
+char *qemuBuildNetworkDriveURI(int proto,
+ const char *src,
+ size_t nhosts,
+ virDomainDiskHostDefPtr hosts,
+ const char *username,
+ const char *secret);
int qemuNetworkIfaceConnect(virDomainDefPtr def,
virConnectPtr conn,
--
1.8.4.3
12:30 p.m.
New subject: [libvirt] [PATCHv1.5 05/27] qemu: Split out formatting of network disk source URI
On 26.11.2013 17:48, Peter Krempa wrote:
The snapshot code will need to use qemu-style formatted URIs of
network
disks. Split out the code to avoid duplication.
---
src/qemu/qemu_command.c | 146 ++++++++++++++++++++++++++++--------------------
src/qemu/qemu_command.h | 6 ++
2 files changed, 91 insertions(+), 61 deletions(-)
ACK
Michal
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 06/27] qemu: Simplify call pattern of qemuBuildDriveURIString
Automatically assign secret type from the disk source definition and
pull in adding of the comma. Then update callers to keep generated
output the same.
---
src/qemu/qemu_command.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 36bdc15..2326221 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3699,9 +3699,7 @@ cleanup:
static int
qemuBuildDriveURIString(virConnectPtr conn,
virDomainDiskDefPtr disk,
- virBufferPtr opt,
- int protocol,
- virSecretUsageType secretUsageType)
+ virBufferPtr opt)
{
char *secret = NULL;
char *builturi = NULL;
@@ -3709,20 +3707,22 @@ qemuBuildDriveURIString(virConnectPtr conn,
virBufferAddLit(opt, "file=");
- if (disk->auth.username && secretUsageType != VIR_SECRET_USAGE_TYPE_NONE)
{
+ if (disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI &&
+ disk->auth.username) {
/* Get the secret string using the virDomainDiskDef */
if (!(secret = qemuGetSecretString(conn,
- virDomainDiskProtocolTypeToString(protocol),
+
virDomainDiskProtocolTypeToString(disk->protocol),
false,
disk->auth.secretType,
disk->auth.username,
disk->auth.secret.uuid,
disk->auth.secret.usage,
- secretUsageType)))
+ VIR_SECRET_USAGE_TYPE_ISCSI)))
goto cleanup;
}
- if (!(builturi = qemuBuildNetworkDriveURI(protocol,
+
+ if (!(builturi = qemuBuildNetworkDriveURI(disk->protocol,
disk->src,
disk->nhosts,
disk->hosts,
@@ -3731,6 +3731,7 @@ qemuBuildDriveURIString(virConnectPtr conn,
goto cleanup;
virBufferEscape(opt, ',', ",", "%s", builturi);
+ virBufferAddChar(opt, ',');
ret = 0;
@@ -3760,9 +3761,7 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk,
virBufferPtr op
&& !disk->hosts->name)
|| (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX
&& disk->hosts->socket && disk->hosts->socket[0]
!= '/'))
- return qemuBuildDriveURIString(conn, disk, opt,
- VIR_DOMAIN_DISK_PROTOCOL_NBD,
- VIR_SECRET_USAGE_TYPE_NONE);
+ return qemuBuildDriveURIString(conn, disk, opt);
virBufferAddLit(opt, "file=nbd:");
@@ -3792,6 +3791,8 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk,
virBufferPtr op
if (disk->src)
virBufferEscape(opt, ',', ",", ":exportname=%s",
disk->src);
+ virBufferAddChar(opt, ',');
+
return 0;
}
@@ -3921,7 +3922,6 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
case VIR_DOMAIN_DISK_PROTOCOL_NBD:
if (qemuBuildNBDString(conn, disk, &opt) < 0)
goto error;
- virBufferAddChar(&opt, ',');
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
virBufferAddLit(&opt, "file=");
@@ -3929,19 +3929,11 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
goto error;
virBufferAddChar(&opt, ',');
break;
+
case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
- if (qemuBuildDriveURIString(conn, disk, &opt,
- VIR_DOMAIN_DISK_PROTOCOL_GLUSTER,
- VIR_SECRET_USAGE_TYPE_NONE) < 0)
- goto error;
- virBufferAddChar(&opt, ',');
- break;
case VIR_DOMAIN_DISK_PROTOCOL_ISCSI:
- if (qemuBuildDriveURIString(conn, disk, &opt,
- VIR_DOMAIN_DISK_PROTOCOL_ISCSI,
- VIR_SECRET_USAGE_TYPE_ISCSI) < 0)
+ if (qemuBuildDriveURIString(conn, disk, &opt) < 0)
goto error;
- virBufferAddChar(&opt, ',');
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
--
1.8.4.3
12:30 p.m.
New subject: [libvirt] [PATCHv1.5 06/27] qemu: Simplify call pattern of qemuBuildDriveURIString
On 26.11.2013 17:48, Peter Krempa wrote:
Automatically assign secret type from the disk source definition and
pull in adding of the comma. Then update callers to keep generated
output the same.
---
src/qemu/qemu_command.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 36bdc15..2326221 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3699,9 +3699,7 @@ cleanup:
static int
qemuBuildDriveURIString(virConnectPtr conn,
virDomainDiskDefPtr disk,
- virBufferPtr opt,
- int protocol,
- virSecretUsageType secretUsageType)
+ virBufferPtr opt)
{
char *secret = NULL;
char *builturi = NULL;
@@ -3709,20 +3707,22 @@ qemuBuildDriveURIString(virConnectPtr conn,
virBufferAddLit(opt, "file=");
- if (disk->auth.username && secretUsageType != VIR_SECRET_USAGE_TYPE_NONE)
{
+ if (disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI &&
+ disk->auth.username) {
/* Get the secret string using the virDomainDiskDef */
if (!(secret = qemuGetSecretString(conn,
- virDomainDiskProtocolTypeToString(protocol),
+
virDomainDiskProtocolTypeToString(disk->protocol),
false,
disk->auth.secretType,
disk->auth.username,
disk->auth.secret.uuid,
disk->auth.secret.usage,
- secretUsageType)))
+ VIR_SECRET_USAGE_TYPE_ISCSI)))
I know we were currently using this only for _ISCSI, but when touching
this how about making it more robust and choosing the correct
VIR_SECRET_USAGE_TYPE_ here? The @disk is passed anyway so the decision
can be made here.
goto cleanup;
}
- if (!(builturi = qemuBuildNetworkDriveURI(protocol,
+
+ if (!(builturi = qemuBuildNetworkDriveURI(disk->protocol,
disk->src,
disk->nhosts,
disk->hosts,
@@ -3731,6 +3731,7 @@ qemuBuildDriveURIString(virConnectPtr conn,
goto cleanup;
virBufferEscape(opt, ',', ",", "%s", builturi);
+ virBufferAddChar(opt, ',');
ret = 0;
@@ -3760,9 +3761,7 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk,
virBufferPtr op
&& !disk->hosts->name)
|| (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX
&& disk->hosts->socket && disk->hosts->socket[0]
!= '/'))
- return qemuBuildDriveURIString(conn, disk, opt,
- VIR_DOMAIN_DISK_PROTOCOL_NBD,
- VIR_SECRET_USAGE_TYPE_NONE);
+ return qemuBuildDriveURIString(conn, disk, opt);
virBufferAddLit(opt, "file=nbd:");
@@ -3792,6 +3791,8 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk,
virBufferPtr op
if (disk->src)
virBufferEscape(opt, ',', ",", ":exportname=%s",
disk->src);
+ virBufferAddChar(opt, ',');
+
return 0;
}
@@ -3921,7 +3922,6 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
case VIR_DOMAIN_DISK_PROTOCOL_NBD:
if (qemuBuildNBDString(conn, disk, &opt) < 0)
goto error;
- virBufferAddChar(&opt, ',');
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
virBufferAddLit(&opt, "file=");
@@ -3929,19 +3929,11 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
goto error;
virBufferAddChar(&opt, ',');
break;
+
case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
- if (qemuBuildDriveURIString(conn, disk, &opt,
- VIR_DOMAIN_DISK_PROTOCOL_GLUSTER,
- VIR_SECRET_USAGE_TYPE_NONE) < 0)
- goto error;
- virBufferAddChar(&opt, ',');
- break;
case VIR_DOMAIN_DISK_PROTOCOL_ISCSI:
- if (qemuBuildDriveURIString(conn, disk, &opt,
- VIR_DOMAIN_DISK_PROTOCOL_ISCSI,
- VIR_SECRET_USAGE_TYPE_ISCSI) < 0)
+ if (qemuBuildDriveURIString(conn, disk, &opt) < 0)
goto error;
- virBufferAddChar(&opt, ',');
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
Weak ACK due to comment above.
Michal
10:02 a.m.
New subject: [libvirt] [PATCHv1.5 06/27] qemu: Simplify call pattern of qemuBuildDriveURIString
On 11/26/13 19:30, Michal Privoznik wrote:
On 26.11.2013 17:48, Peter Krempa wrote:
> Automatically assign secret type from the disk source definition and
> pull in adding of the comma. Then update callers to keep generated
> output the same.
> ---
> src/qemu/qemu_command.c | 34 +++++++++++++---------------------
> 1 file changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 36bdc15..2326221 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
...
> @@ -3709,20 +3707,22 @@ qemuBuildDriveURIString(virConnectPtr
conn,
>
> virBufferAddLit(opt, "file=");
>
> - if (disk->auth.username && secretUsageType !=
VIR_SECRET_USAGE_TYPE_NONE) {
> + if (disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI &&
> + disk->auth.username) {
> /* Get the secret string using the virDomainDiskDef */
> if (!(secret = qemuGetSecretString(conn,
> -
virDomainDiskProtocolTypeToString(protocol),
> +
virDomainDiskProtocolTypeToString(disk->protocol),
> false,
> disk->auth.secretType,
> disk->auth.username,
> disk->auth.secret.uuid,
> disk->auth.secret.usage,
> - secretUsageType)))
> + VIR_SECRET_USAGE_TYPE_ISCSI)))
I know we were currently using this only for _ISCSI, but when touching
this how about making it more robust and choosing the correct
VIR_SECRET_USAGE_TYPE_ here? The @disk is passed anyway so the decision
can be made here.
Well it is "choosing the correct type" right now technically :). See
patch 10/27 where I move RBD stuff here too that will do exactly what
you are asking here :)
>
....
Weak ACK due to comment above.
Michal
PEter
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 07/27] qemu: Use qemuBuildNetworkDriveURI to handle http/ftp and friends
Prepare the function to integrate other protocols and start folding
other network protocols into a common place.
---
src/qemu/qemu_command.c | 199 +++++++++++++++++++++++++++++-------------------
1 file changed, 122 insertions(+), 77 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2326221..09ebd00 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3627,6 +3627,61 @@ error:
}
+static int
+qemuNetworkDriveGetPort(int protocol,
+ const char *port)
+{
+ int ret = 0;
+
+ if (port) {
+ if (virStrToLong_i(port, NULL, 10, &ret) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to parse port number '%s'"),
+ port);
+ return -1;
+ }
+
+ return ret;
+ }
+
+ switch ((enum virDomainDiskProtocol) protocol) {
+ case VIR_DOMAIN_DISK_PROTOCOL_HTTP:
+ return 80;
+
+ case VIR_DOMAIN_DISK_PROTOCOL_HTTPS:
+ return 443;
+
+ case VIR_DOMAIN_DISK_PROTOCOL_FTP:
+ return 21;
+
+ case VIR_DOMAIN_DISK_PROTOCOL_FTPS:
+ return 990;
+
+ case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
+ return 69;
+
+ case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
+ return 7000;
+
+ case VIR_DOMAIN_DISK_PROTOCOL_NBD:
+ return 10809;
+
+ case VIR_DOMAIN_DISK_PROTOCOL_ISCSI:
+ case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
+ /* no default port specified */
+ return 0;
+
+ case VIR_DOMAIN_DISK_PROTOCOL_RBD:
+ case VIR_DOMAIN_DISK_PROTOCOL_LAST:
+ /* not aplicable */
+ return -1;
+ }
+
+ return -1;
+}
+
+
+
char *
qemuBuildNetworkDriveURI(int protocol,
const char *src,
@@ -3638,56 +3693,74 @@ qemuBuildNetworkDriveURI(int protocol,
char *ret = NULL;
virURIPtr uri = NULL;
- if (nhosts != 1) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("protocol '%s' accepts only one host"),
- virDomainDiskProtocolTypeToString(protocol));
- return NULL;
- }
-
- if (VIR_ALLOC(uri) < 0)
- return NULL;
+ switch ((enum virDomainDiskProtocol) protocol) {
+ case VIR_DOMAIN_DISK_PROTOCOL_HTTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_HTTPS:
+ case VIR_DOMAIN_DISK_PROTOCOL_FTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_FTPS:
+ case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_ISCSI:
+ case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
+ case VIR_DOMAIN_DISK_PROTOCOL_NBD:
+ if (nhosts != 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("protocol '%s' accepts only one
host"),
+ virDomainDiskProtocolTypeToString(protocol));
+ goto cleanup;
+ }
- if (hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP) {
- if (VIR_STRDUP(uri->scheme, virDomainDiskProtocolTypeToString(protocol)) <
0)
- goto cleanup;
- } else {
- if (virAsprintf(&uri->scheme, "%s+%s",
- virDomainDiskProtocolTypeToString(protocol),
- virDomainDiskProtocolTransportTypeToString(hosts->transport))
< 0)
- goto cleanup;
- }
+ if (VIR_ALLOC(uri) < 0)
+ goto cleanup;
- if (src &&
- virAsprintf(&uri->path, "/%s", src) < 0)
- goto cleanup;
+ if (hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP) {
+ if (VIR_STRDUP(uri->scheme,
+ virDomainDiskProtocolTypeToString(protocol)) < 0)
+ goto cleanup;
+ } else {
+ if (virAsprintf(&uri->scheme, "%s+%s",
+ virDomainDiskProtocolTypeToString(protocol),
+
virDomainDiskProtocolTransportTypeToString(hosts->transport)) < 0)
+ goto cleanup;
+ }
- if (hosts->port &&
- virStrToLong_i(hosts->port, NULL, 10, &uri->port) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse port number '%s'"),
- hosts->port);
- goto cleanup;
- }
+ if ((uri->port = qemuNetworkDriveGetPort(protocol, hosts->port)) <
0)
+ goto cleanup;
- if (hosts->socket &&
- virAsprintf(&uri->query, "socket=%s", hosts->socket) < 0)
- goto cleanup;
+ if (src &&
+ virAsprintf(&uri->path, "%s%s",
+ src[0] == '/' ? "" : "/",
+ src) < 0)
+ goto cleanup;
- if (username) {
- if (secret) {
- if (virAsprintf(&uri->user, "%s:%s", username, secret) <
0)
+ if (hosts->socket &&
+ virAsprintf(&uri->query, "socket=%s", hosts->socket)
< 0)
goto cleanup;
- } else {
- if (VIR_STRDUP(uri->user, username) < 0)
+
+ if (username) {
+ if (secret) {
+ if (virAsprintf(&uri->user, "%s:%s", username,
secret) < 0)
+ goto cleanup;
+ } else {
+ if (VIR_STRDUP(uri->user, username) < 0)
+ goto cleanup;
+ }
+ }
+
+ if (VIR_STRDUP(uri->server, hosts->name) < 0)
goto cleanup;
- }
- }
- if (VIR_STRDUP(uri->server, hosts->name) < 0)
- goto cleanup;
+ ret = virURIFormat(uri);
- ret = virURIFormat(uri);
+ break;
+
+ case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
+ case VIR_DOMAIN_DISK_PROTOCOL_RBD:
+ case VIR_DOMAIN_DISK_PROTOCOL_LAST:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("network disk protocol '%s' not
supported"),
+ virDomainDiskProtocolTypeToString(protocol));
+ goto cleanup;
+ }
cleanup:
virURIFree(uri);
@@ -3756,11 +3829,9 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk,
virBufferPtr op
return -1;
}
- if ((disk->hosts->name && strchr(disk->hosts->name,
':'))
- || (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP
- && !disk->hosts->name)
- || (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX
- && disk->hosts->socket && disk->hosts->socket[0]
!= '/'))
+ if ((disk->hosts->name && strchr(disk->hosts->name, ':'))
||
+ (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP &&
!disk->hosts->name) ||
+ (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX &&
disk->hosts->socket && disk->hosts->socket[0] != '/'))
return qemuBuildDriveURIString(conn, disk, opt);
virBufferAddLit(opt, "file=nbd:");
@@ -3930,6 +4001,11 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virBufferAddChar(&opt, ',');
break;
+ case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_FTPS:
+ case VIR_DOMAIN_DISK_PROTOCOL_FTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_HTTPS:
+ case VIR_DOMAIN_DISK_PROTOCOL_HTTP:
case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
case VIR_DOMAIN_DISK_PROTOCOL_ISCSI:
if (qemuBuildDriveURIString(conn, disk, &opt) < 0)
@@ -3948,37 +4024,6 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virBufferEscape(&opt, ',', ",",
"%s,", disk->src);
}
break;
-
- case VIR_DOMAIN_DISK_PROTOCOL_HTTP:
- virBufferAsprintf(&opt, "file=http://%s:%s",
- disk->hosts->name,
- disk->hosts->port ? disk->hosts->port :
"80");
- virBufferEscape(&opt, ',', ",", "%s,",
disk->src);
- break;
- case VIR_DOMAIN_DISK_PROTOCOL_HTTPS:
- virBufferAsprintf(&opt, "file=https://%s:%s",
- disk->hosts->name,
- disk->hosts->port ? disk->hosts->port :
"443");
- virBufferEscape(&opt, ',', ",", "%s,",
disk->src);
- break;
- case VIR_DOMAIN_DISK_PROTOCOL_FTP:
- virBufferAsprintf(&opt, "file=ftp://%s:%s",
- disk->hosts->name,
- disk->hosts->port ? disk->hosts->port :
"21");
- virBufferEscape(&opt, ',', ",", "%s,",
disk->src);
- break;
- case VIR_DOMAIN_DISK_PROTOCOL_FTPS:
- virBufferAsprintf(&opt, "file=ftps://%s:%s",
- disk->hosts->name,
- disk->hosts->port ? disk->hosts->port :
"990");
- virBufferEscape(&opt, ',', ",", "%s,",
disk->src);
- break;
- case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
- virBufferAsprintf(&opt, "file=tftp://%s:%s",
- disk->hosts->name,
- disk->hosts->port ? disk->hosts->port :
"69");
- virBufferEscape(&opt, ',', ",", "%s,",
disk->src);
- break;
}
} else {
if ((actualType == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
--
1.8.4.3
12:30 p.m.
New subject: [libvirt] [PATCHv1.5 07/27] qemu: Use qemuBuildNetworkDriveURI to handle http/ftp and friends
On 26.11.2013 17:48, Peter Krempa wrote:
Prepare the function to integrate other protocols and start folding
other network protocols into a common place.
---
src/qemu/qemu_command.c | 199 +++++++++++++++++++++++++++++-------------------
1 file changed, 122 insertions(+), 77 deletions(-)
ACK
Michal
(this is the point where I have to stop for today. I'll continue
tomorrow unless somebody reviews the rest)
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 08/27] qemu: Migrate sheepdog source generation into common function
---
src/qemu/qemu_command.c | 37 ++++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 09ebd00..6ba8df9 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3754,6 +3754,29 @@ qemuBuildNetworkDriveURI(int protocol,
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
+ if (!src) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing disk source for 'sheepdog'
protocol"));
+ goto cleanup;
+ }
+
+ if (nhosts == 0) {
+ if (virAsprintf(&ret, "sheepdog:%s", src) < 0)
+ goto cleanup;
+ } else if (nhosts == 1) {
+ if (virAsprintf(&ret, "sheepdog:%s:%s:%s",
+ hosts->name,
+ hosts->port ? hosts->port : "7000",
+ src) < 0)
+ goto cleanup;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("protocol 'sheepdog' accepts up to one
host"));
+ goto cleanup;
+ }
+
+ break;
+
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
case VIR_DOMAIN_DISK_PROTOCOL_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -4001,6 +4024,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virBufferAddChar(&opt, ',');
break;
+ case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
case VIR_DOMAIN_DISK_PROTOCOL_FTPS:
case VIR_DOMAIN_DISK_PROTOCOL_FTP:
@@ -4011,19 +4035,6 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
if (qemuBuildDriveURIString(conn, disk, &opt) < 0)
goto error;
break;
-
- case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
- if (disk->nhosts == 0) {
- virBufferEscape(&opt, ',', ",",
"file=sheepdog:%s,",
- disk->src);
- } else {
- /* only one host is supported now */
- virBufferAsprintf(&opt, "file=sheepdog:%s:%s:",
- disk->hosts->name,
- disk->hosts->port ? disk->hosts->port :
"7000");
- virBufferEscape(&opt, ',', ",",
"%s,", disk->src);
- }
- break;
}
} else {
if ((actualType == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
--
1.8.4.3
5:16 a.m.
New subject: [libvirt] [PATCHv1.5 08/27] qemu: Migrate sheepdog source generation into common function
On 26.11.2013 17:48, Peter Krempa wrote:
---
src/qemu/qemu_command.c | 37 ++++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 09ebd00..6ba8df9 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3754,6 +3754,29 @@ qemuBuildNetworkDriveURI(int protocol,
break;
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
+ if (!src) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing disk source for 'sheepdog'
protocol"));
+ goto cleanup;
+ }
+
+ if (nhosts == 0) {
+ if (virAsprintf(&ret, "sheepdog:%s", src) < 0)
+ goto cleanup;
+ } else if (nhosts == 1) {
+ if (virAsprintf(&ret, "sheepdog:%s:%s:%s",
+ hosts->name,
+ hosts->port ? hosts->port : "7000",
+ src) < 0)
+ goto cleanup;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("protocol 'sheepdog' accepts up to one
host"));
+ goto cleanup;
+ }
+
+ break;
+
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
case VIR_DOMAIN_DISK_PROTOCOL_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -4001,6 +4024,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virBufferAddChar(&opt, ',');
break;
+ case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
case VIR_DOMAIN_DISK_PROTOCOL_FTPS:
case VIR_DOMAIN_DISK_PROTOCOL_FTP:
@@ -4011,19 +4035,6 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
if (qemuBuildDriveURIString(conn, disk, &opt) < 0)
goto error;
break;
-
- case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
- if (disk->nhosts == 0) {
- virBufferEscape(&opt, ',', ",",
"file=sheepdog:%s,",
- disk->src);
- } else {
- /* only one host is supported now */
- virBufferAsprintf(&opt, "file=sheepdog:%s:%s:",
- disk->hosts->name,
- disk->hosts->port ? disk->hosts->port
: "7000");
- virBufferEscape(&opt, ',', ",",
"%s,", disk->src);
- }
- break;
}
} else {
if ((actualType == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
ACK
Michal
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 09/27] qemu: Split out NBD command generation
---
src/qemu/qemu_command.c | 117 +++++++++++++++++++++++++-----------------------
1 file changed, 61 insertions(+), 56 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6ba8df9..15a6e9b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3680,7 +3680,7 @@ qemuNetworkDriveGetPort(int protocol,
return -1;
}
-
+#define QEMU_DEFAULT_NBD_PORT "10809"
char *
qemuBuildNetworkDriveURI(int protocol,
@@ -3691,9 +3691,67 @@ qemuBuildNetworkDriveURI(int protocol,
const char *secret)
{
char *ret = NULL;
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
virURIPtr uri = NULL;
switch ((enum virDomainDiskProtocol) protocol) {
+ case VIR_DOMAIN_DISK_PROTOCOL_NBD:
+ if (nhosts != 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("protocol '%s' accepts only one
host"),
+ virDomainDiskProtocolTypeToString(protocol));
+ goto cleanup;
+ }
+
+ if (!((hosts->name && strchr(hosts->name, ':')) ||
+ (hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP &&
+ !hosts->name) ||
+ (hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX &&
+ hosts->socket &&
+ hosts->socket[0] != '/'))) {
+
+ virBufferAddLit(&buf, "nbd:");
+
+ switch (hosts->transport) {
+ case VIR_DOMAIN_DISK_PROTO_TRANS_TCP:
+ virBufferStrcat(&buf, hosts->name, NULL);
+ virBufferAsprintf(&buf, ":%s",
+ hosts->port ? hosts->port :
+ QEMU_DEFAULT_NBD_PORT);
+ break;
+
+ case VIR_DOMAIN_DISK_PROTO_TRANS_UNIX:
+ if (!hosts->socket) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("socket attribute required for "
+ "nix transport"));
+ goto cleanup;
+ }
+
+ virBufferAsprintf(&buf, "unix:%s", hosts->socket);
+ break;
+
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("nbd does not support transport
'%s'"),
+
virDomainDiskProtocolTransportTypeToString(hosts->transport));
+ goto cleanup;
+ }
+
+ if (src)
+ virBufferAsprintf(&buf, ":exportname=%s", src);
+
+ if (virBufferError(&buf) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ ret = virBufferContentAndReset(&buf);
+ goto cleanup;
+ }
+ /* fallthrough */
+ /* NBD code uses same formatting scheme as others in some cases */
+
case VIR_DOMAIN_DISK_PROTOCOL_HTTP:
case VIR_DOMAIN_DISK_PROTOCOL_HTTPS:
case VIR_DOMAIN_DISK_PROTOCOL_FTP:
@@ -3701,7 +3759,6 @@ qemuBuildNetworkDriveURI(int protocol,
case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
case VIR_DOMAIN_DISK_PROTOCOL_ISCSI:
case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
- case VIR_DOMAIN_DISK_PROTOCOL_NBD:
if (nhosts != 1) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("protocol '%s' accepts only one
host"),
@@ -3786,6 +3843,7 @@ qemuBuildNetworkDriveURI(int protocol,
}
cleanup:
+ virBufferFreeAndReset(&buf);
virURIFree(uri);
return ret;
@@ -3839,56 +3897,6 @@ cleanup:
}
-#define QEMU_DEFAULT_NBD_PORT "10809"
-
-static int
-qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr opt)
-{
- const char *transp;
-
- if (disk->nhosts != 1) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("nbd accepts only one host"));
- return -1;
- }
-
- if ((disk->hosts->name && strchr(disk->hosts->name, ':'))
||
- (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP &&
!disk->hosts->name) ||
- (disk->hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX &&
disk->hosts->socket && disk->hosts->socket[0] != '/'))
- return qemuBuildDriveURIString(conn, disk, opt);
-
- virBufferAddLit(opt, "file=nbd:");
-
- switch (disk->hosts->transport) {
- case VIR_DOMAIN_DISK_PROTO_TRANS_TCP:
- if (disk->hosts->name)
- virBufferEscape(opt, ',', ",", "%s",
disk->hosts->name);
- virBufferEscape(opt, ',', ",", ":%s",
- disk->hosts->port ? disk->hosts->port :
- QEMU_DEFAULT_NBD_PORT);
- break;
- case VIR_DOMAIN_DISK_PROTO_TRANS_UNIX:
- if (!disk->hosts->socket) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("socket attribute required for unix transport"));
- return -1;
- }
- virBufferEscape(opt, ',', ",", "unix:%s",
disk->hosts->socket);
- break;
- default:
- transp =
virDomainDiskProtocolTransportTypeToString(disk->hosts->transport);
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("nbd does not support transport '%s'"),
transp);
- break;
- }
-
- if (disk->src)
- virBufferEscape(opt, ',', ",", ":exportname=%s",
disk->src);
-
- virBufferAddChar(opt, ',');
-
- return 0;
-}
char *
@@ -4013,10 +4021,6 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virBufferEscape(&opt, ',', ",",
"file=fat:%s,", disk->src);
} else if (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK) {
switch (disk->protocol) {
- case VIR_DOMAIN_DISK_PROTOCOL_NBD:
- if (qemuBuildNBDString(conn, disk, &opt) < 0)
- goto error;
- break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
virBufferAddLit(&opt, "file=");
if (qemuBuildRBDString(conn, disk, &opt) < 0)
@@ -4024,6 +4028,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virBufferAddChar(&opt, ',');
break;
+ case VIR_DOMAIN_DISK_PROTOCOL_NBD:
case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
case VIR_DOMAIN_DISK_PROTOCOL_FTPS:
--
1.8.4.3
5:15 a.m.
New subject: [libvirt] [PATCHv1.5 09/27] qemu: Split out NBD command generation
On 26.11.2013 17:48, Peter Krempa wrote:
---
src/qemu/qemu_command.c | 117 +++++++++++++++++++++++++-----------------------
1 file changed, 61 insertions(+), 56 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6ba8df9..15a6e9b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3680,7 +3680,7 @@ qemuNetworkDriveGetPort(int protocol,
return -1;
}
-
+#define QEMU_DEFAULT_NBD_PORT "10809"
char *
qemuBuildNetworkDriveURI(int protocol,
@@ -3691,9 +3691,67 @@ qemuBuildNetworkDriveURI(int protocol,
const char *secret)
{
char *ret = NULL;
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
virURIPtr uri = NULL;
switch ((enum virDomainDiskProtocol) protocol) {
+ case VIR_DOMAIN_DISK_PROTOCOL_NBD:
+ if (nhosts != 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("protocol '%s' accepts only one
host"),
+ virDomainDiskProtocolTypeToString(protocol));
+ goto cleanup;
+ }
+
+ if (!((hosts->name && strchr(hosts->name, ':')) ||
+ (hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_TCP &&
+ !hosts->name) ||
+ (hosts->transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX &&
+ hosts->socket &&
+ hosts->socket[0] != '/'))) {
+
+ virBufferAddLit(&buf, "nbd:");
+
+ switch (hosts->transport) {
+ case VIR_DOMAIN_DISK_PROTO_TRANS_TCP:
+ virBufferStrcat(&buf, hosts->name, NULL);
+ virBufferAsprintf(&buf, ":%s",
+ hosts->port ? hosts->port :
+ QEMU_DEFAULT_NBD_PORT);
+ break;
+
+ case VIR_DOMAIN_DISK_PROTO_TRANS_UNIX:
+ if (!hosts->socket) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("socket attribute required for "
+ "nix transport"));
s/nix/unix/
+ goto cleanup;
+ }
+
+ virBufferAsprintf(&buf, "unix:%s", hosts->socket);
+ break;
+
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("nbd does not support transport
'%s'"),
+
virDomainDiskProtocolTransportTypeToString(hosts->transport));
+ goto cleanup;
+ }
+
+ if (src)
+ virBufferAsprintf(&buf, ":exportname=%s", src);
+
+ if (virBufferError(&buf) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ ret = virBufferContentAndReset(&buf);
+ goto cleanup;
+ }
+ /* fallthrough */
+ /* NBD code uses same formatting scheme as others in some cases */
+
case VIR_DOMAIN_DISK_PROTOCOL_HTTP:
case VIR_DOMAIN_DISK_PROTOCOL_HTTPS:
case VIR_DOMAIN_DISK_PROTOCOL_FTP:
ACK
Michal
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 10/27] qemu: Unify formatting of RBD sources
---
src/qemu/qemu_command.c | 155 +++++++++++++++++++-----------------------------
1 file changed, 61 insertions(+), 94 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 15a6e9b..799209d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3253,72 +3253,6 @@ cleanup:
return secret;
}
-static int
-qemuBuildRBDString(virConnectPtr conn,
- virDomainDiskDefPtr disk,
- virBufferPtr opt)
-{
- size_t i;
- int ret = 0;
- char *secret = NULL;
-
- if (strchr(disk->src, ':')) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("':' not allowed in RBD source volume name
'%s'"),
- disk->src);
- return -1;
- }
-
- virBufferEscape(opt, ',', ",", "rbd:%s", disk->src);
- if (disk->auth.username) {
- virBufferEscape(opt, '\\', ":", ":id=%s",
disk->auth.username);
- /* Get the secret string using the virDomainDiskDef
- * NOTE: qemu/librbd wants it base64 encoded
- */
- if (!(secret = qemuGetSecretString(conn, "rbd", true,
- disk->auth.secretType,
- disk->auth.username,
- disk->auth.secret.uuid,
- disk->auth.secret.usage,
- VIR_SECRET_USAGE_TYPE_CEPH)))
- goto error;
-
-
- virBufferEscape(opt, '\\', ":",
- ":key=%s:auth_supported=cephx\\;none",
- secret);
- } else {
- virBufferAddLit(opt, ":auth_supported=none");
- }
-
- if (disk->nhosts > 0) {
- virBufferAddLit(opt, ":mon_host=");
- for (i = 0; i < disk->nhosts; ++i) {
- if (i) {
- virBufferAddLit(opt, "\\;");
- }
-
- /* assume host containing : is ipv6 */
- if (strchr(disk->hosts[i].name, ':')) {
- virBufferEscape(opt, '\\', ":", "[%s]",
disk->hosts[i].name);
- } else {
- virBufferAsprintf(opt, "%s", disk->hosts[i].name);
- }
- if (disk->hosts[i].port) {
- virBufferAsprintf(opt, "\\:%s", disk->hosts[i].port);
- }
- }
- }
-
-cleanup:
- VIR_FREE(secret);
-
- return ret;
-
-error:
- ret = -1;
- goto cleanup;
-}
static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport)
{
@@ -3693,6 +3627,7 @@ qemuBuildNetworkDriveURI(int protocol,
char *ret = NULL;
virBuffer buf = VIR_BUFFER_INITIALIZER;
virURIPtr uri = NULL;
+ size_t i;
switch ((enum virDomainDiskProtocol) protocol) {
case VIR_DOMAIN_DISK_PROTOCOL_NBD:
@@ -3835,10 +3770,51 @@ qemuBuildNetworkDriveURI(int protocol,
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
+ if (strchr(src, ':')) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("':' not allowed in RBD source volume name
'%s'"),
+ src);
+ goto cleanup;
+ }
+
+ virBufferStrcat(&buf, "rbd:", src, NULL);
+
+ if (username) {
+ virBufferEscape(&buf, '\\', ":",
":id=%s", username);
+ virBufferEscape(&buf, '\\', ":",
+ ":key=%s:auth_supported=cephx\\;none",
+ secret);
+ } else {
+ virBufferAddLit(&buf, ":auth_supported=none");
+ }
+
+ if (nhosts > 0) {
+ virBufferAddLit(&buf, ":mon_host=");
+ for (i = 0; i < nhosts; i++) {
+ if (i)
+ virBufferAddLit(&buf, "\\;");
+
+ /* assume host containing : is ipv6 */
+ if (strchr(hosts[i].name, ':'))
+ virBufferEscape(&buf, '\\', ":",
"[%s]", hosts[i].name);
+ else
+ virBufferAsprintf(&buf, "%s", hosts[i].name);
+
+ if (hosts[i].port)
+ virBufferAsprintf(&buf, "\\:%s", hosts[i].port);
+ }
+ }
+
+ if (virBufferError(&buf) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ ret = virBufferContentAndReset(&buf);
+ break;
+
+
case VIR_DOMAIN_DISK_PROTOCOL_LAST:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("network disk protocol '%s' not
supported"),
- virDomainDiskProtocolTypeToString(protocol));
goto cleanup;
}
@@ -3861,17 +3837,26 @@ qemuBuildDriveURIString(virConnectPtr conn,
virBufferAddLit(opt, "file=");
- if (disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI &&
+ if ((disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI ||
+ disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) &&
disk->auth.username) {
- /* Get the secret string using the virDomainDiskDef */
+ bool encode = false;
+ int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
+
+ if (disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
+ /* qemu requires the secret to be encoded for RBD */
+ encode = true;
+ secretType = VIR_SECRET_USAGE_TYPE_CEPH;
+ }
+
if (!(secret = qemuGetSecretString(conn,
virDomainDiskProtocolTypeToString(disk->protocol),
- false,
+ encode,
disk->auth.secretType,
disk->auth.username,
disk->auth.secret.uuid,
disk->auth.secret.usage,
- VIR_SECRET_USAGE_TYPE_ISCSI)))
+ secretType)))
goto cleanup;
}
@@ -4019,28 +4004,10 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
disk->src);
else
virBufferEscape(&opt, ',', ",",
"file=fat:%s,", disk->src);
- } else if (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK) {
- switch (disk->protocol) {
- case VIR_DOMAIN_DISK_PROTOCOL_RBD:
- virBufferAddLit(&opt, "file=");
- if (qemuBuildRBDString(conn, disk, &opt) < 0)
- goto error;
- virBufferAddChar(&opt, ',');
- break;
- case VIR_DOMAIN_DISK_PROTOCOL_NBD:
- case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
- case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
- case VIR_DOMAIN_DISK_PROTOCOL_FTPS:
- case VIR_DOMAIN_DISK_PROTOCOL_FTP:
- case VIR_DOMAIN_DISK_PROTOCOL_HTTPS:
- case VIR_DOMAIN_DISK_PROTOCOL_HTTP:
- case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
- case VIR_DOMAIN_DISK_PROTOCOL_ISCSI:
- if (qemuBuildDriveURIString(conn, disk, &opt) < 0)
- goto error;
- break;
- }
+ } else if (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK) {
+ if (qemuBuildDriveURIString(conn, disk, &opt) < 0)
+ goto error;
} else {
if ((actualType == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
(disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
--
1.8.4.3
5:15 a.m.
New subject: [libvirt] [PATCHv1.5 10/27] qemu: Unify formatting of RBD sources
On 26.11.2013 17:48, Peter Krempa wrote:
---
src/qemu/qemu_command.c | 155 +++++++++++++++++++-----------------------------
1 file changed, 61 insertions(+), 94 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 15a6e9b..799209d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3253,72 +3253,6 @@ cleanup:
return secret;
}
-static int
-qemuBuildRBDString(virConnectPtr conn,
- virDomainDiskDefPtr disk,
- virBufferPtr opt)
-{
- size_t i;
- int ret = 0;
- char *secret = NULL;
-
- if (strchr(disk->src, ':')) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("':' not allowed in RBD source volume name
'%s'"),
- disk->src);
- return -1;
- }
-
- virBufferEscape(opt, ',', ",", "rbd:%s", disk->src);
- if (disk->auth.username) {
- virBufferEscape(opt, '\\', ":", ":id=%s",
disk->auth.username);
- /* Get the secret string using the virDomainDiskDef
- * NOTE: qemu/librbd wants it base64 encoded
- */
- if (!(secret = qemuGetSecretString(conn, "rbd", true,
- disk->auth.secretType,
- disk->auth.username,
- disk->auth.secret.uuid,
- disk->auth.secret.usage,
- VIR_SECRET_USAGE_TYPE_CEPH)))
- goto error;
-
-
- virBufferEscape(opt, '\\', ":",
- ":key=%s:auth_supported=cephx\\;none",
- secret);
- } else {
- virBufferAddLit(opt, ":auth_supported=none");
- }
-
- if (disk->nhosts > 0) {
- virBufferAddLit(opt, ":mon_host=");
- for (i = 0; i < disk->nhosts; ++i) {
- if (i) {
- virBufferAddLit(opt, "\\;");
- }
-
- /* assume host containing : is ipv6 */
- if (strchr(disk->hosts[i].name, ':')) {
- virBufferEscape(opt, '\\', ":", "[%s]",
disk->hosts[i].name);
- } else {
- virBufferAsprintf(opt, "%s", disk->hosts[i].name);
- }
- if (disk->hosts[i].port) {
- virBufferAsprintf(opt, "\\:%s", disk->hosts[i].port);
- }
- }
- }
-
-cleanup:
- VIR_FREE(secret);
-
- return ret;
-
-error:
- ret = -1;
- goto cleanup;
-}
static int qemuAddRBDHost(virDomainDiskDefPtr disk, char *hostport)
{
@@ -3693,6 +3627,7 @@ qemuBuildNetworkDriveURI(int protocol,
char *ret = NULL;
virBuffer buf = VIR_BUFFER_INITIALIZER;
virURIPtr uri = NULL;
+ size_t i;
switch ((enum virDomainDiskProtocol) protocol) {
case VIR_DOMAIN_DISK_PROTOCOL_NBD:
@@ -3835,10 +3770,51 @@ qemuBuildNetworkDriveURI(int protocol,
break;
case VIR_DOMAIN_DISK_PROTOCOL_RBD:
+ if (strchr(src, ':')) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("':' not allowed in RBD source volume name
'%s'"),
+ src);
+ goto cleanup;
+ }
+
+ virBufferStrcat(&buf, "rbd:", src, NULL);
+
+ if (username) {
+ virBufferEscape(&buf, '\\', ":",
":id=%s", username);
+ virBufferEscape(&buf, '\\', ":",
+ ":key=%s:auth_supported=cephx\\;none",
+ secret);
+ } else {
+ virBufferAddLit(&buf, ":auth_supported=none");
+ }
+
+ if (nhosts > 0) {
+ virBufferAddLit(&buf, ":mon_host=");
+ for (i = 0; i < nhosts; i++) {
+ if (i)
+ virBufferAddLit(&buf, "\\;");
+
+ /* assume host containing : is ipv6 */
+ if (strchr(hosts[i].name, ':'))
+ virBufferEscape(&buf, '\\', ":",
"[%s]", hosts[i].name);
+ else
+ virBufferAsprintf(&buf, "%s", hosts[i].name);
+
+ if (hosts[i].port)
+ virBufferAsprintf(&buf, "\\:%s", hosts[i].port);
+ }
+ }
+
+ if (virBufferError(&buf) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ ret = virBufferContentAndReset(&buf);
+ break;
+
+
case VIR_DOMAIN_DISK_PROTOCOL_LAST:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("network disk protocol '%s' not
supported"),
- virDomainDiskProtocolTypeToString(protocol));
goto cleanup;
}
@@ -3861,17 +3837,26 @@ qemuBuildDriveURIString(virConnectPtr conn,
virBufferAddLit(opt, "file=");
- if (disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI &&
+ if ((disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI ||
+ disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) &&
disk->auth.username) {
- /* Get the secret string using the virDomainDiskDef */
+ bool encode = false;
+ int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
+
+ if (disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
+ /* qemu requires the secret to be encoded for RBD */
+ encode = true;
+ secretType = VIR_SECRET_USAGE_TYPE_CEPH;
+ }
+
if (!(secret = qemuGetSecretString(conn,
virDomainDiskProtocolTypeToString(disk->protocol),
- false,
+ encode,
disk->auth.secretType,
disk->auth.username,
disk->auth.secret.uuid,
disk->auth.secret.usage,
- VIR_SECRET_USAGE_TYPE_ISCSI)))
+ secretType)))
goto cleanup;
}
@@ -4019,28 +4004,10 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
disk->src);
else
virBufferEscape(&opt, ',', ",",
"file=fat:%s,", disk->src);
- } else if (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK) {
- switch (disk->protocol) {
- case VIR_DOMAIN_DISK_PROTOCOL_RBD:
- virBufferAddLit(&opt, "file=");
- if (qemuBuildRBDString(conn, disk, &opt) < 0)
- goto error;
- virBufferAddChar(&opt, ',');
- break;
- case VIR_DOMAIN_DISK_PROTOCOL_NBD:
- case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
- case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
- case VIR_DOMAIN_DISK_PROTOCOL_FTPS:
- case VIR_DOMAIN_DISK_PROTOCOL_FTP:
- case VIR_DOMAIN_DISK_PROTOCOL_HTTPS:
- case VIR_DOMAIN_DISK_PROTOCOL_HTTP:
- case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
- case VIR_DOMAIN_DISK_PROTOCOL_ISCSI:
- if (qemuBuildDriveURIString(conn, disk, &opt) < 0)
- goto error;
- break;
- }
+ } else if (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK) {
+ if (qemuBuildDriveURIString(conn, disk, &opt) < 0)
+ goto error;
} else {
if ((actualType == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
(disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
ACK
Michal
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 11/27] qemu: Refactor disk source string formatting
---
src/qemu/qemu_command.c | 128 ++++++++++++++++++++++++++++++++----------------
1 file changed, 86 insertions(+), 42 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 799209d..0738de2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3827,19 +3827,62 @@ cleanup:
static int
-qemuBuildDriveURIString(virConnectPtr conn,
- virDomainDiskDefPtr disk,
- virBufferPtr opt)
+qemuGetDriveSourceString(int type,
+ const char *src,
+ int protocol,
+ size_t nhosts,
+ virDomainDiskHostDefPtr hosts,
+ const char *username,
+ const char *secret,
+ char **path)
{
+ *path = NULL;
+
+ switch ((enum virDomainDiskType) type) {
+ case VIR_DOMAIN_DISK_TYPE_BLOCK:
+ case VIR_DOMAIN_DISK_TYPE_FILE:
+ case VIR_DOMAIN_DISK_TYPE_DIR:
+ if (!src)
+ return 1;
+
+ if (VIR_STRDUP(*path, src) < 0)
+ return -1;
+
+ break;
+
+ case VIR_DOMAIN_DISK_TYPE_NETWORK:
+ if (!(*path = qemuBuildNetworkDriveURI(protocol,
+ src,
+ nhosts,
+ hosts,
+ username,
+ secret)))
+ return -1;
+ break;
+
+ case VIR_DOMAIN_DISK_TYPE_VOLUME:
+ case VIR_DOMAIN_DISK_TYPE_LAST:
+ break;
+ }
+
+ return 0;
+}
+
+static int
+qemuDomainDiskGetSourceString(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ char **source)
+{
+ int actualType = qemuDiskGetActualType(disk);
char *secret = NULL;
- char *builturi = NULL;
int ret = -1;
- virBufferAddLit(opt, "file=");
+ *source = NULL;
- if ((disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI ||
- disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) &&
- disk->auth.username) {
+ if (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK &&
+ disk->auth.username &&
+ (disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI ||
+ disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD)) {
bool encode = false;
int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
@@ -3860,32 +3903,23 @@ qemuBuildDriveURIString(virConnectPtr conn,
goto cleanup;
}
-
- if (!(builturi = qemuBuildNetworkDriveURI(disk->protocol,
- disk->src,
- disk->nhosts,
- disk->hosts,
- disk->auth.username,
- secret)))
- goto cleanup;
-
- virBufferEscape(opt, ',', ",", "%s", builturi);
- virBufferAddChar(opt, ',');
-
- ret = 0;
+ ret = qemuGetDriveSourceString(qemuDiskGetActualType(disk),
+ disk->src,
+ disk->protocol,
+ disk->nhosts,
+ disk->hosts,
+ disk->auth.username,
+ secret,
+ source);
cleanup:
VIR_FREE(secret);
- VIR_FREE(builturi);
-
return ret;
}
-
-
char *
-qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
+qemuBuildDriveStr(virConnectPtr conn,
virDomainDiskDefPtr disk,
bool bootable,
virQEMUCapsPtr qemuCaps)
@@ -3896,6 +3930,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virDomainDiskGeometryTransTypeToString(disk->geometry.trans);
int idx = virDiskNameToIndex(disk->dst);
int busid = -1, unitid = -1;
+ char *source = NULL;
int actualType = qemuDiskGetActualType(disk);
if (idx < 0) {
@@ -3978,15 +4013,18 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
break;
}
- /* disk->src is NULL when we use nbd disks */
- if ((disk->src ||
- (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK &&
- disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_NBD)) &&
+ if (qemuDomainDiskGetSourceString(conn, disk, &source) < 0)
+ goto error;
+
+ if (source &&
!((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
- if (actualType == VIR_DOMAIN_DISK_TYPE_DIR) {
+ virBufferAddLit(&opt, "file=");
+
+ switch (actualType) {
+ case VIR_DOMAIN_DISK_TYPE_DIR:
/* QEMU only supports magic FAT format for now */
if (disk->format > 0 && disk->format !=
VIR_STORAGE_FILE_FAT) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -3994,31 +4032,37 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virStorageFileFormatTypeToString(disk->format));
goto error;
}
+
if (!disk->readonly) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot create virtual FAT disks in read-write
mode"));
goto error;
}
+
+ virBufferAddLit(&opt, "fat:");
+
if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
- virBufferEscape(&opt, ',', ",",
"file=fat:floppy:%s,",
- disk->src);
- else
- virBufferEscape(&opt, ',', ",",
"file=fat:%s,", disk->src);
+ virBufferAddLit(&opt, "floppy:");
- } else if (actualType == VIR_DOMAIN_DISK_TYPE_NETWORK) {
- if (qemuBuildDriveURIString(conn, disk, &opt) < 0)
- goto error;
- } else {
- if ((actualType == VIR_DOMAIN_DISK_TYPE_BLOCK) &&
- (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
+ break;
+
+ case VIR_DOMAIN_DISK_TYPE_BLOCK:
+ if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("tray status 'open' is invalid for "
"block type disk"));
goto error;
}
- virBufferEscape(&opt, ',', ",", "file=%s,",
disk->src);
+
+ break;
+
+ default:
+ break;
}
+
+ virBufferEscape(&opt, ',', ",", "%s,", source);
}
+
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
virBufferAddLit(&opt, "if=none");
else
--
1.8.4.3
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 12/27] conf: Support disk source formatting without needing a virDomainDiskDefPtr
The <source> element formatting function was expecting a
virDomainDiskDefPtr to store the data. As snapshots are not using this
data structure to hold the data, we need to add an internal function
which splits out individual fields separately.
---
src/conf/domain_conf.c | 129 ++++++++++++++++++++++++++++++-------------------
1 file changed, 78 insertions(+), 51 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 140eb80..afeadd7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14352,28 +14352,38 @@ virDomainDiskBlockIoDefFormat(virBufferPtr buf,
}
static int
-virDomainDiskSourceDefFormat(virBufferPtr buf,
- virDomainDiskDefPtr def,
- unsigned int flags)
+virDomainDiskSourceDefFormatInternal(virBufferPtr buf,
+ int type,
+ const char *src,
+ int policy,
+ int protocol,
+ size_t nhosts,
+ virDomainDiskHostDefPtr hosts,
+ size_t nseclabels,
+ virSecurityDeviceLabelDefPtr *seclabels,
+ virDomainDiskSourcePoolDefPtr srcpool,
+ unsigned int flags)
{
- int n;
- const char *startupPolicy =
virDomainStartupPolicyTypeToString(def->startupPolicy);
+ size_t n;
+ const char *startupPolicy = NULL;
- if (def->src || def->nhosts > 0 || def->srcpool ||
- def->startupPolicy) {
- switch (def->type) {
+ if (policy)
+ startupPolicy = virDomainStartupPolicyTypeToString(policy);
+
+ if (src || nhosts > 0 || srcpool || startupPolicy) {
+ switch (type) {
case VIR_DOMAIN_DISK_TYPE_FILE:
virBufferAddLit(buf, " <source");
- if (def->src)
- virBufferEscapeString(buf, " file='%s'", def->src);
- if (def->startupPolicy)
+ if (src)
+ virBufferEscapeString(buf, " file='%s'", src);
+ if (startupPolicy)
virBufferEscapeString(buf, " startupPolicy='%s'",
startupPolicy);
- if (def->nseclabels) {
+ if (nseclabels) {
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 8);
- for (n = 0; n < def->nseclabels; n++)
- virSecurityDeviceLabelDefFormat(buf, def->seclabels[n],
+ for (n = 0; n < nseclabels; n++)
+ virSecurityDeviceLabelDefFormat(buf, seclabels[n],
flags);
virBufferAdjustIndent(buf, -8);
virBufferAddLit(buf, " </source>\n");
@@ -14383,15 +14393,15 @@ virDomainDiskSourceDefFormat(virBufferPtr buf,
break;
case VIR_DOMAIN_DISK_TYPE_BLOCK:
virBufferAddLit(buf, " <source");
- virBufferEscapeString(buf, " dev='%s'", def->src);
- if (def->startupPolicy)
+ virBufferEscapeString(buf, " dev='%s'", src);
+ if (startupPolicy)
virBufferEscapeString(buf, " startupPolicy='%s'",
startupPolicy);
- if (def->nseclabels) {
+ if (nseclabels) {
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 8);
- for (n = 0; n < def->nseclabels; n++)
- virSecurityDeviceLabelDefFormat(buf, def->seclabels[n],
+ for (n = 0; n < nseclabels; n++)
+ virSecurityDeviceLabelDefFormat(buf, seclabels[n],
flags);
virBufferAdjustIndent(buf, -8);
virBufferAddLit(buf, " </source>\n");
@@ -14400,41 +14410,38 @@ virDomainDiskSourceDefFormat(virBufferPtr buf,
}
break;
case VIR_DOMAIN_DISK_TYPE_DIR:
- virBufferEscapeString(buf, " <source dir='%s'",
- def->src);
- if (def->startupPolicy)
+ virBufferEscapeString(buf, " <source dir='%s'",
src);
+ if (startupPolicy)
virBufferEscapeString(buf, " startupPolicy='%s'",
startupPolicy);
virBufferAddLit(buf, "/>\n");
break;
case VIR_DOMAIN_DISK_TYPE_NETWORK:
virBufferAsprintf(buf, " <source protocol='%s'",
- virDomainDiskProtocolTypeToString(def->protocol));
- if (def->src) {
- virBufferEscapeString(buf, " name='%s'", def->src);
- }
- if (def->nhosts == 0) {
+ virDomainDiskProtocolTypeToString(protocol));
+ if (src)
+ virBufferEscapeString(buf, " name='%s'", src);
+
+ if (nhosts == 0) {
virBufferAddLit(buf, "/>\n");
} else {
- size_t i;
-
virBufferAddLit(buf, ">\n");
- for (i = 0; i < def->nhosts; i++) {
+ for (n = 0; n < nhosts; n++) {
virBufferAddLit(buf, " <host");
- if (def->hosts[i].name) {
- virBufferEscapeString(buf, " name='%s'",
def->hosts[i].name);
- }
- if (def->hosts[i].port) {
+ if (hosts[n].name)
+ virBufferEscapeString(buf, " name='%s'",
hosts[n].name);
+
+ if (hosts[n].port)
virBufferEscapeString(buf, " port='%s'",
- def->hosts[i].port);
- }
- if (def->hosts[i].transport) {
+ hosts[n].port);
+
+ if (hosts[n].transport)
virBufferAsprintf(buf, " transport='%s'",
-
virDomainDiskProtocolTransportTypeToString(def->hosts[i].transport));
- }
- if (def->hosts[i].socket) {
- virBufferEscapeString(buf, " socket='%s'",
def->hosts[i].socket);
- }
+
virDomainDiskProtocolTransportTypeToString(hosts[n].transport));
+
+ if (hosts[n].socket)
+ virBufferEscapeString(buf, " socket='%s'",
hosts[n].socket);
+
virBufferAddLit(buf, "/>\n");
}
virBufferAddLit(buf, " </source>\n");
@@ -14443,21 +14450,21 @@ virDomainDiskSourceDefFormat(virBufferPtr buf,
case VIR_DOMAIN_DISK_TYPE_VOLUME:
virBufferAddLit(buf, " <source");
- if (def->srcpool) {
+ if (srcpool) {
virBufferAsprintf(buf, " pool='%s'
volume='%s'",
- def->srcpool->pool, def->srcpool->volume);
- if (def->srcpool->mode)
+ srcpool->pool, srcpool->volume);
+ if (srcpool->mode)
virBufferAsprintf(buf, " mode='%s'",
-
virDomainDiskSourcePoolModeTypeToString(def->srcpool->mode));
+
virDomainDiskSourcePoolModeTypeToString(srcpool->mode));
}
- if (def->startupPolicy)
+ if (startupPolicy)
virBufferEscapeString(buf, " startupPolicy='%s'",
startupPolicy);
- if (def->nseclabels) {
+ if (nseclabels) {
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 8);
- for (n = 0; n < def->nseclabels; n++)
- virSecurityDeviceLabelDefFormat(buf, def->seclabels[n],
+ for (n = 0; n < nseclabels; n++)
+ virSecurityDeviceLabelDefFormat(buf, seclabels[n],
flags);
virBufferAdjustIndent(buf, -8);
virBufferAddLit(buf, " </source>\n");
@@ -14468,7 +14475,7 @@ virDomainDiskSourceDefFormat(virBufferPtr buf,
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected disk type %s"),
- virDomainDiskTypeToString(def->type));
+ virDomainDiskTypeToString(type));
return -1;
}
}
@@ -14476,6 +14483,26 @@ virDomainDiskSourceDefFormat(virBufferPtr buf,
return 0;
}
+
+static int
+virDomainDiskSourceDefFormat(virBufferPtr buf,
+ virDomainDiskDefPtr def,
+ unsigned int flags)
+{
+ return virDomainDiskSourceDefFormatInternal(buf,
+ def->type,
+ def->src,
+ def->startupPolicy,
+ def->protocol,
+ def->nhosts,
+ def->hosts,
+ def->nseclabels,
+ def->seclabels,
+ def->srcpool,
+ flags);
+}
+
+
static int
virDomainDiskDefFormat(virBufferPtr buf,
virDomainDiskDefPtr def,
--
1.8.4.3
5:15 a.m.
New subject: [libvirt] [PATCHv1.5 12/27] conf: Support disk source formatting without needing a virDomainDiskDefPtr
On 26.11.2013 17:48, Peter Krempa wrote:
The <source> element formatting function was expecting a
virDomainDiskDefPtr to store the data. As snapshots are not using this
data structure to hold the data, we need to add an internal function
which splits out individual fields separately.
---
src/conf/domain_conf.c | 129 ++++++++++++++++++++++++++++++-------------------
1 file changed, 78 insertions(+), 51 deletions(-)
ACK
Michal
7:57 a.m.
New subject: [libvirt] [PATCHv1.5 12/27] conf: Support disk source formatting without needing a virDomainDiskDefPtr
On 11/27/13 12:15, Michal Privoznik wrote:
On 26.11.2013 17:48, Peter Krempa wrote:
> The <source> element formatting function was expecting a
> virDomainDiskDefPtr to store the data. As snapshots are not using this
> data structure to hold the data, we need to add an internal function
> which splits out individual fields separately.
> ---
> src/conf/domain_conf.c | 129 ++++++++++++++++++++++++++++++-------------------
> 1 file changed, 78 insertions(+), 51 deletions(-)
ACK
Michal
Thanks; Pushed.
Peter
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 13/27] conf: Clean up virDomainDiskSourceDefFormatInternal
Avoid if statements when used with virBufferEscapeString which
automaticaly omits the whole string. Also add some line breaks to
visualy separate the code.
---
src/conf/domain_conf.c | 48 ++++++++++++++++++++----------------------------
1 file changed, 20 insertions(+), 28 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index afeadd7..05f8964 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14374,53 +14374,49 @@ virDomainDiskSourceDefFormatInternal(virBufferPtr buf,
switch (type) {
case VIR_DOMAIN_DISK_TYPE_FILE:
virBufferAddLit(buf, " <source");
- if (src)
- virBufferEscapeString(buf, " file='%s'", src);
- if (startupPolicy)
- virBufferEscapeString(buf, " startupPolicy='%s'",
- startupPolicy);
+ virBufferEscapeString(buf, " file='%s'", src);
+ virBufferEscapeString(buf, " startupPolicy='%s'",
startupPolicy);
+
if (nseclabels) {
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 8);
for (n = 0; n < nseclabels; n++)
- virSecurityDeviceLabelDefFormat(buf, seclabels[n],
- flags);
+ virSecurityDeviceLabelDefFormat(buf, seclabels[n], flags);
virBufferAdjustIndent(buf, -8);
virBufferAddLit(buf, " </source>\n");
} else {
virBufferAddLit(buf, "/>\n");
}
break;
+
case VIR_DOMAIN_DISK_TYPE_BLOCK:
virBufferAddLit(buf, " <source");
virBufferEscapeString(buf, " dev='%s'", src);
- if (startupPolicy)
- virBufferEscapeString(buf, " startupPolicy='%s'",
- startupPolicy);
+ virBufferEscapeString(buf, " startupPolicy='%s'",
startupPolicy);
+
if (nseclabels) {
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 8);
for (n = 0; n < nseclabels; n++)
- virSecurityDeviceLabelDefFormat(buf, seclabels[n],
- flags);
+ virSecurityDeviceLabelDefFormat(buf, seclabels[n], flags);
virBufferAdjustIndent(buf, -8);
virBufferAddLit(buf, " </source>\n");
} else {
virBufferAddLit(buf, "/>\n");
}
break;
+
case VIR_DOMAIN_DISK_TYPE_DIR:
- virBufferEscapeString(buf, " <source dir='%s'",
src);
- if (startupPolicy)
- virBufferEscapeString(buf, " startupPolicy='%s'",
- startupPolicy);
+ virBufferAddLit(buf, " <source");
+ virBufferEscapeString(buf, " dir='%s'", src);
+ virBufferEscapeString(buf, " startupPolicy='%s'",
startupPolicy);
virBufferAddLit(buf, "/>\n");
break;
+
case VIR_DOMAIN_DISK_TYPE_NETWORK:
virBufferAsprintf(buf, " <source protocol='%s'",
virDomainDiskProtocolTypeToString(protocol));
- if (src)
- virBufferEscapeString(buf, " name='%s'", src);
+ virBufferEscapeString(buf, " name='%s'", src);
if (nhosts == 0) {
virBufferAddLit(buf, "/>\n");
@@ -14428,25 +14424,21 @@ virDomainDiskSourceDefFormatInternal(virBufferPtr buf,
virBufferAddLit(buf, ">\n");
for (n = 0; n < nhosts; n++) {
virBufferAddLit(buf, " <host");
- if (hosts[n].name)
- virBufferEscapeString(buf, " name='%s'",
hosts[n].name);
-
- if (hosts[n].port)
- virBufferEscapeString(buf, " port='%s'",
- hosts[n].port);
+ virBufferEscapeString(buf, " name='%s'",
hosts[n].name);
+ virBufferEscapeString(buf, " port='%s'",
hosts[n].port);
if (hosts[n].transport)
virBufferAsprintf(buf, " transport='%s'",
virDomainDiskProtocolTransportTypeToString(hosts[n].transport));
- if (hosts[n].socket)
- virBufferEscapeString(buf, " socket='%s'",
hosts[n].socket);
+ virBufferEscapeString(buf, " socket='%s'",
hosts[n].socket);
virBufferAddLit(buf, "/>\n");
}
virBufferAddLit(buf, " </source>\n");
}
break;
+
case VIR_DOMAIN_DISK_TYPE_VOLUME:
virBufferAddLit(buf, " <source");
@@ -14457,8 +14449,7 @@ virDomainDiskSourceDefFormatInternal(virBufferPtr buf,
virBufferAsprintf(buf, " mode='%s'",
virDomainDiskSourcePoolModeTypeToString(srcpool->mode));
}
- if (startupPolicy)
- virBufferEscapeString(buf, " startupPolicy='%s'",
startupPolicy);
+ virBufferEscapeString(buf, " startupPolicy='%s'",
startupPolicy);
if (nseclabels) {
virBufferAddLit(buf, ">\n");
@@ -14472,6 +14463,7 @@ virDomainDiskSourceDefFormatInternal(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}
break;
+
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected disk type %s"),
--
1.8.4.3
5:15 a.m.
New subject: [libvirt] [PATCHv1.5 13/27] conf: Clean up virDomainDiskSourceDefFormatInternal
On 26.11.2013 17:48, Peter Krempa wrote:
Avoid if statements when used with virBufferEscapeString which
automaticaly omits the whole string. Also add some line breaks to
visualy separate the code.
---
src/conf/domain_conf.c | 48 ++++++++++++++++++++----------------------------
1 file changed, 20 insertions(+), 28 deletions(-)
ACK
Michal
7:57 a.m.
New subject: [libvirt] [PATCHv1.5 13/27] conf: Clean up virDomainDiskSourceDefFormatInternal
On 11/27/13 12:15, Michal Privoznik wrote:
On 26.11.2013 17:48, Peter Krempa wrote:
> Avoid if statements when used with virBufferEscapeString which
> automaticaly omits the whole string. Also add some line breaks to
> visualy separate the code.
> ---
> src/conf/domain_conf.c | 48 ++++++++++++++++++++----------------------------
> 1 file changed, 20 insertions(+), 28 deletions(-)
>
ACK
Michal
Thanks; Pushed.
Peter
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 14/27] conf: Split out seclabel formating code for disk source
The code is common for all the various disk types. Split it out to a
common function.
---
src/conf/domain_conf.c | 62 ++++++++++++++++++++++++--------------------------
1 file changed, 30 insertions(+), 32 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 05f8964..8f45f2e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14351,6 +14351,32 @@ virDomainDiskBlockIoDefFormat(virBufferPtr buf,
}
}
+
+/* virDomainDiskSourceDefFormatSeclabel:
+ *
+ * This function automaticaly closes the <source> element and formats any
+ * possible seclabels.
+ */
+static void
+virDomainDiskSourceDefFormatSeclabel(virBufferPtr buf,
+ size_t nseclabels,
+ virSecurityDeviceLabelDefPtr *seclabels,
+ unsigned int flags)
+{
+ size_t n;
+
+ if (nseclabels) {
+ virBufferAddLit(buf, ">\n");
+ virBufferAdjustIndent(buf, 8);
+ for (n = 0; n < nseclabels; n++)
+ virSecurityDeviceLabelDefFormat(buf, seclabels[n], flags);
+ virBufferAdjustIndent(buf, -8);
+ virBufferAddLit(buf, " </source>\n");
+ } else {
+ virBufferAddLit(buf, "/>\n");
+ }
+}
+
static int
virDomainDiskSourceDefFormatInternal(virBufferPtr buf,
int type,
@@ -14377,33 +14403,15 @@ virDomainDiskSourceDefFormatInternal(virBufferPtr buf,
virBufferEscapeString(buf, " file='%s'", src);
virBufferEscapeString(buf, " startupPolicy='%s'",
startupPolicy);
- if (nseclabels) {
- virBufferAddLit(buf, ">\n");
- virBufferAdjustIndent(buf, 8);
- for (n = 0; n < nseclabels; n++)
- virSecurityDeviceLabelDefFormat(buf, seclabels[n], flags);
- virBufferAdjustIndent(buf, -8);
- virBufferAddLit(buf, " </source>\n");
- } else {
- virBufferAddLit(buf, "/>\n");
- }
- break;
+ virDomainDiskSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags);
+ break;
case VIR_DOMAIN_DISK_TYPE_BLOCK:
virBufferAddLit(buf, " <source");
virBufferEscapeString(buf, " dev='%s'", src);
virBufferEscapeString(buf, " startupPolicy='%s'",
startupPolicy);
- if (nseclabels) {
- virBufferAddLit(buf, ">\n");
- virBufferAdjustIndent(buf, 8);
- for (n = 0; n < nseclabels; n++)
- virSecurityDeviceLabelDefFormat(buf, seclabels[n], flags);
- virBufferAdjustIndent(buf, -8);
- virBufferAddLit(buf, " </source>\n");
- } else {
- virBufferAddLit(buf, "/>\n");
- }
+ virDomainDiskSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags);
break;
case VIR_DOMAIN_DISK_TYPE_DIR:
@@ -14451,17 +14459,7 @@ virDomainDiskSourceDefFormatInternal(virBufferPtr buf,
}
virBufferEscapeString(buf, " startupPolicy='%s'",
startupPolicy);
- if (nseclabels) {
- virBufferAddLit(buf, ">\n");
- virBufferAdjustIndent(buf, 8);
- for (n = 0; n < nseclabels; n++)
- virSecurityDeviceLabelDefFormat(buf, seclabels[n],
- flags);
- virBufferAdjustIndent(buf, -8);
- virBufferAddLit(buf, " </source>\n");
- } else {
- virBufferAddLit(buf, "/>\n");
- }
+ virDomainDiskSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags);
break;
default:
--
1.8.4.3
5:15 a.m.
New subject: [libvirt] [PATCHv1.5 14/27] conf: Split out seclabel formating code for disk source
On 26.11.2013 17:48, Peter Krempa wrote:
The code is common for all the various disk types. Split it out to a
common function.
---
src/conf/domain_conf.c | 62 ++++++++++++++++++++++++--------------------------
1 file changed, 30 insertions(+), 32 deletions(-)
ACK
Michal
7:58 a.m.
New subject: [libvirt] [PATCHv1.5 14/27] conf: Split out seclabel formating code for disk source
On 11/27/13 12:15, Michal Privoznik wrote:
On 26.11.2013 17:48, Peter Krempa wrote:
> The code is common for all the various disk types. Split it out to a
> common function.
> ---
> src/conf/domain_conf.c | 62 ++++++++++++++++++++++++--------------------------
> 1 file changed, 30 insertions(+), 32 deletions(-)
>
ACK
Michal
Thanks; Pushed.
Peter
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 15/27] conf: Export disk source formatter and parser
This code will be reused in the snapshot disk definition parser.
---
src/conf/domain_conf.c | 4 ++--
src/conf/domain_conf.h | 20 ++++++++++++++++++++
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8f45f2e..0561d9d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4737,7 +4737,7 @@ cleanup:
}
-static int
+int
virDomainDiskSourceDefParse(xmlNodePtr node,
int type,
char **source,
@@ -14377,7 +14377,7 @@ virDomainDiskSourceDefFormatSeclabel(virBufferPtr buf,
}
}
-static int
+int
virDomainDiskSourceDefFormatInternal(virBufferPtr buf,
int type,
const char *src,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a5ef2ca..e9800a5 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2345,6 +2345,18 @@ int virDomainDefFormatInternal(virDomainDefPtr def,
unsigned int flags,
virBufferPtr buf);
+int virDomainDiskSourceDefFormatInternal(virBufferPtr buf,
+ int type,
+ const char *src,
+ int policy,
+ int protocol,
+ size_t nhosts,
+ virDomainDiskHostDefPtr hosts,
+ size_t nseclabels,
+ virSecurityDeviceLabelDefPtr *seclabels,
+ virDomainDiskSourcePoolDefPtr srcpool,
+ unsigned int flags);
+
int virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev);
@@ -2379,6 +2391,14 @@ virDomainDiskDefPtr
virDomainDiskRemove(virDomainDefPtr def, size_t i);
virDomainDiskDefPtr
virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
+int virDomainDiskSourceDefParse(xmlNodePtr node,
+ int type,
+ char **source,
+ int *proto,
+ size_t *nhosts,
+ virDomainDiskHostDefPtr *hosts,
+ virDomainDiskSourcePoolDefPtr *srcpool);
+
bool virDomainHasDiskMirror(virDomainObjPtr vm);
int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net);
--
1.8.4.3
5:15 a.m.
New subject: [libvirt] [PATCHv1.5 15/27] conf: Export disk source formatter and parser
On 26.11.2013 17:48, Peter Krempa wrote:
This code will be reused in the snapshot disk definition parser.
---
src/conf/domain_conf.c | 4 ++--
src/conf/domain_conf.h | 20 ++++++++++++++++++++
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8f45f2e..0561d9d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4737,7 +4737,7 @@ cleanup:
}
-static int
+int
virDomainDiskSourceDefParse(xmlNodePtr node,
int type,
char **source,
@@ -14377,7 +14377,7 @@ virDomainDiskSourceDefFormatSeclabel(virBufferPtr buf,
}
}
-static int
+int
virDomainDiskSourceDefFormatInternal(virBufferPtr buf,
int type,
const char *src,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a5ef2ca..e9800a5 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2345,6 +2345,18 @@ int virDomainDefFormatInternal(virDomainDefPtr def,
unsigned int flags,
virBufferPtr buf);
+int virDomainDiskSourceDefFormatInternal(virBufferPtr buf,
It seems rather uncommon to export Internal(). But let's see if you need
this or the virDomainDiskSourceDefFormat() wrapper (if you'd need the
wrapper you hadn't undergo the torture of splitting out the Internal(),
right?)
Okay, it's used in the next patch. I can live with Internal() exported.
+ int type,
+ const char *src,
+ int policy,
+ int protocol,
+ size_t nhosts,
+ virDomainDiskHostDefPtr hosts,
+ size_t nseclabels,
+ virSecurityDeviceLabelDefPtr *seclabels,
+ virDomainDiskSourcePoolDefPtr srcpool,
+ unsigned int flags);
+
int virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev);
@@ -2379,6 +2391,14 @@ virDomainDiskDefPtr
virDomainDiskRemove(virDomainDefPtr def, size_t i);
virDomainDiskDefPtr
virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
+int virDomainDiskSourceDefParse(xmlNodePtr node,
+ int type,
+ char **source,
+ int *proto,
+ size_t *nhosts,
+ virDomainDiskHostDefPtr *hosts,
+ virDomainDiskSourcePoolDefPtr *srcpool);
+
bool virDomainHasDiskMirror(virDomainObjPtr vm);
int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net);
ACK
Michal
7:58 a.m.
New subject: [libvirt] [PATCHv1.5 15/27] conf: Export disk source formatter and parser
On 11/27/13 12:15, Michal Privoznik wrote:
On 26.11.2013 17:48, Peter Krempa wrote:
> This code will be reused in the snapshot disk definition parser.
> ---
> src/conf/domain_conf.c | 4 ++--
> src/conf/domain_conf.h | 20 ++++++++++++++++++++
> 2 files changed, 22 insertions(+), 2 deletions(-)
...
ACK
Michal
Thanks; Pushed.
Peter
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 16/27] snapshot: conf: Use common parsing and formatting functions for source
Disk source elements for snapshots were using separate code from our
config parser. As snapshots can be stored on more than just regular
files, we will need the universal parser to allow us to expose a variety
of snapshot disk targets. This patch reuses the config parsers and
formatters to do the job.
This initial support only changes the code without any visible XML
change.
---
src/conf/snapshot_conf.c | 70 +++++++++++++++++++++++++++++++++---------------
src/conf/snapshot_conf.h | 1 +
2 files changed, 49 insertions(+), 22 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 94a74d2..7258daa 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -128,27 +128,42 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
}
}
- cur = node->children;
- while (cur) {
- if (cur->type == XML_ELEMENT_NODE) {
- if (!def->file &&
- xmlStrEqual(cur->name, BAD_CAST "source")) {
- def->file = virXMLPropString(cur, "file");
- } else if (!def->format &&
- xmlStrEqual(cur->name, BAD_CAST "driver")) {
- char *driver = virXMLPropString(cur, "type");
- def->format = virStorageFileFormatTypeFromString(driver);
- if (def->format <= 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unknown disk snapshot driver
'%s'"),
- driver);
- VIR_FREE(driver);
- goto cleanup;
- }
+ def->type = -1;
+
+ for (cur = node->children; cur; cur = cur->next) {
+ if (cur->type != XML_ELEMENT_NODE)
+ continue;
+
+ if (!def->file &&
+ xmlStrEqual(cur->name, BAD_CAST "source")) {
+
+ int backingtype = def->type;
+
+ if (backingtype < 0)
+ backingtype = VIR_DOMAIN_DISK_TYPE_FILE;
+
+ if (virDomainDiskSourceDefParse(cur,
+ backingtype,
+ &def->file,
+ NULL,
+ NULL,
+ NULL,
+ NULL) < 0)
+ goto cleanup;
+
+ } else if (!def->format &&
+ xmlStrEqual(cur->name, BAD_CAST "driver")) {
+ char *driver = virXMLPropString(cur, "type");
+ def->format = virStorageFileFormatTypeFromString(driver);
+ if (def->format <= 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unknown disk snapshot driver '%s'"),
+ driver);
VIR_FREE(driver);
+ goto cleanup;
}
+ VIR_FREE(driver);
}
- cur = cur->next;
}
if (!def->snapshot && (def->file || def->format))
@@ -577,6 +592,8 @@ static void
virDomainSnapshotDiskDefFormat(virBufferPtr buf,
virDomainSnapshotDiskDefPtr disk)
{
+ int type = disk->type;
+
if (!disk->name)
return;
@@ -584,6 +601,13 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
if (disk->snapshot > 0)
virBufferAsprintf(buf, " snapshot='%s'",
virDomainSnapshotLocationTypeToString(disk->snapshot));
+
+ if (type < 0)
+ type = VIR_DOMAIN_DISK_TYPE_FILE;
+ else
+ virBufferAsprintf(buf, " type='%s'",
+ virDomainDiskTypeToString(type));
+
if (!disk->file && disk->format == 0) {
virBufferAddLit(buf, "/>\n");
return;
@@ -591,12 +615,14 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
virBufferAddLit(buf, ">\n");
- virBufferAdjustIndent(buf, 6);
if (disk->format > 0)
- virBufferEscapeString(buf, "<driver type='%s'/>\n",
+ virBufferEscapeString(buf, " <driver
type='%s'/>\n",
virStorageFileFormatTypeToString(disk->format));
- virBufferEscapeString(buf, "<source file='%s'/>\n",
disk->file);
- virBufferAdjustIndent(buf, -6);
+ virDomainDiskSourceDefFormatInternal(buf,
+ type,
+ disk->file,
+ 0, 0, 0, NULL, 0, NULL, NULL, 0);
+
virBufferAddLit(buf, " </disk>\n");
}
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index ff3dca2..241d63c 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -51,6 +51,7 @@ struct _virDomainSnapshotDiskDef {
char *name; /* name matching the <target dev='...' of the domain */
int index; /* index within snapshot->dom->disks that matches name */
int snapshot; /* enum virDomainSnapshotLocation */
+ int type; /* enum virDomainDiskType */
char *file; /* new source file when snapshot is external */
int format; /* enum virStorageFileFormat */
};
--
1.8.4.3
5:15 a.m.
New subject: [libvirt] [PATCHv1.5 16/27] snapshot: conf: Use common parsing and formatting functions for source
On 26.11.2013 17:48, Peter Krempa wrote:
Disk source elements for snapshots were using separate code from our
config parser. As snapshots can be stored on more than just regular
files, we will need the universal parser to allow us to expose a variety
of snapshot disk targets. This patch reuses the config parsers and
formatters to do the job.
This initial support only changes the code without any visible XML
change.
---
src/conf/snapshot_conf.c | 70 +++++++++++++++++++++++++++++++++---------------
src/conf/snapshot_conf.h | 1 +
2 files changed, 49 insertions(+), 22 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 94a74d2..7258daa 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -128,27 +128,42 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
}
}
- cur = node->children;
- while (cur) {
- if (cur->type == XML_ELEMENT_NODE) {
- if (!def->file &&
- xmlStrEqual(cur->name, BAD_CAST "source")) {
- def->file = virXMLPropString(cur, "file");
- } else if (!def->format &&
- xmlStrEqual(cur->name, BAD_CAST "driver")) {
- char *driver = virXMLPropString(cur, "type");
- def->format = virStorageFileFormatTypeFromString(driver);
- if (def->format <= 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unknown disk snapshot driver
'%s'"),
- driver);
- VIR_FREE(driver);
- goto cleanup;
- }
+ def->type = -1;
+
+ for (cur = node->children; cur; cur = cur->next) {
+ if (cur->type != XML_ELEMENT_NODE)
+ continue;
+
+ if (!def->file &&
+ xmlStrEqual(cur->name, BAD_CAST "source")) {
+
+ int backingtype = def->type;
+
+ if (backingtype < 0)
+ backingtype = VIR_DOMAIN_DISK_TYPE_FILE;
So far, def->type is never parsed, so the @backingtype is always
VIR_DOMAIN_DISK_TYPE_FILE. Mmm, okay.
+
+ if (virDomainDiskSourceDefParse(cur,
+ backingtype,
+ &def->file,
+ NULL,
+ NULL,
+ NULL,
+ NULL) < 0)
+ goto cleanup;
+
+ } else if (!def->format &&
+ xmlStrEqual(cur->name, BAD_CAST "driver")) {
+ char *driver = virXMLPropString(cur, "type");
+ def->format = virStorageFileFormatTypeFromString(driver);
+ if (def->format <= 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unknown disk snapshot driver
'%s'"),
+ driver);
VIR_FREE(driver);
+ goto cleanup;
}
+ VIR_FREE(driver);
}
- cur = cur->next;
}
if (!def->snapshot && (def->file || def->format))
@@ -577,6 +592,8 @@ static void
virDomainSnapshotDiskDefFormat(virBufferPtr buf,
virDomainSnapshotDiskDefPtr disk)
{
+ int type = disk->type;
+
if (!disk->name)
return;
@@ -584,6 +601,13 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
if (disk->snapshot > 0)
virBufferAsprintf(buf, " snapshot='%s'",
virDomainSnapshotLocationTypeToString(disk->snapshot));
+
+ if (type < 0)
+ type = VIR_DOMAIN_DISK_TYPE_FILE;
+ else
+ virBufferAsprintf(buf, " type='%s'",
+ virDomainDiskTypeToString(type));
+
This is a very thin line between no XML change and outputting a new
'type' attribute into XML. Doesn't make much sense now, but I see this
is to be changed in 25/27. Anyway, I think it would be better to save
these small snippets up till then.
if (!disk->file && disk->format == 0) {
virBufferAddLit(buf, "/>\n");
return;
@@ -591,12 +615,14 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
virBufferAddLit(buf, ">\n");
- virBufferAdjustIndent(buf, 6);
if (disk->format > 0)
- virBufferEscapeString(buf, "<driver type='%s'/>\n",
+ virBufferEscapeString(buf, " <driver
type='%s'/>\n",
virStorageFileFormatTypeToString(disk->format));
- virBufferEscapeString(buf, "<source file='%s'/>\n",
disk->file);
- virBufferAdjustIndent(buf, -6);
+ virDomainDiskSourceDefFormatInternal(buf,
+ type,
+ disk->file,
+ 0, 0, 0, NULL, 0, NULL, NULL, 0);
+
virBufferAddLit(buf, " </disk>\n");
}
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index ff3dca2..241d63c 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -51,6 +51,7 @@ struct _virDomainSnapshotDiskDef {
char *name; /* name matching the <target dev='...' of the domain */
int index; /* index within snapshot->dom->disks that matches name */
int snapshot; /* enum virDomainSnapshotLocation */
+ int type; /* enum virDomainDiskType */
char *file; /* new source file when snapshot is external */
int format; /* enum virStorageFileFormat */
};
ACK to the rest.
Michal
8 a.m.
New subject: [libvirt] [PATCHv1.5 16/27] snapshot: conf: Use common parsing and formatting functions for source
On 11/27/13 12:15, Michal Privoznik wrote:
On 26.11.2013 17:48, Peter Krempa wrote:
> Disk source elements for snapshots were using separate code from our
> config parser. As snapshots can be stored on more than just regular
> files, we will need the universal parser to allow us to expose a variety
> of snapshot disk targets. This patch reuses the config parsers and
> formatters to do the job.
>
> This initial support only changes the code without any visible XML
> change.
> ---
> src/conf/snapshot_conf.c | 70 +++++++++++++++++++++++++++++++++---------------
> src/conf/snapshot_conf.h | 1 +
> 2 files changed, 49 insertions(+), 22 deletions(-)
>
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 94a74d2..7258daa 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
...
> @@ -584,6 +601,13 @@ virDomainSnapshotDiskDefFormat(virBufferPtr
buf,
> if (disk->snapshot > 0)
> virBufferAsprintf(buf, " snapshot='%s'",
>
virDomainSnapshotLocationTypeToString(disk->snapshot));
> +
> + if (type < 0)
> + type = VIR_DOMAIN_DISK_TYPE_FILE;
> + else
> + virBufferAsprintf(buf, " type='%s'",
> + virDomainDiskTypeToString(type));
> +
This is a very thin line between no XML change and outputting a new
'type' attribute into XML. Doesn't make much sense now, but I see this
is to be changed in 25/27. Anyway, I think it would be better to save
these small snippets up till then.
I got rid of the else section here until it will really be used.
> if (!disk->file && disk->format == 0) {
> virBufferAddLit(buf, "/>\n");
> return;
...
>
ACK to the rest.
Thanks; Pushed.
Peter
10:48 a.m.
New subject: [libvirt] [PATCHv1.5 17/27] snapshot: conf: Fix NULL dereference when <driver> element is empty
Consider the following valid snapshot XML as the <driver> element is
allowed to be empty in the domainsnapshot.rng schema:
$ cat snap.xml
<domainsnapshot>
<disks>
<disk name='vda' snapshot='external'>
<source file='/tmp/foo'/>
<driver/>
</disk>
</disks>
</domainsnapshot>
produces the following error:
$ virsh snapshot-create domain snap.xml
error: internal error: unknown disk snapshot driver '(null)'
The driver type is parsed as NULL from the XML as the attribute is not
present and then directly used to produce the error message.
With this patch the attempt to parse the driver type is skipped if not
present to avoid changing the schema to forbid the empty driver element.
---
src/conf/snapshot_conf.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 7258daa..5958f13 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -154,15 +154,17 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
} else if (!def->format &&
xmlStrEqual(cur->name, BAD_CAST "driver")) {
char *driver = virXMLPropString(cur, "type");
- def->format = virStorageFileFormatTypeFromString(driver);
- if (def->format <= 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unknown disk snapshot driver '%s'"),
- driver);
+ if (driver) {
+ def->format = virStorageFileFormatTypeFromString(driver);
+ if (def->format <= 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unknown disk snapshot driver
'%s'"),
+ driver);
+ VIR_FREE(driver);
+ goto cleanup;
+ }
VIR_FREE(driver);
- goto cleanup;
}
- VIR_FREE(driver);
}
}
--
1.8.4.3
5:15 a.m.
New subject: [libvirt] [PATCHv1.5 17/27] snapshot: conf: Fix NULL dereference when <driver> element is empty
On 26.11.2013 17:48, Peter Krempa wrote:
Consider the following valid snapshot XML as the <driver>
element is
allowed to be empty in the domainsnapshot.rng schema:
$ cat snap.xml
<domainsnapshot>
<disks>
<disk name='vda' snapshot='external'>
<source file='/tmp/foo'/>
<driver/>
</disk>
</disks>
</domainsnapshot>
produces the following error:
$ virsh snapshot-create domain snap.xml
error: internal error: unknown disk snapshot driver '(null)'
The driver type is parsed as NULL from the XML as the attribute is not
present and then directly used to produce the error message.
With this patch the attempt to parse the driver type is skipped if not
present to avoid changing the schema to forbid the empty driver element.
---
src/conf/snapshot_conf.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 7258daa..5958f13 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -154,15 +154,17 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
} else if (!def->format &&
xmlStrEqual(cur->name, BAD_CAST "driver")) {
char *driver = virXMLPropString(cur, "type");
- def->format = virStorageFileFormatTypeFromString(driver);
- if (def->format <= 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unknown disk snapshot driver
'%s'"),
- driver);
+ if (driver) {
+ def->format = virStorageFileFormatTypeFromString(driver);
+ if (def->format <= 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unknown disk snapshot driver
'%s'"),
+ driver);
+ VIR_FREE(driver);
+ goto cleanup;
+ }
VIR_FREE(driver);
- goto cleanup;
}
- VIR_FREE(driver);
}
}
ACK and worth backporting on *-maint branches.
Michal
8:01 a.m.
New subject: [libvirt] [PATCHv1.5 17/27] snapshot: conf: Fix NULL dereference when <driver> element is empty
On 11/27/13 12:15, Michal Privoznik wrote:
On 26.11.2013 17:48, Peter Krempa wrote:
> Consider the following valid snapshot XML as the <driver> element is
> allowed to be empty in the domainsnapshot.rng schema:
>
> $ cat snap.xml
> <domainsnapshot>
> <disks>
> <disk name='vda' snapshot='external'>
> <source file='/tmp/foo'/>
> <driver/>
> </disk>
> </disks>
> </domainsnapshot>
>
> produces the following error:
>
> $ virsh snapshot-create domain snap.xml
> error: internal error: unknown disk snapshot driver '(null)'
>
> The driver type is parsed as NULL from the XML as the attribute is not
> present and then directly used to produce the error message.
>
> With this patch the attempt to parse the driver type is skipped if not
> present to avoid changing the schema to forbid the empty driver element.
> ---
> src/conf/snapshot_conf.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
...
>
ACK and worth backporting on *-maint branches.
Thanks; Pushed. The backport won't be clean unless a few other patches
are backported, thus I will not do it until somebody will complain.
Peter
4:59 p.m.
New subject: [libvirt] [PATCHv1.5 17/27] snapshot: conf: Fix NULL dereference when <driver> element is empty
On 11/27/2013 04:15 AM, Michal Privoznik wrote:
On 26.11.2013 17:48, Peter Krempa wrote:
> Consider the following valid snapshot XML as the <driver> element is
> allowed to be empty in the domainsnapshot.rng schema:
>
> $ cat snap.xml
> <domainsnapshot>
> <disks>
> <disk name='vda' snapshot='external'>
> <source file='/tmp/foo'/>
> <driver/>
> </disk>
> </disks>
> </domainsnapshot>
>
> produces the following error:
>
> $ virsh snapshot-create domain snap.xml
> error: internal error: unknown disk snapshot driver '(null)'
>
> The driver type is parsed as NULL from the XML as the attribute is not
> present and then directly used to produce the error message.
>
> With this patch the attempt to parse the driver type is skipped if not
> present to avoid changing the schema to forbid the empty driver element.
> ---
> src/conf/snapshot_conf.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
Shouldn't we add this example somewhere in the tests/ subdirectory to
ensure we don't regress?
ACK and worth backporting on *-maint branches.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:10 p.m.
New subject: [libvirt] [PATCHv1.5 17/27] snapshot: conf: Fix NULL dereference when <driver> element is empty
On 12/02/13 23:59, Eric Blake wrote:
On 11/27/2013 04:15 AM, Michal Privoznik wrote:
> On 26.11.2013 17:48, Peter Krempa wrote:
>> Consider the following valid snapshot XML as the <driver> element is
>> allowed to be empty in the domainsnapshot.rng schema:
>>
>> $ cat snap.xml
>> <domainsnapshot>
>> <disks>
>> <disk name='vda' snapshot='external'>
>> <source file='/tmp/foo'/>
>> <driver/>
>> </disk>
>> </disks>
>> </domainsnapshot>
>>
>> produces the following error:
>>
>> $ virsh snapshot-create domain snap.xml
>> error: internal error: unknown disk snapshot driver '(null)'
>>
>> The driver type is parsed as NULL from the XML as the attribute is not
>> present and then directly used to produce the error message.
>>
>> With this patch the attempt to parse the driver type is skipped if not
>> present to avoid changing the schema to forbid the empty driver element.
>> ---
>> src/conf/snapshot_conf.c | 16 +++++++++-------
>> 1 file changed, 9 insertions(+), 7 deletions(-)
Shouldn't we add this example somewhere in the tests/ subdirectory to
ensure we don't regress?
Hmm, yes, I'll send a patch.
Peter
10:49 a.m.
New subject: [libvirt] [PATCHv1.5 18/27] conf: Add functions to copy and free network disk source definitions
To simplify operations on virDomainDiskHostDef arrays we will need deep
copy and freeing functions. Add and properly export them.
---
src/conf/domain_conf.c | 55 +++++++++++++++++++++++++++++++++++++++++++++---
src/conf/domain_conf.h | 3 +++
src/libvirt_private.syms | 2 ++
3 files changed, 57 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0561d9d..e0d6903 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1216,9 +1216,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
VIR_FREE(def->seclabels);
}
- for (i = 0; i < def->nhosts; i++)
- virDomainDiskHostDefClear(&def->hosts[i]);
- VIR_FREE(def->hosts);
+ virDomainDiskHostDefFree(def->nhosts, def->hosts);
VIR_FREE(def);
}
@@ -1233,6 +1231,57 @@ void virDomainDiskHostDefClear(virDomainDiskHostDefPtr def)
VIR_FREE(def->socket);
}
+
+void
+virDomainDiskHostDefFree(size_t nhosts,
+ virDomainDiskHostDefPtr hosts)
+{
+ size_t i;
+
+ if (!hosts)
+ return;
+
+ for (i = 0; i < nhosts; i++)
+ virDomainDiskHostDefClear(&hosts[i]);
+
+ VIR_FREE(hosts);
+}
+
+
+virDomainDiskHostDefPtr
+virDomainDiskHostDefCopy(size_t nhosts,
+ virDomainDiskHostDefPtr hosts)
+{
+ virDomainDiskHostDefPtr ret = NULL;
+ size_t i;
+
+ if (VIR_ALLOC_N(ret, nhosts) < 0)
+ goto error;
+
+ for (i = 0; i < nhosts; i++) {
+ virDomainDiskHostDefPtr src = &hosts[i];
+ virDomainDiskHostDefPtr dst = &ret[i];
+
+ dst->transport = src->transport;
+
+ if (VIR_STRDUP(dst->name, src->name) < 0)
+ goto error;
+
+ if (VIR_STRDUP(dst->port, src->port) < 0)
+ goto error;
+
+ if (VIR_STRDUP(dst->socket, src->socket) < 0)
+ goto error;
+ }
+
+ return ret;
+
+error:
+ virDomainDiskHostDefFree(nhosts, ret);
+ return NULL;
+}
+
+
void virDomainControllerDefFree(virDomainControllerDefPtr def)
{
if (!def)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e9800a5..ee018f0 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2211,6 +2211,9 @@ void virDomainInputDefFree(virDomainInputDefPtr def);
void virDomainDiskDefFree(virDomainDiskDefPtr def);
void virDomainLeaseDefFree(virDomainLeaseDefPtr def);
void virDomainDiskHostDefClear(virDomainDiskHostDefPtr def);
+void virDomainDiskHostDefFree(size_t nhosts, virDomainDiskHostDefPtr hosts);
+virDomainDiskHostDefPtr virDomainDiskHostDefCopy(size_t nhosts,
+ virDomainDiskHostDefPtr hosts);
int virDomainDeviceFindControllerModel(virDomainDefPtr def,
virDomainDeviceInfoPtr info,
int controllerType);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index aeb3568..f952a12 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -198,6 +198,8 @@ virDomainDiskFindByBusAndDst;
virDomainDiskGeometryTransTypeFromString;
virDomainDiskGeometryTransTypeToString;
virDomainDiskHostDefClear;
+virDomainDiskHostDefCopy;
+virDomainDiskHostDefFree;
virDomainDiskIndexByName;
virDomainDiskInsert;
virDomainDiskInsertPreAlloced;
--
1.8.4.3
5:15 a.m.
New subject: [libvirt] [PATCHv1.5 18/27] conf: Add functions to copy and free network disk source definitions
On 26.11.2013 17:49, Peter Krempa wrote:
To simplify operations on virDomainDiskHostDef arrays we will need
deep
copy and freeing functions. Add and properly export them.
---
src/conf/domain_conf.c | 55 +++++++++++++++++++++++++++++++++++++++++++++---
src/conf/domain_conf.h | 3 +++
src/libvirt_private.syms | 2 ++
3 files changed, 57 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0561d9d..e0d6903 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1216,9 +1216,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
VIR_FREE(def->seclabels);
}
- for (i = 0; i < def->nhosts; i++)
- virDomainDiskHostDefClear(&def->hosts[i]);
- VIR_FREE(def->hosts);
+ virDomainDiskHostDefFree(def->nhosts, def->hosts);
VIR_FREE(def);
}
@@ -1233,6 +1231,57 @@ void virDomainDiskHostDefClear(virDomainDiskHostDefPtr def)
VIR_FREE(def->socket);
}
+
+void
+virDomainDiskHostDefFree(size_t nhosts,
+ virDomainDiskHostDefPtr hosts)
+{
+ size_t i;
+
+ if (!hosts)
+ return;
+
+ for (i = 0; i < nhosts; i++)
+ virDomainDiskHostDefClear(&hosts[i]);
+
+ VIR_FREE(hosts);
+}
+
+
+virDomainDiskHostDefPtr
+virDomainDiskHostDefCopy(size_t nhosts,
+ virDomainDiskHostDefPtr hosts)
+{
+ virDomainDiskHostDefPtr ret = NULL;
+ size_t i;
+
+ if (VIR_ALLOC_N(ret, nhosts) < 0)
+ goto error;
+
+ for (i = 0; i < nhosts; i++) {
+ virDomainDiskHostDefPtr src = &hosts[i];
+ virDomainDiskHostDefPtr dst = &ret[i];
+
+ dst->transport = src->transport;
+
+ if (VIR_STRDUP(dst->name, src->name) < 0)
+ goto error;
+
+ if (VIR_STRDUP(dst->port, src->port) < 0)
+ goto error;
+
+ if (VIR_STRDUP(dst->socket, src->socket) < 0)
+ goto error;
Could have been joined into one 'if' with or-ed VIR_STRDUP(). But this
is just fine too.
+ }
+
+ return ret;
+
+error:
+ virDomainDiskHostDefFree(nhosts, ret);
+ return NULL;
+}
+
+
void virDomainControllerDefFree(virDomainControllerDefPtr def)
{
if (!def)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e9800a5..ee018f0 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2211,6 +2211,9 @@ void virDomainInputDefFree(virDomainInputDefPtr def);
void virDomainDiskDefFree(virDomainDiskDefPtr def);
void virDomainLeaseDefFree(virDomainLeaseDefPtr def);
void virDomainDiskHostDefClear(virDomainDiskHostDefPtr def);
+void virDomainDiskHostDefFree(size_t nhosts, virDomainDiskHostDefPtr hosts);
+virDomainDiskHostDefPtr virDomainDiskHostDefCopy(size_t nhosts,
+ virDomainDiskHostDefPtr hosts);
int virDomainDeviceFindControllerModel(virDomainDefPtr def,
virDomainDeviceInfoPtr info,
int controllerType);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index aeb3568..f952a12 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -198,6 +198,8 @@ virDomainDiskFindByBusAndDst;
virDomainDiskGeometryTransTypeFromString;
virDomainDiskGeometryTransTypeToString;
virDomainDiskHostDefClear;
+virDomainDiskHostDefCopy;
+virDomainDiskHostDefFree;
virDomainDiskIndexByName;
virDomainDiskInsert;
virDomainDiskInsertPreAlloced;
ACK
Michal
8:03 a.m.
New subject: [libvirt] [PATCHv1.5 18/27] conf: Add functions to copy and free network disk source definitions
On 11/27/13 12:15, Michal Privoznik wrote:
On 26.11.2013 17:49, Peter Krempa wrote:
> To simplify operations on virDomainDiskHostDef arrays we will need deep
> copy and freeing functions. Add and properly export them.
> ---
> src/conf/domain_conf.c | 55 +++++++++++++++++++++++++++++++++++++++++++++---
> src/conf/domain_conf.h | 3 +++
> src/libvirt_private.syms | 2 ++
> 3 files changed, 57 insertions(+), 3 deletions(-)
>
>
...
ACK
Thanks; Pushed.
Peter
5:01 p.m.
New subject: [libvirt] [PATCHv1.5 18/27] conf: Add functions to copy and free network disk source definitions
On 11/27/2013 04:15 AM, Michal Privoznik wrote:
On 26.11.2013 17:49, Peter Krempa wrote:
> To simplify operations on virDomainDiskHostDef arrays we will need deep
> copy and freeing functions. Add and properly export them.
> ---
> + if (VIR_STRDUP(dst->port, src->port) < 0)
> + goto error;
> +
> + if (VIR_STRDUP(dst->socket, src->socket) < 0)
> + goto error;
Could have been joined into one 'if' with or-ed VIR_STRDUP(). But this
is just fine too.
Dan has been (successfully) convincing me that separate 'if' is better
for -O2 debugging, because then gdb is better able to show you which
line of code failed.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:46 a.m.
New subject: [libvirt] [PATCHv1.5 18/27] conf: Add functions to copy and free network disk source definitions
On 12/03/13 00:01, Eric Blake wrote:
On 11/27/2013 04:15 AM, Michal Privoznik wrote:
> On 26.11.2013 17:49, Peter Krempa wrote:
>> To simplify operations on virDomainDiskHostDef arrays we will need deep
>> copy and freeing functions. Add and properly export them.
>> ---
>> + if (VIR_STRDUP(dst->port, src->port) < 0)
>> + goto error;
>> +
>> + if (VIR_STRDUP(dst->socket, src->socket) < 0)
>> + goto error;
>
> Could have been joined into one 'if' with or-ed VIR_STRDUP(). But this
> is just fine too.
Dan has been (successfully) convincing me that separate 'if' is better
for -O2 debugging, because then gdb is better able to show you which
line of code failed.
Yep, I've read the conversation and kept this separate exactly for that
reason :)
Peter
10:49 a.m.
New subject: [libvirt] [PATCHv1.5 19/27] qemu: snapshot: Detect internal snapshots also for sheepdog and RBD
When doing an internal snapshot on a VM with sheepdog or RBD disks we
would not set a flag to mark the domain is using internal snapshots and
might end up creating a mixed snapshot. Move the setting of the variable
to avoid this problem.
---
src/qemu/qemu_driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8a1eefd..d2dbaf5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11764,6 +11764,8 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
virDomainSnapshotDefPtr def,
switch (disk->snapshot) {
case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL:
+ found_internal = true;
+
if (def->state != VIR_DOMAIN_DISK_SNAPSHOT &&
dom_disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
(dom_disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG ||
@@ -11787,7 +11789,6 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
virDomainSnapshotDefPtr def,
disk->name);
goto cleanup;
}
- found_internal = true;
break;
case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL:
--
1.8.4.3
5:15 a.m.
New subject: [libvirt] [PATCHv1.5 19/27] qemu: snapshot: Detect internal snapshots also for sheepdog and RBD
On 26.11.2013 17:49, Peter Krempa wrote:
When doing an internal snapshot on a VM with sheepdog or RBD disks
we
would not set a flag to mark the domain is using internal snapshots and
might end up creating a mixed snapshot. Move the setting of the variable
to avoid this problem.
---
src/qemu/qemu_driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8a1eefd..d2dbaf5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11764,6 +11764,8 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
virDomainSnapshotDefPtr def,
switch (disk->snapshot) {
case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL:
+ found_internal = true;
+
if (def->state != VIR_DOMAIN_DISK_SNAPSHOT &&
dom_disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
(dom_disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG ||
@@ -11787,7 +11789,6 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
virDomainSnapshotDefPtr def,
disk->name);
goto cleanup;
}
- found_internal = true;
break;
case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL:
ACK, and again wort backporting.
Michal
10:49 a.m.
New subject: [libvirt] [PATCHv1.5 20/27] conf: Add helper do clear disk source authentication struct
Add virDomainDiskAuthClear to help cleaning out the struct in other
places too.
---
src/conf/domain_conf.c | 17 ++++++++++++++---
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e0d6903..454b21c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1201,12 +1201,9 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
VIR_FREE(def->driverName);
virStorageFileFreeMetadata(def->backingChain);
VIR_FREE(def->mirror);
- VIR_FREE(def->auth.username);
VIR_FREE(def->wwn);
VIR_FREE(def->vendor);
VIR_FREE(def->product);
- if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE)
- VIR_FREE(def->auth.secret.usage);
virStorageEncryptionFree(def->encryption);
virDomainDeviceInfoClear(&def->info);
@@ -1217,10 +1214,24 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
}
virDomainDiskHostDefFree(def->nhosts, def->hosts);
+ virDomainDiskAuthClear(def);
VIR_FREE(def);
}
+
+void
+virDomainDiskAuthClear(virDomainDiskDefPtr def)
+{
+ VIR_FREE(def->auth.username);
+
+ if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE)
+ VIR_FREE(def->auth.secret.usage);
+
+ def->auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_NONE;
+}
+
+
void virDomainDiskHostDefClear(virDomainDiskHostDefPtr def)
{
if (!def)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ee018f0..4934911 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2210,6 +2210,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def);
void virDomainInputDefFree(virDomainInputDefPtr def);
void virDomainDiskDefFree(virDomainDiskDefPtr def);
void virDomainLeaseDefFree(virDomainLeaseDefPtr def);
+void virDomainDiskAuthClear(virDomainDiskDefPtr def);
void virDomainDiskHostDefClear(virDomainDiskHostDefPtr def);
void virDomainDiskHostDefFree(size_t nhosts, virDomainDiskHostDefPtr hosts);
virDomainDiskHostDefPtr virDomainDiskHostDefCopy(size_t nhosts,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f952a12..f8e774c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -180,6 +180,7 @@ virDomainDeviceFindControllerModel;
virDomainDeviceInfoCopy;
virDomainDeviceInfoIterate;
virDomainDeviceTypeToString;
+virDomainDiskAuthClear;
virDomainDiskBusTypeToString;
virDomainDiskCacheTypeFromString;
virDomainDiskCacheTypeToString;
--
1.8.4.3
5:15 a.m.
New subject: [libvirt] [PATCHv1.5 20/27] conf: Add helper do clear disk source authentication struct
On 26.11.2013 17:49, Peter Krempa wrote:
Add virDomainDiskAuthClear to help cleaning out the struct in other
places too.
---
src/conf/domain_conf.c | 17 ++++++++++++++---
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
3 files changed, 16 insertions(+), 3 deletions(-)
ACK
Michal
8:03 a.m.
New subject: [libvirt] [PATCHv1.5 20/27] conf: Add helper do clear disk source authentication struct
On 11/27/13 12:15, Michal Privoznik wrote:
On 26.11.2013 17:49, Peter Krempa wrote:
> Add virDomainDiskAuthClear to help cleaning out the struct in other
> places too.
> ---
> src/conf/domain_conf.c | 17 ++++++++++++++---
> src/conf/domain_conf.h | 1 +
> src/libvirt_private.syms | 1 +
> 3 files changed, 16 insertions(+), 3 deletions(-)
ACK
Thanks; Pushed.
Peter
10:49 a.m.
New subject: [libvirt] [PATCHv1.5 21/27] qemu: Clear old translated pool source
Clear the old data to avoid leaking it when attempting to re-translate a
pool on the same domain object.
---
src/qemu/qemu_conf.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 58a0500..639e2ff 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1360,6 +1360,10 @@ qemuTranslateDiskSourcePool(virConnectPtr conn,
goto cleanup;
}
+ VIR_FREE(def->src);
+ virDomainDiskHostDefFree(def->nhosts, def->hosts);
+ virDomainDiskAuthClear(def);
+
switch ((enum virStoragePoolType) pooldef->type) {
case VIR_STORAGE_POOL_DIR:
case VIR_STORAGE_POOL_FS:
--
1.8.4.3
5:15 a.m.
New subject: [libvirt] [PATCHv1.5 21/27] qemu: Clear old translated pool source
On 26.11.2013 17:49, Peter Krempa wrote:
Clear the old data to avoid leaking it when attempting to
re-translate a
pool on the same domain object.
---
src/qemu/qemu_conf.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 58a0500..639e2ff 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1360,6 +1360,10 @@ qemuTranslateDiskSourcePool(virConnectPtr conn,
goto cleanup;
}
+ VIR_FREE(def->src);
+ virDomainDiskHostDefFree(def->nhosts, def->hosts);
+ virDomainDiskAuthClear(def);
+
switch ((enum virStoragePoolType) pooldef->type) {
case VIR_STORAGE_POOL_DIR:
case VIR_STORAGE_POOL_FS:
ACK
Michal
10:49 a.m.
New subject: [libvirt] [PATCHv1.5 22/27] qemu: snapshot: Touch up error message
---
src/qemu/qemu_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d2dbaf5..96bc87e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11858,8 +11858,8 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
virDomainSnapshotDefPtr def,
* offline snapshots */
if (found_internal && external) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("mixing internal and external snapshots is not "
- "supported yet"));
+ _("mixing internal and external targets for a snapshot
"
+ "is not yet supported"));
goto cleanup;
}
--
1.8.4.3
5:14 a.m.
New subject: [libvirt] [PATCHv1.5 22/27] qemu: snapshot: Touch up error message
On 26.11.2013 17:49, Peter Krempa wrote:
---
src/qemu/qemu_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d2dbaf5..96bc87e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11858,8 +11858,8 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
virDomainSnapshotDefPtr def,
* offline snapshots */
if (found_internal && external) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("mixing internal and external snapshots is not "
- "supported yet"));
+ _("mixing internal and external targets for a snapshot
"
+ "is not yet supported"));
goto cleanup;
}
I'm not a native speaker, but I remember being told in school that 'yet'
goes always at the end. So I'll leave this one for somebody else.
Michal
8:30 a.m.
New subject: [libvirt] [PATCHv1.5 22/27] qemu: snapshot: Touch up error message
On 11/27/2013 01:14 PM, Michal Privoznik wrote:
On 26.11.2013 17:49, Peter Krempa wrote:
> ---
> src/qemu/qemu_driver.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d2dbaf5..96bc87e 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11858,8 +11858,8 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
virDomainSnapshotDefPtr def,
> * offline snapshots */
> if (found_internal && external) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("mixing internal and external snapshots is not "
> - "supported yet"));
> + _("mixing internal and external targets for a snapshot
"
> + "is not yet supported"));
> goto cleanup;
> }
>
I'm not a native speaker, but I remember being told in school that 'yet'
goes always at the end. So I'll leave this one for somebody else.
No, that usage is fine.
8:04 a.m.
New subject: [libvirt] [PATCHv1.5 22/27] qemu: snapshot: Touch up error message
On 11/29/13 15:30, Laine Stump wrote:
On 11/27/2013 01:14 PM, Michal Privoznik wrote:
> On 26.11.2013 17:49, Peter Krempa wrote:
>> ---
>> src/qemu/qemu_driver.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index d2dbaf5..96bc87e 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -11858,8 +11858,8 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
virDomainSnapshotDefPtr def,
>> * offline snapshots */
>> if (found_internal && external) {
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> - _("mixing internal and external snapshots is not
"
>> - "supported yet"));
>> + _("mixing internal and external targets for a
snapshot "
>> + "is not yet supported"));
>> goto cleanup;
>> }
>>
> I'm not a native speaker, but I remember being told in school that 'yet'
> goes always at the end. So I'll leave this one for somebody else.
No, that usage is fine.
Thanks; Pushed.
Peter
10:49 a.m.
New subject: [libvirt] [PATCHv1.5 23/27] qemu: snapshot: Add functions similar to disk source pool translation
To avoid future pain, add placeholder functions to get the actual
snapshot disk type.
---
src/qemu/qemu_conf.c | 23 +++++++++++++++++++++++
src/qemu/qemu_conf.h | 6 ++++++
2 files changed, 29 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 639e2ff..210608e 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1453,3 +1453,26 @@ cleanup:
virStoragePoolDefFree(pooldef);
return ret;
}
+
+
+int
+qemuSnapshotDiskGetActualType(virDomainSnapshotDiskDefPtr def)
+{
+ if (def->type == -1)
+ return VIR_DOMAIN_DISK_TYPE_FILE;
+
+ return def->type;
+}
+
+
+int
+qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virDomainSnapshotDiskDefPtr def)
+{
+ if (def->type != VIR_DOMAIN_DISK_TYPE_VOLUME)
+ return 0;
+
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Snapshots are not yet supported with 'pool'
volumes"));
+ return -1;
+}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index b9786b1..0cb7901 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -29,6 +29,7 @@
# include "capabilities.h"
# include "network_conf.h"
# include "domain_conf.h"
+# include "snapshot_conf.h"
# include "domain_event.h"
# include "virthread.h"
# include "security/security_manager.h"
@@ -309,4 +310,9 @@ int qemuDiskGetActualType(virDomainDiskDefPtr def);
int qemuTranslateDiskSourcePool(virConnectPtr conn,
virDomainDiskDefPtr def);
+int qemuSnapshotDiskGetActualType(virDomainSnapshotDiskDefPtr def);
+
+int qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn,
+ virDomainSnapshotDiskDefPtr def);
+
#endif /* __QEMUD_CONF_H */
--
1.8.4.3
5:14 a.m.
New subject: [libvirt] [PATCHv1.5 23/27] qemu: snapshot: Add functions similar to disk source pool translation
On 26.11.2013 17:49, Peter Krempa wrote:
To avoid future pain, add placeholder functions to get the actual
snapshot disk type.
---
src/qemu/qemu_conf.c | 23 +++++++++++++++++++++++
src/qemu/qemu_conf.h | 6 ++++++
2 files changed, 29 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 639e2ff..210608e 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1453,3 +1453,26 @@ cleanup:
virStoragePoolDefFree(pooldef);
return ret;
}
+
+
+int
+qemuSnapshotDiskGetActualType(virDomainSnapshotDiskDefPtr def)
+{
+ if (def->type == -1)
+ return VIR_DOMAIN_DISK_TYPE_FILE;
+
+ return def->type;
+}
+
+
+int
+qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virDomainSnapshotDiskDefPtr def)
+{
+ if (def->type != VIR_DOMAIN_DISK_TYPE_VOLUME)
+ return 0;
+
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Snapshots are not yet supported with 'pool'
volumes"));
+ return -1;
+}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index b9786b1..0cb7901 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -29,6 +29,7 @@
# include "capabilities.h"
# include "network_conf.h"
# include "domain_conf.h"
+# include "snapshot_conf.h"
# include "domain_event.h"
# include "virthread.h"
# include "security/security_manager.h"
@@ -309,4 +310,9 @@ int qemuDiskGetActualType(virDomainDiskDefPtr def);
int qemuTranslateDiskSourcePool(virConnectPtr conn,
virDomainDiskDefPtr def);
+int qemuSnapshotDiskGetActualType(virDomainSnapshotDiskDefPtr def);
+
+int qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn,
+ virDomainSnapshotDiskDefPtr def);
+
#endif /* __QEMUD_CONF_H */
ACK
Michal
8:05 a.m.
New subject: [libvirt] [PATCHv1.5 23/27] qemu: snapshot: Add functions similar to disk source pool translation
On 11/27/13 12:14, Michal Privoznik wrote:
On 26.11.2013 17:49, Peter Krempa wrote:
> To avoid future pain, add placeholder functions to get the actual
> snapshot disk type.
> ---
> src/qemu/qemu_conf.c | 23 +++++++++++++++++++++++
> src/qemu/qemu_conf.h | 6 ++++++
> 2 files changed, 29 insertions(+)
>
...
ACK
Thanks; Pushed.
Peter
10:49 a.m.
New subject: [libvirt] [PATCHv1.5 24/27] qemu: snapshots: Declare supported and unsupported snapshot configs
Currently the snapshot code did not check if it actually supports
snapshots on various disk backends for domains. To avoid future problems
add checkers that whitelist the supported configurations.
---
src/qemu/qemu_driver.c | 264 +++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 234 insertions(+), 30 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 96bc87e..b9c270b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11738,13 +11738,226 @@ endjob:
}
static int
-qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def,
+qemuDomainSnapshotPrepareDiskExternalBacking(virDomainDiskDefPtr disk)
+{
+ int actualType = qemuDiskGetActualType(disk);
+
+ switch ((enum virDomainDiskType) actualType) {
+ case VIR_DOMAIN_DISK_TYPE_BLOCK:
+ case VIR_DOMAIN_DISK_TYPE_FILE:
+ return 0;
+
+ case VIR_DOMAIN_DISK_TYPE_NETWORK:
+ switch ((enum virDomainDiskProtocol) disk->protocol) {
+ case VIR_DOMAIN_DISK_PROTOCOL_NBD:
+ case VIR_DOMAIN_DISK_PROTOCOL_RBD:
+ case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
+ case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
+ case VIR_DOMAIN_DISK_PROTOCOL_ISCSI:
+ case VIR_DOMAIN_DISK_PROTOCOL_HTTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_HTTPS:
+ case VIR_DOMAIN_DISK_PROTOCOL_FTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_FTPS:
+ case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_LAST:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("external inactive snapshots are not supported on
"
+ "'network' disks using '%s'
protocol"),
+ virDomainDiskProtocolTypeToString(disk->protocol));
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_DISK_TYPE_DIR:
+ case VIR_DOMAIN_DISK_TYPE_VOLUME:
+ case VIR_DOMAIN_DISK_TYPE_LAST:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("external inactive snapshots are not supported on "
+ "'%s' disks"),
virDomainDiskTypeToString(actualType));
+ return -1;
+ }
+
+ return 0;
+}
+
+
+static int
+qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr disk)
+{
+ int actualType = qemuSnapshotDiskGetActualType(disk);
+
+ switch ((enum virDomainDiskType) actualType) {
+ case VIR_DOMAIN_DISK_TYPE_BLOCK:
+ case VIR_DOMAIN_DISK_TYPE_FILE:
+ return 0;
+
+ case VIR_DOMAIN_DISK_TYPE_NETWORK:
+ case VIR_DOMAIN_DISK_TYPE_DIR:
+ case VIR_DOMAIN_DISK_TYPE_VOLUME:
+ case VIR_DOMAIN_DISK_TYPE_LAST:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("external active snapshots are not supported on "
+ "'%s' disks"),
virDomainDiskTypeToString(actualType));
+ return -1;
+ }
+
+ return 0;
+}
+
+
+static int
+qemuDomainSnapshotPrepareDiskExternalOverlayInactive(virDomainSnapshotDiskDefPtr disk)
+{
+ int actualType = qemuSnapshotDiskGetActualType(disk);
+
+ switch ((enum virDomainDiskType) actualType) {
+ case VIR_DOMAIN_DISK_TYPE_BLOCK:
+ case VIR_DOMAIN_DISK_TYPE_FILE:
+ return 0;
+
+ case VIR_DOMAIN_DISK_TYPE_NETWORK:
+ case VIR_DOMAIN_DISK_TYPE_DIR:
+ case VIR_DOMAIN_DISK_TYPE_VOLUME:
+ case VIR_DOMAIN_DISK_TYPE_LAST:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("external inactive snapshots are not supported on "
+ "'%s' disks"),
virDomainDiskTypeToString(actualType));
+ return -1;
+ }
+
+ return 0;
+}
+
+
+
+static int
+qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ virDomainSnapshotDiskDefPtr snapdisk,
+ bool active,
+ bool reuse)
+{
+ int actualType;
+ struct stat st;
+
+ if (qemuTranslateSnapshotDiskSourcePool(conn, snapdisk) < 0)
+ return -1;
+
+ if (!active) {
+ if (qemuTranslateDiskSourcePool(conn, disk) < 0)
+ return -1;
+
+ if (qemuDomainSnapshotPrepareDiskExternalBacking(disk) < 0)
+ return -1;
+
+ if (qemuDomainSnapshotPrepareDiskExternalOverlayInactive(snapdisk) < 0)
+ return -1;
+ } else {
+ if (qemuDomainSnapshotPrepareDiskExternalOverlayActive(snapdisk) < 0)
+ return -1;
+ }
+
+ actualType = qemuSnapshotDiskGetActualType(snapdisk);
+
+ switch ((enum virDomainDiskType) actualType) {
+ case VIR_DOMAIN_DISK_TYPE_BLOCK:
+ case VIR_DOMAIN_DISK_TYPE_FILE:
+ if (stat(snapdisk->file, &st) < 0) {
+ if (errno != ENOENT) {
+ virReportSystemError(errno,
+ _("unable to stat for disk %s: %s"),
+ snapdisk->name, snapdisk->file);
+ return -1;
+ } else if (reuse) {
+ virReportSystemError(errno,
+ _("missing existing file for disk %s:
%s"),
+ snapdisk->name, snapdisk->file);
+ return -1;
+ }
+ } else if (!S_ISBLK(st.st_mode) && st.st_size && !reuse) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("external snapshot file for disk %s already "
+ "exists and is not a block device: %s"),
+ snapdisk->name, snapdisk->file);
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_DISK_TYPE_NETWORK:
+ case VIR_DOMAIN_DISK_TYPE_DIR:
+ case VIR_DOMAIN_DISK_TYPE_VOLUME:
+ case VIR_DOMAIN_DISK_TYPE_LAST:
+ break;
+ }
+
+ return 0;
+}
+
+
+static int
+qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ bool active)
+{
+ int actualType;
+
+ /* active disks are handeled by qemu itself so no need to worry about those */
+ if (active)
+ return 0;
+
+ if (qemuTranslateDiskSourcePool(conn, disk) < 0)
+ return -1;
+
+ actualType = qemuDiskGetActualType(disk);
+
+ switch ((enum virDomainDiskType) actualType) {
+ case VIR_DOMAIN_DISK_TYPE_BLOCK:
+ case VIR_DOMAIN_DISK_TYPE_FILE:
+ return 0;
+
+ case VIR_DOMAIN_DISK_TYPE_NETWORK:
+ switch ((enum virDomainDiskProtocol) disk->protocol) {
+ case VIR_DOMAIN_DISK_PROTOCOL_NBD:
+ case VIR_DOMAIN_DISK_PROTOCOL_RBD:
+ case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
+ case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
+ case VIR_DOMAIN_DISK_PROTOCOL_ISCSI:
+ case VIR_DOMAIN_DISK_PROTOCOL_HTTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_HTTPS:
+ case VIR_DOMAIN_DISK_PROTOCOL_FTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_FTPS:
+ case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_LAST:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("internal inactive snapshots are not supported on
"
+ "'network' disks using '%s'
protocol"),
+ virDomainDiskProtocolTypeToString(disk->protocol));
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_DISK_TYPE_DIR:
+ case VIR_DOMAIN_DISK_TYPE_VOLUME:
+ case VIR_DOMAIN_DISK_TYPE_LAST:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("internal inactive snapshots are not supported on "
+ "'%s' disks"),
virDomainDiskTypeToString(actualType));
+ return -1;
+ }
+
+ return 0;
+}
+
+
+static int
+qemuDomainSnapshotPrepare(virConnectPtr conn,
+ virDomainObjPtr vm,
+ virDomainSnapshotDefPtr def,
unsigned int *flags)
{
int ret = -1;
size_t i;
bool active = virDomainObjIsActive(vm);
- struct stat st;
bool reuse = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
bool atomic = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0;
bool found_internal = false;
@@ -11766,8 +11979,19 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
virDomainSnapshotDefPtr def,
case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL:
found_internal = true;
- if (def->state != VIR_DOMAIN_DISK_SNAPSHOT &&
- dom_disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
+ if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && active) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("active qemu domains require external disk "
+ "snapshots; disk %s requested internal"),
+ disk->name);
+ goto cleanup;
+ }
+
+ if (qemuDomainSnapshotPrepareDiskInternal(conn, dom_disk,
+ active) < 0)
+ goto cleanup;
+
+ if (dom_disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
(dom_disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG ||
dom_disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD)) {
break;
@@ -11782,13 +12006,6 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
virDomainSnapshotDefPtr def,
vm->def->disks[i]->format));
goto cleanup;
}
- if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && active) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("active qemu domains require external disk "
- "snapshots; disk %s requested internal"),
- disk->name);
- goto cleanup;
- }
break;
case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL:
@@ -11803,25 +12020,11 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
virDomainSnapshotDefPtr def,
virStorageFileFormatTypeToString(disk->format));
goto cleanup;
}
- if (stat(disk->file, &st) < 0) {
- if (errno != ENOENT) {
- virReportSystemError(errno,
- _("unable to stat for disk %s: %s"),
- disk->name, disk->file);
- goto cleanup;
- } else if (reuse) {
- virReportSystemError(errno,
- _("missing existing file for disk %s:
%s"),
- disk->name, disk->file);
- goto cleanup;
- }
- } else if (!S_ISBLK(st.st_mode) && st.st_size && !reuse) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("external snapshot file for disk %s already "
- "exists and is not a block device: %s"),
- disk->name, disk->file);
+
+ if (qemuDomainSnapshotPrepareDiskExternal(conn, dom_disk, disk,
+ active, reuse) < 0)
goto cleanup;
- }
+
external++;
break;
@@ -12326,6 +12529,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
const char *xmlDesc,
unsigned int flags)
{
+ virConnectPtr conn = domain->conn;
virQEMUDriverPtr driver = domain->conn->privateData;
virDomainObjPtr vm = NULL;
char *xml = NULL;
@@ -12464,7 +12668,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
}
if (virDomainSnapshotAlignDisks(def, align_location,
align_match) < 0 ||
- qemuDomainSnapshotPrepare(vm, def, &flags) < 0)
+ qemuDomainSnapshotPrepare(conn, vm, def, &flags) < 0)
goto cleanup;
}
--
1.8.4.3
5:14 a.m.
New subject: [libvirt] [PATCHv1.5 24/27] qemu: snapshots: Declare supported and unsupported snapshot configs
On 26.11.2013 17:49, Peter Krempa wrote:
Currently the snapshot code did not check if it actually supports
snapshots on various disk backends for domains. To avoid future problems
add checkers that whitelist the supported configurations.
---
src/qemu/qemu_driver.c | 264 +++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 234 insertions(+), 30 deletions(-)
ACK
Michal
10:49 a.m.
New subject: [libvirt] [PATCHv1.5 25/27] RFC: snapshot: Add support for specifying snapshot disk backing type
Add support for specifying various types when doing snapshots. This will
later allow to do snapshots on network backed volumes.
RFC: This patch is lacking tests and domain schema touch up.
---
src/conf/snapshot_conf.c | 9 ++++++++
src/qemu/qemu_driver.c | 56 +++++++++++++++++++++++++++++++-----------------
2 files changed, 45 insertions(+), 20 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 5958f13..f6f170e 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -108,6 +108,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
{
int ret = -1;
char *snapshot = NULL;
+ char *type = NULL;
xmlNodePtr cur;
def->name = virXMLPropString(node, "name");
@@ -129,6 +130,13 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
}
def->type = -1;
+ if ((type = virXMLPropString(node, "type"))) {
+ if ((def->type = virDomainDiskTypeFromString(type)) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unknown disk snapshot type '%s'"),
type);
+ goto cleanup;
+ }
+ }
for (cur = node->children; cur; cur = cur->next) {
if (cur->type != XML_ELEMENT_NODE)
@@ -174,6 +182,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
ret = 0;
cleanup:
VIR_FREE(snapshot);
+ VIR_FREE(type);
if (ret < 0)
virDomainSnapshotDiskDefClear(def);
return ret;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b9c270b..e6d4f47 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12115,33 +12115,48 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr
driver,
}
if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0 ||
- VIR_STRDUP(source, snap->file) < 0 ||
(persistDisk && VIR_STRDUP(persistSource, source) < 0))
goto cleanup;
- /* create the stub file and set selinux labels; manipulate disk in
- * place, in a way that can be reverted on failure. */
- if (!reuse) {
- fd = qemuOpenFile(driver, vm, source, O_WRONLY | O_TRUNC | O_CREAT,
- &need_unlink, NULL);
- if (fd < 0)
- goto cleanup;
- VIR_FORCE_CLOSE(fd);
- }
-
/* XXX Here, we know we are about to alter disk->backingChain if
- * successful, so we nuke the existing chain so that future
- * commands will recompute it. Better would be storing the chain
- * ourselves rather than reprobing, but this requires modifying
- * domain_conf and our XML to fully track the chain across
- * libvirtd restarts. */
+ * successful, so we nuke the existing chain so that future commands will
+ * recompute it. Better would be storing the chain ourselves rather than
+ * reprobing, but this requires modifying domain_conf and our XML to fully
+ * track the chain across libvirtd restarts. */
virStorageFileFreeMetadata(disk->backingChain);
disk->backingChain = NULL;
- if (qemuDomainPrepareDiskChainElement(driver, vm, disk, source,
- VIR_DISK_CHAIN_READ_WRITE) < 0) {
- qemuDomainPrepareDiskChainElement(driver, vm, disk, source,
- VIR_DISK_CHAIN_NO_ACCESS);
+ switch (snap->type) {
+ case VIR_DOMAIN_DISK_TYPE_BLOCK:
+ reuse = true;
+ /* fallthrough */
+ case -1: /* type was not provided in snapshot conf */
+ case VIR_DOMAIN_DISK_TYPE_FILE:
+ if (VIR_STRDUP(source, snap->file) < 0)
+ goto cleanup;
+
+ /* create the stub file and set selinux labels; manipulate disk in
+ * place, in a way that can be reverted on failure. */
+ if (!reuse) {
+ fd = qemuOpenFile(driver, vm, source, O_WRONLY | O_TRUNC | O_CREAT,
+ &need_unlink, NULL);
+ if (fd < 0)
+ goto cleanup;
+ VIR_FORCE_CLOSE(fd);
+ }
+
+ if (qemuDomainPrepareDiskChainElement(driver, vm, disk, source,
+ VIR_DISK_CHAIN_READ_WRITE) < 0) {
+ qemuDomainPrepareDiskChainElement(driver, vm, disk, source,
+ VIR_DISK_CHAIN_NO_ACCESS);
+ goto cleanup;
+ }
+ break;
+
+ default:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("snapshots are not supported on '%s'
volumes"),
+ virDomainDiskTypeToString(snap->type));
goto cleanup;
}
@@ -12160,6 +12175,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
disk->src = source;
source = NULL;
disk->format = format;
+ disk->type = snap->type;
if (persistDisk) {
VIR_FREE(persistDisk->src);
persistDisk->src = persistSource;
--
1.8.4.3
10:49 a.m.
New subject: [libvirt] [PATCHv1.5 26/27] RFC: conf: snapshot: Parse more snapshot information
Add fields to hold information about network volumes for snapshots.
RFC: Missing schema and tests.
---
src/conf/snapshot_conf.c | 12 ++++++++----
src/conf/snapshot_conf.h | 15 +++++++++------
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index f6f170e..7ad605f 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -153,9 +153,9 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
if (virDomainDiskSourceDefParse(cur,
backingtype,
&def->file,
- NULL,
- NULL,
- NULL,
+ &def->protocol,
+ &def->nhosts,
+ &def->hosts,
NULL) < 0)
goto cleanup;
@@ -632,7 +632,11 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
virDomainDiskSourceDefFormatInternal(buf,
type,
disk->file,
- 0, 0, 0, NULL, 0, NULL, NULL, 0);
+ 0,
+ disk->protocol,
+ disk->nhosts,
+ disk->hosts,
+ 0, NULL, NULL, 0);
virBufferAddLit(buf, " </disk>\n");
}
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 241d63c..bcd92dc 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -48,12 +48,15 @@ enum virDomainSnapshotState {
typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef;
typedef virDomainSnapshotDiskDef *virDomainSnapshotDiskDefPtr;
struct _virDomainSnapshotDiskDef {
- char *name; /* name matching the <target dev='...' of the domain */
- int index; /* index within snapshot->dom->disks that matches name */
- int snapshot; /* enum virDomainSnapshotLocation */
- int type; /* enum virDomainDiskType */
- char *file; /* new source file when snapshot is external */
- int format; /* enum virStorageFileFormat */
+ char *name; /* name matching the <target dev='...' of the domain */
+ int index; /* index within snapshot->dom->disks that matches name */
+ int snapshot; /* enum virDomainSnapshotLocation */
+ int type; /* enum virDomainDiskType */
+ char *file; /* new source file when snapshot is external */
+ int format; /* enum virStorageFileFormat */
+ int protocol; /* network source protocol */
+ size_t nhosts; /* network source hosts count */
+ virDomainDiskHostDefPtr hosts; /* network source hosts */
};
/* Stores the complete snapshot metadata */
--
1.8.4.3
10:49 a.m.
New subject: [libvirt] [PATCHv1.5 27/27] RFC: qemu: snapshot: Add support for external active snapshots on gluster
This is a basic implementation of snapshots on gluster. Many things are
lacking, especially cleanup after a failed snapshot.
---
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_command.h | 9 ++++
src/qemu/qemu_driver.c | 106 ++++++++++++++++++++++++++++++++++++++++++++----
3 files changed, 109 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0738de2..5e8348f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3826,7 +3826,7 @@ cleanup:
}
-static int
+int
qemuGetDriveSourceString(int type,
const char *src,
int protocol,
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 66c23cc..ec944ea 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -312,4 +312,13 @@ qemuParseKeywords(const char *str,
int *retnkeywords,
int allowEmptyValue);
+int qemuGetDriveSourceString(int type,
+ const char *src,
+ int protocol,
+ size_t nhosts,
+ virDomainDiskHostDefPtr hosts,
+ const char *username,
+ const char *secret,
+ char **path);
+
#endif /* __QEMU_COMMAND_H__*/
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e6d4f47..4aa88c8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11412,6 +11412,24 @@ cleanup:
return ret;
}
+
+static int
+qemuDomainSnapshotDiskGetSourceString(virDomainSnapshotDiskDefPtr disk,
+ char **source)
+{
+ *source = NULL;
+
+ return qemuGetDriveSourceString(qemuSnapshotDiskGetActualType(disk),
+ disk->file,
+ disk->protocol,
+ disk->nhosts,
+ disk->hosts,
+ NULL,
+ NULL,
+ source);
+}
+
+
typedef enum {
VIR_DISK_CHAIN_NO_ACCESS,
VIR_DISK_CHAIN_READ_ONLY,
@@ -11792,6 +11810,29 @@
qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d
return 0;
case VIR_DOMAIN_DISK_TYPE_NETWORK:
+ switch ((enum virDomainDiskProtocol) disk->protocol) {
+ case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
+ return 0;
+
+ case VIR_DOMAIN_DISK_PROTOCOL_NBD:
+ case VIR_DOMAIN_DISK_PROTOCOL_RBD:
+ case VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG:
+ case VIR_DOMAIN_DISK_PROTOCOL_ISCSI:
+ case VIR_DOMAIN_DISK_PROTOCOL_HTTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_HTTPS:
+ case VIR_DOMAIN_DISK_PROTOCOL_FTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_FTPS:
+ case VIR_DOMAIN_DISK_PROTOCOL_TFTP:
+ case VIR_DOMAIN_DISK_PROTOCOL_LAST:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("external active snapshots are not supported on "
+ "'network' disks using '%s'
protocol"),
+ virDomainDiskProtocolTypeToString(disk->protocol));
+ return -1;
+
+ }
+ break;
+
case VIR_DOMAIN_DISK_TYPE_DIR:
case VIR_DOMAIN_DISK_TYPE_VOLUME:
case VIR_DOMAIN_DISK_TYPE_LAST:
@@ -12101,6 +12142,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = vm->privateData;
char *device = NULL;
char *source = NULL;
+ char *newsource = NULL;
+ virDomainDiskHostDefPtr newhosts = NULL;
+ virDomainDiskHostDefPtr persistHosts = NULL;
int format = snap->format;
const char *formatStr = NULL;
char *persistSource = NULL;
@@ -12114,8 +12158,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
return -1;
}
- if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0 ||
- (persistDisk && VIR_STRDUP(persistSource, source) < 0))
+ if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0)
goto cleanup;
/* XXX Here, we know we are about to alter disk->backingChain if
@@ -12126,14 +12169,22 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr
driver,
virStorageFileFreeMetadata(disk->backingChain);
disk->backingChain = NULL;
+ if (qemuDomainSnapshotDiskGetSourceString(snap, &source) < 0)
+ goto cleanup;
+
+ if (VIR_STRDUP(newsource, snap->file) < 0)
+ goto cleanup;
+
+ if (persistDisk &&
+ VIR_STRDUP(persistSource, snap->file) < 0)
+ goto cleanup;
+
switch (snap->type) {
case VIR_DOMAIN_DISK_TYPE_BLOCK:
reuse = true;
/* fallthrough */
case -1: /* type was not provided in snapshot conf */
case VIR_DOMAIN_DISK_TYPE_FILE:
- if (VIR_STRDUP(source, snap->file) < 0)
- goto cleanup;
/* create the stub file and set selinux labels; manipulate disk in
* place, in a way that can be reverted on failure. */
@@ -12153,6 +12204,27 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr
driver,
}
break;
+ case VIR_DOMAIN_DISK_TYPE_NETWORK:
+ switch (snap->protocol) {
+ case VIR_DOMAIN_DISK_PROTOCOL_GLUSTER:
+ if (!(newhosts = virDomainDiskHostDefCopy(snap->nhosts, snap->hosts)))
+ goto cleanup;
+
+ if (persistDisk &&
+ !(persistHosts = virDomainDiskHostDefCopy(snap->nhosts,
snap->hosts)))
+ goto cleanup;
+
+ break;
+
+ default:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("snapshots on volumes using '%s' protocol
"
+ "are not supported"),
+ virDomainDiskProtocolTypeToString(snap->protocol));
+ goto cleanup;
+ }
+ break;
+
default:
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("snapshots are not supported on '%s'
volumes"),
@@ -12171,16 +12243,33 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr
driver,
/* Update vm in place to match changes. */
need_unlink = false;
+
VIR_FREE(disk->src);
- disk->src = source;
- source = NULL;
+ virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
+
+ disk->src = newsource;
disk->format = format;
disk->type = snap->type;
+ disk->protocol = snap->protocol;
+ disk->nhosts = snap->nhosts;
+ disk->hosts = newhosts;
+
+ newsource = NULL;
+ newhosts = NULL;
+
if (persistDisk) {
VIR_FREE(persistDisk->src);
+ virDomainDiskHostDefFree(persistDisk->nhosts, persistDisk->hosts);
+
persistDisk->src = persistSource;
- persistSource = NULL;
persistDisk->format = format;
+ persistDisk->type = snap->type;
+ persistDisk->protocol = snap->protocol;
+ persistDisk->nhosts = snap->nhosts;
+ persistDisk->hosts = persistHosts;
+
+ persistSource = NULL;
+ persistHosts = NULL;
}
cleanup:
@@ -12188,7 +12277,10 @@ cleanup:
VIR_WARN("unable to unlink just-created %s", source);
VIR_FREE(device);
VIR_FREE(source);
+ VIR_FREE(newsource);
VIR_FREE(persistSource);
+ virDomainDiskHostDefFree(snap->nhosts, newhosts);
+ virDomainDiskHostDefFree(snap->nhosts, persistHosts);
return ret;
}
--
1.8.4.3
4007
days inactive
4014
days old
73 comments
5 participants
participants (5)
-
Eric Blake -
Laine Stump -
Michal Privoznik -
Osier Yang -
Peter Krempa