[libvirt] libvirt [PATCHv2 1/2] Refactor ESX storage driver to implement facade pattern

[Matthias Bolte]

2012/11/10 Ata E Husain Bohra <ata.husain at 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)
>
> One of the task base storage driver need to do is to decide which backend
> driver needs to be invoked for a given request. This approach extends
> virStoragePool and virStorageVol to store extra parameters:
> 1. privateData: stores pointer to respective backend storage driver.
> 2. privateDataFreeFunc: stores cleanup function pointer.
>
> virGetStoragePool and virGetStorageVol are modfiied to accept these extra
> parameters as user params. virStoragePoolDispose and virStorageVolDispose
> checks for cleanup operation if available.
> ---
>  daemon/remote.c                    |    6 +-
>  src/Makefile.am                    |    1 +
>  src/conf/storage_conf.c            |    3 +-
>  src/datatypes.c                    |   25 +-
>  src/datatypes.h                    |   24 +-
>  src/esx/esx_driver.c               |    6 +-
>  src/esx/esx_storage_backend_vmfs.c | 1491 ++++++++++++++++++++++++++++++++++++
>  src/esx/esx_storage_backend_vmfs.h |   30 +
>  src/esx/esx_storage_driver.c       | 1319 +++++--------------------------
>  src/esx/esx_vi.c                   |    5 +-
>  src/esx/esx_vi.h                   |    3 +-
>  src/parallels/parallels_storage.c  |   24 +-
>  src/remote/remote_driver.c         |    6 +-
>  src/storage/storage_driver.c       |   28 +-
>  src/test/test_driver.c             |   30 +-
>  src/vbox/vbox_tmpl.c               |   14 +-
>  16 files changed, 1843 insertions(+), 1172 deletions(-)
>  create mode 100644 src/esx/esx_storage_backend_vmfs.c
>  create mode 100644 src/esx/esx_storage_backend_vmfs.h

> @@ -444,6 +451,10 @@ virStoragePoolDispose(void *obj)
>
>      VIR_FREE(pool->name);
>      virObjectUnref(pool->conn);
> +
> +    if (pool->privateDataFreeFunc) {
> +        pool->privateDataFreeFunc(pool->privateData);
> +    }

This should be done before VIR_FREE(pool->name), because the private
data might store pointer to the owning storage pool and the given free
function might access the storage pool. Calling the free function
before starting to dispose the storage object presents the free
function via a valid storage pool object.

> @@ -520,6 +537,10 @@ virStorageVolDispose(void *obj)
>      VIR_FREE(vol->name);
>      VIR_FREE(vol->pool);
>      virObjectUnref(vol->conn);
> +
> +    if (vol->privateDataFreeFunc) {
> +        vol->privateDataFreeFunc(vol->privateData);
> +    }

The same comment as for virStoragePoolDispose applies here too.

Anyway, I split the changes to virStoragePool/Vol into a separate
patch, applied the mentioned changes, and pushed it.

> diff --git a/src/esx/esx_storage_backend_vmfs.c b/src/esx/esx_storage_backend_vmfs.c
> new file mode 100644
> index 0000000..d934e57
> --- /dev/null
> +++ b/src/esx/esx_storage_backend_vmfs.c
> @@ -0,0 +1,1491 @@
> +
> +/*
> + * esx_storage_backend_vmfs.c: ESX backend storage driver for
> + *                             managing VMFS datastores
> + *
> + * Copyright (C) 2012 Ata E Husain Bohra <ata.husain at hotmail.com>

Better than before, but as this file contains mostly code from
esx_storage_driver.c with your modifications, the original copyrights
from esx_storage_driver.c still applies here and have to be mentioned.

> +virStorageDriver esxStorageBackendVMFSDrv = {
> +    .name = "ESX VMFS backend",
> +    .open = NULL, /* 1.0.0 */
> +    .close = NULL, /* 1.0.0 */
> +    .numOfPools = esxStorageBackendVMFSNumberOfStoragePools, /* 1.0.0 */
> +    .listPools = esxStorageBackendVMFSListStoragePools, /* 1.0.0 */
> +    .poolLookupByName = esxStorageBackendVMFSPoolLookupByName, /* 1.0.0 */
> +    .poolLookupByUUID = esxStorageBackendVMFSPoolLookupByUUID, /* 1.0.0 */
> +    .poolRefresh = esxStorageBackendVMFSPoolRefresh, /* 1.0.0 */
> +    .poolGetInfo = esxStorageBackendVMFSPoolGetInfo, /* 1.0.0 */
> +    .poolGetXMLDesc = esxStorageBackendVMFSPoolGetXMLDesc, /* 1.0.0 */
> +    .poolNumOfVolumes = esxStorageBackendVMFSPoolNumberOfStorageVolumes, /* 1.0.0 */
> +    .poolListVolumes = esxStorageBackendVMFSPoolListStorageVolumes, /* 1.0.0 */
> +    .volLookupByName = esxStorageBackendVMFSVolumeLookupByName, /* 1.0.0 */
> +    .volLookupByKey = esxStorageBackendVMFSVolumeLookupByKey, /* 1.0.0 */
> +    .volLookupByPath = esxStorageBackendVMFSVolumeLookupByPath, /* 1.0.0 */
> +    .volCreateXML = esxStorageBackendVMFSVolumeCreateXML, /* 1.0.0 */
> +    .volCreateXMLFrom = esxStorageBackendVMFSVolumeCreateXMLFrom, /* 1.0.0 */
> +    .volGetXMLDesc = esxStorageBackendVMFSVolumeGetXMLDesc, /* 1.0.0 */
> +    .volDelete = esxStorageBackendVMFSVolumeDelete, /* 1.0.0 */
> +    .volWipe = esxStorageBackendVMFSVolumeWipe, /* 1.0.0 */
> +    .volGetPath = esxStorageBackendVMFSVolumeGetPath, /* 1.0.0 */

The old version annotation should be used here, because non of this
functions is new in 1.0.0. They just got refactored.

Also volGetInfo is missing here.

You sticked to the pattern with cleanup label in esx_storage_driver.c.
This can be rewritten to simpler code like this:

static char *
esxStoragePoolGetXMLDesc(virStoragePoolPtr pool, unsigned int flags)
{
    esxPrivate *priv = pool->conn->storagePrivateData;
    virStorageDriverPtr backend = pool->privateData;

    virCheckNonNullArgReturn(pool->privateData, NULL);

    if (esxVI_EnsureSession(priv->primary) < 0) {
        return NULL;
    }

    return backend->poolGetXMLDesc(pool, flags);
}

You missed to add esx_storage_backend_vmfs.c to po/POTFILES.in, make
syntax-check complains about this.

Finally, ACK and to save yet another round trip I fixed all my
comments and pushed the result.

Sorry, for taking soo long to get progress on this and thanks for
keeping up with it :)

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