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

Re: [libvirt] [PATCH] Refactor ESX storage driver and add iSCSI support



2012/8/20 Ata E Husain Bohra <ata husain hotmail com>:
> The patch refactors the current ESX storage driver due to following reasons:
>
> 1. Given most of the public APIs exposed by the storage driver in Libvirt
>    remains same, ESX storage driver should not implement logic specific
>    for only one supported format (current implementation only supports VMFS).
> 2. Decoupling interface from specific storage implementation gives us an
>    extensible design to hook implementation for other supported storage
>    formats.
>
> This patch refactors the current driver to implement it as a facade pattern i.e.
> the driver exposes all the public libvirt APIs, but uses backend drivers to get
> the required task done. The backend drivers provide implementation specific to
> the type of storage device.
>
> File changes:
> ------------------
>  esx_storage_driver.c ----> esx_storage_driver.c (base storage driver)
>                       |
>                       |---> esx_storage_backend_vmfs.c (VMFS backend)
>                       |
>                       |---> esx_storage_backend_iscsi.c (iSCSI backend)
>
> 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
> ---

Sorry, that it took me so long to start reviewing your work.

> diff --git a/src/esx/esx_storage_backend_iscsi.c b/src/esx/esx_storage_backend_iscsi.c
> new file mode 100644
> index 0000000..4f79a0f
> --- /dev/null
> +++ b/src/esx/esx_storage_backend_iscsi.c
> @@ -0,0 +1,794 @@
> +/*
> + * esx_storage_backend_iscsi.c: ESX storage backend for iSCSI handling
> + *
> + * Copyright (C) 2007-2008, 2010-2012 Red Hat, Inc.

I think this is wrong. This is all new code, isn't it? So the
copyright line should read as this

Copyright (C) 2012 Ata E Husain Bohra <your email address>

> +#include <string.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/stat.h>

You're not using anything from sys/stat.h, so this include can be removed.

> +static int
> +esxStorageBackendISCSINumberOfStoragePools(virConnectPtr conn)
> +{
> +    int count = 0;
> +    esxPrivate *priv = conn->storagePrivateData;
> +    esxVI_HostInternetScsiHba *hostInternetScsiHba = NULL;
> +    bool success = false;
> +
> +    if (esxVI_LookupHostInternetScsiHba(
> +          priv->primary, &hostInternetScsiHba) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +          _("Unable to obtain iSCSI adapter"));
> +        goto cleanup;
> +    }
> +
> +    if (hostInternetScsiHba == NULL) {
> +        /* iSCSI adapter may not be enabled for this host */
> +        return 0;
> +    }

This logic suggests that there can only be at most one
HostInternetScsiHba per ESX server. Is that really true?

> +    if (hostInternetScsiHba->configuredStaticTarget) {
> +        const esxVI_HostInternetScsiHbaStaticTarget *target = NULL;
> +        for (target = hostInternetScsiHba->configuredStaticTarget;
> +             target != NULL; target = target->_next) {
> +            ++count;
> +        }
> +    }

The if around the for loop is not necessary ans can be removed.

> +static int
> +esxStorageBackendISCSIListStoragePools(virConnectPtr conn,
> +                                       char **const names,
> +                                       const int maxnames)
> +{

> +    if (hostInternetScsiHba->configuredStaticTarget) {
> +        const esxVI_HostInternetScsiHbaStaticTarget *target = NULL;
> +        for (target = hostInternetScsiHba->configuredStaticTarget;
> +          target != NULL && count < maxnames;
> +          target = target->_next, ++count) {
> +            names[count] = strdup(target->iScsiName);
> +
> +            if (names[count] == NULL) {
> +                virReportOOMError();
> +                goto cleanup;
> +            }
> +        }
> +    }

The if around the for loop is not necessary ans can be removed.


> +
> +static virStoragePoolPtr
> +esxStorageBackendISCSIPoolLookupByName(virConnectPtr conn,
> +                                       const char *name)
> +{
> +    esxPrivate *priv = conn->storagePrivateData;
> +    esxVI_HostInternetScsiHbaStaticTarget *target = NULL;
> +    /* MD5_DIGEST_SIZE = VIR_UUID_BUFLEN = 16 */
> +    unsigned char md5[MD5_DIGEST_SIZE];
> +    virStoragePoolPtr pool = NULL;
> +
> +    if (esxVI_LookupHostInternetScsiHbaStaticTargetByName(
> +          priv->primary, name, &target, esxVI_Occurrence_OptionalItem) < 0) {
> +        goto cleanup;
> +    }
> +
> +    /**
> +     * HostInternetScsiHbaStaticTarget does not provide a uuid field,
> +     * but iSsiName (or widely known as IQN) is unique across the multiple

Typo in 'iSsiName'.

> +     * hosts, using it to compute key
> +     */
> +
> +    md5_buffer(target->iScsiName, strlen(target->iScsiName), md5);

esxVI_LookupHostInternetScsiHbaStaticTargetByName has an occurrence
parameter that is set to esxVI_Occurrence_OptionalItem. This means
that esxVI_LookupHostInternetScsiHbaStaticTargetByName is allowed to
return target as NULL. But you're blindly dereferencing it here. I
think occurrence should be passed as esxVI_Occurrence_RequiredItem
here instead.

> +static virStoragePoolPtr
> +esxStorageBackendISCSIPoolLookupByUUID(virConnectPtr conn,
> +                                       const unsigned char *uuid)
> +{

> +    if (hostInternetScsiHba->configuredStaticTarget) {
> +        for (target = hostInternetScsiHba->configuredStaticTarget;
> +          target != NULL; target = target->_next) {
> +            md5_buffer(target->iScsiName, strlen(target->iScsiName), md5);
> +
> +            if (memcmp(uuid, md5, VIR_UUID_STRING_BUFLEN) == 0) {
> +                break;
> +            }
> +        }
> +    }

Again, the if around the for is not necessary here.

> +    if (target == NULL) {
> +        /* pool not found */
> +        goto cleanup;
> +    }

Error reporting is missing here.

> +static int
> +esxStorageBackendISCSIPoolGetInfo(virStoragePoolPtr pool ATTRIBUTE_UNUSED,
> +                                  virStoragePoolInfoPtr info)
> +{
> +    /* these fields are not valid for iSCSI pool */
> +    info->allocation = info->capacity = info->available = 0;
> +    info->state = esxVI_Boolean_True;

This is wrong. info->state should be VIR_STORAGE_POOL_RUNNING or
another value from the virStoragePoolState if the state can be
determined in more detail.

> +static char *
> +esxStorageBackendISCSIPoolGetXMLDesc(virStoragePoolPtr pool, unsigned int flags)
> +{

> +    if (hostInternetScsiHba->configuredStaticTarget) {
> +        for (target = hostInternetScsiHba->configuredStaticTarget;
> +             target != NULL; target = target->_next) {
> +            if (STREQ(target->iScsiName, pool->name)) {
> +                break;
> +            }
> +        }
> +    }

Again, the if around the for is not necessary here.

> +    if (target == NULL) {
> +        goto cleanup;
> +    }

Error reporting is missing here.

> +static int
> +esxStorageBackendISCSIPoolListStorageVolumes(virStoragePoolPtr pool,
> +                                             char **const names,
> +                                             int maxnames)
> +{

> +    if (esxVI_LookupScsiLunList(priv->primary, &scsiLunList) < 0) {
> +        goto cleanup;
> +    }
> +
> +    /* O^2 but still faster than hash given N is not that large */
> +    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);

What does a typical device name look like? Is it easily
distinguishable from the volume names of the VMFS backend driver? Can
you give some examples?


> +static virStorageVolPtr
> +esxStorageBackendISCSIVolumeLookupByName(virStoragePoolPtr pool,
> +                                         const char *name)
> +{

> +    for (scsiLun = scsiLunList; scsiLun != NULL;
> +         scsiLun = scsiLun->_next) {
> +        if (STREQ(scsiLun->deviceName, name)) {
> +            /**
> +             * ScsiLun provides an UUID field that is unique accross
> +             * multiple servers. But this field length is ~55 characters
> +             * compute MD5 hash to transform it to an acceptable
> +             * libvirt format
> +             */
> +            md5_buffer(scsiLun->uuid, strlen(scsiLun->uuid), md5);
> +
> +            virUUIDFormat(md5, uuid_string);

I'm not sure if this is the best approach. What does a typical ScsiLun
look like? Can you give some examples?


> +static virStorageVolPtr
> +esxStorageBackendISCSIVolumeLookupByPath(virConnectPtr conn, const char *path)
> +{
> +    virStorageVolPtr volume = NULL;
> +    esxPrivate *priv = conn->storagePrivateData;
> +    char *poolName = NULL;
> +    esxVI_ScsiLun *scsiLunList = NULL;
> +    const esxVI_ScsiLun *scsiLun = NULL;
> +    const esxVI_HostScsiDisk *hostScsiDisk = NULL;

Remove the const here...

> +    /* MD5_DIGEST_SIZE = VIR_UUID_BUFLEN = 16 */
> +    unsigned char md5[MD5_DIGEST_SIZE];
> +    char uuid_string[VIR_UUID_STRING_BUFLEN] = "";
> +
> +    if (esxVI_LookupScsiLunList(priv->primary, &scsiLunList) < 0) {
> +        goto cleanup;
> +    }
> +
> +    for (scsiLun = scsiLunList ; scsiLun != NULL;
> +         scsiLun = scsiLun->_next) {
> +         hostScsiDisk =
> +            esxVI_HostScsiDisk_DynamicCast((esxVI_ScsiLun *)scsiLun);

.. so that the (esxVI_ScsiLun *) cast is no longer necessary.

> +        if (hostScsiDisk != NULL &&
> +            STREQ(hostScsiDisk->devicePath, path)) {
> +            /* Found matching device */
> +            if (esxVI_LookupStoragePoolNameByScsiLunKey(
> +                  priv->primary, hostScsiDisk->key, &poolName) < 0) {
> +                goto cleanup;
> +            }
> +
> +            md5_buffer(scsiLun->uuid, strlen(scsiLun->uuid), md5);
> +
> +            virUUIDFormat(md5, uuid_string);
> +
> +            volume = virGetStorageVol(conn, poolName, path, uuid_string);

Passing the path is not correct I think, assuming that the volume name
and path are not the same for iSCSI.

> +virStorageDriver esxStorageBackendISCSIDrv = {
> +    .name = "ESX ISCSI backend",
> +    .open = NULL, /* 0.10.0 */
> +    .close = NULL, /* 0.10.0 */
> +    .numOfPools = esxStorageBackendISCSINumberOfStoragePools, /* 0.10.0 */
> +    .listPools = esxStorageBackendISCSIListStoragePools, /* 0.10.0 */
> +    .poolLookupByName = esxStorageBackendISCSIPoolLookupByName, /* 0.10.0 */
> +    .poolLookupByUUID = esxStorageBackendISCSIPoolLookupByUUID, /* 0.10.0 */
> +    .poolRefresh = esxStorageBackendISCSIPoolRefresh, /* 0.10.0 */
> +    .poolGetInfo = esxStorageBackendISCSIPoolGetInfo, /* 0.10.0 */
> +    .poolGetXMLDesc = esxStorageBackendISCSIPoolGetXMLDesc, /* 0.10.0 */
> +    .poolNumOfVolumes = esxStorageBackendISCSIPoolNumberOfStorageVolumes, /* 0.10.0 */
> +    .poolListVolumes = esxStorageBackendISCSIPoolListStorageVolumes, /* 0.10.0 */
> +    .volLookupByName = esxStorageBackendISCSIVolumeLookupByName, /* 0.10.0 */
> +    .volLookupByKey = esxStorageBackendISCSIVolumeLookupByKey, /* 0.10.0 */
> +    .volLookupByPath = esxStorageBackendISCSIVolumeLookupByPath, /* 0.10.0 */
> +    .volCreateXML = esxStorageBackendISCSIVolumeCreateXML, /* 0.10.0 */
> +    .volCreateXMLFrom = esxStorageBackendISCSIVolumeCreateXMLFrom, /* 0.10.0 */
> +    .volGetXMLDesc = esxStorageBackendISCSIVolumeGetXMLDesc, /* 0.10.0 */
> +    .volDelete = esxStorageBackendISCSIVolumeDelete, /* 0.10.0 */
> +    .volWipe = esxStorageBackendISCSIVolumeWipe, /* 0.10.0 */
> +    .volGetPath = esxStorageBackendISCSIVolumeGetPath, /* 0.10.0 */
> +
> +};

The version number here are outdated now. It should be 0.10.2.

> diff --git a/src/esx/esx_storage_backend_iscsi.h b/src/esx/esx_storage_backend_iscsi.h
> new file mode 100644
> index 0000000..ca756ac
> --- /dev/null
> +++ b/src/esx/esx_storage_backend_iscsi.h
> @@ -0,0 +1,31 @@
> +/*
> + * esx_storage_backend_iscsi.h: ESX storage backend for iSCSI handling
> + *
> + * Copyright (C) 2007-2008 Red Hat, Inc.

Again, that's not the right copyright notice.

> diff --git a/src/esx/esx_storage_backend_vmfs.c b/src/esx/esx_storage_backend_vmfs.c
> new file mode 100644
> index 0000000..6550196
> --- /dev/null
> +++ b/src/esx/esx_storage_backend_vmfs.c
> @@ -0,0 +1,1471 @@
> +
> +/*
> + * esx_storage_backend_vmfs.c: ESX storage backend for VMFS datastores
> + *
> + * Copyright (C) 2010-2011 Red Hat, Inc.
> + * Copyright (C) 2010 Matthias Bolte <matthias bolte googlemail com>

Put your copyright line here in the form of

Copyright (C) 2012 Ata E Husain Bohra <your email address>

instead of an author list on the end of the license statement.

> +#include <string.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/stat.h>

This include is unused and can be removed.


This is as far as I got today with the review. I'll continue in the
next days, hopefully it won't take me weeks again to get back to this
:)

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


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