[libvirt] [PATCH v3 0/7] Support to use iscsi storage in domain conf
An update to yesterday's posting: https://www.redhat.com/archives/libvir-list/2013-July/msg01213.html Changes in v3 over v2 * Update patch 1 per code review * Use 'direct' instead of 'uri' and supporting documentaiton * In patch 4, use qemuAddISCSIPoolSourceHost() instead of qemuAddDiskISCSIUri. Also added a check in the function that (pooldef->source.nhost != 1) to make sure we at least have a host defined in the pool and of course that there's only one. John Ferlan (5): storage_iscsi: Reflect the default target port conf: Introduce new XML tag "mode" for disk source conf: Introduce virDomainDiskSourceIsBlockType qemu: Translate the iscsi pool/volume disk source tests: Add various network and volume definitions Osier Yang (2): conf: Ignore the volume type disk if its mode is "direct" qemu: Translate the volume type disk source before cgroup setting docs/formatdomain.html.in | 11 +- docs/schemas/domaincommon.rng | 8 ++ src/conf/domain_conf.c | 67 ++++++++++- src/conf/domain_conf.h | 26 +++++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 20 +++- src/qemu/qemu_conf.c | 124 +++++++++++++++++---- src/qemu/qemu_process.c | 13 ++- src/storage/storage_backend_iscsi.c | 19 ++-- .../qemuxml2argv-disk-source-pool-mode.xml | 48 ++++++++ tests/qemuxml2xmltest.c | 1 + tests/securityselinuxlabeldata/netdisks.txt | 5 + tests/securityselinuxlabeldata/netdisks.xml | 58 ++++++++++ tests/securityselinuxlabeldata/voldisks.txt | 5 + tests/securityselinuxlabeldata/voldisks.xml | 45 ++++++++ tests/securityselinuxlabeltest.c | 2 + 16 files changed, 414 insertions(+), 39 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml create mode 100644 tests/securityselinuxlabeldata/netdisks.txt create mode 100644 tests/securityselinuxlabeldata/netdisks.xml create mode 100644 tests/securityselinuxlabeldata/voldisks.txt create mode 100644 tests/securityselinuxlabeldata/voldisks.xml -- 1.8.1.4
Make sure default iSCSI target is 3260. --- src/storage/storage_backend_iscsi.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index ba4f388..54bcd14 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -44,12 +44,12 @@ #define VIR_FROM_THIS VIR_FROM_STORAGE +#define ISCSI_DEFAULT_TARGET_PORT 3260 + static char * virStorageBackendISCSIPortal(virStoragePoolSourcePtr source) { char *portal = NULL; - const char *host; - int port = 3260; if (source->nhost != 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -57,14 +57,17 @@ virStorageBackendISCSIPortal(virStoragePoolSourcePtr source) return NULL; } - host = source->hosts[0].name; - if (source->hosts[0].port != 0) - port = source->hosts[0].port; + if (source->hosts[0].port == 0) + source->hosts[0].port = ISCSI_DEFAULT_TARGET_PORT; - if (strchr(host, ':')) { - ignore_value(virAsprintf(&portal, "[%s]:%d,1", host, port)); + if (strchr(source->hosts[0].name, ':')) { + ignore_value(virAsprintf(&portal, "[%s]:%d,1", + source->hosts[0].name, + source->hosts[0].port)); } else { - ignore_value(virAsprintf(&portal, "%s:%d,1", host, port)); + ignore_value(virAsprintf(&portal, "%s:%d,1", + source->hosts[0].name, + source->hosts[0].port)); } return portal; -- 1.8.1.4
On Fri, Jul 19, 2013 at 08:32:27AM -0400, John Ferlan wrote:
Make sure default iSCSI target is 3260. --- src/storage/storage_backend_iscsi.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
ACK 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 :|
There are two ways to use a iSCSI LUN as disk source for qemu. * The LUN's path as it shows up on host, e.g. /dev/disk/by-path/ip-$ip:3260-iscsi-$iqn-fc18:iscsi.iscsi0-lun-1 * The libiscsi URI from the storage pool source element host attribute, e.g. iscsi://demo.org:6000/iqn.1992-01.com.example/1 For a "volume" type disk, if the specified "pool" is of iscsi type, we should support to use the LUN in either of above 2 ways. That's why to introduce a new XML tag "mode" for the disk source (libvirt should support iscsi pool with libiscsi, but it's another new feature, which should be done later). The "mode" can be either of "host" or "direct". Use "host" to indicate use of the LUN with the path as it shows up on host. Use "direct" to indicate to use it with the source pool host URI (future patches may support to use network type libvirt storage too, e.g. Ceph) --- docs/formatdomain.html.in | 11 ++++- docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 22 +++++++++- src/conf/domain_conf.h | 22 ++++++++++ .../qemuxml2argv-disk-source-pool-mode.xml | 48 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 110 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index b1c3bfc..7601aaa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1601,7 +1601,16 @@ <code>pool</code> and <code>volume</code>. Attribute <code>pool</code> specifies the name of storage pool (managed by libvirt) where the disk source resides, and attribute <code>volume</code> specifies the name of - storage volume (managed by libvirt) used as the disk source. + storage volume (managed by libvirt) used as the disk source. For a + "volume" type disk, if the underlying storage pool is "iscsi", attribute + <code>mode</code> (<span class="since">since 1.1.1</span>) can be used + to indicate how to represent the LUN as the disk source. The value + "host" indicates to use the LUN's path as it shows up on host, e.g. + /dev/disk/by-path/ip-10.11.12.9:3260-iscsi-iqn.2013-06.fc:iscsi.iscsi0-lun-1). + The value "direct" indicates to use the storage pool's + <code>source</code> element <code>host</code> attribute as the + disk source for the libiscsi URI, e.g. + file=iscsi://demo.org:6000/iqn.1992-01.com.example/1. <span class="since">Since 0.0.3; <code>type='dir'</code> since 0.7.5; <code>type='network'</code> since 0.8.7; <code>protocol='iscsi'</code> since 1.0.4; diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7a6852b..745b959 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1174,6 +1174,14 @@ <ref name="volName"/> </attribute> <optional> + <attribute name="mode"> + <choice> + <value>host</value> + <value>direct</value> + </choice> + </attribute> + </optional> + <optional> <ref name="startupPolicy"/> </optional> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 57cd9b1..419958f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -761,6 +761,11 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST, "default", "unmap", "ignore") +VIR_ENUM_IMPL(virDomainDiskSourcePoolMode, + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_LAST, + "default", + "host", + "direct") #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE @@ -4645,10 +4650,12 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, { char *pool = NULL; char *volume = NULL; + char *mode = NULL; int ret = -1; pool = virXMLPropString(node, "pool"); volume = virXMLPropString(node, "volume"); + mode = virXMLPropString(node, "mode"); /* CD-ROM and Floppy allows no source */ if (!pool && !volume) @@ -4664,6 +4671,14 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, if (VIR_ALLOC(def->srcpool) < 0) goto cleanup; + if (mode && (def->srcpool->mode = + virDomainDiskSourcePoolModeTypeFromString(mode)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown source mode '%s' for volume type disk"), + mode); + goto cleanup; + } + def->srcpool->pool = pool; pool = NULL; def->srcpool->volume = volume; @@ -4674,6 +4689,7 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, cleanup: VIR_FREE(pool); VIR_FREE(volume); + VIR_FREE(mode); return ret; } @@ -14157,9 +14173,13 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, case VIR_DOMAIN_DISK_TYPE_VOLUME: virBufferAddLit(buf, " <source"); - if (def->srcpool) + if (def->srcpool) { virBufferAsprintf(buf, " pool='%s' volume='%s'", def->srcpool->pool, def->srcpool->volume); + if (def->srcpool->mode) + virBufferAsprintf(buf, " mode='%s'", + virDomainDiskSourcePoolModeTypeToString(def->srcpool->mode)); + } if (def->startupPolicy) virBufferEscapeString(buf, " startupPolicy='%s'", startupPolicy); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 25dad16..982a94e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -652,11 +652,32 @@ struct _virDomainBlockIoTuneInfo { }; typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr; +/* + * Used for volume "type" disk to indicate how to represent + * the disk source if the specified "pool" is of iscsi type. + */ +enum virDomainDiskSourcePoolMode { + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DEFAULT = 0, + + /* Use the path as it shows up on host, e.g. + * /dev/disk/by-path/ip-$ip-iscsi-$iqn:iscsi.iscsi-pool0-lun-1 + */ + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST, + + /* Use the URI from the storage pool source element host attribute. E.g. + * file=iscsi://demo.org:6000/iqn.1992-01.com.example/1. + */ + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT, + + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_LAST +}; + typedef struct _virDomainDiskSourcePoolDef virDomainDiskSourcePoolDef; struct _virDomainDiskSourcePoolDef { char *pool; /* pool name */ char *volume; /* volume name */ int voltype; /* enum virStorageVolType, internal only */ + int mode; /* enum virDomainDiskSourcePoolMode */ }; typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr; @@ -2554,6 +2575,7 @@ VIR_ENUM_DECL(virDomainDiskSecretType) VIR_ENUM_DECL(virDomainDeviceSGIO) VIR_ENUM_DECL(virDomainDiskTray) VIR_ENUM_DECL(virDomainDiskDiscard) +VIR_ENUM_DECL(virDomainDiskSourcePoolMode) VIR_ENUM_DECL(virDomainIoEventFd) VIR_ENUM_DECL(virDomainVirtioEventIdx) VIR_ENUM_DECL(virDomainDiskCopyOnRead) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml new file mode 100644 index 0000000..b907633 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml @@ -0,0 +1,48 @@ +<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='volume' device='cdrom'> + <source pool='blk-pool0' volume='blk-pool0-vol0' mode='host' startupPolicy='optional'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> + <target dev='hda' bus='ide'/> + <readonly/> + <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'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> + <target dev='hda' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='0' target='0' unit='2'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/idedisk.img'/> + <target dev='hdc' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='3'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='ide' index='1'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 76570c5..77cac3f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -264,6 +264,7 @@ mymain(void) DO_TEST("disk-scsi-disk-vpd"); DO_TEST("disk-source-pool"); + DO_TEST("disk-source-pool-mode"); DO_TEST("disk-drive-discard"); -- 1.8.1.4
On Fri, Jul 19, 2013 at 08:32:28AM -0400, John Ferlan wrote:
There are two ways to use a iSCSI LUN as disk source for qemu.
* The LUN's path as it shows up on host, e.g. /dev/disk/by-path/ip-$ip:3260-iscsi-$iqn-fc18:iscsi.iscsi0-lun-1
* The libiscsi URI from the storage pool source element host attribute, e.g. iscsi://demo.org:6000/iqn.1992-01.com.example/1
For a "volume" type disk, if the specified "pool" is of iscsi type, we should support to use the LUN in either of above 2 ways. That's why to introduce a new XML tag "mode" for the disk source (libvirt should support iscsi pool with libiscsi, but it's another new feature, which should be done later).
The "mode" can be either of "host" or "direct". Use "host" to indicate use of the LUN with the path as it shows up on host. Use "direct" to indicate to use it with the source pool host URI (future patches may support to use network type libvirt storage too, e.g. Ceph) --- docs/formatdomain.html.in | 11 ++++- docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 22 +++++++++- src/conf/domain_conf.h | 22 ++++++++++ .../qemuxml2argv-disk-source-pool-mode.xml | 48 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 110 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
ACK 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 :|
Il 19/07/2013 14:32, John Ferlan ha scritto:
There are two ways to use a iSCSI LUN as disk source for qemu.
* The LUN's path as it shows up on host, e.g. /dev/disk/by-path/ip-$ip:3260-iscsi-$iqn-fc18:iscsi.iscsi0-lun-1
* The libiscsi URI from the storage pool source element host attribute, e.g. iscsi://demo.org:6000/iqn.1992-01.com.example/1
For a "volume" type disk, if the specified "pool" is of iscsi type, we should support to use the LUN in either of above 2 ways. That's why to introduce a new XML tag "mode" for the disk source (libvirt should support iscsi pool with libiscsi, but it's another new feature, which should be done later).
The "mode" can be either of "host" or "direct". Use "host" to indicate use of the LUN with the path as it shows up on host. Use "direct" to indicate to use it with the source pool host URI (future patches may support to use network type libvirt storage too, e.g. Ceph)
What is the default? Should 'direct' be valid for non-network pools? Paolo
--- docs/formatdomain.html.in | 11 ++++- docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 22 +++++++++- src/conf/domain_conf.h | 22 ++++++++++ .../qemuxml2argv-disk-source-pool-mode.xml | 48 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 110 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index b1c3bfc..7601aaa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1601,7 +1601,16 @@ <code>pool</code> and <code>volume</code>. Attribute <code>pool</code> specifies the name of storage pool (managed by libvirt) where the disk source resides, and attribute <code>volume</code> specifies the name of - storage volume (managed by libvirt) used as the disk source. + storage volume (managed by libvirt) used as the disk source. For a + "volume" type disk, if the underlying storage pool is "iscsi", attribute + <code>mode</code> (<span class="since">since 1.1.1</span>) can be used + to indicate how to represent the LUN as the disk source. The value + "host" indicates to use the LUN's path as it shows up on host, e.g. + /dev/disk/by-path/ip-10.11.12.9:3260-iscsi-iqn.2013-06.fc:iscsi.iscsi0-lun-1). + The value "direct" indicates to use the storage pool's + <code>source</code> element <code>host</code> attribute as the + disk source for the libiscsi URI, e.g. + file=iscsi://demo.org:6000/iqn.1992-01.com.example/1. <span class="since">Since 0.0.3; <code>type='dir'</code> since 0.7.5; <code>type='network'</code> since 0.8.7; <code>protocol='iscsi'</code> since 1.0.4; diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7a6852b..745b959 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1174,6 +1174,14 @@ <ref name="volName"/> </attribute> <optional> + <attribute name="mode"> + <choice> + <value>host</value> + <value>direct</value> + </choice> + </attribute> + </optional> + <optional> <ref name="startupPolicy"/> </optional> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 57cd9b1..419958f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -761,6 +761,11 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST, "default", "unmap", "ignore") +VIR_ENUM_IMPL(virDomainDiskSourcePoolMode, + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_LAST, + "default", + "host", + "direct")
#define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE @@ -4645,10 +4650,12 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, { char *pool = NULL; char *volume = NULL; + char *mode = NULL; int ret = -1;
pool = virXMLPropString(node, "pool"); volume = virXMLPropString(node, "volume"); + mode = virXMLPropString(node, "mode");
/* CD-ROM and Floppy allows no source */ if (!pool && !volume) @@ -4664,6 +4671,14 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, if (VIR_ALLOC(def->srcpool) < 0) goto cleanup;
+ if (mode && (def->srcpool->mode = + virDomainDiskSourcePoolModeTypeFromString(mode)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown source mode '%s' for volume type disk"), + mode); + goto cleanup; + } + def->srcpool->pool = pool; pool = NULL; def->srcpool->volume = volume; @@ -4674,6 +4689,7 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, cleanup: VIR_FREE(pool); VIR_FREE(volume); + VIR_FREE(mode); return ret; }
@@ -14157,9 +14173,13 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, case VIR_DOMAIN_DISK_TYPE_VOLUME: virBufferAddLit(buf, " <source");
- if (def->srcpool) + if (def->srcpool) { virBufferAsprintf(buf, " pool='%s' volume='%s'", def->srcpool->pool, def->srcpool->volume); + if (def->srcpool->mode) + virBufferAsprintf(buf, " mode='%s'", + virDomainDiskSourcePoolModeTypeToString(def->srcpool->mode)); + } if (def->startupPolicy) virBufferEscapeString(buf, " startupPolicy='%s'", startupPolicy);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 25dad16..982a94e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -652,11 +652,32 @@ struct _virDomainBlockIoTuneInfo { }; typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr;
+/* + * Used for volume "type" disk to indicate how to represent + * the disk source if the specified "pool" is of iscsi type. + */ +enum virDomainDiskSourcePoolMode { + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DEFAULT = 0, + + /* Use the path as it shows up on host, e.g. + * /dev/disk/by-path/ip-$ip-iscsi-$iqn:iscsi.iscsi-pool0-lun-1 + */ + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST, + + /* Use the URI from the storage pool source element host attribute. E.g. + * file=iscsi://demo.org:6000/iqn.1992-01.com.example/1. + */ + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT, + + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_LAST +}; + typedef struct _virDomainDiskSourcePoolDef virDomainDiskSourcePoolDef; struct _virDomainDiskSourcePoolDef { char *pool; /* pool name */ char *volume; /* volume name */ int voltype; /* enum virStorageVolType, internal only */ + int mode; /* enum virDomainDiskSourcePoolMode */ }; typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr;
@@ -2554,6 +2575,7 @@ VIR_ENUM_DECL(virDomainDiskSecretType) VIR_ENUM_DECL(virDomainDeviceSGIO) VIR_ENUM_DECL(virDomainDiskTray) VIR_ENUM_DECL(virDomainDiskDiscard) +VIR_ENUM_DECL(virDomainDiskSourcePoolMode) VIR_ENUM_DECL(virDomainIoEventFd) VIR_ENUM_DECL(virDomainVirtioEventIdx) VIR_ENUM_DECL(virDomainDiskCopyOnRead) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml new file mode 100644 index 0000000..b907633 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml @@ -0,0 +1,48 @@ +<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='volume' device='cdrom'> + <source pool='blk-pool0' volume='blk-pool0-vol0' mode='host' startupPolicy='optional'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> + <target dev='hda' bus='ide'/> + <readonly/> + <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'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> + <target dev='hda' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='0' target='0' unit='2'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/idedisk.img'/> + <target dev='hdc' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='3'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='ide' index='1'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 76570c5..77cac3f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -264,6 +264,7 @@ mymain(void)
DO_TEST("disk-scsi-disk-vpd"); DO_TEST("disk-source-pool"); + DO_TEST("disk-source-pool-mode");
DO_TEST("disk-drive-discard");
Introduce a new helper to check if the disk source is of block type --- src/conf/domain_conf.c | 32 ++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 5 ++--- src/qemu/qemu_conf.c | 23 +++++------------------ 5 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 419958f..dddd55d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -41,6 +41,7 @@ #include "virbuffer.h" #include "virlog.h" #include "nwfilter_conf.h" +#include "storage_conf.h" #include "virstoragefile.h" #include "virfile.h" #include "virbitmap.h" @@ -18375,3 +18376,34 @@ virDomainDefFindDevice(virDomainDefPtr def, return 0; } + +/** + * virDomainDiskSourceIsBlockType: + * + * Check if the disk *source* is of block type. This just tries + * to check from the type of disk def, not to probe the underlying + * storage. + * + * Return true if its source is block type, or false otherwise. + */ +bool +virDomainDiskSourceIsBlockType(virDomainDiskDefPtr def) +{ + /* No reason to think the disk source is block type if + * the source is empty + */ + if (!def->src) + return false; + + if (def->type == VIR_DOMAIN_DISK_TYPE_BLOCK) + return true; + + /* For volume types, check the srcpool. + * If it's a block type source pool, then it's possible + */ + if (def->type == VIR_DOMAIN_DISK_TYPE_VOLUME && def->srcpool && + def->srcpool->voltype == VIR_STORAGE_VOL_BLOCK) { + return true; + } + return false; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 982a94e..4db981d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2714,4 +2714,7 @@ int virDomainDefFindDevice(virDomainDefPtr def, const char *devAlias, virDomainDeviceDefPtr dev); +bool virDomainDiskSourceIsBlockType(virDomainDiskDefPtr def) + ATTRIBUTE_NONNULL(1); + #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7790ede..f13a8e2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -206,6 +206,7 @@ virDomainDiskProtocolTransportTypeToString; virDomainDiskProtocolTypeToString; virDomainDiskRemove; virDomainDiskRemoveByName; +virDomainDiskSourceIsBlockType; virDomainDiskTypeFromString; virDomainDiskTypeToString; virDomainEmulatorPinAdd; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8c58a7c..5265f77 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -42,6 +42,7 @@ #include "domain_audit.h" #include "domain_conf.h" #include "snapshot_conf.h" +#include "storage_conf.h" #include "network/bridge_driver.h" #include "virnetdevtap.h" #include "base64.h" @@ -3500,9 +3501,7 @@ qemuBuildDriveDevStr(virDomainDefPtr def, virDomainDiskProtocolTypeToString(disk->protocol)); goto error; } - } else if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && - !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && - disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK)) { + } else if (!virDomainDiskSourceIsBlockType(disk)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("disk device='lun' is only valid for block type disk source")); goto error; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 64214b9..332b9f2 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -52,6 +52,7 @@ #include "virfile.h" #include "virstring.h" #include "viratomic.h" +#include "storage_conf.h" #include "configmake.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -867,12 +868,7 @@ qemuAddSharedDevice(virQEMUDriverPtr driver, if (dev->type == VIR_DOMAIN_DEVICE_DISK) { disk = dev->data.disk; - if (!disk->shared || - !disk->src || - (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && - !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && - disk->srcpool && - disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) + if (!disk->shared || !virDomainDiskSourceIsBlockType(disk)) return 0; } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { hostdev = dev->data.hostdev; @@ -978,12 +974,7 @@ qemuRemoveSharedDevice(virQEMUDriverPtr driver, if (dev->type == VIR_DOMAIN_DEVICE_DISK) { disk = dev->data.disk; - if (!disk->shared || - !disk->src || - (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && - !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && - disk->srcpool && - disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) + if (!disk->shared || !virDomainDiskSourceIsBlockType(disk)) return 0; } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { hostdev = dev->data.hostdev; @@ -1073,12 +1064,8 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) if (dev->type == VIR_DOMAIN_DEVICE_DISK) { disk = dev->data.disk; - if (!disk->src || - disk->device != VIR_DOMAIN_DISK_DEVICE_LUN || - (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && - !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && - disk->srcpool && - disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) + if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN || + virDomainDiskSourceIsBlockType(disk)) return 0; path = disk->src; -- 1.8.1.4
On Fri, Jul 19, 2013 at 08:32:29AM -0400, John Ferlan wrote:
Introduce a new helper to check if the disk source is of block type --- src/conf/domain_conf.c | 32 ++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 5 ++--- src/qemu/qemu_conf.c | 23 +++++------------------ 5 files changed, 43 insertions(+), 21 deletions(-)
ACK 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 :|
The difference with already supported pool types (dir, fs, block) is: there are two modes for iscsi pool (or network pools in future), one can specify it either to use the volume target path (the path showed up on host) with mode='host', or to use the remote URI qemu supports (e.g. file=iscsi://example.org:6000/iqn.1992-01.com.example/1) with mode='direct'. For 'host' mode, it copies the volume target path into disk->src. For 'direct' mode, the corresponding info in the *one* pool source host def is copied to disk->hosts[0]. --- src/conf/domain_conf.c | 8 ++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 15 ++++++- src/qemu/qemu_conf.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 123 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dddd55d..231edb5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18403,6 +18403,14 @@ virDomainDiskSourceIsBlockType(virDomainDiskDefPtr def) */ if (def->type == VIR_DOMAIN_DISK_TYPE_VOLUME && def->srcpool && def->srcpool->voltype == VIR_STORAGE_VOL_BLOCK) { + /* We don't think the volume accessed by remote URI is + * block type source, since we can't/shouldn't manage it + * (e.g. set sgio=filtered|unfiltered for it) in libvirt. + */ + if (def->srcpool->pooltype == VIR_STORAGE_POOL_ISCSI && + def->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) + return false; + return true; } return false; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4db981d..327f3f9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -677,6 +677,7 @@ struct _virDomainDiskSourcePoolDef { char *pool; /* pool name */ char *volume; /* volume name */ int voltype; /* enum virStorageVolType, internal only */ + int pooltype; /* enum virStoragePoolType, internal only */ int mode; /* enum virDomainDiskSourcePoolMode */ }; typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5265f77..4e4ee9e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3214,7 +3214,20 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, "block type volume")); goto error; } - virBufferEscape(&opt, ',', ",", "file=%s,", disk->src); + + 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 error; + 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: virBufferEscape(&opt, ',', ",", "file=%s,", disk->src); diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 332b9f2..6e6163f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1125,12 +1125,75 @@ int qemuDriverAllocateID(virQEMUDriverPtr driver) return virAtomicIntInc(&driver->nextvmid); } +static int +qemuAddISCSIPoolSourceHost(virDomainDiskDefPtr def, + virStoragePoolDefPtr pooldef) +{ + int ret = -1; + char **tokens = NULL; + + /* Only support one host */ + if (pooldef->source.nhost != 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Expected exactly 1 host for the storage pool")); + goto cleanup; + } + + /* iscsi pool only supports one host */ + def->nhosts = 1; + + if (VIR_ALLOC_N(def->hosts, def->nhosts) < 0) + goto cleanup; + + if (VIR_STRDUP(def->hosts[0].name, pooldef->source.hosts[0].name) < 0) + goto cleanup; + + if (virAsprintf(&def->hosts[0].port, "%d", + pooldef->source.hosts[0].port ? + pooldef->source.hosts[0].port : + 3260) < 0) + goto cleanup; + + /* iscsi volume has name like "unit:0:0:1" */ + if (!(tokens = virStringSplit(def->srcpool->volume, ":", 0))) + goto cleanup; + + if (virStringListLength(tokens) != 4) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected iscsi volume name '%s'"), + def->srcpool->volume); + goto cleanup; + } + + /* iscsi pool has only one source device path */ + if (virAsprintf(&def->src, "%s/%s", + pooldef->source.devices[0].path, + tokens[3]) < 0) + goto cleanup; + + /* Storage pool have not supported these 2 attributes yet, + * use the defaults. + */ + def->hosts[0].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; + def->hosts[0].socket = NULL; + + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_ISCSI; + + ret = 0; + +cleanup: + virStringFreeList(tokens); + return ret; +} + int qemuTranslateDiskSourcePool(virConnectPtr conn, virDomainDiskDefPtr def) { + virStoragePoolDefPtr pooldef = NULL; virStoragePoolPtr pool = NULL; virStorageVolPtr vol = NULL; + char *poolxml = NULL; virStorageVolInfo info; int ret = -1; @@ -1158,11 +1221,45 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, switch (info.type) { case VIR_STORAGE_VOL_FILE: - case VIR_STORAGE_VOL_BLOCK: case VIR_STORAGE_VOL_DIR: 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")); + 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; + } + } else { + if (!(def->src = virStorageVolGetPath(vol))) + goto cleanup; + } + + break; case VIR_STORAGE_VOL_NETWORK: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Using network volume as disk source is not supported")); @@ -1174,5 +1271,7 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, cleanup: virStoragePoolFree(pool); virStorageVolFree(vol); + VIR_FREE(poolxml); + virStoragePoolDefFree(pooldef); return ret; } -- 1.8.1.4
On 19/07/13 20:32, John Ferlan wrote:
The difference with already supported pool types (dir, fs, block) is: there are two modes for iscsi pool (or network pools in future), one can specify it either to use the volume target path (the path showed up on host) with mode='host', or to use the remote URI qemu supports (e.g. file=iscsi://example.org:6000/iqn.1992-01.com.example/1) with mode='direct'.
For 'host' mode, it copies the volume target path into disk->src. For 'direct' mode, the corresponding info in the *one* pool source host def is copied to disk->hosts[0]. --- src/conf/domain_conf.c | 8 ++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 15 ++++++- src/qemu/qemu_conf.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 123 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dddd55d..231edb5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18403,6 +18403,14 @@ virDomainDiskSourceIsBlockType(virDomainDiskDefPtr def) */ if (def->type == VIR_DOMAIN_DISK_TYPE_VOLUME && def->srcpool && def->srcpool->voltype == VIR_STORAGE_VOL_BLOCK) { + /* We don't think the volume accessed by remote URI is + * block type source, since we can't/shouldn't manage it + * (e.g. set sgio=filtered|unfiltered for it) in libvirt. + */ + if (def->srcpool->pooltype == VIR_STORAGE_POOL_ISCSI && + def->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) + return false; + return true; } return false; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4db981d..327f3f9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -677,6 +677,7 @@ struct _virDomainDiskSourcePoolDef { char *pool; /* pool name */ char *volume; /* volume name */ int voltype; /* enum virStorageVolType, internal only */ + int pooltype; /* enum virStoragePoolType, internal only */ int mode; /* enum virDomainDiskSourcePoolMode */ }; typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5265f77..4e4ee9e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3214,7 +3214,20 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, "block type volume")); goto error; } - virBufferEscape(&opt, ',', ",", "file=%s,", disk->src); + + 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 error; + 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: virBufferEscape(&opt, ',', ",", "file=%s,", disk->src); diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 332b9f2..6e6163f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1125,12 +1125,75 @@ int qemuDriverAllocateID(virQEMUDriverPtr driver) return virAtomicIntInc(&driver->nextvmid); }
+static int +qemuAddISCSIPoolSourceHost(virDomainDiskDefPtr def, + virStoragePoolDefPtr pooldef)
+{ + int ret = -1; + char **tokens = NULL; + + /* Only support one host */ + if (pooldef->source.nhost != 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Expected exactly 1 host for the storage pool")); + goto cleanup; + } + + /* iscsi pool only supports one host */ + def->nhosts = 1; + + if (VIR_ALLOC_N(def->hosts, def->nhosts) < 0) + goto cleanup; + + if (VIR_STRDUP(def->hosts[0].name, pooldef->source.hosts[0].name) < 0) + goto cleanup; + + if (virAsprintf(&def->hosts[0].port, "%d", + pooldef->source.hosts[0].port ? + pooldef->source.hosts[0].port : + 3260) < 0) + goto cleanup; + + /* iscsi volume has name like "unit:0:0:1" */ + if (!(tokens = virStringSplit(def->srcpool->volume, ":", 0))) + goto cleanup; + + if (virStringListLength(tokens) != 4) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected iscsi volume name '%s'"), + def->srcpool->volume); + goto cleanup; + } + + /* iscsi pool has only one source device path */ + if (virAsprintf(&def->src, "%s/%s", + pooldef->source.devices[0].path, + tokens[3]) < 0) + goto cleanup; + + /* Storage pool have not supported these 2 attributes yet, + * use the defaults. + */ + def->hosts[0].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; + def->hosts[0].socket = NULL; + + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_ISCSI; + + ret = 0; + +cleanup: + virStringFreeList(tokens); + return ret; +} + int qemuTranslateDiskSourcePool(virConnectPtr conn, virDomainDiskDefPtr def) { + virStoragePoolDefPtr pooldef = NULL; virStoragePoolPtr pool = NULL; virStorageVolPtr vol = NULL; + char *poolxml = NULL; virStorageVolInfo info; int ret = -1;
@@ -1158,11 +1221,45 @@ qemuTranslateDiskSourcePool(virConnectPtr conn,
switch (info.type) { case VIR_STORAGE_VOL_FILE: - case VIR_STORAGE_VOL_BLOCK: case VIR_STORAGE_VOL_DIR: 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")); + 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; + } + } else { + if (!(def->src = virStorageVolGetPath(vol))) + goto cleanup; + } + + break; case VIR_STORAGE_VOL_NETWORK: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Using network volume as disk source is not supported")); @@ -1174,5 +1271,7 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, cleanup: virStoragePoolFree(pool); virStorageVolFree(vol); + VIR_FREE(poolxml); + virStoragePoolDefFree(pooldef); return ret; }
nice abstraction. it's good other network pools' support in future. though i'm the original author, but it's long time and there are changes. ACK.
Il 22/07/2013 12:30, Osier Yang ha scritto:
+ 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; + }
Ok, this answers my question. :) I think the default mode should be direct, because otherwise things such as persistent reservations do not work. Paolo
On Tue, Jul 23, 2013 at 03:22:54PM +0200, Paolo Bonzini wrote:
Il 22/07/2013 12:30, Osier Yang ha scritto:
+ 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; + }
Ok, this answers my question. :)
I think the default mode should be direct, because otherwise things such as persistent reservations do not work.
No, the default has to be host mode, because that is the only mode that is guaranteed to be usable with any QEMU. The direct mode requires a QEMU that is new enough, and a distro to have enabled it. We can't rely on that as a default choice. 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 :|
Il 23/07/2013 15:26, Daniel P. Berrange ha scritto:
Ok, this answers my question. :)
I think the default mode should be direct, because otherwise things such as persistent reservations do not work.
No, the default has to be host mode, because that is the only mode that is guaranteed to be usable with any QEMU. The direct mode requires a QEMU that is new enough, and a distro to have enabled it. We can't rely on that as a default choice.
Volume sources are also new enough that you can assume a good QEMU. Host mode for iSCSI is broken. It also doesn't reconnect well if you have a network problem, because after a LUN rescan the inode may change. Paolo
On Tue, Jul 23, 2013 at 03:29:40PM +0200, Paolo Bonzini wrote:
Il 23/07/2013 15:26, Daniel P. Berrange ha scritto:
Ok, this answers my question. :)
I think the default mode should be direct, because otherwise things such as persistent reservations do not work.
No, the default has to be host mode, because that is the only mode that is guaranteed to be usable with any QEMU. The direct mode requires a QEMU that is new enough, and a distro to have enabled it. We can't rely on that as a default choice.
Volume sources are also new enough that you can assume a good QEMU.
No we can't assume that. New libvirt is frequently used with old QEMU and we want to have good default behaviour there.
Host mode for iSCSI is broken. It also doesn't reconnect well if you have a network problem, because after a LUN rescan the inode may change.
That's a much smaller level of brokeness, than if QEMU doesn't support the iscsi block protocol & thus the default configuration won't even boot. 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 :|
Il 23/07/2013 15:36, Daniel P. Berrange ha scritto:
On Tue, Jul 23, 2013 at 03:29:40PM +0200, Paolo Bonzini wrote:
Il 23/07/2013 15:26, Daniel P. Berrange ha scritto:
Ok, this answers my question. :)
I think the default mode should be direct, because otherwise things such as persistent reservations do not work.
No, the default has to be host mode, because that is the only mode that is guaranteed to be usable with any QEMU. The direct mode requires a QEMU that is new enough, and a distro to have enabled it. We can't rely on that as a default choice.
Volume sources are also new enough that you can assume a good QEMU.
No we can't assume that. New libvirt is frequently used with old QEMU and we want to have good default behaviour there.
New libvirt, yes. But can't new libvirt features also assume a new QEMU if the old one causes more trouble than anything?
Host mode for iSCSI is broken. It also doesn't reconnect well if you have a network problem, because after a LUN rescan the inode may change.
That's a much smaller level of brokeness, than if QEMU doesn't support the iscsi block protocol & thus the default configuration won't even boot.
Failure to reconnect is not part of the brokenness. :) If you use host mode and the guest issues reservation commands, you may get data corruption. That is the brokenness. Another difference is that direct mode doesn't require the pool to be started before starting the domain. For authenticated iSCSI LUNs, this is better because you cannot autostart authenticated iSCSI pools. Perhaps the default could be specified in a configuration file (and the default should be the safe one). Paolo
On Tue, Jul 23, 2013 at 03:52:56PM +0200, Paolo Bonzini wrote:
Il 23/07/2013 15:36, Daniel P. Berrange ha scritto:
On Tue, Jul 23, 2013 at 03:29:40PM +0200, Paolo Bonzini wrote:
Il 23/07/2013 15:26, Daniel P. Berrange ha scritto:
Ok, this answers my question. :)
I think the default mode should be direct, because otherwise things such as persistent reservations do not work.
No, the default has to be host mode, because that is the only mode that is guaranteed to be usable with any QEMU. The direct mode requires a QEMU that is new enough, and a distro to have enabled it. We can't rely on that as a default choice.
Volume sources are also new enough that you can assume a good QEMU.
No we can't assume that. New libvirt is frequently used with old QEMU and we want to have good default behaviour there.
New libvirt, yes. But can't new libvirt features also assume a new QEMU if the old one causes more trouble than anything?
I've no problem with new features requiring new QEMU but that's not the issue here. This is a question about what the default configuration should be. For default configuration we aim for maximum compatibility not the latest possible features.
Host mode for iSCSI is broken. It also doesn't reconnect well if you have a network problem, because after a LUN rescan the inode may change.
That's a much smaller level of brokeness, than if QEMU doesn't support the iscsi block protocol & thus the default configuration won't even boot.
Failure to reconnect is not part of the brokenness. :)
If you use host mode and the guest issues reservation commands, you may get data corruption. That is the brokenness.
So be it, that's been the case for every version of libvirt and QEMU in existance up until iscsi protocol support was added, so it is not a new issue. That doesn't justify choosing a default that is guaranteed to not work at all.
Another difference is that direct mode doesn't require the pool to be started before starting the domain. For authenticated iSCSI LUNs, this is better because you cannot autostart authenticated iSCSI pools.
Autostarting of authenticated pools is simply a bug to be fixed.
Perhaps the default could be specified in a configuration file (and the default should be the safe one).
No, that is even worse because now the default is not predictable.. We simply default to host mode and if applications want to use the other mode they can configure the XML as desired. 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 :|
Il 23/07/2013 16:14, Daniel P. Berrange ha scritto:
On Tue, Jul 23, 2013 at 03:52:56PM +0200, Paolo Bonzini wrote:
Il 23/07/2013 15:36, Daniel P. Berrange ha scritto:
On Tue, Jul 23, 2013 at 03:29:40PM +0200, Paolo Bonzini wrote:
Il 23/07/2013 15:26, Daniel P. Berrange ha scritto:
> > Ok, this answers my question. :) > > I think the default mode should be direct, because otherwise things such > as persistent reservations do not work. No, the default has to be host mode, because that is the only mode that is guaranteed to be usable with any QEMU. The direct mode requires a QEMU that is new enough, and a distro to have enabled it. We can't rely on that as a default choice.
Volume sources are also new enough that you can assume a good QEMU.
No we can't assume that. New libvirt is frequently used with old QEMU and we want to have good default behaviour there.
New libvirt, yes. But can't new libvirt features also assume a new QEMU if the old one causes more trouble than anything?
I've no problem with new features requiring new QEMU but that's not the issue here. This is a question about what the default configuration should be. For default configuration we aim for maximum compatibility not the latest possible features.
Shouldn't default configuration be simply "what makes the most sense"?
If you use host mode and the guest issues reservation commands, you may get data corruption. That is the brokenness.
So be it, that's been the case for every version of libvirt and QEMU in existance up until iscsi protocol support was added, so it is not a new issue. That doesn't justify choosing a default that is guaranteed to not work at all.
I started adding iSCSI support to ensure that iSCSI LUNs would work. This makes them _not_ work by default. It makes me regret asking to add support for volumes as a disk source. Paolo
Il 23/07/2013 16:14, Daniel P. Berrange ha scritto:
Perhaps the default could be specified in a configuration file (and the default should be the safe one). No, that is even worse because now the default is not predictable..
We simply default to host mode and if applications want to use the other mode they can configure the XML as desired.
Can we just forbid mode='default' for iSCSI and force the user to specify host vs. direct? At least the error message will be nice and independent of the QEMU version. Paolo
On Tue, Jul 23, 2013 at 05:35:57PM +0200, Paolo Bonzini wrote:
Il 23/07/2013 16:14, Daniel P. Berrange ha scritto:
Perhaps the default could be specified in a configuration file (and the default should be the safe one). No, that is even worse because now the default is not predictable..
We simply default to host mode and if applications want to use the other mode they can configure the XML as desired.
Can we just forbid mode='default' for iSCSI and force the user to specify host vs. direct?
That would mean that apps cannot simply configure a guest volume without first checking to find out what type of pool it is, and then specifying this extra arg for iSCSI. IMHO the value of the <volume> XML is that you don't have to know anything about the pool to be able to configure it - we're completely decoupled. 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 :|
Il 23/07/2013 18:01, Daniel P. Berrange ha scritto:
On Tue, Jul 23, 2013 at 05:35:57PM +0200, Paolo Bonzini wrote:
Il 23/07/2013 16:14, Daniel P. Berrange ha scritto:
Perhaps the default could be specified in a configuration file (and the default should be the safe one). No, that is even worse because now the default is not predictable..
We simply default to host mode and if applications want to use the other mode they can configure the XML as desired.
Can we just forbid mode='default' for iSCSI and force the user to specify host vs. direct?
That would mean that apps cannot simply configure a guest volume without first checking to find out what type of pool it is, and then specifying this extra arg for iSCSI. IMHO the value of the <volume> XML is that you don't have to know anything about the pool to be able to configure it - we're completely decoupled.
Thinking more about it, it would only be needed for <disk type='volume' device='lun'>. And for that case, some knowledge of the pool is necessary anyway (for one thing, it won't work with filesystem or LVM pools). So if we could forbid mode='default' for that case only, it would be enough as far as I'm concernde. Paolo
On 07/23/2013 12:18 PM, Paolo Bonzini wrote:
Il 23/07/2013 18:01, Daniel P. Berrange ha scritto:
On Tue, Jul 23, 2013 at 05:35:57PM +0200, Paolo Bonzini wrote:
Il 23/07/2013 16:14, Daniel P. Berrange ha scritto:
Perhaps the default could be specified in a configuration file (and the default should be the safe one). No, that is even worse because now the default is not predictable..
We simply default to host mode and if applications want to use the other mode they can configure the XML as desired.
Can we just forbid mode='default' for iSCSI and force the user to specify host vs. direct?
That would mean that apps cannot simply configure a guest volume without first checking to find out what type of pool it is, and then specifying this extra arg for iSCSI. IMHO the value of the <volume> XML is that you don't have to know anything about the pool to be able to configure it - we're completely decoupled.
Thinking more about it, it would only be needed for <disk type='volume' device='lun'>. And for that case, some knowledge of the pool is necessary anyway (for one thing, it won't work with filesystem or LVM pools). So if we could forbid mode='default' for that case only, it would be enough as far as I'm concernde.
Paolo
Using "default" in the mode field would result in the following XML error message (I just quickly changed a test to prove the point): 121) QEMU XML-2-XML disk-source-pool-mode ... libvirt: Domain Config error : XML error: unknown source mode 'default' for volume type disk FAILED The XML parsing code only looks for "mode='direct'" or "mode='host'". If "mode" isn't present in the XML, that's when that default comes into play. Since 'mode' is new there could be configurations where its not in an XML file, thus a 0 (zero e.g. "default") value is provided for the field. Once the XML is parsed we still needed a default when it's going to be added, thus the "magic" to set the default to HOST is in qemu_conf.c in qemuTranslateDiskSourcePool(): 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; I think this answers your primary concern - correct? John
Il 23/07/2013 18:47, John Ferlan ha scritto:
On 07/23/2013 12:18 PM, Paolo Bonzini wrote:
Il 23/07/2013 18:01, Daniel P. Berrange ha scritto:
On Tue, Jul 23, 2013 at 05:35:57PM +0200, Paolo Bonzini wrote:
Il 23/07/2013 16:14, Daniel P. Berrange ha scritto:
> Perhaps the default could be specified in a configuration file (and the > default should be the safe one). No, that is even worse because now the default is not predictable..
We simply default to host mode and if applications want to use the other mode they can configure the XML as desired.
Can we just forbid mode='default' for iSCSI and force the user to specify host vs. direct?
That would mean that apps cannot simply configure a guest volume without first checking to find out what type of pool it is, and then specifying this extra arg for iSCSI. IMHO the value of the <volume> XML is that you don't have to know anything about the pool to be able to configure it - we're completely decoupled.
Thinking more about it, it would only be needed for <disk type='volume' device='lun'>. And for that case, some knowledge of the pool is necessary anyway (for one thing, it won't work with filesystem or LVM pools). So if we could forbid mode='default' for that case only, it would be enough as far as I'm concernde.
Using "default" in the mode field would result in the following XML error message (I just quickly changed a test to prove the point):
121) QEMU XML-2-XML disk-source-pool-mode ... libvirt: Domain Config error : XML error: unknown source mode 'default' for volume type disk FAILED
Sorry, by mode='default' I really meant "no mode at all" (I was under the false impression that you could also specify a mode='default' with the same effect as no mode at all).
The XML parsing code only looks for "mode='direct'" or "mode='host'". If "mode" isn't present in the XML, that's when that default comes into play. Since 'mode' is new there could be configurations where its not in an XML file, thus a 0 (zero e.g. "default") value is provided for the field.
Once the XML is parsed we still needed a default when it's going to be added, thus the "magic" to set the default to HOST is in qemu_conf.c in qemuTranslateDiskSourcePool():
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;
I think this answers your primary concern - correct?
No, my concern is that mode='host' is a bad default for the device='lun' case. Paolo
On 24/07/13 01:11, Paolo Bonzini wrote:
On 07/23/2013 12:18 PM, Paolo Bonzini wrote:
Il 23/07/2013 16:14, Daniel P. Berrange ha scritto:
>> Perhaps the default could be specified in a configuration file (and the >> default should be the safe one). No, that is even worse because now the default is not predictable..
We simply default to host mode and if applications want to use the other mode they can configure the XML as desired. Can we just forbid mode='default' for iSCSI and force the user to specify host vs. direct? That would mean that apps cannot simply configure a guest volume without first checking to find out what type of pool it is, and
On Tue, Jul 23, 2013 at 05:35:57PM +0200, Paolo Bonzini wrote: then specifying this extra arg for iSCSI. IMHO the value of the <volume> XML is that you don't have to know anything about the pool to be able to configure it - we're completely decoupled. Thinking more about it, it would only be needed for <disk type='volume' device='lun'>. And for that case, some knowledge of the pool is necessary anyway (for one thing, it won't work with filesystem or LVM
Il 23/07/2013 18:01, Daniel P. Berrange ha scritto: pools). So if we could forbid mode='default' for that case only, it would be enough as far as I'm concernde. Using "default" in the mode field would result in the following XML error message (I just quickly changed a test to prove the point):
121) QEMU XML-2-XML disk-source-pool-mode ... libvirt: Domain Config error : XML error: unknown source mode 'default' for volume type disk FAILED Sorry, by mode='default' I really meant "no mode at all" (I was under
Il 23/07/2013 18:47, John Ferlan ha scritto: the false impression that you could also specify a mode='default' with the same effect as no mode at all).
The XML parsing code only looks for "mode='direct'" or "mode='host'". If "mode" isn't present in the XML, that's when that default comes into play. Since 'mode' is new there could be configurations where its not in an XML file, thus a 0 (zero e.g. "default") value is provided for the field.
Once the XML is parsed we still needed a default when it's going to be added, thus the "magic" to set the default to HOST is in qemu_conf.c in qemuTranslateDiskSourcePool():
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;
I think this answers your primary concern - correct? No, my concern is that mode='host' is a bad default for the device='lun' case.
after going through the thread...... i think the point is the relationship between the "host" and "direct" mode. if users used the "host" mode in the past, but most of them suffered from the persistent reservatioins, and "direct" mode is the answer for them, then i indent to agree with paolo that defaulting to "direct" is better. if they used the "host" mode broadly, but persistent reservation is just requirement of a small part of the users. then i don't see any problem to default to "host" mode. i.e. the point is if "direct" mode is the answer for "host" mode. but anyway, we provide good enough documentations, so if one wants to use "direct" mode, he can still change the xml to use it, any big problems here? though defaulting to "direct" is more convenient for these users. osier
On Wed, Jul 24, 2013 at 11:12:18AM +0800, Osier Yang wrote:
On 24/07/13 01:11, Paolo Bonzini wrote:
On 07/23/2013 12:18 PM, Paolo Bonzini wrote:
Il 23/07/2013 16:14, Daniel P. Berrange ha scritto: >>>Perhaps the default could be specified in a configuration file (and the >>>default should be the safe one). >No, that is even worse because now the default is not predictable.. > >We simply default to host mode and if applications want to use the >other mode they can configure the XML as desired. Can we just forbid mode='default' for iSCSI and force the user to specify host vs. direct? That would mean that apps cannot simply configure a guest volume without first checking to find out what type of pool it is, and
On Tue, Jul 23, 2013 at 05:35:57PM +0200, Paolo Bonzini wrote: then specifying this extra arg for iSCSI. IMHO the value of the <volume> XML is that you don't have to know anything about the pool to be able to configure it - we're completely decoupled. Thinking more about it, it would only be needed for <disk type='volume' device='lun'>. And for that case, some knowledge of the pool is necessary anyway (for one thing, it won't work with filesystem or LVM
Il 23/07/2013 18:01, Daniel P. Berrange ha scritto: pools). So if we could forbid mode='default' for that case only, it would be enough as far as I'm concernde. Using "default" in the mode field would result in the following XML error message (I just quickly changed a test to prove the point):
121) QEMU XML-2-XML disk-source-pool-mode ... libvirt: Domain Config error : XML error: unknown source mode 'default' for volume type disk FAILED Sorry, by mode='default' I really meant "no mode at all" (I was under
Il 23/07/2013 18:47, John Ferlan ha scritto: the false impression that you could also specify a mode='default' with the same effect as no mode at all).
The XML parsing code only looks for "mode='direct'" or "mode='host'". If "mode" isn't present in the XML, that's when that default comes into play. Since 'mode' is new there could be configurations where its not in an XML file, thus a 0 (zero e.g. "default") value is provided for the field.
Once the XML is parsed we still needed a default when it's going to be added, thus the "magic" to set the default to HOST is in qemu_conf.c in qemuTranslateDiskSourcePool():
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;
I think this answers your primary concern - correct? No, my concern is that mode='host' is a bad default for the device='lun' case.
after going through the thread......
i think the point is the relationship between the "host" and "direct" mode.
if users used the "host" mode in the past, but most of them suffered from the persistent reservatioins, and "direct" mode is the answer for them, then i indent to agree with paolo that defaulting to "direct" is better.
if they used the "host" mode broadly, but persistent reservation is just requirement of a small part of the users. then i don't see any problem to default to "host" mode.
i.e. the point is if "direct" mode is the answer for "host" mode.
but anyway, we provide good enough documentations, so if one wants to use "direct" mode, he can still change the xml to use it, any big problems here? though defaulting to "direct" is more convenient for these users.
Defaulting to "direct" is *NOT* more convenient for all users, only those who happen to have a new enough QEMU, from a vendor who actually decided to compile in the iSCSI support. If they're lacking such a QEMU then the "direct" mode will prevent the default config from even starting. This can never be an acceptable default choice. mode="host", while it huas some limitations in features, is the only option that is capable of working with any QEMU 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 :|
Il 24/07/2013 11:54, Daniel P. Berrange ha scritto:
but anyway, we provide good enough documentations, so if one wants to use "direct" mode, he can still change the xml to use it, any big problems here? though defaulting to "direct" is more convenient for these users.
Defaulting to "direct" is *NOT* more convenient for all users, only those who happen to have a new enough QEMU, from a vendor who actually decided to compile in the iSCSI support. If they're lacking such a QEMU then the "direct" mode will prevent the default config from even starting. This can never be an acceptable default choice. mode="host", while it huas some limitations in features, is the only option that is capable of working with any QEMU
s/limitations in features/possibility of wreaking havoc to clusters/ Didn't you "break features that worked" when you started forbidding migration unless cache=none? This time there is even hardly a feature that worked, because the feature is new. Paolo
On Wed, Jul 24, 2013 at 12:25:17PM +0200, Paolo Bonzini wrote:
Il 24/07/2013 11:54, Daniel P. Berrange ha scritto:
but anyway, we provide good enough documentations, so if one wants to use "direct" mode, he can still change the xml to use it, any big problems here? though defaulting to "direct" is more convenient for these users.
Defaulting to "direct" is *NOT* more convenient for all users, only those who happen to have a new enough QEMU, from a vendor who actually decided to compile in the iSCSI support. If they're lacking such a QEMU then the "direct" mode will prevent the default config from even starting. This can never be an acceptable default choice. mode="host", while it huas some limitations in features, is the only option that is capable of working with any QEMU
s/limitations in features/possibility of wreaking havoc to clusters/
This isn't a new issue. People have been using KVM with iSCSI in host mode for as long as KVM has existed and the sky hasn't fallen in clusters. As long as it is a documented limitation, that admins can address with a config setting if desired, I don't see that this is suddenly something we must fix at the cost of breaking the default config for QEMU's without iSCSI support. 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 :|
Il 24/07/2013 12:31, Daniel P. Berrange ha scritto:
On Wed, Jul 24, 2013 at 12:25:17PM +0200, Paolo Bonzini wrote:
Il 24/07/2013 11:54, Daniel P. Berrange ha scritto:
but anyway, we provide good enough documentations, so if one wants to use "direct" mode, he can still change the xml to use it, any big problems here? though defaulting to "direct" is more convenient for these users.
Defaulting to "direct" is *NOT* more convenient for all users, only those who happen to have a new enough QEMU, from a vendor who actually decided to compile in the iSCSI support. If they're lacking such a QEMU then the "direct" mode will prevent the default config from even starting. This can never be an acceptable default choice. mode="host", while it huas some limitations in features, is the only option that is capable of working with any QEMU
s/limitations in features/possibility of wreaking havoc to clusters/
This isn't a new issue. People have been using KVM with iSCSI in host mode for as long as KVM has existed and the sky hasn't fallen in clusters. As long as it is a documented limitation, that admins can address with a config setting if desired, I don't see that this is suddenly something we must fix at the cost of breaking the default config for QEMU's without iSCSI support.
Understood, that's why I suggested not having a default config at all for type='lun' devices. Also, can libvirt at least detect using a volume on a pool that wasn't started, and fail unless mode='direct'? Paolo
On Wed, Jul 24, 2013 at 12:37:10PM +0200, Paolo Bonzini wrote:
Il 24/07/2013 12:31, Daniel P. Berrange ha scritto:
On Wed, Jul 24, 2013 at 12:25:17PM +0200, Paolo Bonzini wrote:
Il 24/07/2013 11:54, Daniel P. Berrange ha scritto:
but anyway, we provide good enough documentations, so if one wants to use "direct" mode, he can still change the xml to use it, any big problems here? though defaulting to "direct" is more convenient for these users.
Defaulting to "direct" is *NOT* more convenient for all users, only those who happen to have a new enough QEMU, from a vendor who actually decided to compile in the iSCSI support. If they're lacking such a QEMU then the "direct" mode will prevent the default config from even starting. This can never be an acceptable default choice. mode="host", while it huas some limitations in features, is the only option that is capable of working with any QEMU
s/limitations in features/possibility of wreaking havoc to clusters/
This isn't a new issue. People have been using KVM with iSCSI in host mode for as long as KVM has existed and the sky hasn't fallen in clusters. As long as it is a documented limitation, that admins can address with a config setting if desired, I don't see that this is suddenly something we must fix at the cost of breaking the default config for QEMU's without iSCSI support.
Understood, that's why I suggested not having a default config at all for type='lun' devices.
As I said before, that causes apps to have to do special handling for iSCSI pools, which is not something we want.
Also, can libvirt at least detect using a volume on a pool that wasn't started, and fail unless mode='direct'?
It should always fail to start the guest if the pool is not started, regardless of the mode used. 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 :|
Il 24/07/2013 12:39, Daniel P. Berrange ha scritto:
On Wed, Jul 24, 2013 at 12:37:10PM +0200, Paolo Bonzini wrote:
Il 24/07/2013 12:31, Daniel P. Berrange ha scritto:
breaking the default config for QEMU's without iSCSI support.
Understood, that's why I suggested not having a default config at all for type='lun' devices.
As I said before, that causes apps to have to do special handling for iSCSI pools, which is not something we want.
This is not about iSCSI pools, it is about type='lun' devices. And as I said before, knowing the kind of pool is something you have to do anyway for type='lun' devices, because they won't work if the underlying pool is filesystem- or LVM-based.
Also, can libvirt at least detect using a volume on a pool that wasn't started, and fail unless mode='direct'?
It should always fail to start the guest if the pool is not started, regardless of the mode used.
Another point of mode='direct' is that the pool need not be started. Paolo
On Wed, Jul 24, 2013 at 12:41:49PM +0200, Paolo Bonzini wrote:
Il 24/07/2013 12:39, Daniel P. Berrange ha scritto:
On Wed, Jul 24, 2013 at 12:37:10PM +0200, Paolo Bonzini wrote:
Il 24/07/2013 12:31, Daniel P. Berrange ha scritto:
breaking the default config for QEMU's without iSCSI support.
Understood, that's why I suggested not having a default config at all for type='lun' devices.
As I said before, that causes apps to have to do special handling for iSCSI pools, which is not something we want.
This is not about iSCSI pools, it is about type='lun' devices.
And as I said before, knowing the kind of pool is something you have to do anyway for type='lun' devices, because they won't work if the underlying pool is filesystem- or LVM-based.
Regardless of whether all pools types can use type=lun, I don't want to see special case handling of this attribute. If it is optional in the XML schema, it should be unconditionally optional for any usage.
Also, can libvirt at least detect using a volume on a pool that wasn't started, and fail unless mode='direct'?
It should always fail to start the guest if the pool is not started, regardless of the mode used.
Another point of mode='direct' is that the pool need not be started.
That is not something we can allow in the long term. When we expand our fine grained access control further, we're likely going to need to do ACL checks for the virStorageVolPtr object referenced by the XML config here. We can't do that unless the pool is running. So we cannot set a precedent of allowing use without the pool being active IMHO. 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 :|
Il 24/07/2013 12:46, Daniel P. Berrange ha scritto:
On Wed, Jul 24, 2013 at 12:41:49PM +0200, Paolo Bonzini wrote:
Il 24/07/2013 12:39, Daniel P. Berrange ha scritto:
On Wed, Jul 24, 2013 at 12:37:10PM +0200, Paolo Bonzini wrote:
Il 24/07/2013 12:31, Daniel P. Berrange ha scritto:
breaking the default config for QEMU's without iSCSI support.
Understood, that's why I suggested not having a default config at all for type='lun' devices.
As I said before, that causes apps to have to do special handling for iSCSI pools, which is not something we want.
This is not about iSCSI pools, it is about type='lun' devices.
And as I said before, knowing the kind of pool is something you have to do anyway for type='lun' devices, because they won't work if the underlying pool is filesystem- or LVM-based.
Regardless of whether all pools types can use type=lun, I don't want to see special case handling of this attribute. If it is optional in the XML schema, it should be unconditionally optional for any usage.
The XML schema can mark it as optional for type!=lun and mandatory for type=lun, but I agree it may be unnecessarily complicated.
Also, can libvirt at least detect using a volume on a pool that wasn't started, and fail unless mode='direct'?
It should always fail to start the guest if the pool is not started, regardless of the mode used.
Another point of mode='direct' is that the pool need not be started.
That is not something we can allow in the long term. When we expand our fine grained access control further, we're likely going to need to do ACL checks for the virStorageVolPtr object referenced by the XML config here. We can't do that unless the pool is running. So we cannot set a precedent of allowing use without the pool being active IMHO.
Can you make the pool active, but at the same time not expose it as devices on the host? LUNs can be discovered by talking directly to the target (similar to the iscsi-ls command included with libiscsi). In other words, perhaps mode='host' vs. mode='direct' could be a property of the pool rather than the disk? That would certainly work fine for me. Paolo
On Wed, Jul 24, 2013 at 12:59:27PM +0200, Paolo Bonzini wrote:
Il 24/07/2013 12:46, Daniel P. Berrange ha scritto:
On Wed, Jul 24, 2013 at 12:41:49PM +0200, Paolo Bonzini wrote:
Il 24/07/2013 12:39, Daniel P. Berrange ha scritto:
On Wed, Jul 24, 2013 at 12:37:10PM +0200, Paolo Bonzini wrote:
Il 24/07/2013 12:31, Daniel P. Berrange ha scritto:
breaking the default config for QEMU's without iSCSI support.
Understood, that's why I suggested not having a default config at all for type='lun' devices.
As I said before, that causes apps to have to do special handling for iSCSI pools, which is not something we want.
This is not about iSCSI pools, it is about type='lun' devices.
And as I said before, knowing the kind of pool is something you have to do anyway for type='lun' devices, because they won't work if the underlying pool is filesystem- or LVM-based.
Regardless of whether all pools types can use type=lun, I don't want to see special case handling of this attribute. If it is optional in the XML schema, it should be unconditionally optional for any usage.
The XML schema can mark it as optional for type!=lun and mandatory for type=lun, but I agree it may be unnecessarily complicated.
Also, can libvirt at least detect using a volume on a pool that wasn't started, and fail unless mode='direct'?
It should always fail to start the guest if the pool is not started, regardless of the mode used.
Another point of mode='direct' is that the pool need not be started.
That is not something we can allow in the long term. When we expand our fine grained access control further, we're likely going to need to do ACL checks for the virStorageVolPtr object referenced by the XML config here. We can't do that unless the pool is running. So we cannot set a precedent of allowing use without the pool being active IMHO.
Can you make the pool active, but at the same time not expose it as devices on the host? LUNs can be discovered by talking directly to the target (similar to the iscsi-ls command included with libiscsi).
I've never considered that actually. From the API POV, what we need to know is the list of LUNS and their sizes. We don't require that they have files visible in the host - for example the VMWare storage pool impls don't expose files on the host. So adding a mode for the pool which let you enumerate LUNs, but not expose them in the FS would be an option.
In other words, perhaps mode='host' vs. mode='direct' could be a property of the pool rather than the disk? That would certainly work fine for me.
It doesn't neccessarily need to be exclusive. If the pool used mode='host' we could still allow mode=host|direct in the guest. Only if the pool used mode=direct, would have to restrict it to mode=direct in the guest. We could make the guest config setting for "mode" default to match the pool's config. 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 :|
Il 24/07/2013 13:03, Daniel P. Berrange ha scritto:
Can you make the pool active, but at the same time not expose it as devices on the host? LUNs can be discovered by talking directly to the target (similar to the iscsi-ls command included with libiscsi).
I've never considered that actually. From the API POV, what we need to know is the list of LUNS and their sizes. We don't require that they have files visible in the host - for example the VMWare storage pool impls don't expose files on the host.
So adding a mode for the pool which let you enumerate LUNs, but not expose them in the FS would be an option.
In other words, perhaps mode='host' vs. mode='direct' could be a property of the pool rather than the disk? That would certainly work fine for me.
It doesn't neccessarily need to be exclusive. If the pool used mode='host' we could still allow mode=host|direct in the guest. Only if the pool used mode=direct, would have to restrict it to mode=direct in the guest. We could make the guest config setting for "mode" default to match the pool's config.
Cool! Sounds like a plan. libvirt itself could use libiscsi to support discovery for mode='direct' pools. Paolo
Although they produce no seclabel data, add some tests for coverage of various network and volume disk definitions --- tests/securityselinuxlabeldata/netdisks.txt | 5 +++ tests/securityselinuxlabeldata/netdisks.xml | 58 +++++++++++++++++++++++++++++ tests/securityselinuxlabeldata/voldisks.txt | 5 +++ tests/securityselinuxlabeldata/voldisks.xml | 45 ++++++++++++++++++++++ tests/securityselinuxlabeltest.c | 2 + 5 files changed, 115 insertions(+) create mode 100644 tests/securityselinuxlabeldata/netdisks.txt create mode 100644 tests/securityselinuxlabeldata/netdisks.xml create mode 100644 tests/securityselinuxlabeldata/voldisks.txt create mode 100644 tests/securityselinuxlabeldata/voldisks.xml diff --git a/tests/securityselinuxlabeldata/netdisks.txt b/tests/securityselinuxlabeldata/netdisks.txt new file mode 100644 index 0000000..b6bf95f --- /dev/null +++ b/tests/securityselinuxlabeldata/netdisks.txt @@ -0,0 +1,5 @@ +/nbd.raw; +/iscsi.raw; +/rbd.raw; +/sheepdog.raw; +/gluster.raw; diff --git a/tests/securityselinuxlabeldata/netdisks.xml b/tests/securityselinuxlabeldata/netdisks.xml new file mode 100644 index 0000000..ab5e964 --- /dev/null +++ b/tests/securityselinuxlabeldata/netdisks.xml @@ -0,0 +1,58 @@ +<domain type='kvm'> + <name>vm1</name> + <uuid>c7b3edbd-edaf-9455-926a-d65c16db1800</uuid> + <memory unit='KiB'>219200</memory> + <os> + <type arch='i686' machine='pc-1.0'>hvm</type> + <boot dev='cdrom'/> + </os> + <devices> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='nbd' file="/nbd.raw"> + <host name='example.org' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example/1' file="/iscsi.raw"> + <host name='example.org' port='6000'/> + </source> + <target dev='vdb' bus='virtio'/> + </disk> + <disk type='network'> + <driver name="qemu" type="raw"/> + <source protocol="rbd" name="image_name2" file="/rbd.raw"> + <host name="hostname" port="7000"/> + </source> + <target dev="hdb" bus="ide"/> + <auth username='myuser'> + <secret type='ceph' usage='mypassid'/> + </auth> + </disk> + <disk type='network'> + <driver name="qemu" type="raw"/> + <source protocol="sheepdog" name="image_name" file="/sheepdog.raw"> + <host name="hostname" port="7000"/> + </source> + <target dev="hdb" bus="ide"/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='gluster' name='Volume/Image' file='/gluster.raw'> + <host name='example.org' port='6000' transport='tcp'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'> + <listen type='address' address='0.0.0.0'/> + </graphics> + </devices> + <seclabel model="selinux" type="dynamic" relabel="yes"> + <label>system_u:system_r:svirt_t:s0:c41,c264</label> + <imagelabel>system_u:object_r:svirt_image_t:s0:c41,c264</imagelabel> + </seclabel> +</domain> diff --git a/tests/securityselinuxlabeldata/voldisks.txt b/tests/securityselinuxlabeldata/voldisks.txt new file mode 100644 index 0000000..bd5d755 --- /dev/null +++ b/tests/securityselinuxlabeldata/voldisks.txt @@ -0,0 +1,5 @@ +/file.raw; +/disk.raw; +/host.raw; +/direct.raw; +/cdrom.raw; diff --git a/tests/securityselinuxlabeldata/voldisks.xml b/tests/securityselinuxlabeldata/voldisks.xml new file mode 100644 index 0000000..ae7e629 --- /dev/null +++ b/tests/securityselinuxlabeldata/voldisks.xml @@ -0,0 +1,45 @@ +<domain type='kvm'> + <name>vm1</name> + <uuid>c7b3edbd-edaf-9455-926a-d65c16db1800</uuid> + <memory unit='KiB'>219200</memory> + <os> + <type arch='i686' machine='pc-1.0'>hvm</type> + <boot dev='cdrom'/> + </os> + <devices> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='dir-pool0' volume='dir-pool0-vol0' file='/file.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='dir-pool0' volume='dir-pool0-vol0' mode='host' file='/host.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='dir-pool0' volume='dir-pool0-vol0' mode='direct' file='/direct.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='blk-pool0' volume='blk-pool0-vol0' file='/plain.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='cdrom'> + <driver name='qemu' type='raw'/> + <source pool='blk-pool0' volume='blk-pool0-vol1' file='/cdrom.raw'/> + <target dev='hda' bus='ide'/> + <readonly/> + </disk> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'> + <listen type='address' address='0.0.0.0'/> + </graphics> + </devices> + <seclabel model="selinux" type="dynamic" relabel="yes"> + <label>system_u:system_r:svirt_t:s0:c41,c264</label> + <imagelabel>system_u:object_r:svirt_image_t:s0:c41,c264</imagelabel> + </seclabel> +</domain> diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index efe825a..8c88cfd 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -332,6 +332,8 @@ mymain(void) setcon((security_context_t)"system_r:system_u:libvirtd_t:s0:c0.c1023"); DO_TEST_LABELING("disks"); + DO_TEST_LABELING("netdisks"); + DO_TEST_LABELING("voldisks"); DO_TEST_LABELING("kernel"); DO_TEST_LABELING("chardev"); -- 1.8.1.4
On 19/07/13 20:32, John Ferlan wrote:
Although they produce no seclabel data, add some tests for coverage of various network and volume disk definitions --- tests/securityselinuxlabeldata/netdisks.txt | 5 +++ tests/securityselinuxlabeldata/netdisks.xml | 58 +++++++++++++++++++++++++++++ tests/securityselinuxlabeldata/voldisks.txt | 5 +++ tests/securityselinuxlabeldata/voldisks.xml | 45 ++++++++++++++++++++++ tests/securityselinuxlabeltest.c | 2 + 5 files changed, 115 insertions(+) create mode 100644 tests/securityselinuxlabeldata/netdisks.txt create mode 100644 tests/securityselinuxlabeldata/netdisks.xml create mode 100644 tests/securityselinuxlabeldata/voldisks.txt create mode 100644 tests/securityselinuxlabeldata/voldisks.xml
diff --git a/tests/securityselinuxlabeldata/netdisks.txt b/tests/securityselinuxlabeldata/netdisks.txt new file mode 100644 index 0000000..b6bf95f --- /dev/null +++ b/tests/securityselinuxlabeldata/netdisks.txt @@ -0,0 +1,5 @@ +/nbd.raw; +/iscsi.raw; +/rbd.raw; +/sheepdog.raw; +/gluster.raw; diff --git a/tests/securityselinuxlabeldata/netdisks.xml b/tests/securityselinuxlabeldata/netdisks.xml new file mode 100644 index 0000000..ab5e964 --- /dev/null +++ b/tests/securityselinuxlabeldata/netdisks.xml @@ -0,0 +1,58 @@ +<domain type='kvm'> + <name>vm1</name> + <uuid>c7b3edbd-edaf-9455-926a-d65c16db1800</uuid> + <memory unit='KiB'>219200</memory> + <os> + <type arch='i686' machine='pc-1.0'>hvm</type> + <boot dev='cdrom'/> + </os> + <devices> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='nbd' file="/nbd.raw"> + <host name='example.org' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example/1' file="/iscsi.raw">
i'm not clear with the security tests, but this xml looks incorrect. "file" is one way to represent the disk source, it's exclusive with other ways (e.g. protocol/name here) in semantics. similar for below. why do you use both "file" and other ways for disk source represention together?
+ <host name='example.org' port='6000'/> + </source> + <target dev='vdb' bus='virtio'/> + </disk> + <disk type='network'> + <driver name="qemu" type="raw"/> + <source protocol="rbd" name="image_name2" file="/rbd.raw"> + <host name="hostname" port="7000"/> + </source> + <target dev="hdb" bus="ide"/> + <auth username='myuser'> + <secret type='ceph' usage='mypassid'/> + </auth> + </disk> + <disk type='network'> + <driver name="qemu" type="raw"/> + <source protocol="sheepdog" name="image_name" file="/sheepdog.raw"> + <host name="hostname" port="7000"/> + </source> + <target dev="hdb" bus="ide"/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='gluster' name='Volume/Image' file='/gluster.raw'> + <host name='example.org' port='6000' transport='tcp'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'> + <listen type='address' address='0.0.0.0'/> + </graphics> + </devices> + <seclabel model="selinux" type="dynamic" relabel="yes"> + <label>system_u:system_r:svirt_t:s0:c41,c264</label> + <imagelabel>system_u:object_r:svirt_image_t:s0:c41,c264</imagelabel> + </seclabel> +</domain> diff --git a/tests/securityselinuxlabeldata/voldisks.txt b/tests/securityselinuxlabeldata/voldisks.txt new file mode 100644 index 0000000..bd5d755 --- /dev/null +++ b/tests/securityselinuxlabeldata/voldisks.txt @@ -0,0 +1,5 @@ +/file.raw; +/disk.raw; +/host.raw; +/direct.raw; +/cdrom.raw; diff --git a/tests/securityselinuxlabeldata/voldisks.xml b/tests/securityselinuxlabeldata/voldisks.xml new file mode 100644 index 0000000..ae7e629 --- /dev/null +++ b/tests/securityselinuxlabeldata/voldisks.xml @@ -0,0 +1,45 @@ +<domain type='kvm'> + <name>vm1</name> + <uuid>c7b3edbd-edaf-9455-926a-d65c16db1800</uuid> + <memory unit='KiB'>219200</memory> + <os> + <type arch='i686' machine='pc-1.0'>hvm</type> + <boot dev='cdrom'/> + </os> + <devices> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='dir-pool0' volume='dir-pool0-vol0' file='/file.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='dir-pool0' volume='dir-pool0-vol0' mode='host' file='/host.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='dir-pool0' volume='dir-pool0-vol0' mode='direct' file='/direct.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='blk-pool0' volume='blk-pool0-vol0' file='/plain.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='cdrom'> + <driver name='qemu' type='raw'/> + <source pool='blk-pool0' volume='blk-pool0-vol1' file='/cdrom.raw'/> + <target dev='hda' bus='ide'/> + <readonly/> + </disk> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'> + <listen type='address' address='0.0.0.0'/> + </graphics> + </devices> + <seclabel model="selinux" type="dynamic" relabel="yes"> + <label>system_u:system_r:svirt_t:s0:c41,c264</label> + <imagelabel>system_u:object_r:svirt_image_t:s0:c41,c264</imagelabel> + </seclabel> +</domain> diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index efe825a..8c88cfd 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -332,6 +332,8 @@ mymain(void) setcon((security_context_t)"system_r:system_u:libvirtd_t:s0:c0.c1023");
DO_TEST_LABELING("disks"); + DO_TEST_LABELING("netdisks"); + DO_TEST_LABELING("voldisks"); DO_TEST_LABELING("kernel"); DO_TEST_LABELING("chardev");
On 07/22/2013 07:06 AM, Osier Yang wrote:
On 19/07/13 20:32, John Ferlan wrote:
Although they produce no seclabel data, add some tests for coverage of various network and volume disk definitions --- tests/securityselinuxlabeldata/netdisks.txt | 5 +++ tests/securityselinuxlabeldata/netdisks.xml | 58 +++++++++++++++++++++++++++++ tests/securityselinuxlabeldata/voldisks.txt | 5 +++ tests/securityselinuxlabeldata/voldisks.xml | 45 ++++++++++++++++++++++ tests/securityselinuxlabeltest.c | 2 + 5 files changed, 115 insertions(+) create mode 100644 tests/securityselinuxlabeldata/netdisks.txt create mode 100644 tests/securityselinuxlabeldata/netdisks.xml create mode 100644 tests/securityselinuxlabeldata/voldisks.txt create mode 100644 tests/securityselinuxlabeldata/voldisks.xml
diff --git a/tests/securityselinuxlabeldata/netdisks.txt b/tests/securityselinuxlabeldata/netdisks.txt new file mode 100644 index 0000000..b6bf95f --- /dev/null +++ b/tests/securityselinuxlabeldata/netdisks.txt @@ -0,0 +1,5 @@ +/nbd.raw; +/iscsi.raw; +/rbd.raw; +/sheepdog.raw; +/gluster.raw; diff --git a/tests/securityselinuxlabeldata/netdisks.xml b/tests/securityselinuxlabeldata/netdisks.xml new file mode 100644 index 0000000..ab5e964 --- /dev/null +++ b/tests/securityselinuxlabeldata/netdisks.xml @@ -0,0 +1,58 @@ +<domain type='kvm'> + <name>vm1</name> + <uuid>c7b3edbd-edaf-9455-926a-d65c16db1800</uuid> + <memory unit='KiB'>219200</memory> + <os> + <type arch='i686' machine='pc-1.0'>hvm</type> + <boot dev='cdrom'/> + </os> + <devices> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='nbd' file="/nbd.raw"> + <host name='example.org' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example/1' file="/iscsi.raw">
i'm not clear with the security tests, but this xml looks incorrect. "file" is one way to represent the disk source, it's exclusive with other ways (e.g. protocol/name here) in semantics. similar for below. why do you use both "file" and other ways for disk source represention together?
Following syntax found in the following files tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-*.xml where '*' is {gluster, nbd, rbd, sheepdog, & iscsi} I can hold off pushing this patch if desired. The 'file' names are found in the 'netdisks.txt' file which are where the seclabels get listed for other tests. For the network types there are no seclabels. John
+ <host name='example.org' port='6000'/> + </source> + <target dev='vdb' bus='virtio'/> + </disk> + <disk type='network'> + <driver name="qemu" type="raw"/> + <source protocol="rbd" name="image_name2" file="/rbd.raw"> + <host name="hostname" port="7000"/> + </source> + <target dev="hdb" bus="ide"/> + <auth username='myuser'> + <secret type='ceph' usage='mypassid'/> + </auth> + </disk> + <disk type='network'> + <driver name="qemu" type="raw"/> + <source protocol="sheepdog" name="image_name" file="/sheepdog.raw"> + <host name="hostname" port="7000"/> + </source> + <target dev="hdb" bus="ide"/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='gluster' name='Volume/Image' file='/gluster.raw'> + <host name='example.org' port='6000' transport='tcp'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'> + <listen type='address' address='0.0.0.0'/> + </graphics> + </devices> + <seclabel model="selinux" type="dynamic" relabel="yes"> + <label>system_u:system_r:svirt_t:s0:c41,c264</label> + <imagelabel>system_u:object_r:svirt_image_t:s0:c41,c264</imagelabel> + </seclabel> +</domain> diff --git a/tests/securityselinuxlabeldata/voldisks.txt b/tests/securityselinuxlabeldata/voldisks.txt new file mode 100644 index 0000000..bd5d755 --- /dev/null +++ b/tests/securityselinuxlabeldata/voldisks.txt @@ -0,0 +1,5 @@ +/file.raw; +/disk.raw; +/host.raw; +/direct.raw; +/cdrom.raw; diff --git a/tests/securityselinuxlabeldata/voldisks.xml b/tests/securityselinuxlabeldata/voldisks.xml new file mode 100644 index 0000000..ae7e629 --- /dev/null +++ b/tests/securityselinuxlabeldata/voldisks.xml @@ -0,0 +1,45 @@ +<domain type='kvm'> + <name>vm1</name> + <uuid>c7b3edbd-edaf-9455-926a-d65c16db1800</uuid> + <memory unit='KiB'>219200</memory> + <os> + <type arch='i686' machine='pc-1.0'>hvm</type> + <boot dev='cdrom'/> + </os> + <devices> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='dir-pool0' volume='dir-pool0-vol0' file='/file.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='dir-pool0' volume='dir-pool0-vol0' mode='host' file='/host.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='dir-pool0' volume='dir-pool0-vol0' mode='direct' file='/direct.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='blk-pool0' volume='blk-pool0-vol0' file='/plain.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='cdrom'> + <driver name='qemu' type='raw'/> + <source pool='blk-pool0' volume='blk-pool0-vol1' file='/cdrom.raw'/> + <target dev='hda' bus='ide'/> + <readonly/> + </disk> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'> + <listen type='address' address='0.0.0.0'/> + </graphics> + </devices> + <seclabel model="selinux" type="dynamic" relabel="yes"> + <label>system_u:system_r:svirt_t:s0:c41,c264</label> + <imagelabel>system_u:object_r:svirt_image_t:s0:c41,c264</imagelabel> + </seclabel> +</domain> diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index efe825a..8c88cfd 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -332,6 +332,8 @@ mymain(void)
setcon((security_context_t)"system_r:system_u:libvirtd_t:s0:c0.c1023"); DO_TEST_LABELING("disks"); + DO_TEST_LABELING("netdisks"); + DO_TEST_LABELING("voldisks"); DO_TEST_LABELING("kernel"); DO_TEST_LABELING("chardev");
On 22/07/13 23:40, John Ferlan wrote:
On 07/22/2013 07:06 AM, Osier Yang wrote:
Although they produce no seclabel data, add some tests for coverage of various network and volume disk definitions --- tests/securityselinuxlabeldata/netdisks.txt | 5 +++ tests/securityselinuxlabeldata/netdisks.xml | 58 +++++++++++++++++++++++++++++ tests/securityselinuxlabeldata/voldisks.txt | 5 +++ tests/securityselinuxlabeldata/voldisks.xml | 45 ++++++++++++++++++++++ tests/securityselinuxlabeltest.c | 2 + 5 files changed, 115 insertions(+) create mode 100644 tests/securityselinuxlabeldata/netdisks.txt create mode 100644 tests/securityselinuxlabeldata/netdisks.xml create mode 100644 tests/securityselinuxlabeldata/voldisks.txt create mode 100644 tests/securityselinuxlabeldata/voldisks.xml
diff --git a/tests/securityselinuxlabeldata/netdisks.txt b/tests/securityselinuxlabeldata/netdisks.txt new file mode 100644 index 0000000..b6bf95f --- /dev/null +++ b/tests/securityselinuxlabeldata/netdisks.txt @@ -0,0 +1,5 @@ +/nbd.raw; +/iscsi.raw; +/rbd.raw; +/sheepdog.raw; +/gluster.raw; diff --git a/tests/securityselinuxlabeldata/netdisks.xml b/tests/securityselinuxlabeldata/netdisks.xml new file mode 100644 index 0000000..ab5e964 --- /dev/null +++ b/tests/securityselinuxlabeldata/netdisks.xml @@ -0,0 +1,58 @@ +<domain type='kvm'> + <name>vm1</name> + <uuid>c7b3edbd-edaf-9455-926a-d65c16db1800</uuid> + <memory unit='KiB'>219200</memory> + <os> + <type arch='i686' machine='pc-1.0'>hvm</type> + <boot dev='cdrom'/> + </os> + <devices> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='nbd' file="/nbd.raw"> + <host name='example.org' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example/1' file="/iscsi.raw"> i'm not clear with the security tests, but this xml looks incorrect. "file" is one way to represent the disk source, it's exclusive with other ways (e.g.
On 19/07/13 20:32, John Ferlan wrote: protocol/name here) in semantics. similar for below. why do you use both "file" and other ways for disk source represention together?
Following syntax found in the following files
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-*.xml
where '*' is {gluster, nbd, rbd, sheepdog, & iscsi}
I can hold off pushing this patch if desired.
i believe you will need a further patch to fix it if it's pushed. :\
The 'file' names are found in the 'netdisks.txt' file which are where the seclabels get listed for other tests. For the network types there are no seclabels.
these still don't answer my question, the (file="$file") and other representation ways are conflicted in semantics, though xml parsing could report no error, since one of the ways (e.g. "file") is ignored when parsing. see the disk source rng schema: <group> <attribute name="type"> <value>file</value> </attribute> <interleave> <optional> <element name="source"> <optional> <attribute name="file"> <ref name="absFilePath"/> </attribute> </optional> <optional> <ref name="startupPolicy"/> </optional> <optional> <ref name='devSeclabel'/> </optional> </element> </optional> <ref name="diskspec"/> </interleave> </group> <group> <attribute name="type"> <value>block</value> </attribute> <interleave> <optional> <element name="source"> <attribute name="dev"> <ref name="absFilePath"/> </attribute> <optional> <ref name='devSeclabel'/> </optional> </element> </optional> <ref name="diskspec"/> </interleave> </group> <group> <attribute name="type"> <value>dir</value> </attribute> <interleave> <optional> <element name="source"> <attribute name="dir"> <ref name="absFilePath"/> </attribute> <empty/> </element> </optional> <ref name="diskspec"/> </interleave> </group> <group> <attribute name="type"> <value>network</value> </attribute> <interleave> <optional> <element name="source"> <ref name='diskSourceNetwork'/> </element> </optional> <ref name="diskspec"/> </interleave> </group> <group> <attribute name="type"> <value>volume</value> </attribute> <interleave> <optional> <element name="source"> <attribute name="pool"> <ref name="genericName"/> </attribute> <attribute name="volume"> <ref name="volName"/> </attribute> <optional> <ref name="startupPolicy"/> </optional> <optional> <ref name='devSeclabel'/> </optional> </element> </optional> <ref name="diskspec"/> </interleave> </group> this patch is not that important for the feature. so i think it can be delayed till there is a right solution.
John
+ <host name='example.org' port='6000'/> + </source> + <target dev='vdb' bus='virtio'/> + </disk> + <disk type='network'> + <driver name="qemu" type="raw"/> + <source protocol="rbd" name="image_name2" file="/rbd.raw"> + <host name="hostname" port="7000"/> + </source> + <target dev="hdb" bus="ide"/> + <auth username='myuser'> + <secret type='ceph' usage='mypassid'/> + </auth> + </disk> + <disk type='network'> + <driver name="qemu" type="raw"/> + <source protocol="sheepdog" name="image_name" file="/sheepdog.raw"> + <host name="hostname" port="7000"/> + </source> + <target dev="hdb" bus="ide"/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='gluster' name='Volume/Image' file='/gluster.raw'> + <host name='example.org' port='6000' transport='tcp'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'> + <listen type='address' address='0.0.0.0'/> + </graphics> + </devices> + <seclabel model="selinux" type="dynamic" relabel="yes"> + <label>system_u:system_r:svirt_t:s0:c41,c264</label> + <imagelabel>system_u:object_r:svirt_image_t:s0:c41,c264</imagelabel> + </seclabel> +</domain> diff --git a/tests/securityselinuxlabeldata/voldisks.txt b/tests/securityselinuxlabeldata/voldisks.txt new file mode 100644 index 0000000..bd5d755 --- /dev/null +++ b/tests/securityselinuxlabeldata/voldisks.txt @@ -0,0 +1,5 @@ +/file.raw; +/disk.raw; +/host.raw; +/direct.raw; +/cdrom.raw; diff --git a/tests/securityselinuxlabeldata/voldisks.xml b/tests/securityselinuxlabeldata/voldisks.xml new file mode 100644 index 0000000..ae7e629 --- /dev/null +++ b/tests/securityselinuxlabeldata/voldisks.xml @@ -0,0 +1,45 @@ +<domain type='kvm'> + <name>vm1</name> + <uuid>c7b3edbd-edaf-9455-926a-d65c16db1800</uuid> + <memory unit='KiB'>219200</memory> + <os> + <type arch='i686' machine='pc-1.0'>hvm</type> + <boot dev='cdrom'/> + </os> + <devices> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='dir-pool0' volume='dir-pool0-vol0' file='/file.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='dir-pool0' volume='dir-pool0-vol0' mode='host' file='/host.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='dir-pool0' volume='dir-pool0-vol0' mode='direct' file='/direct.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='blk-pool0' volume='blk-pool0-vol0' file='/plain.raw'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='volume' device='cdrom'> + <driver name='qemu' type='raw'/> + <source pool='blk-pool0' volume='blk-pool0-vol1' file='/cdrom.raw'/> + <target dev='hda' bus='ide'/> + <readonly/> + </disk> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'> + <listen type='address' address='0.0.0.0'/> + </graphics> + </devices> + <seclabel model="selinux" type="dynamic" relabel="yes"> + <label>system_u:system_r:svirt_t:s0:c41,c264</label> + <imagelabel>system_u:object_r:svirt_image_t:s0:c41,c264</imagelabel> + </seclabel> +</domain> diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index efe825a..8c88cfd 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -332,6 +332,8 @@ mymain(void)
setcon((security_context_t)"system_r:system_u:libvirtd_t:s0:c0.c1023"); DO_TEST_LABELING("disks"); + DO_TEST_LABELING("netdisks"); + DO_TEST_LABELING("voldisks"); DO_TEST_LABELING("kernel"); DO_TEST_LABELING("chardev");
From: Osier Yang <jyang@redhat.com> virDomainDiskDefForeachPath is not only used by the security setting helpers, also used by cgroup setting helpers, so this is to ignore the volume type disk with mode="direct" for cgroup setting. --- 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 231edb5..770b7cc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17533,7 +17533,10 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, size_t depth = 0; virStorageFileMetadata *tmp; - if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) + if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK || + (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT)) return 0; if (iter(disk, disk->src, 0, opaque) < 0) -- 1.8.1.4
On 19/07/13 20:32, John Ferlan wrote:
From: Osier Yang <jyang@redhat.com>
virDomainDiskDefForeachPath is not only used by the security setting helpers, also used by cgroup setting helpers, so this is to ignore the volume type disk with mode="direct" for cgroup setting. --- 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 231edb5..770b7cc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17533,7 +17533,10 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, size_t depth = 0; virStorageFileMetadata *tmp;
- if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) + if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK || + (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT)) return 0;
if (iter(disk, disk->src, 0, opaque) < 0) ack
From: Osier Yang <jyang@redhat.com> The translation must be done before both of cgroup and security setting, otherwise since the disk source is not translated yet, it might be skipped on cgroup and security setting. --- src/qemu/qemu_process.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ef81536..d7deb0f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3627,6 +3627,14 @@ int qemuProcessStart(virConnectPtr conn, } hookData.nodemask = nodemask; + /* "volume" type disk's source must be translated before + * cgroup and security setting. + */ + for (i = 0; i < vm->def->ndisks; i++) { + if (qemuTranslateDiskSourcePool(conn, vm->def->disks[i]) < 0) + goto cleanup; + } + VIR_DEBUG("Setting up domain cgroup (if required)"); if (qemuSetupCgroup(driver, vm, nodemask) < 0) goto cleanup; @@ -3671,11 +3679,6 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; } - for (i = 0; i < vm->def->ndisks; i++) { - if (qemuTranslateDiskSourcePool(conn, vm->def->disks[i]) < 0) - goto cleanup; - } - VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON, priv->qemuCaps, -- 1.8.1.4
On 19/07/13 20:32, John Ferlan wrote:
From: Osier Yang <jyang@redhat.com>
The translation must be done before both of cgroup and security setting, otherwise since the disk source is not translated yet, it might be skipped on cgroup and security setting. --- src/qemu/qemu_process.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ef81536..d7deb0f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3627,6 +3627,14 @@ int qemuProcessStart(virConnectPtr conn, } hookData.nodemask = nodemask;
+ /* "volume" type disk's source must be translated before + * cgroup and security setting. + */ + for (i = 0; i < vm->def->ndisks; i++) { + if (qemuTranslateDiskSourcePool(conn, vm->def->disks[i]) < 0) + goto cleanup; + } + VIR_DEBUG("Setting up domain cgroup (if required)"); if (qemuSetupCgroup(driver, vm, nodemask) < 0) goto cleanup; @@ -3671,11 +3679,6 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; }
- for (i = 0; i < vm->def->ndisks; i++) { - if (qemuTranslateDiskSourcePool(conn, vm->def->disks[i]) < 0) - goto cleanup; - } - VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON, priv->qemuCaps, ack, a bit strange to ack my own patch though. may need to wait for a second ack from others.
On 07/19/2013 08:32 AM, John Ferlan wrote:
An update to yesterday's posting:
https://www.redhat.com/archives/libvir-list/2013-July/msg01213.html
Changes in v3 over v2 * Update patch 1 per code review * Use 'direct' instead of 'uri' and supporting documentaiton * In patch 4, use qemuAddISCSIPoolSourceHost() instead of qemuAddDiskISCSIUri. Also added a check in the function that (pooldef->source.nhost != 1) to make sure we at least have a host defined in the pool and of course that there's only one.
John Ferlan (5): storage_iscsi: Reflect the default target port conf: Introduce new XML tag "mode" for disk source conf: Introduce virDomainDiskSourceIsBlockType qemu: Translate the iscsi pool/volume disk source tests: Add various network and volume definitions
Osier Yang (2): conf: Ignore the volume type disk if its mode is "direct" qemu: Translate the volume type disk source before cgroup setting
docs/formatdomain.html.in | 11 +- docs/schemas/domaincommon.rng | 8 ++ src/conf/domain_conf.c | 67 ++++++++++- src/conf/domain_conf.h | 26 +++++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 20 +++- src/qemu/qemu_conf.c | 124 +++++++++++++++++---- src/qemu/qemu_process.c | 13 ++- src/storage/storage_backend_iscsi.c | 19 ++-- .../qemuxml2argv-disk-source-pool-mode.xml | 48 ++++++++ tests/qemuxml2xmltest.c | 1 + tests/securityselinuxlabeldata/netdisks.txt | 5 + tests/securityselinuxlabeldata/netdisks.xml | 58 ++++++++++ tests/securityselinuxlabeldata/voldisks.txt | 5 + tests/securityselinuxlabeldata/voldisks.xml | 45 ++++++++ tests/securityselinuxlabeltest.c | 2 + 16 files changed, 414 insertions(+), 39 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml create mode 100644 tests/securityselinuxlabeldata/netdisks.txt create mode 100644 tests/securityselinuxlabeldata/netdisks.xml create mode 100644 tests/securityselinuxlabeldata/voldisks.txt create mode 100644 tests/securityselinuxlabeldata/voldisks.xml
I pushed patches 1-4, 6, & 7. Will wait for more feedback on 5. Thanks for the reviews. John
participants (4)
-
Daniel P. Berrange -
John Ferlan -
Osier Yang -
Paolo Bonzini