On Fri, Nov 13, 2009 at 05:18:54PM +0530, Sudhir_Bellad@xxxxxxxx wrote: > The following patch set realizes the multi-IQN concept discussed in an > earlier thread > http://www.mail-archive.com/libvir-list@xxxxxxxxxx/msg16706.html > > And here .. > http://www.mail-archive.com/libvir-list@xxxxxxxxxx/msg17499.html > > The patch realizes an XML schema like the one below and allows libvirt > to read through it to create storage pools. > These XMLs when created using a virtualization manager realize unique VM > to storage LUN mappings through a single console and thus opening up > possibilities for the following scenarios - > > * possibility of multiple IQNs for a single Guest > * option for hypervisor's initiator to use these IQNs on behalf of the > guest > > Change Log from v0.3: > 1) Set default tab space to 4 I don't know what you did with indentation but the patch looks completely broken in this respect ... > 2) Use Case Description for Commit Log > 3) Review comments from Dave Allan > a) No initiator iqn in the xml would mean use of the default > initiator iqn name > b) Initiator iqn provided would mean a unique session to be > created using the provided iqn name. > c) Single iSCSI session per pool > 4) Added syntax in doc/schemas/storagepool.rng > > There are no new errors introduced by this patch with "make check" and > "make syntax-check" tests. Please attach patches with mime type text/plain or something so that they can be viewed and commented in line even if attached :-) >From 7d76cba952e67317dc6f19b480a89d0077299cd8 Mon Sep 17 00:00:00 2001 >From: Sudhir Bellad <sudhir_bellad@xxxxxxxx> >Date: Fri, 13 Nov 2009 14:42:49 +0530 >Subject: [PATCH 2/2] ISCSI Multi-IQN > >The following patch set realizes the multi-IQN concept discussed in an earlier thread http://www.mail-archive.com/libvir-list@xxxxxxxxxx/msg16706.html > >And here .. http://www.mail-archive.com/libvir-list@xxxxxxxxxx/msg17499.html > >The patch realizes an XML schema like the one below and allows libvirt to read through it to create storage pools. >These XMLs when created using a virtualization manager realize unique VM to storage LUN mappings through a single console and thus opening up possibilities for the following scenarios - > >* possibility of multiple IQNs for a single Guest >* option for hypervisor's initiator to use these IQNs on behalf of the guest > >Change Log from v0.3: >1) Set default tab space to 4 >2) Use Case Description for Commit Log >3) Review comments from Dave Allan > a) No initiator iqn in the xml would mean use of the default initiator iqn name > b) Initiator iqn provided would mean a unique session to be created using the provided iqn name. > c) Single iSCSI session per pool >4) Added syntax in doc/schemas/storagepool.rng > >There are no new errors introduced by this patch with "make check" and "make syntax-check" tests. > >Signed-off-by: Sudhir Bellad <sudhir_bellad@xxxxxxxx> >Signed-off-by: Shyam Iyer <shyam_iyer@xxxxxxxx> > >--- > 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'> >+ <attribute name='name'> >+ <text/> need indent >+ </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> looks fine from a schemas POV >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) { >+ if (!(dir = opendir(IFACES_DIR))) { >+ if (errno == ENOENT) >+ return 0; >+ virReportSystemError(conn, errno, _("Failed to open dir '%s'"), >+ IFACES_DIR); >+ return -1; >+ } >+ while ((entry = readdir(dir))) { >+ FILE *fp; >+ char path[PATH_MAX]; we try to avoid allocating 4K arrays on the stack >+ if (entry->d_name[0] == '.') >+ continue; >+ >+ sprintf(path,"%s", IFACES_DIR); >+ strcat(path,(const char *)entry->d_name); better to use virAsprintf() from util.h >+ if ((fp = fopen(path, "r"))){ >+ char buff[256]; >+ 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; >+ } >+ } I'm not sure I fully understand the loop (indentation doesn't help !) First you look for all entries under IFACES_DIR, then for each of them you look for the iqnname initiator in it, then if it contains the substring (strange matching) then you execute ISCSIADM ... but you continue with the loop. So possibly you could call ISCSIADM many time when calling virStorageBackendISCSIConnection(). Is that really expected ? >+ } >+ 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; > } indentation is completely broken, apparently your editor doesn't show properly tab from spaces nor extra spaces at end of lines. >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); >+ } >+ need to cut long line I would use "string(./initiator/iqnname[1]/@name)" in case there is an error with extra iqnname element(s). So if the option is set but there is no iqnname, the structure just get NULL and it's fine, right ? > 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; Using a full struct for just embbedding the initiator name may be a bit overkill, but that's just cosmetic. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list