[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/6] qemu: Translate the iscsi pool/volume disk source



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;
>  }
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]