Please see inline. > Date: Sun, 9 Sep 2012 19:39:34 +0200
Subject: Re: [libvirt] [PATCH] Refactor ESX storage driver and add iSCSI support From: matthias.bolte@googlemail.com To: ata.husain@hotmail.com CC: libvir-list@redhat.com
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. [AB]: Sure will remove that. > > +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? [AB]: The proposed solution works with software iSCSI adapter, there is only one software iSCSI adapter per ESX host. Thanks for pointing this out, I wanted to add these comments but missed it :). > > + 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. [AB]: I guess I am spoiled with the coding standards followed at my workplace; we try to protect conditional loops with parenthesis even if its a single line code statements. I will make necessary correction if this is not acceptable style with Libvirt community.
+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. [AB]: "poolLookupByName" is also used by esx_storage_drive.c:esxStorageGetBackendDriver(), in this function we want to go over both VMFS as well iSCSI pools list to determine the matching backend driver. I think the flaw is I need to check for valid target pointer (NON-NULL) before deferencing it.
+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? [AB]: Device name looks similar to a device path in linux, for instance on my sample machine device path for a iSCSI lun is "/vmfs/devices/disks/t10.F405E46494C45425F49615840723D214D476A4D2E457B416". As mentioned in one of my comments, technically we can use "scsiLun->canonicalName" as "t10.F405E46494C45425F49615840723D214D476A4D2E457B416" is nothing but a canonicalName for that iSCSI lun. Only reason I used deviceName was it is a mandatory field, whereas, canonicalName is an options field inside scsiLun data object.
+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? [AB]: I think you mean sample scssilun->uuid, but I am printing the compelte structure just incase along with specific mention of scsilun->uuid: (gdb) p *scsiLun $2 = {_next = 0x0, _type = esxVI_Type_HostScsiDisk, deviceName = 0x682da0 "/vmfs/devices/disks/t10.F405E46494C45425F49615840723D214D476A4D2E457B416", deviceType = 0x704af0 "disk", lunType = 0x704b70 "disk", operationalState = 0x707570, uuid = 0x6832a0 "01000000004f69514870322d414d674a2d4e754b61564952545541", alternateName = 0x618660, canonicalName = 0x683160 "t10.F405E46494C45425F49615840723D214D476A4D2E457B416", capabilities = 0x0, descriptor = 0x0, displayName = 0x0, durableName = 0x61b340, key = 0x704b10 "key-vim.host.ScsiDisk-01000000004f69514870322d414d674a2d4e754b61564952545541", model = 0x704bb0 "VIRTUAL-DISK ", queueDepth = 0x0, revision = 0x704bd0 "0 ", scsiLevel = 0x704bf0, serialNumber = 0x704c10 "unavailable", standardInquiry = 0x705590, vendor = 0x704b90 "OPNFILER"} scsilun->uuid looks like:(gdb) p scsiLun->uuid $4 = 0x6832a0 "01000000004f69514870322d414d674a2d4e754b61564952545541".
+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...[AB]: As the variable is used just to iterate the list IMHO "const" assures the reader that the memory need not to be freed. I aggree there is an extra static_cast needed to perform "dynamic_cast" operation but I thought code readibility will surpass ths cost. Please let me know if this can be accepted.
+ /* 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. [AB]: For user deviceName and devicePath should be same. HostDevice dataobjects defines deviceName as:
"deviceName xsd:string The name of the device on the host. For example, /dev/cdrom or \\serverX\device_name. "As mentioned earlier, sample deviceName looks like:"/vmfs/devices/disks/t10.F405E46494C45425F49615840723D214D476A4D2E457B416"; it is a valid ESX device path as well. I will document it to make it more clear.
+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 [AB]: I intentionally left it "unchanged". Almost all the code written in this file is written by you, I just renamed the file and I do not want to put myself as an author just for renaming the filename :). 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 :) [AB]: I highly appreciate your attention and time on my patches. I'm looking forward for your comments on above mentioned points before updating a newer version for the patch. -- Matthias Bolte http://photron.blogspot.com Thanks again! Regards,Ata