[libvirt] Re: [Patch v0.4] iSCSI Multi-IQN (Libvirt Support)
Dave Allan
dallan at redhat.com
Mon Nov 16 18:58:08 UTC 2009
Sudhir_Bellad at 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 at dell.com>
> Signed-off-by: Shyam Iyer <shyam_iyer at 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
More information about the libvir-list
mailing list