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
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'>
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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list