[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



Sudhir_Bellad Dell com wrote:
The following patch set realizes the multi-IQN concept discussed in an
earlier thread
http://www.mail-archive.com/libvir-list redhat com/msg16706.html

And here ..
http://www.mail-archive.com/libvir-list redhat com/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 dell com>
Signed-off-by: Shyam Iyer <shyam_iyer dell com>



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]