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

Re: [libvirt] libvirt [PATCHv2 2/2] Add iSCSI backend storage driver for ESX.



2012/11/10 Ata E Husain Bohra <ata husain hotmail com>:
> The patch adds the backend driver to support iSCSI format storage pools
> and volumes for ESX host. The mapping of ESX iSCSI specifics to Libvirt
> is as follows:
>
> 1. ESX static iSCSI target <------> Libvirt Storage Pools
> 2. ESX iSCSI LUNs          <------> Libvirt Storage Volumes.
>
> The above understanding is based on http://libvirt.org/storage.html.
>
> The operation supported on iSCSI pools includes:
>
> 1. List storage pools & volumes.
> 2. Get xml descriptor operaion on pools & volumes.
> 3. Lookup operation on pools & volumes by name, uuid and path (if applicable).
>
> iSCSI pools does not support operations such as: Create / remove pools
> and volumes.
> ---
>  src/Makefile.am                     |    1 +
>  src/esx/esx_storage_backend_iscsi.c |  807 +++++++++++++++++++++++++++++++++++
>  src/esx/esx_storage_backend_iscsi.h |   29 ++
>  src/esx/esx_storage_driver.c        |    8 +-
>  src/esx/esx_vi.c                    |  332 ++++++++++++++
>  src/esx/esx_vi.h                    |   18 +
>  src/esx/esx_vi_generator.input      |  302 +++++++++++++
>  src/esx/esx_vi_generator.py         |   19 +
>  8 files changed, 1515 insertions(+), 1 deletion(-)
>  create mode 100644 src/esx/esx_storage_backend_iscsi.c
>  create mode 100644 src/esx/esx_storage_backend_iscsi.h
>

> --- /dev/null
> +++ b/src/esx/esx_storage_backend_iscsi.c

> +static int
> +esxStorageBackendISCSIPoolListStorageVolumes(virStoragePoolPtr pool,
> +                                             char **const names,
> +                                             int maxnames)
> +{
> +    int  count = 0;
> +    esxPrivate *priv = pool->conn->storagePrivateData;
> +    esxVI_HostScsiTopologyLun *hostScsiTopologyLunList = NULL;
> +    const esxVI_HostScsiTopologyLun *hostScsiTopologyLun = NULL;
> +    esxVI_ScsiLun *scsiLunList = NULL;
> +    const esxVI_ScsiLun *scsiLun = NULL;
> +    bool success = false;
> +    int i = 0;
> +
> +    if (esxVI_LookupHostScsiTopologyLunListByTargetName(
> +          priv->primary, pool->name, &hostScsiTopologyLunList) < 0) {
> +        goto cleanup;
> +    }
> +
> +    if (hostScsiTopologyLunList == NULL) {
> +        /* iSCSI adapter may not be enabled on ESX host */
> +        return 0;
> +    }
> +
> +    if (esxVI_LookupScsiLunList(priv->primary, &scsiLunList) < 0) {
> +        goto cleanup;
> +    }
> +
> +    /* O^2 but still faster than hash given N is not that large */

This comment is wrong here, because a hash wouldn't help here. In
order to get the storage volume list this 2-dimensional list need to
be iterated. There is no way to do this faster, except adding a cache
but then you get the problem of ehen to invalidate it. So this 2 level
for loop is totally fine and the comment should be removed, because it
is misleading.

> +    for (scsiLun = scsiLunList; scsiLun != NULL && count < maxnames;
> +         scsiLun = scsiLun->_next) {
> +        for (hostScsiTopologyLun = hostScsiTopologyLunList;
> +             hostScsiTopologyLun != NULL && count < maxnames;
> +             hostScsiTopologyLun = hostScsiTopologyLun->_next) {
> +            if (STREQ(hostScsiTopologyLun->scsiLun, scsiLun->key)) {
> +                names[count] = strdup(scsiLun->deviceName);
> +
> +                if (names[count] == NULL) {
> +                    virReportOOMError();
> +                    goto cleanup;
> +                }
> +                ++count;
> +            }
> +        }
> +    }
> +
> +    success = true;
> +
> +  cleanup:
> +    if (! success) {
> +        for (i = 0; i < count; ++i) {
> +            VIR_FREE(names[i]);
> +        }
> +        count = -1;
> +    }
> +
> +    esxVI_HostScsiTopologyLun_Free(&hostScsiTopologyLunList);
> +    esxVI_ScsiLun_Free(&scsiLunList);
> +
> +    return count;
> +}


> +static int
> +esxStorageBackendISCSIPoolRefresh(virStoragePoolPtr pool,
> +                                  unsigned int flags)
> +{
> +    int result = -1;
> +    esxPrivate *priv = pool->conn->storagePrivateData;
> +    esxVI_ManagedObjectReference *hostStorageSystem = NULL;
> +    esxVI_HostInternetScsiHba *hostInternetScsiHba = NULL;
> +    esxVI_ObjectContent *hostSystem = NULL;
> +    esxVI_String *propertyNameList = NULL;
> +
> +    virCheckFlags(0, -1);
> +
> +    if (esxVI_String_AppendValueToList(&propertyNameList,
> +          "configManager.storageSystem\0") < 0 ||
> +        esxVI_LookupHostSystemProperties(priv->primary,
> +            propertyNameList, &hostSystem) < 0 ||
> +        esxVI_GetManagedObjectReference(hostSystem,
> +            "configManager.storageSystem", &hostStorageSystem,
> +            esxVI_Occurrence_RequiredItem) < 0 ||

No need to look up the hostStorageSystem here. It is already available
here: ctx->hostSystem->configManager->storageSystem

> +        esxVI_LookupHostInternetScsiHba(
> +            priv->primary, &hostInternetScsiHba) < 0) {
> +        goto cleanup;
> +    }
> +
> +     /**
> +      * ESX does not allow rescan on a particular target,
> +      * rescan all the static targets
> +     */
> +    if (esxVI_RescanHba(priv->primary, hostStorageSystem,
> +                        hostInternetScsiHba->device) < 0) {
> +        goto cleanup;
> +    }
> +
> +    result = 0;
> +
> + cleanup:
> +    esxVI_String_Free(&propertyNameList);
> +    esxVI_ManagedObjectReference_Free(&hostStorageSystem);
> +    esxVI_ObjectContent_Free(&hostSystem);
> +    esxVI_HostInternetScsiHba_Free(&hostInternetScsiHba);
> +
> +    return result;
> +
> +}

> +static virStorageVolPtr
> +esxStorageBackendISCSIVolumeCreateXML(virStoragePoolPtr pool ATTRIBUTE_UNUSED,
> +                                      const char *xmldesc ATTRIBUTE_UNUSED,
> +                                      unsigned int flags)
> +{
> +    virCheckFlags(0, NULL);

A VIR_ERR_NO_SUPPORT error should be reported here.

> +    /* not supported operation for iSCSI pools */
> +    return NULL;
> +}
> +
> +
> +
> +static virStorageVolPtr
> +esxStorageBackendISCSIVolumeCreateXMLFrom(
> +  virStoragePoolPtr pool ATTRIBUTE_UNUSED,
> +  const char *xmldesc ATTRIBUTE_UNUSED,
> +  virStorageVolPtr sourceVolume ATTRIBUTE_UNUSED,
> +  unsigned int flags)
> +{
> +    virCheckFlags(0, NULL);

A VIR_ERR_NO_SUPPORT error should be reported here.

> +    /* not supported operation for iSCSI pools */
> +    return NULL;
> +}

> +static int
> +esxStorageBackendISCSIVolumeDelete(virStorageVolPtr volume ATTRIBUTE_UNUSED,
> +                                   unsigned int flags)
> +{
> +    virCheckFlags(0, -1);

A VIR_ERR_NO_SUPPORT error should be reported here.

> +    /* unsupported operation for iSCSI volume */
> +    return 1;

Should return -1;

> +}
> +
> +
> +
> +static int
> +esxStorageBackendISCSIVolumeWipe(virStorageVolPtr volume ATTRIBUTE_UNUSED,
> +                                   unsigned int flags)
> +{
> +    virCheckFlags(0, -1);

A VIR_ERR_NO_SUPPORT error should be reported here.

> +    /* unsupported operation for iSCSI volume */
> +    return 1;

Should return -1;

> +}
> +
> +
> +
> +static char*
> +esxStorageBackendISCSIVolumeGetPath(virStorageVolPtr volume)
> +{
> +    char *path;
> +
> +    if (virAsprintf(&path, "%s", volume->name) < 0) {
> +        virReportOOMError();
> +        return NULL;
> +    }

Can be simplified to strdup.

> +    return path;
> +
> +}

> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> index 9fb2c11..12100d7 100644
> --- a/src/esx/esx_vi.c
> +++ b/src/esx/esx_vi.c

> +int
> +esxVI_LookupScsiLunList(esxVI_Context *ctx,
> +                        esxVI_ScsiLun **ret)
> +{
> +    int result = -1;
> +    esxVI_DynamicProperty *dynamicProperty = NULL;
> +    esxVI_ObjectContent *hostSystem = NULL;
> +    esxVI_String *propertyNameList = NULL;
> +    esxVI_ScsiLun *scsiLunList = NULL;
> +
> +    if (esxVI_String_AppendValueToList(&propertyNameList,
> +        "config.storageDevice.scsiLun\0") < 0 ||
> +        esxVI_LookupHostSystemProperties(
> +            ctx, propertyNameList, &hostSystem) < 0) {
> +        goto cleanup;
> +    }
> +
> +    for (dynamicProperty = hostSystem->propSet;
> +         dynamicProperty != NULL;
> +         dynamicProperty = dynamicProperty->_next) {
> +        if (STREQ(dynamicProperty->name,
> +                     "config.storageDevice.scsiLun")) {
> +            if (esxVI_ScsiLun_CastListFromAnyType(dynamicProperty->val,
> +                                                  &scsiLunList) < 0) {
> +                goto cleanup;
> +            }
> +        } else {
> +            VIR_WARN("Unexpected '%s' property", dynamicProperty->name);
> +        }
> +    }
> +
> +    if (scsiLunList == NULL ||
> +        esxVI_ScsiLun_DeepCopyList(ret, scsiLunList) < 0) {
> +        goto cleanup;
> +    }
> +
> +    result = 0;
> +
> +cleanup:
> +
> +    esxVI_ScsiLun_Free(&scsiLunList);

Needs to free propertyNameList and hostSystem too.

> +    return result;
> +
> +}

> diff --git a/src/esx/esx_vi_generator.input b/src/esx/esx_vi_generator.input
> index c4a3e56..21f5b10 100644
> --- a/src/esx/esx_vi_generator.input
> +++ b/src/esx/esx_vi_generator.input

> @@ -128,6 +136,12 @@ enum VirtualMachinePowerState
>  end
>
>
> +enum vStorageSupport
> +    vStorageUnknown
> +end

This enum is not used anywhere and can be removed.


ACK. No major problems left. I fixed the minor things I mentioned,
made make syntax-check pass and pushed the result. Thanks!

-- 
Matthias Bolte
http://photron.blogspot.com


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