[libvirt] [PATCH] Refactor ESX storage driver and add iSCSI support

Sun Sep 9 20:12:47 UTC 2012

2012/9/9 Ata Bohra <ata.husain at hotmail.com>
>
> Please see inline.

> > > + 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.

No that's not what i meant. I didn't mean the {} around the body of
the for loop. I meant the if around the for loop:

Your code has this structure

if (...) {
    for (...) {
    }
}

I meant to simplify it to

for (...) {
}

because the condition of the if is already tested as part of the for
loop conditional part. So there is no gain in wrapping the for in an
additional if here.

> > > + * 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.

Ah, I didn't see the whole concept. So I misunderstood how this is
supposed to work. Now it makes sense that there is no error reporting
in some places.

> > > +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.

That's good. So the different backends can easily detect that a volume
is meant for them.

> >
> > > +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:

Yes, I meant the ScsiLun's 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".

Okay, no I'm convinced that taking the MD5 sum of this long UUID is a
good and robust approach.

> >
> > > +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.

Technically you're right. The problem I see here is that the libvirt
codebase doesn't follow this more strict const handling and trying to
apply it in some places with just result in additional cast to remove
the const again. Therefore, I'd suggest to not apply this stricter
const handling.

> >
> > > + /* 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.

Ah, okay. I wasn't aware of that. I don't have an iSCSI setup at hand
at the moment. So I cannot test how things work and need to rely on
your knowledge in this aspect.

--
Matthias Bolte
http://photron.blogspot.com