[libvirt] [PATCH] Refactor ESX storage driver and add iSCSI support
Matthias Bolte
matthias.bolte at googlemail.com
Sat Oct 6 16:58:21 UTC 2012
Finally, here's the second part of my review.
2012/8/20 Ata E Husain Bohra <ata.husain at hotmail.com>:
> diff --git a/src/esx/esx_storage_backend_vmfs.h b/src/esx/esx_storage_backend_vmfs.h
> new file mode 100644
> index 0000000..d3adf73
> --- /dev/null
> +++ b/src/esx/esx_storage_backend_vmfs.h
> @@ -0,0 +1,31 @@
> +/*
> + * esx_storage_backend_vmfs.h: ESX storage backend for VMFS datastores
> + *
> + * Copyright (C) 2007-2008 Red Hat, Inc.
Wrong copyright.
> + * Author: Ata E Husain Bohra (ata.husain at hotmail.com)
> + */
> +
> +#ifndef __ESX_STORAGE_BACKEND_VMFS_H__
> +# define __ESX_STORAGE_BACKEND_VMFS_H__
> +
> +#include "driver.h"
Should be
# include "driver.h"
to make the syntax-check happy.
> diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c
> index 348bd62..d4e81f3 100644
> --- a/src/esx/esx_storage_driver.c
> +++ b/src/esx/esx_storage_driver.c
> -/*
> - * The UUID of a storage pool is the MD5 sum of it's mount path. Therefore,
> - * verify that UUID and MD5 sum match in size, because we rely on that.
> +/**
> + * ESX storage driver implements a facade pattern;
> + * the driver exposes the routines supported by Libvirt
> + * public interface to manage ESX storage devices. Internally
> + * it uses backend drivers to perform the required task.
> */
> -verify(MD5_DIGEST_SIZE == VIR_UUID_BUFLEN);
> +enum {
> + ISCSI = 0,
> + VMFS,
> + LAST_DRIVER
> +};
>
> +static virStorageDriverPtr backendDrv[LAST_DRIVER] = {NULL};
>
Instead of touching backendDrv in each esxStorageOpen and
esxStorageClose call you should just initialize it here:
static virStorageDriverPtr backendDrv[] = {
&esxStorageBackendISCSIDrv,
&esxStorageBackendVMFSDrv
};
> +esxStorageGetBackendDriver(virConnectPtr conn, const char *name,
> + virStorageDriverPtr *backend)
Rename the name parameter to poolName to make its meaning more clear.
Almost all functions call esxStorageGetBackendDriver, so this approach
slows down the dirver usage. A better appraoch would be to store a
pointer to the backend with the virStoragePool and virStorageVol
objects, so the overhead of calling esxStorageGetBackendDriver for
each operation can be avoided.
Currently virStoragePool and virStorageVol objects don't allow to
store a privateData pointer, so we'll need to discuss/implement this
first:
https://www.redhat.com/archives/libvir-list/2012-October/msg00196.html
> @@ -297,65 +256,31 @@ static virStoragePoolPtr
> esxStoragePoolLookupByUUID(virConnectPtr conn, const unsigned char *uuid)
> {
> - if (datastore == NULL) {
> - virUUIDFormat(uuid, uuid_string);
> -
> - virReportError(VIR_ERR_NO_STORAGE_POOL,
> - _("Could not find datastore with UUID '%s'"),
> - uuid_string);
> -
> - goto cleanup;
> - }
> -
> - if (esxVI_GetStringValue(datastore, "summary.name", &name,
> - esxVI_Occurrence_RequiredItem) < 0) {
> - goto cleanup;
> + if (pool == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not find storage pool with uuid '%s'"), uuid);
uuid isn't a printable string here. You need to format it to a string
first using virUUIDFormat as it was done befor your patch here.
> @@ -1432,97 +633,39 @@ esxStorageVolumeDelete(virStorageVolPtr volume, unsigned int flags)
> static int
> esxStorageVolumeWipe(virStorageVolPtr volume, unsigned int flags)
> {
> + if (esxStorageGetBackendDriver(volume->conn, volume->pool, &backend) < 0 ||
> + backend->volDelete(volume , flags) < 0) {
Copy&paste error, should be volWipe instead of volDelete.
> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> index 65e1d9a..0e647d4 100644
> --- a/src/esx/esx_vi.c
> +++ b/src/esx/esx_vi.c
> @@ -3011,7 +3011,7 @@ esxVI_LookupDatastoreByName(esxVI_Context *ctx, const char *name,
>
> if (*datastore == NULL && occurrence != esxVI_Occurrence_OptionalItem) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Could not find datastore with name '%s'"), name);
> + _("Could not find datastore with name '%s'"), name);
Why this indentation change? This changed should be removed from the patch.
> @@ -3118,7 +3118,8 @@ esxVI_LookupDatastoreByAbsolutePath(esxVI_Context *ctx,
> int
> esxVI_LookupDatastoreHostMount(esxVI_Context *ctx,
> esxVI_ManagedObjectReference *datastore,
> - esxVI_DatastoreHostMount **hostMount)
> + esxVI_DatastoreHostMount **hostMount,
> + esxVI_Occurrence occurrence)
> {
> int result = -1;
> esxVI_String *propertyNameList = NULL;
> @@ -3166,9 +3167,9 @@ esxVI_LookupDatastoreHostMount(esxVI_Context *ctx,
> break;
> }
>
> - if (*hostMount == NULL) {
> + if (*hostMount == NULL && occurrence != esxVI_Occurrence_OptionalItem) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Could not lookup datastore host mount"));
> + _("Could not lookup datastore host mount"));
Again, unnecessary indentation change.
> +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) {
> + goto cleanup;
> + }
> +
> + /**
> + * FIXME: deep list copy operation fails with error:
> + * " libvir: ESX Driver error :
> + * internal error Call to esxVI_HostDevice_Free for
> + * unexpected type 'HostScsiDisk' "
> + * HostScsiDisk extends ScsiLun
> + */
> + *ret = scsiLunList;
> + scsiLunList = NULL; /* prevent double free */
You should have reported this problem. There was a bug in the dynamic
dispatch that resulted in dynamic dispatch errors when two types were
not direct ancestors. HostDevice and HostScsiDisk are not directly
related, because ScsiLun sits in between.
This patch should fix this problem. Could you verify this?
https://www.redhat.com/archives/libvir-list/2012-October/msg00197.html
> diff --git a/src/esx/esx_vi_generator.input b/src/esx/esx_vi_generator.input
> index c4a3e56..6e50be5 100644
> object HostIpConfig
> Boolean dhcp r
> String ipAddress o
> String subnetMask o
> -end
Don't remove this end.
> @@ -416,6 +622,19 @@ object HostPortGroupSpec
> Int vlanId r
> String vswitchName r
> HostNetworkPolicy policy r
An end is missing here.
> @@ -467,7 +717,6 @@ object HostVirtualSwitchSpec
> HostVirtualSwitchBridge bridge o
> HostNetworkPolicy policy o
> Int mtu o
> -end
Don't remove this end.
> object ServiceContent
> ManagedObjectReference rootFolder r
> ManagedObjectReference propertyCollector r
> @@ -1135,6 +1434,11 @@ end
> method RemoveVirtualSwitch
> ManagedObjectReference _this r
> String vswitchName r
An end is missing here.
Finally, make sure to run make syntax-check and ensure that it passes.
--
Matthias Bolte
http://photron.blogspot.com
More information about the libvir-list
mailing list