[libvirt] [PATCHv1.5 00/27] Gluster snapshot support (RFC) and refactors
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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
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
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
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
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
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
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)
--- 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
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
--- 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
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
--- 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
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
--- 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
On 26.11.2013 17:48, Peter Krempa wrote:
--- src/qemu/qemu_command.c | 128 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 86 insertions(+), 42 deletions(-)
ACK Michal
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
--- 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
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
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.
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
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
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
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
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
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
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
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
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
participants (5)
-
Eric Blake -
Laine Stump -
Michal Privoznik -
Osier Yang -
Peter Krempa