12:39 p.m.
2012/8/20 Ata E Husain Bohra <ata.husain(a)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(a)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