[libvirt] [PATCH 0/5] Fix starting of VMs with backing chains containing unknown backing file specification
When a backing chain element specifies a parent in a format not known to libvirt we'd fail to start the VM as the chain would appear broken. To prevent this happening introduce a new disk type to collect unknow format specs and avoid startup failures with such disk type. Peter Krempa (5): util: storage: Convert disk locality check to switch statement conf: Introduce raw disk string passthrough conf: Mark backing chain ending with "raw" volume as broken tests: Add tests for disk type 'raw' utils: storage: Fall back to "raw" disk on backing store parse failure docs/schemas/domaincommon.rng | 14 ++++ src/conf/domain_conf.c | 12 ++- src/qemu/qemu_command.c | 1 + src/qemu/qemu_driver.c | 4 + src/storage/storage_driver.c | 8 ++ src/util/virstoragefile.c | 35 +++++++- src/util/virstoragefile.h | 1 + .../qemuxml2argv-disk-backing-chains-raw.xml | 94 +++++++++++++++++++++ .../qemuxml2argv-disk-virtio-raw.args | 9 ++ .../qemuxml2argv-disk-virtio-raw.xml | 45 ++++++++++ tests/qemuxml2argvtest.c | 1 + ...muxml2xmlout-disk-backing-chains-raw-active.xml | 95 ++++++++++++++++++++++ ...xml2xmlout-disk-backing-chains-raw-inactive.xml | 59 ++++++++++++++ tests/qemuxml2xmltest.c | 2 + 14 files changed, 375 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-backing-chains-raw.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-raw.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-raw.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-raw-active.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-raw-inactive.xml -- 2.0.2
To allow the compiler to track future additions of disk types, convert the function to use a switch statement with the correct type. --- src/util/virstoragefile.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5b6b2f5..299edcd 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1956,7 +1956,22 @@ virStorageSourceGetActualType(virStorageSourcePtr def) bool virStorageSourceIsLocalStorage(virStorageSourcePtr src) { - return virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK; + virStorageType type = virStorageSourceGetActualType(src); + + switch (type) { + case VIR_STORAGE_TYPE_FILE: + case VIR_STORAGE_TYPE_BLOCK: + case VIR_STORAGE_TYPE_DIR: + return true; + + case VIR_STORAGE_TYPE_NETWORK: + case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_LAST: + case VIR_STORAGE_TYPE_NONE: + return false; + } + + return false; } -- 2.0.2
On Tue, Sep 09, 2014 at 10:45:44AM +0200, Peter Krempa wrote:
To allow the compiler to track future additions of disk types, convert the function to use a switch statement with the correct type. --- src/util/virstoragefile.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 09/10/14 12:04, Daniel P. Berrange wrote:
On Tue, Sep 09, 2014 at 10:45:44AM +0200, Peter Krempa wrote:
To allow the compiler to track future additions of disk types, convert the function to use a switch statement with the correct type. --- src/util/virstoragefile.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
ACK
I've pushed this one. Thanks. Peter
On 09/09/2014 02:45 AM, Peter Krempa wrote:
To allow the compiler to track future additions of disk types, convert the function to use a switch statement with the correct type. --- src/util/virstoragefile.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5b6b2f5..299edcd 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1956,7 +1956,22 @@ virStorageSourceGetActualType(virStorageSourcePtr def) bool virStorageSourceIsLocalStorage(virStorageSourcePtr src) { - return virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK; + virStorageType type = virStorageSourceGetActualType(src); + + switch (type) { + case VIR_STORAGE_TYPE_FILE: + case VIR_STORAGE_TYPE_BLOCK: + case VIR_STORAGE_TYPE_DIR: + return true; + + case VIR_STORAGE_TYPE_NETWORK: + case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_LAST: + case VIR_STORAGE_TYPE_NONE:
Might be worth a comment that TYPE_NETWORK (and maybe TYPE_NONE) is expected, while the other cases represent coding error (the GetActualType should have gotten rid of TYPE_VOLUME, and TYPE_LAST should never be present; TYPE_NONE is possible only if we hit something that we truly do not know what type it has). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch adds a new disk type "raw" that can be used twofold: 1) To pass arbitrary strings as disk sources to the hypervisor of choice. This allows to use not-yet-supported storage specification formats. 2) To return backing chain element names that libvirt doesn't yet know how to parse. Backing chain elements may specify names that libvirt isn't able to parse. To allow us reporting it back to the user, unparsable strings will be reported as disk type="raw". To avoid attempts to label or do other operations on "raw" disks mark them as remote and unsupported by the storage driver explicitly. Tests for the new format are being added separately. --- docs/schemas/domaincommon.rng | 14 ++++++++++++++ src/conf/domain_conf.c | 7 +++++++ src/qemu/qemu_command.c | 1 + src/qemu/qemu_driver.c | 4 ++++ src/storage/storage_driver.c | 8 ++++++++ src/util/virstoragefile.c | 4 +++- src/util/virstoragefile.h | 1 + 7 files changed, 38 insertions(+), 1 deletion(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index cedceae..b7246c5 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1296,6 +1296,7 @@ <ref name="diskSourceDir"/> <ref name="diskSourceNetwork"/> <ref name="diskSourceVolume"/> + <ref name="diskSourceRaw"/> </choice> </define> @@ -1461,6 +1462,19 @@ </interleave> </define> + <define name="diskSourceRaw"> + <attribute name="type"> + <value>raw</value> + </attribute> + <optional> + <element name="source"> + <attribute name="raw"> + <data type="string"/> + </attribute> + </element> + </optional> + </define> + <define name="diskTarget"> <data type="string"> <param name="pattern">(ioemu:)?(fd|hd|sd|vd|xvd|ubd)[a-zA-Z0-9_]+</param> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aac78a6..189a4e8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5305,6 +5305,9 @@ virDomainDiskSourceParse(xmlNodePtr node, if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0) goto cleanup; break; + case VIR_STORAGE_TYPE_RAW: + src->path = virXMLPropString(node, "raw"); + break; case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -15557,6 +15560,10 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, skipSeclabels); break; + case VIR_STORAGE_TYPE_RAW: + virBufferEscapeString(buf, "<source raw='%s'/>\n", src->path); + break; + case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7b87a31..82111ca 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3184,6 +3184,7 @@ qemuGetDriveSourceString(virStorageSourcePtr src, case VIR_STORAGE_TYPE_BLOCK: case VIR_STORAGE_TYPE_FILE: case VIR_STORAGE_TYPE_DIR: + case VIR_STORAGE_TYPE_RAW: if (!src->path) { ret = 1; goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d724eeb..c4af401 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12460,6 +12460,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: + case VIR_STORAGE_TYPE_RAW: virReportError(VIR_ERR_INTERNAL_ERROR, _("external inactive snapshots are not supported on " "'%s' disks"), virStorageTypeToString(actualType)); @@ -12523,6 +12524,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NONE: case VIR_STORAGE_TYPE_LAST: + case VIR_STORAGE_TYPE_RAW: virReportError(VIR_ERR_INTERNAL_ERROR, _("external active snapshots are not supported on " "'%s' disks"), virStorageTypeToString(actualType)); @@ -12547,6 +12549,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayInactive(virDomainSnapshotDiskDefPtr case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_RAW: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("external inactive snapshots are not supported on " @@ -12665,6 +12668,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn, case VIR_STORAGE_TYPE_DIR: case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_RAW: case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("internal inactive snapshots are not supported on " diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 433d7b7..25ce748 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2448,6 +2448,10 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) return false; actualType = virStorageSourceGetActualType(src); + /* explicitly reject raw disk strings */ + if (actualType == VIR_STORAGE_TYPE_RAW) + return false; + if (src->drv) { backend = src->drv->backend; } else { @@ -2481,6 +2485,10 @@ virStorageFileSupportsSecurityDriver(virStorageSourcePtr src) return false; actualType = virStorageSourceGetActualType(src); + /* explicitly reject raw disk strings */ + if (actualType == VIR_STORAGE_TYPE_RAW) + return false; + if (src->drv) { backend = src->drv->backend; } else { diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 299edcd..a17ced1 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -57,7 +57,8 @@ VIR_ENUM_IMPL(virStorage, VIR_STORAGE_TYPE_LAST, "block", "dir", "network", - "volume") + "volume", + "raw") VIR_ENUM_IMPL(virStorageFileFormat, VIR_STORAGE_FILE_LAST, @@ -1968,6 +1969,7 @@ virStorageSourceIsLocalStorage(virStorageSourcePtr src) case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_LAST: case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_RAW: return false; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index eccbf4e..e643095 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -49,6 +49,7 @@ typedef enum { VIR_STORAGE_TYPE_DIR, VIR_STORAGE_TYPE_NETWORK, VIR_STORAGE_TYPE_VOLUME, + VIR_STORAGE_TYPE_RAW, VIR_STORAGE_TYPE_LAST } virStorageType; -- 2.0.2
On Tue, Sep 09, 2014 at 10:45:45AM +0200, Peter Krempa wrote:
This patch adds a new disk type "raw" that can be used twofold:
1) To pass arbitrary strings as disk sources to the hypervisor of choice. This allows to use not-yet-supported storage specification formats.
2) To return backing chain element names that libvirt doesn't yet know how to parse. Backing chain elements may specify names that libvirt isn't able to parse. To allow us reporting it back to the user, unparsable strings will be reported as disk type="raw".
To avoid attempts to label or do other operations on "raw" disks mark them as remote and unsupported by the storage driver explicitly.
Tests for the new format are being added separately.
NACK. Arbitrary passthrough or reporting of unsupported features is an explicit non-goal of the libvirt XML. We have QEMU command line passthrough for that goal. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Do not output the </backingStore> element in the xml if the backing chain ends with a volume specification unknown by libvirt. --- src/conf/domain_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 189a4e8..2170607 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15589,6 +15589,7 @@ virDomainDiskSourceFormat(virBufferPtr buf, static int virDomainDiskBackingStoreFormat(virBufferPtr buf, virStorageSourcePtr backingStore, + int parentType, const char *backingStoreRaw, unsigned int idx) { @@ -15596,7 +15597,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, const char *format; if (!backingStore) { - if (!backingStoreRaw) + if (!backingStoreRaw && parentType != VIR_STORAGE_TYPE_RAW) virBufferAddLit(buf, "<backingStore/>\n"); return 0; } @@ -15626,6 +15627,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 || virDomainDiskBackingStoreFormat(buf, backingStore->backingStore, + backingStore->type, backingStore->backingStoreRaw, idx + 1) < 0) return -1; @@ -15751,6 +15753,7 @@ virDomainDiskDefFormat(virBufferPtr buf, * persistent storage of backing chains is ready. */ if (!(flags & VIR_DOMAIN_XML_INACTIVE) && virDomainDiskBackingStoreFormat(buf, def->src->backingStore, + def->src->type, def->src->backingStoreRaw, 1) < 0) return -1; -- 2.0.2
On Tue, Sep 09, 2014 at 10:45:46AM +0200, Peter Krempa wrote:
Do not output the </backingStore> element in the xml if the backing chain ends with a volume specification unknown by libvirt. --- src/conf/domain_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
NACK, because passthrough should not exist. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Test the qemu command generator and the xml parsers and formatters. The tests are added separately due to the size of the test files. Note that tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-raw.args is split into lines in a weird way as "sc_prohibit_long_lines" forced me to. --- .../qemuxml2argv-disk-backing-chains-raw.xml | 94 +++++++++++++++++++++ .../qemuxml2argv-disk-virtio-raw.args | 9 ++ .../qemuxml2argv-disk-virtio-raw.xml | 45 ++++++++++ tests/qemuxml2argvtest.c | 1 + ...muxml2xmlout-disk-backing-chains-raw-active.xml | 95 ++++++++++++++++++++++ ...xml2xmlout-disk-backing-chains-raw-inactive.xml | 59 ++++++++++++++ tests/qemuxml2xmltest.c | 2 + 7 files changed, 305 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-backing-chains-raw.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-raw.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-raw.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-raw-active.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-raw-inactive.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-backing-chains-raw.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-backing-chains-raw.xml new file mode 100644 index 0000000..86fc60a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-backing-chains-raw.xml @@ -0,0 +1,94 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='qcow2'/> + <source protocol='gluster' name='Volume2/Image'> + <host transport='unix' socket='/path/to/sock'/> + </source> + <backingStore type='file' index='1'> + <format type='qcow2'/> + <source file='/tmp/missing-backing-store.qcow'/> + </backingStore> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='qcow2'/> + <source protocol='nbd' name='bar'> + <host transport='unix' socket='/var/run/nbdsock'/> + </source> + <backingStore type='block' index='1'> + <format type='qcow2'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <backingStore type='file' index='2'> + <format type='qcow2'/> + <source file='/tmp/image2.qcow'/> + <backingStore type='file' index='3'> + <format type='qcow2'/> + <source file='/tmp/image3.qcow'/> + <backingStore type='file' index='4'> + <format type='qcow2'/> + <source file='/tmp/image4.qcow'/> + <backingStore type='file' index='5'> + <source file='/tmp/image5.qcow'/> + <format type='qcow2'/> + <backingStore type='raw' index='6'> + <format type='qcow2'/> + <source raw='json: { "file.driver":"file", "file.filename":"/libvirt/bsdf.img"}'/> + <backingStore/> + </backingStore> + </backingStore> + </backingStore> + </backingStore> + </backingStore> + </backingStore> + <target dev='vdb' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <backingStore/> + <source protocol='gluster' name='Volume1/Image'> + <host name='example.org' port='6000'/> + </source> + <target dev='vdc' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='qcow2'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + </source> + <backingStore type='file' index='1'> + <source file='/tmp/image.qcow'/> + <backingStore/> + <format type='qcow2'/> + </backingStore> + <target dev='vdd' bus='virtio'/> + </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2'/> + <source dev='/dev/HostVG/QEMUGuest11'/> + <target dev='vde' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-raw.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-raw.args new file mode 100644 index 0000000..84dc761 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-raw.args @@ -0,0 +1,9 @@ +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 \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ +-drive 'file=json: { "file.driver":"file",, \ +"file.filename":"/libvirt/bsdf.img"},if=ide,bus=0,unit=0' \ +-drive 'file=test test \n test; ^;.,, test &!@#$%\n,if=ide,\ +media=cdrom,bus=1,unit=0' \ +-drive file=/tmp/data.img,if=virtio \ +-drive file=/tmp/logs.img,if=virtio -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-raw.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-raw.xml new file mode 100644 index 0000000..d084ba5 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-raw.xml @@ -0,0 +1,45 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='raw' device='disk'> + <driver name='qemu' type='raw'/> + <source raw='json: { "file.driver":"file", "file.filename":"/libvirt/bsdf.img"}'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='raw' device='cdrom'> + <driver name='qemu' type='raw'/> + <source raw='test test \n test; ^;., test &!@#$%\n'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/tmp/data.img'/> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/tmp/logs.img'/> + <target dev='vdb' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 3feb2fe..3a782d7 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -706,6 +706,7 @@ mymain(void) QEMU_CAPS_DEVICE, QEMU_CAPS_VIRTIO_S390); DO_TEST("disk-many", NONE); DO_TEST("disk-virtio", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_BOOT); + DO_TEST("disk-virtio-raw", QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_BOOT); DO_TEST("disk-virtio-ccw", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_VIRTIO_CCW, QEMU_CAPS_VIRTIO_S390); DO_TEST("disk-virtio-ccw-many", QEMU_CAPS_DRIVE, diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-raw-active.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-raw-active.xml new file mode 100644 index 0000000..5b1ac7b --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-raw-active.xml @@ -0,0 +1,95 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='qcow2'/> + <source protocol='gluster' name='Volume2/Image'> + <host transport='unix' socket='/path/to/sock'/> + </source> + <backingStore type='file' index='1'> + <format type='qcow2'/> + <source file='/tmp/missing-backing-store.qcow'/> + <backingStore/> + </backingStore> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='qcow2'/> + <source protocol='nbd' name='bar'> + <host transport='unix' socket='/var/run/nbdsock'/> + </source> + <backingStore type='block' index='1'> + <format type='qcow2'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <backingStore type='file' index='2'> + <format type='qcow2'/> + <source file='/tmp/image2.qcow'/> + <backingStore type='file' index='3'> + <format type='qcow2'/> + <source file='/tmp/image3.qcow'/> + <backingStore type='file' index='4'> + <format type='qcow2'/> + <source file='/tmp/image4.qcow'/> + <backingStore type='file' index='5'> + <format type='qcow2'/> + <source file='/tmp/image5.qcow'/> + <backingStore type='raw' index='6'> + <format type='qcow2'/> + <source raw='json: { "file.driver":"file", "file.filename":"/libvirt/bsdf.img"}'/> + </backingStore> + </backingStore> + </backingStore> + </backingStore> + </backingStore> + </backingStore> + <target dev='vdb' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='gluster' name='Volume1/Image'> + <host name='example.org' port='6000'/> + </source> + <backingStore/> + <target dev='vdc' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='qcow2'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + </source> + <backingStore type='file' index='1'> + <format type='qcow2'/> + <source file='/tmp/image.qcow'/> + <backingStore/> + </backingStore> + <target dev='vdd' bus='virtio'/> + </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2'/> + <source dev='/dev/HostVG/QEMUGuest11'/> + <backingStore/> + <target dev='vde' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-raw-inactive.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-raw-inactive.xml new file mode 100644 index 0000000..5b59aad --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-raw-inactive.xml @@ -0,0 +1,59 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='qcow2'/> + <source protocol='gluster' name='Volume2/Image'> + <host transport='unix' socket='/path/to/sock'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='qcow2'/> + <source protocol='nbd' name='bar'> + <host transport='unix' socket='/var/run/nbdsock'/> + </source> + <target dev='vdb' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='gluster' name='Volume1/Image'> + <host name='example.org' port='6000'/> + </source> + <target dev='vdc' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='qcow2'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + </source> + <target dev='vdd' bus='virtio'/> + </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2'/> + <source dev='/dev/HostVG/QEMUGuest11'/> + <target dev='vde' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 03c05da..3a2b76e 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -213,6 +213,7 @@ mymain(void) DO_TEST("disk-xenvbd"); DO_TEST("disk-usb"); DO_TEST("disk-virtio"); + DO_TEST("disk-virtio-raw"); DO_TEST("floppy-drive-fat"); DO_TEST("disk-drive-fat"); DO_TEST("disk-drive-fmt-qcow"); @@ -385,6 +386,7 @@ mymain(void) DO_TEST("panic"); DO_TEST_DIFFERENT("disk-backing-chains"); + DO_TEST_DIFFERENT("disk-backing-chains-raw"); DO_TEST("chardev-label"); -- 2.0.2
On Tue, Sep 09, 2014 at 10:45:47AM +0200, Peter Krempa wrote:
Test the qemu command generator and the xml parsers and formatters. The tests are added separately due to the size of the test files.
Note that tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-raw.args is split into lines in a weird way as "sc_prohibit_long_lines" forced me to.
+ <disk type='network' device='disk'> + <driver name='qemu' type='qcow2'/> + <source protocol='nbd' name='bar'> + <host transport='unix' socket='/var/run/nbdsock'/> + </source> + <backingStore type='block' index='1'> + <format type='qcow2'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <backingStore type='file' index='2'> + <format type='qcow2'/> + <source file='/tmp/image2.qcow'/> + <backingStore type='file' index='3'> + <format type='qcow2'/> + <source file='/tmp/image3.qcow'/> + <backingStore type='file' index='4'> + <format type='qcow2'/> + <source file='/tmp/image4.qcow'/> + <backingStore type='file' index='5'> + <source file='/tmp/image5.qcow'/> + <format type='qcow2'/> + <backingStore type='raw' index='6'> + <format type='qcow2'/> + <source raw='json: { "file.driver":"file", "file.filename":"/libvirt/bsdf.img"}'/>
NACK precisely because it allows this kind of arbitrary passthrough of QEMU specific command syntax. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
When libvirt can't parse the backing store format for some reasons we should fall back to something safe rather than marking the backing chain as broken. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1134878 --- src/util/virstoragefile.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a17ced1..b471e37 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2287,6 +2287,9 @@ virStorageSourceNewFromBackingAbsolute(const char *path) if (VIR_ALLOC(ret) < 0) return NULL; + /* XXX We will need hypervisor-specific disk source parser callbacks here + * in the future */ + if (virStorageIsFile(path)) { ret->type = VIR_STORAGE_TYPE_FILE; @@ -2298,15 +2301,22 @@ virStorageSourceNewFromBackingAbsolute(const char *path) /* handle URI formatted backing stores */ if (strstr(path, "://")) { if (virStorageSourceParseBackingURI(ret, path) < 0) - goto error; + goto fallback; } else { if (virStorageSourceParseBackingColon(ret, path) < 0) - goto error; + goto fallback; } } return ret; + fallback: + ret->type = VIR_STORAGE_TYPE_RAW; + if (VIR_STRDUP(ret->path, path) < 0) + goto error; + + return ret; + error: virStorageSourceFree(ret); return NULL; -- 2.0.2
On Tue, Sep 09, 2014 at 10:45:48AM +0200, Peter Krempa wrote:
When libvirt can't parse the backing store format for some reasons we should fall back to something safe rather than marking the backing chain as broken.
I'm not really convinced that falling back to raw is the "safe" option vs reporting an error for the backing chain. There are a few reasons why we probe backing files - To report the backing file in the storage vol XML - To relabel disks to grant QEMU access (SElinux, DAC, CGroups) - To support APIs like block rebase that affect backing file I don't think that falling back to raw is going to make any of those scenarios "do the right thing" when they find an unknown backing store format. Rather things will simply fail in different, more obscure ways due to libvirt treating the backing file differently than QEMU. I think it is better than we report an explicit error upfront when we can't interpret backing store, as this will make it clear that libvirt can't work and likely mean that we find out about new features we need to support sooner. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 09/09/14 11:01, Daniel P. Berrange wrote:
On Tue, Sep 09, 2014 at 10:45:48AM +0200, Peter Krempa wrote:
When libvirt can't parse the backing store format for some reasons we should fall back to something safe rather than marking the backing chain as broken.
I'm not really convinced that falling back to raw is the "safe" option vs reporting an error for the backing chain.
There are a few reasons why we probe backing files
- To report the backing file in the storage vol XML - To relabel disks to grant QEMU access (SElinux, DAC, CGroups) - To support APIs like block rebase that affect backing file
I don't think that falling back to raw is going to make any of those scenarios "do the right thing" when they find an unknown backing store format. Rather things will simply fail in different, more obscure ways due to libvirt treating the backing file differently than QEMU. I think it is better than we report an explicit error upfront when we can't interpret backing store, as this will make it clear that libvirt can't work and likely mean that we find out about new features we need to support sooner.
Well we do exactly that right now. Although this disallows to start a VM that uses an obscure (read as: unknown to libvirt) backing path specification which causes grief. Should we then add a flag for the vm starting API that will bypass the check for backing chain integrity? Peter
On Tue, Sep 09, 2014 at 11:04:18AM +0200, Peter Krempa wrote:
On 09/09/14 11:01, Daniel P. Berrange wrote:
On Tue, Sep 09, 2014 at 10:45:48AM +0200, Peter Krempa wrote:
When libvirt can't parse the backing store format for some reasons we should fall back to something safe rather than marking the backing chain as broken.
I'm not really convinced that falling back to raw is the "safe" option vs reporting an error for the backing chain.
There are a few reasons why we probe backing files
- To report the backing file in the storage vol XML - To relabel disks to grant QEMU access (SElinux, DAC, CGroups) - To support APIs like block rebase that affect backing file
I don't think that falling back to raw is going to make any of those scenarios "do the right thing" when they find an unknown backing store format. Rather things will simply fail in different, more obscure ways due to libvirt treating the backing file differently than QEMU. I think it is better than we report an explicit error upfront when we can't interpret backing store, as this will make it clear that libvirt can't work and likely mean that we find out about new features we need to support sooner.
Well we do exactly that right now. Although this disallows to start a VM that uses an obscure (read as: unknown to libvirt) backing path specification which causes grief.
Not being able to start a VM because we can't parse the backing store is a desirable feature, because that means we will not have correctly configured SELinux / DAC / cgroups for the backing store, nor correctly reported auditing info for the disks.
Should we then add a flag for the vm starting API that will bypass the check for backing chain integrity?
I don't think we need that. If we don't support the new disk format then reporting errors is the right thing to do. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Sep 09, 2014 at 10:45:43AM +0200, Peter Krempa wrote:
When a backing chain element specifies a parent in a format not known to libvirt we'd fail to start the VM as the chain would appear broken.
To prevent this happening introduce a new disk type to collect unknow format specs and avoid startup failures with such disk type.
I tested the patch series as described here: https://bugzilla.redhat.com/show_bug.cgi?id=1134878#c2 and it sort of works. Instead of the original error, I now get an SELinux / labelling error: process exited while connecting to monitor: 2014-09-09T11:31:12.591266Z qemu-system-x86_64: -drive file=/tmp/test2.img,if=none,id=drive-scsi0-0-0-0,format=qcow2,cache=writeback: could not open disk image /tmp/test2.img: Could not open backing file: Could not open '/tmp/test1.img': Permission denied [code=1 domain=10] because of course the backing file is ignored rather than being labelled: $ ll -Z /tmp/test[12].img -rw-rw-r--. rjones rjones unconfined_u:object_r:user_tmp_t:s0 /tmp/test1.img -rw-r--r--. rjones rjones unconfined_u:object_r:svirt_image_t:s0:c117,c209 /tmp/test2.img Now for the case that *I* care about, which is ssh and https backing files, this doesn't matter because those are not labelled. Unfortunately this reveals another bug, which is that the SSH_* variables that are required by the ssh driver are not passed through to libvirtd. Ho hum. Anyway you can add: Tested-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On Tue, Sep 09, 2014 at 12:38:04PM +0100, Richard W.M. Jones wrote:
Unfortunately this reveals another bug, which is that the SSH_* variables that are required by the ssh driver are not passed through to libvirtd. Ho hum.
Filed as: https://bugzilla.redhat.com/show_bug.cgi?id=1139659 Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Tue, Sep 09, 2014 at 12:38:04PM +0100, Richard W.M. Jones wrote:
On Tue, Sep 09, 2014 at 10:45:43AM +0200, Peter Krempa wrote:
When a backing chain element specifies a parent in a format not known to libvirt we'd fail to start the VM as the chain would appear broken.
To prevent this happening introduce a new disk type to collect unknow format specs and avoid startup failures with such disk type.
I tested the patch series as described here:
https://bugzilla.redhat.com/show_bug.cgi?id=1134878#c2
and it sort of works. Instead of the original error, I now get an SELinux / labelling error:
process exited while connecting to monitor: 2014-09-09T11:31:12.591266Z qemu-system-x86_64: -drive file=/tmp/test2.img,if=none,id=drive-scsi0-0-0-0,format=qcow2,cache=writeback: could not open disk image /tmp/test2.img: Could not open backing file: Could not open '/tmp/test1.img': Permission denied [code=1 domain=10]
because of course the backing file is ignored rather than being labelled:
$ ll -Z /tmp/test[12].img -rw-rw-r--. rjones rjones unconfined_u:object_r:user_tmp_t:s0 /tmp/test1.img -rw-r--r--. rjones rjones unconfined_u:object_r:svirt_image_t:s0:c117,c209 /tmp/test2.img
Now for the case that *I* care about, which is ssh and https backing files, this doesn't matter because those are not labelled.
Unfortunately this reveals another bug, which is that the SSH_* variables that are required by the ssh driver are not passed through to libvirtd. Ho hum.
This is exactly why not treating unknown backing files as a fatal error is generally not helpful or desirable. When trying to rely on use of a feature that isn't supported, ocassionally you might get lucky with all the planets aligning leading the feature to work, but more often than not it is going to hit further bugs, which are even harder to diagnose. Certainly libvirt should support this new backing file format, including support for SSH, HTTP and so on, but until we do this explicitly, we should raise an immediate error as we do today rather than pretending that we expect it to work. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa -
Richard W.M. Jones