<html> <head> </head> <body class='hmmessage'><div dir='ltr'> <style></style> <div dir="ltr">Please see inline. <div>> 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. </div><div> </div><div>> > +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 :).</div><div> </div><div>> > + 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. </div><div> > > +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.</div><div> > > +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. </div><div>> > +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".</div><div> </div><div>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.</div><div> > > > +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:</div><div> </div><div>(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"} </div><div>scsilun->uuid looks like:</div><div>(gdb) p scsiLun->uuid $4 = 0x6832a0 "01000000004f69514870322d414d674a2d4e754b61564952545541".</div><div> > > > +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...</div><div>[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:</div><div><table cellSpacing="0"> <tbody> <tr class="r1"><td noWrap=""><a id="deviceName" name="deviceName"></a>"deviceName</td> <td>xsd:string </td> <td> The name of the device on the host. For example, /dev/cdrom or \\serverX\device_name. </td></tr> </tbody> </table></div><div>"</div><div>As mentioned earlier, sample deviceName looks like:</div><div>"/vmfs/devices/disks/t10.F405E46494C45425F49615840723D214D476A4D2E457B416"; it is a valid ESX device path as well.</div><div> </div><div>I will document it to make it more clear.</div><div> > > +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 > <a href="http://photron.blogspot.com">http://photron.blogspot.com</a> </div><div>Thanks again!</div><div> </div><div>Regards,</div><div>Ata</div></div> </div></body> </html>