[libvirt] Re: [Patch v0.4] iSCSI Multi-IQN (Libvirt Support)

Sudhir_Bellad(a)Dell.com wrote: --- docs/schemas/storagepool.rng | 10 +++++++ src/storage_backend_iscsi.c | 59 +++++++++++++++++++++++++++++++++++++----- src/storage_backend_iscsi.h | 2 + src/storage_conf.c | 20 ++++++++++---- src/storage_conf.h | 9 ++++++ 5 files changed, 87 insertions(+), 13 deletions(-) diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index d225f97..ea14f31 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -175,6 +175,15 @@ </element> </define> + <define name='initiatorinfoiqnname'> + <element name='iqnname'> Please make this element iqn, not iqnname. IQN is iSCSI Qualified Name so iqnname is redundant. + <attribute name='name'> + <text/> + </attribute> + <empty/> + </element> + </define> + <define name='devextents'> <oneOrMore> <element name='freeExtent'> @@ -328,6 +337,7 @@ <element name='source'> <ref name='sourceinfohost'/> <ref name='sourceinfodev'/> + <ref name='initiatorinfoiqnname'/> </element> </define> diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index b516add..1fb21a5 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -39,6 +39,10 @@ #include "storage_backend_iscsi.h" #include "util.h" #include "memory.h" +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h> #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -159,13 +163,54 @@ virStorageBackendISCSIConnection(virConnectPtr conn, const char *portal, const char *action) { - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, action, NULL - }; - - if (virRun(conn, cmdargv, NULL) < 0) - return -1; + DIR *dir; + struct dirent *entry; + + + if (pool->def->source.initiator.iqnname != NULL) { What's the point of this loop? At best, it's unneeded complexity, at worst it will match multiple interface names which will create the multiple sessions per pool scenario that I explicitly want to avoid. Secondly, if you want to do some sort of validation of the iqn, why are you reading from a hardcoded directory? Can you use the output of iscsiadm? That is likely to be a more stable interface than the directory which I would expect is a compile time option to the iscsi initiator. + if (!(dir = opendir(IFACES_DIR))) { + if (errno == ENOENT) + return 0; It's not success if the directory doesn't exist; what's the purpose of this special case? + virReportSystemError(conn, errno, _("Failed to open dir '%s'"), + IFACES_DIR); + return -1; + } + while ((entry = readdir(dir))) { + FILE *fp; + char path[PATH_MAX]; Agreed with DV, don't allocate this memory on the stack. + if (entry->d_name[0] == '.') + continue; + + sprintf(path,"%s", IFACES_DIR); + strcat(path,(const char *)entry->d_name); + + if ((fp = fopen(path, "r"))){ + char buff[256]; Don't allocate this buffer on the stack, either. + while (fp != NULL && fgets(buff, sizeof(buff), fp) != NULL) { + if (strstr(buff, pool->def->source.initiator.iqnname) != NULL) { + const char *const cmdargv[] = { + ISCSIADM, "--mode", "node", "--portal", portal, + "--targetname", pool->def->source.devices[0].path, "-I", entry->d_name, action, NULL + }; + + if (virRun(conn, cmdargv, NULL) < 0) + return -1; + } + } + } + fclose(fp); + } + closedir(dir); + } + else{ + const char *const cmdargv[] = { + ISCSIADM, "--mode", "node", "--portal", portal, + "--targetname", pool->def->source.devices[0].path, action, NULL + }; + if (virRun(conn, cmdargv, NULL) < 0) + return -1; + } return 0; } I'm still puzzled as to why this patch isn't limited to extending the schema and the 5 or so lines of code required create the iscsiadm command with an additional -I parameter. diff --git a/src/storage_backend_iscsi.h b/src/storage_backend_iscsi.h index 665ed13..1bb8892 100644 --- a/src/storage_backend_iscsi.h +++ b/src/storage_backend_iscsi.h @@ -28,4 +28,6 @@ extern virStorageBackend virStorageBackendISCSI; +#define IFACES_DIR "/var/lib/iscsi/ifaces/" + #endif /* __VIR_STORAGE_BACKEND_ISCSI_H__ */ diff --git a/src/storage_conf.c b/src/storage_conf.c index 1633aac..db191e6 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -106,11 +106,12 @@ struct _virStorageVolOptions { /* Flags to indicate mandatory components in the pool source */ enum { - VIR_STORAGE_POOL_SOURCE_HOST = (1<<0), - VIR_STORAGE_POOL_SOURCE_DEVICE = (1<<1), - VIR_STORAGE_POOL_SOURCE_DIR = (1<<2), - VIR_STORAGE_POOL_SOURCE_ADAPTER = (1<<3), - VIR_STORAGE_POOL_SOURCE_NAME = (1<<4), + VIR_STORAGE_POOL_SOURCE_HOST = (1<<0), + VIR_STORAGE_POOL_SOURCE_DEVICE = (1<<1), + VIR_STORAGE_POOL_SOURCE_DIR = (1<<2), + VIR_STORAGE_POOL_SOURCE_ADAPTER = (1<<3), + VIR_STORAGE_POOL_SOURCE_NAME = (1<<4), + VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN = (1<<5), }; @@ -179,7 +180,8 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { { .poolType = VIR_STORAGE_POOL_ISCSI, .poolOptions = { .flags = (VIR_STORAGE_POOL_SOURCE_HOST | - VIR_STORAGE_POOL_SOURCE_DEVICE), + VIR_STORAGE_POOL_SOURCE_DEVICE | + VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN), }, .volOptions = { .formatToString = virStoragePoolFormatDiskTypeToString, @@ -283,6 +285,7 @@ virStoragePoolSourceFree(virStoragePoolSourcePtr source) { VIR_FREE(source->dir); VIR_FREE(source->name); VIR_FREE(source->adapter); + VIR_FREE(source->initiator.iqnname); if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) { VIR_FREE(source->auth.chap.login); @@ -532,6 +535,11 @@ virStoragePoolDefParseXML(virConnectPtr conn, goto cleanup; } } + + if (options->flags & VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN) { + ret->source.initiator.iqnname = virXPathString(conn, "string(./initiator/iqnname/@name)", ctxt); + } + if (options->flags & VIR_STORAGE_POOL_SOURCE_DEVICE) { xmlNodePtr *nodeset = NULL; int nsource, i; diff --git a/src/storage_conf.h b/src/storage_conf.h index 9fedb12..4152e18 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -182,6 +182,12 @@ struct _virStoragePoolSourceDeviceExtent { int type; /* free space type */ }; +typedef struct _virStoragePoolSourceInitiatorAttr virStoragePoolSourceInitiatorAttr; +struct _virStoragePoolSourceInitiatorAttr { + /* Initiator iqn name */ + char *iqnname; +}; + /* * Pools can be backed by one or more devices, and some * allow us to track free space on underlying devices. @@ -223,6 +229,9 @@ struct _virStoragePoolSource { /* Or a name */ char *name; + /* initiator iqn name */ + virStoragePoolSourceInitiatorAttr initiator; + int authType; /* virStoragePoolAuthType */ union { virStoragePoolAuthChap chap; -- 1.6.2.2