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;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list