[libvirt] [PATCH 0/6] Support to use iscsi storage in domain conf
This supports to use libvirt iscsi storage volume for qemu with either the LUN's path on host (e.g. /dev/disk/by-path/*-lun-1), or the the libiscsi uri (e.g iscsi://demo.org:6000/$iqn/1)in domain conf. Osier Yang (6): storage_iscsi: Reflect the default target port conf: Introduce new XML tag "mode" for disk source qemu: Translate the iscsi pool/volume disk source security: Ignore to manage the volume type disk if its mode is uri conf: Ignore the volume type disk if its mode is "uri" qemu: Translate the volume type disk source before cgroup setting docs/formatdomain.html.in | 9 ++- docs/schemas/domaincommon.rng | 8 +++ src/conf/domain_conf.c | 71 +++++++++++++++++++++- src/conf/domain_conf.h | 25 ++++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 20 ++++-- src/qemu/qemu_conf.c | 117 +++++++++++++++++++++++++++++------- src/qemu/qemu_process.c | 13 ++-- src/security/security_apparmor.c | 10 ++- src/security/security_dac.c | 10 ++- src/security/security_selinux.c | 10 ++- src/storage/storage_backend_iscsi.c | 6 +- tests/qemuxml2xmltest.c | 1 + 13 files changed, 261 insertions(+), 40 deletions(-)
The default port for iSCSI target is 3260, which should be reflected to the pool's def. --- src/storage/storage_backend_iscsi.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 0a4cd22..eb8e128 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -44,12 +44,14 @@ #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; + int port; if (source->nhost != 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -60,6 +62,8 @@ virStorageBackendISCSIPortal(virStoragePoolSourcePtr source) host = source->hosts[0].name; if (source->hosts[0].port != 0) port = source->hosts[0].port; + else + port = source->hosts[0].port = ISCSI_DEFAULT_TARGET_PORT; if (strchr(host, ':')) { if (virAsprintf(&portal, "[%s]:%d,1", host, port) < 0) -- 1.8.1.4
On 06/18/2013 04:36 AM, Osier Yang wrote:
The default port for iSCSI target is 3260, which should be reflected to the pool's def. --- src/storage/storage_backend_iscsi.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
ACK John
There are two ways to use a iSCSI LUN as disk source for qemu. * The LUN's path showed up on host, e.g. /dev/disk/by-path/ip-$ip:3260-iscsi-$iqn-fc18:iscsi.iscsi0-lun-1 * The libiscsi URI, 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 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 "uri". "host" to indicate use the LUN with the path showed up on host. "uri" to indicate use it with the libiscsi URI (future patches may support to use network type libvirt storage too, e.g. Ceph, that's why to use a more general name "uri"). --- docs/formatdomain.html.in | 9 ++++++++- docs/schemas/domaincommon.rng | 8 ++++++++ src/conf/domain_conf.c | 24 +++++++++++++++++++++++- src/conf/domain_conf.h | 22 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 62 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 755d084..4e344a2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1557,7 +1557,14 @@ <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.0.7</span>) can be used + to indicate how to represent the LUN as the disk source. value "host" + indicate to use the LUN's path showed up on host, e.g. + /dev/disk/by-path/ip-10.11.12.9:3260-iscsi-iqn.2013-06.fc:iscsi.iscsi0-lun-1) + . Value "uri" indicates to use the libiscsi URI, e.g. + file=iscsi://demo.org:6000/iqn.1992-01.com.example/1. Defaults to "host". <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 3cace35..ac61582 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1145,6 +1145,14 @@ <ref name="volName"/> </attribute> <optional> + <attribute name="mode"> + <choice> + <value>host</value> + <value>uri</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 3e81013..009a8aa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -756,6 +756,12 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST, "unmap", "ignore") +VIR_ENUM_IMPL(virDomainDiskSourcePoolMode, + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_LAST, + "default", + "host", + "uri") + #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE @@ -4600,10 +4606,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) @@ -4621,6 +4629,14 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, 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; @@ -4631,6 +4647,7 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, cleanup: VIR_FREE(pool); VIR_FREE(volume); + VIR_FREE(mode); return ret; } @@ -13856,9 +13873,14 @@ 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 b8edb1e..d57b7c3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -646,11 +646,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 showed 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. E.g. + * file=iscsi://demo.org:6000/iqn.1992-01.com.example/1. + */ + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_URI, + + 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; @@ -2497,6 +2518,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/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 06e4206..cb14cb9 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -261,6 +261,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 06/18/2013 04:36 AM, Osier Yang wrote:
There are two ways to use a iSCSI LUN as disk source for qemu.
* The LUN's path showed up on host, e.g. /dev/disk/by-path/ip-$ip:3260-iscsi-$iqn-fc18:iscsi.iscsi0-lun-1
* The libiscsi URI, 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 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 "uri". "host" to indicate use the LUN with the path showed up on host. "uri" to indicate use it with the libiscsi URI (future patches may support to use network type libvirt storage too, e.g. Ceph, that's why to use a more general name "uri"). --- docs/formatdomain.html.in | 9 ++++++++- docs/schemas/domaincommon.rng | 8 ++++++++ src/conf/domain_conf.c | 24 +++++++++++++++++++++++- src/conf/domain_conf.h | 22 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 62 insertions(+), 2 deletions(-)
I think you're missing something from the commit (from my valgrind run): TEST: qemuxml2xmltest ........................................ 40 ........................................ 80 ......................................./home/jferlan/libvirt.work/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml: failed to open: No such file or directory /home/jferlan/libvirt.work/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml: failed to open: No such file or directory ! 120 ............................ 148 FAIL
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 755d084..4e344a2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1557,7 +1557,14 @@ <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.0.7</span>) can be used + to indicate how to represent the LUN as the disk source. value "host" + indicate to use the LUN's path showed up on host, e.g. + /dev/disk/by-path/ip-10.11.12.9:3260-iscsi-iqn.2013-06.fc:iscsi.iscsi0-lun-1) + . Value "uri" indicates to use the libiscsi URI, e.g. + file=iscsi://demo.org:6000/iqn.1992-01.com.example/1. Defaults to "host". <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 3cace35..ac61582 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1145,6 +1145,14 @@ <ref name="volName"/> </attribute> <optional> + <attribute name="mode"> + <choice> + <value>host</value> + <value>uri</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 3e81013..009a8aa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -756,6 +756,12 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST, "unmap", "ignore")
+VIR_ENUM_IMPL(virDomainDiskSourcePoolMode, + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_LAST, + "default", + "host", + "uri") +
Can someone actually provide "default" as an option or was this a cut-n-paste type error? It'll be zero which I think doesn't do anything according to what I read below. It seems you're just following the virDomainDiskDiscard model, right?
#define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE
@@ -4600,10 +4606,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) @@ -4621,6 +4629,14 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, 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; + } +
Hmm.. guess that answer's my question above as 'default' would translate to 0 which would cause failure.
def->srcpool->pool = pool; pool = NULL; def->srcpool->volume = volume; @@ -4631,6 +4647,7 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, cleanup: VIR_FREE(pool); VIR_FREE(volume); + VIR_FREE(mode); return ret; }
@@ -13856,9 +13873,14 @@ 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 b8edb1e..d57b7c3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -646,11 +646,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 showed 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. E.g. + * file=iscsi://demo.org:6000/iqn.1992-01.com.example/1. + */ + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_URI, + + 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;
@@ -2497,6 +2518,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/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 06e4206..cb14cb9 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -261,6 +261,7 @@ mymain(void)
DO_TEST("disk-scsi-disk-vpd"); DO_TEST("disk-source-pool"); + DO_TEST("disk-source-pool-mode");
NOTE: See above - something is missing from the commit.
DO_TEST("disk-drive-discard");
I think once the test is include, this is good to go. John
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='uri'. For 'host' mode, it copies the volume target path into disk->src. For 'uri' mode, the conresponded info in the *one* pool source host def are copied to disk->hosts[0]. * src/conf/domain_conf.[ch]: (Introduce a new helper to check if the disk source is of block type: virDomainDiskSourceIsBlockType; Introduce "pooltype" for disk->srcpool, to indicate the pool type) src/libvirt_private.syms: (Expose the new helper's symbol) * src/qemu/qemu_conf.c: (Use the helper to ease the checking in "shared disk", "sgio" related helpers; Translate the iscsi pool/volume disk source) * src/qemu/qemu_command.c (Use the helper to ease the checking in qemuBuildDriveDevStr; Build qemu command line for iscsi pool/volume disk source) --- src/conf/domain_conf.c | 42 +++++++++++++++++ src/conf/domain_conf.h | 3 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 20 ++++++-- src/qemu/qemu_conf.c | 117 ++++++++++++++++++++++++++++++++++++++--------- 5 files changed, 158 insertions(+), 25 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 009a8aa..04b14dc 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" @@ -17950,3 +17951,44 @@ virDomainDiskDefGenSecurityLabelDef(const char *model) return seclabel; } + +/** + * 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) +{ + if (!def) + return false; + + /* No reason to think the disk source is block type if + * the source is empty + */ + if (!def->src && !def->srcpool) + return false; + + if (def->type == VIR_DOMAIN_DISK_TYPE_BLOCK) + return true; + + if (def->type == VIR_DOMAIN_DISK_TYPE_VOLUME) { + if (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/shoudn't manage it + * (e.g. set sgio=filtered|unfilered for it) in libvirt. + */ + if (def->srcpool->pooltype == VIR_STORAGE_POOL_ISCSI && + def->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_URI) + return false; + + return true; + } + } + + return false; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d57b7c3..3c60293 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -671,6 +671,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; @@ -2652,4 +2653,6 @@ virDomainDefMaybeAddController(virDomainDefPtr def, char *virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps); +bool virDomainDiskSourceIsBlockType(virDomainDiskDefPtr def); + #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1ea7467..fa9f079 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -161,6 +161,7 @@ virDomainDiskProtocolTransportTypeToString; virDomainDiskProtocolTypeToString; virDomainDiskRemove; virDomainDiskRemoveByName; +virDomainDiskSourceIsBlockType; virDomainDiskTypeFromString; virDomainDiskTypeToString; virDomainEmulatorPinAdd; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 486682e..ae5f7dd 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" @@ -3163,7 +3164,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_URI) { + 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); @@ -3450,9 +3464,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 094f9f7..b67f182 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 @@ -1180,12 +1181,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; @@ -1295,12 +1291,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; @@ -1392,12 +1383,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; @@ -1463,9 +1450,12 @@ int qemuTranslateDiskSourcePool(virConnectPtr conn, virDomainDiskDefPtr def) { + virStoragePoolDefPtr pooldef = NULL; virStoragePoolPtr pool = NULL; virStorageVolPtr vol = NULL; + char *poolxml = NULL; virStorageVolInfo info; + char **tokens = NULL; int ret = -1; if (def->type != VIR_DOMAIN_DISK_TYPE_VOLUME) @@ -1492,11 +1482,91 @@ 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 (pooldef->type != VIR_STORAGE_POOL_ISCSI && + def->srcpool->mode) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("disk source mode is only valid when " + "storage pool is of iscsi type")); + goto cleanup; + } + + 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_URI) { + /* iscsi pool only supports one host */ + def->nhosts = 1; + + if (VIR_ALLOC_N(def->hosts, def->nhosts) < 0) { + virReportOOMError(); + 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) { + virReportOOMError(); + 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) { + virReportOOMError(); + goto cleanup; + } + + /* Storage pool have not supportted 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; + } 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; + } + + def->srcpool->pooltype = pooldef->type; + break; case VIR_STORAGE_VOL_NETWORK: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Using network volume as disk source is not supported")); @@ -1506,7 +1576,12 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, def->srcpool->voltype = info.type; ret = 0; cleanup: - virStoragePoolFree(pool); - virStorageVolFree(vol); + if (pool) + virStoragePoolFree(pool); + if (vol) + virStorageVolFree(vol); + VIR_FREE(poolxml); + virStoragePoolDefFree(pooldef); + virStringFreeList(tokens); return ret; } -- 1.8.1.4
On 06/18/2013 04:36 AM, Osier Yang 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='uri'.
For 'host' mode, it copies the volume target path into disk->src. For 'uri' mode, the conresponded info in the *one* pool source host def
s/conresponded/corresponding/ ?? Not sure I understand the "*one* pool" reference.
are copied to disk->hosts[0].
* src/conf/domain_conf.[ch]: (Introduce a new helper to check if the disk source is of block type: virDomainDiskSourceIsBlockType; Introduce "pooltype" for disk->srcpool, to indicate the pool type) src/libvirt_private.syms: (Expose the new helper's symbol) * src/qemu/qemu_conf.c: (Use the helper to ease the checking in "shared disk", "sgio" related helpers; Translate the iscsi pool/volume disk source) * src/qemu/qemu_command.c (Use the helper to ease the checking in qemuBuildDriveDevStr; Build qemu command line for iscsi pool/volume disk source) --- src/conf/domain_conf.c | 42 +++++++++++++++++ src/conf/domain_conf.h | 3 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 20 ++++++-- src/qemu/qemu_conf.c | 117 ++++++++++++++++++++++++++++++++++++++--------- 5 files changed, 158 insertions(+), 25 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 009a8aa..04b14dc 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" @@ -17950,3 +17951,44 @@ virDomainDiskDefGenSecurityLabelDef(const char *model)
return seclabel; } + +/** + * 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) +{ + if (!def) + return false; + + /* No reason to think the disk source is block type if + * the source is empty + */ + if (!def->src && !def->srcpool) + return false;
Logic changed slightly over the previous code, but I think it's OK. I'm reading this as a block source will be always part of some block pool. And that we only care to check the srcpool if we're a volume. I guess the question is - will srcpool be set? We previously only cared about srcpool if we had a src that was a volume. Now we're always checking. It's a slight change, but could be important.
+ + if (def->type == VIR_DOMAIN_DISK_TYPE_BLOCK) + return true; + + if (def->type == VIR_DOMAIN_DISK_TYPE_VOLUME) { + if (def->srcpool->voltype == VIR_STORAGE_VOL_BLOCK) {
NIT: Two "ifs" that could be combined - that is we have a source of volume type backed by a pool of volume block devices)
+ /* We don't think the volume accessed by remote URI is + * block type source, since we can't/shoudn't manage it
s/shoudn't/shouldn't/
+ * (e.g. set sgio=filtered|unfilered for it) in libvirt.
s/unfilered/unfiltered/
+ */ + if (def->srcpool->pooltype == VIR_STORAGE_POOL_ISCSI && + def->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_URI) + return false; + + return true; + } + } + + return false; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d57b7c3..3c60293 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -671,6 +671,7 @@ struct _virDomainDiskSourcePoolDef { char *pool; /* pool name */ char *volume; /* volume name */ int voltype; /* enum virStorageVolType, internal only */ + int pooltype; /* enum virStoragePoolType, internal only */
This is being made generic to all disk source pool types, but only ever defined when/if it's a VIR_DOMAIN_DISK_TYPE_VOLUME and VIR_STORAGE_VOL_BLOCK (I assume that's a volume back by a pool of volume block devices).
int mode; /* enum virDomainDiskSourcePoolMode */ }; typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr; @@ -2652,4 +2653,6 @@ virDomainDefMaybeAddController(virDomainDefPtr def,
char *virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps);
+bool virDomainDiskSourceIsBlockType(virDomainDiskDefPtr def); + #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1ea7467..fa9f079 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -161,6 +161,7 @@ virDomainDiskProtocolTransportTypeToString; virDomainDiskProtocolTypeToString; virDomainDiskRemove; virDomainDiskRemoveByName; +virDomainDiskSourceIsBlockType; virDomainDiskTypeFromString; virDomainDiskTypeToString; virDomainEmulatorPinAdd; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 486682e..ae5f7dd 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" @@ -3163,7 +3164,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_URI) { + 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); @@ -3450,9 +3464,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 094f9f7..b67f182 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 @@ -1180,12 +1181,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; @@ -1295,12 +1291,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; @@ -1392,12 +1383,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; @@ -1463,9 +1450,12 @@ int qemuTranslateDiskSourcePool(virConnectPtr conn, virDomainDiskDefPtr def) { + virStoragePoolDefPtr pooldef = NULL; virStoragePoolPtr pool = NULL; virStorageVolPtr vol = NULL; + char *poolxml = NULL; virStorageVolInfo info; + char **tokens = NULL; int ret = -1;
if (def->type != VIR_DOMAIN_DISK_TYPE_VOLUME)
Remainder only ever run for a source of volume type...
@@ -1492,11 +1482,91 @@ 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;
So no need to check/set pooltype here?
break; + case VIR_STORAGE_VOL_BLOCK: + if (!(poolxml = virStoragePoolGetXMLDesc(pool, 0))) + goto cleanup; + + if (!(pooldef = virStoragePoolDefParseString(poolxml))) + goto cleanup; + + if (pooldef->type != VIR_STORAGE_POOL_ISCSI && + def->srcpool->mode) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("disk source mode is only valid when " + "storage pool is of iscsi type")); + goto cleanup; + } + + 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_URI) { + /* iscsi pool only supports one host */ + def->nhosts = 1; + + if (VIR_ALLOC_N(def->hosts, def->nhosts) < 0) { + virReportOOMError(); + 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) {
From 1/6 in storage_backend_iscsi:
#define ISCSI_DEFAULT_TARGET_PORT 3260 Maybe that needs to be put in a more common include file?
+ virReportOOMError(); + goto cleanup; + } + + /* iscsi volume has name like "unit:0:0:1" */ + if (!(tokens = virStringSplit(def->srcpool->volume, + ":", 0)))
s/0/4/ ?? Although if you do 5 or more, then the proceeding check 4 is "more valid" (in the even someone has "unit:1:2:3:4")
+ 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 */
So using the '4th' value makes the name unique enough? I'm not familiar with the naming scheme, but it would seem that "unit:0:0:1" and "unit:1:1:1" would result in the same name, correct? Or am I missing something. Don't want to assume anything.
+ if (virAsprintf(&def->src, "%s/%s", + pooldef->source.devices[0].path, + tokens[3]) < 0) { + virReportOOMError(); + goto cleanup; + } + + /* Storage pool have not supportted these 2 attributes yet, + * use the defaults.
s/supportted/supported
+ */ + def->hosts[0].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; + def->hosts[0].socket = NULL; + + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_ISCSI; + } 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; + } + + def->srcpool->pooltype = pooldef->type; + break; case VIR_STORAGE_VOL_NETWORK: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Using network volume as disk source is not supported"));
Again, no need to set "pooltype" then, right?
@@ -1506,7 +1576,12 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, def->srcpool->voltype = info.type; ret = 0; cleanup: - virStoragePoolFree(pool); - virStorageVolFree(vol); + if (pool) + virStoragePoolFree(pool); + if (vol) + virStorageVolFree(vol);
Don't think either is necessary - virStoragePoolFree -> VIR_IS_CONNECTED_STORAGE_POOL(pool) -> VIR_IS_STORAGE_POOL(obj) -> VIR_IS_STORAGE_POOL(obj) -> virObjectIsClass(obj) -> if (!obj) return false virStorageVolFree -> !VIR_IS_STORAGE_VOL(vol) -> virObjectIsClass(obj) -> if (!obj) return false
+ VIR_FREE(poolxml); + virStoragePoolDefFree(pooldef); + virStringFreeList(tokens); return ret; }
It's straightforward to not manage security labels for remote URI like "iscsi://example.org:6000/iqn.1992-01.com.example/1". --- src/security/security_apparmor.c | 10 ++++++++-- src/security/security_dac.c | 10 ++++++++-- src/security/security_selinux.c | 10 ++++++++-- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 87c2777..b8a5be2 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -682,7 +682,10 @@ AppArmorRestoreSecurityImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk) { - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) + if (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_URI)) return 0; return reload_profile(mgr, def, NULL, false); @@ -704,7 +707,10 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, if (secdef->norelabel) return 0; - 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_URI)) return 0; if (secdef->imagelabel) { diff --git a/src/security/security_dac.c b/src/security/security_dac.c index b8d1a92..881101a 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -368,7 +368,10 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, if (!priv->dynamicOwnership) return 0; - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) + if (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_URI)) return 0; params[0] = mgr; @@ -391,7 +394,10 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, if (!priv->dynamicOwnership) return 0; - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) + if (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_URI)) return 0; /* Don't restore labels on readoly/shared disks, because diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b862fbf..829bd89 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1148,7 +1148,10 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, if (disk->readonly || disk->shared) return 0; - 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_URI)) return 0; /* If we have a shared FS & doing migrated, we must not @@ -1248,7 +1251,10 @@ virSecuritySELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr, if (cbdata.secdef->norelabel) return 0; - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) + if (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_URI)) return 0; return virDomainDiskDefForeachPath(disk, -- 1.8.1.4
On 06/18/2013 04:36 AM, Osier Yang wrote:
It's straightforward to not manage security labels for remote URI like "iscsi://example.org:6000/iqn.1992-01.com.example/1". --- src/security/security_apparmor.c | 10 ++++++++-- src/security/security_dac.c | 10 ++++++++-- src/security/security_selinux.c | 10 ++++++++-- 3 files changed, 24 insertions(+), 6 deletions(-)
Seems reasonable to me, although the security management for libvirt is not something I've had much exposure to yet. John
On Tue, Jun 18, 2013 at 04:36:40PM +0800, Osier Yang wrote:
It's straightforward to not manage security labels for remote URI like "iscsi://example.org:6000/iqn.1992-01.com.example/1". --- src/security/security_apparmor.c | 10 ++++++++-- src/security/security_dac.c | 10 ++++++++-- src/security/security_selinux.c | 10 ++++++++-- 3 files changed, 24 insertions(+), 6 deletions(-)
You should be extending tests/securityselinuxlabeldata/disks.xml so that it covers these new types of disk, or perhaps better adding a new data file covering all the various network disk models. 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 :|
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="uri" 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 04b14dc..2f397de 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17155,7 +17155,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_URI)) return 0; if (iter(disk, disk->src, 0, opaque) < 0) -- 1.8.1.4
On 06/18/2013 04:36 AM, Osier Yang wrote:
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="uri" for cgroup setting. --- src/conf/domain_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Seems reasonable, ACK. Should I assume you checked all uses of VIR_DOMAIN_DISK_TYPE_NETWORK to make sure none of the others make similar checks that this volume source pool URI will also need to avoid. John
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 c412ea2..351440b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3552,6 +3552,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; @@ -3598,11 +3606,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 06/18/2013 04:36 AM, Osier Yang wrote:
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(-)
I don't now about this one. What concerns me is the code that was run just before this movement, specifically: /* * Normally PCI addresses are assigned in the virDomainCreate * or virDomainDefine methods. We might still need to assign * some here to cope with the question of upgrades. Regardless * we also need to populate the PCi address set cache for later * use in hotplug */ if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { VIR_DEBUG("Assigning domain PCI addresses"); if ((qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm)) < 0) goto cleanup; } I didn't chase into the code, but I would think that PCI assignment could play a role in disk placements. I guess, I'm not comfortable with taking that leap of faith - perhaps someone else knows for certain. John
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c412ea2..351440b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3552,6 +3552,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; @@ -3598,11 +3606,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,
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Osier Yang