2012/8/20 Ata E Husain Bohra <ata.husain@xxxxxxxxxxx>: > 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@xxxxxxxxxxxxxx> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list