On Tue, Jan 19, 2010 at 10:51:21AM -0500, David Allan wrote:
In other words, permit the initiator to use a variety of IQNs rather than just the system IQN when creating iSCSI pools. --- docs/schemas/storagepool.rng | 12 ++ src/conf/storage_conf.c | 16 ++- src/conf/storage_conf.h | 9 ++ src/storage/storage_backend_iscsi.c | 253 +++++++++++++++++++++++++++++++++- src/storage/storage_backend_iscsi.h | 4 + 5 files changed, 280 insertions(+), 14 deletions(-)
diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 249bf9c..247664e 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -188,6 +188,15 @@ </element> </define>
+ <define name='initiatorinfoiqn'> + <element name='iqn'> + <attribute name='name'> + <text/> + </attribute> + <empty/> + </element> + </define> +
so the construct is <iqn name="..."/> [...]
@@ -421,6 +424,7 @@ virStoragePoolDefParseSource(virConnectPtr conn, }
source->host.name = virXPathString(conn, "string(./host/@name)", ctxt); + source->initiator.iqn = virXPathString(conn, "string(./initiator/iqn)", ctxt);
While this XPath expression really only capture <iqn>...</iqn> Since the attribute is a good idea to preserve for future expansion, I'm fixing this to "string(./initiator/iqn/@name)" Documentation is also needed for that new construct, it's missing from the patch.
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index b516add..e0788cf 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -33,16 +33,17 @@ [...] +static int +virStorageBackendIQNFound(virConnectPtr conn, + virStoragePoolObjPtr pool, + char **ifacename) +{ + int ret = IQN_MISSING, fd = -1; + char ebuf[64]; + FILE *fp = NULL; + pid_t child = 0; + char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL, + *saveptr = NULL; + const char *const prog[] = { + ISCSIADM, "--mode", "iface", NULL + }; + + if (VIR_ALLOC_N(line, LINE_SIZE) != 0) { + ret = IQN_ERROR; + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Could not allocate memory for output of '%s'"), + prog[0]); + goto out; + } + + memset(line, 0, LINE_SIZE); + + if (virExec(conn, prog, NULL, NULL, &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to run '%s' when looking for existing interface with IQN '%s'"), + prog[0], pool->def->source.initiator.iqn); + + ret = IQN_ERROR; + goto out; + } + + if ((fp = fdopen(fd, "r")) == NULL) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to open stream for file descriptor " + "when reading output from '%s': '%s'"), + prog[0], virStrerror(errno, ebuf, sizeof ebuf)); + ret = IQN_ERROR; + goto out; + } + + while (fgets(line, LINE_SIZE, fp) != NULL) { + newline = strrchr(line, '\n'); + if (newline == NULL) { + ret = IQN_ERROR; + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Unexpected line > %d characters " + "when parsing output of '%s'"), + LINE_SIZE, prog[0]); + goto out; + } + *newline = '\0'; + + iqn = strrchr(line, ','); + if (iqn == NULL) { + continue; + } + iqn++; + + if (STREQ(iqn, pool->def->source.initiator.iqn)) { + token = strtok_r(line, " ", &saveptr); + *ifacename = strdup(token); + if (*ifacename == NULL) { + ret = IQN_ERROR; + virReportOOMError(conn); + goto out; + } + VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn); + ret = IQN_FOUND; + break; + } + } + +out: + if (ret == IQN_MISSING) { + VIR_DEBUG("Could not find interface witn IQN '%s'", iqn); + } +
line is never freed so a VIR_FREE(line); is also needed here
+ if (fp != NULL) { + fclose(fp); + } else { + if (fd != -1) { + close(fd); + } + } + + return ret; +} +
+static int +virStorageBackendCreateIface(virConnectPtr conn, + virStoragePoolObjPtr pool, + char **ifacename)
I'm renaming that routine as virStorageBackendCreateIfaceIQN as it should be called only if pool->def->source.initiator.iqn is non NULL
+{ + int ret = -1, exitstatus = -1; + char temp_ifacename[32]; + + if (virRandomInitialize(time(NULL) ^ getpid()) == -1) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to initialize random generator " + "when creating iscsi interface")); + goto out; + } + + snprintf(temp_ifacename, sizeof(temp_ifacename), "libvirt-iface-%08x", virRandom(1024 * 1024 * 1024)); + *ifacename = strdup(temp_ifacename); + if (*ifacename == NULL) { + virReportOOMError(conn); + goto out; + } + + const char *const cmdargv1[] = { + ISCSIADM, "--mode", "iface", "--interface", + *ifacename, "--op", "new", NULL + }; + + VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'", + *ifacename, pool->def->source.initiator.iqn); + + /* Note that we ignore the exitstatus. Older versions of iscsiadm + * tools returned an exit status of > 0, even if they succeeded. + * We will just rely on whether the interface got created + * properly. */ + if (virRun(conn, cmdargv1, &exitstatus) < 0) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to run command '%s' to create new iscsi interface"), + cmdargv1[0]);
The probability of hitting an existing interface name is fairly small but that still sounds unreliable in the long term. I would suggest as a later improvement to check for the resulting temp_ifacename name values and if it exists, loop before the snprintf. Not needed right now though,
+ goto out;
I tend to think that *ifacename should be freed and NULL'ed when there is an error
+ } + + const char *const cmdargv2[] = { + ISCSIADM, "--mode", "iface", "--interface", *ifacename, + "--op", "update", "--name", "iface.initiatorname", "--value", + pool->def->source.initiator.iqn, NULL + }; + + /* Note that we ignore the exitstatus. Older versions of iscsiadm tools + * returned an exit status of > 0, even if they succeeded. We will just + * rely on whether iface file got updated properly. */ + if (virRun(conn, cmdargv2, &exitstatus) < 0) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to run command '%s' to update iscsi interface with IQN '%s'"), + cmdargv1[0], pool->def->source.initiator.iqn);
same here
+ goto out; + } + + /* Check again to make sure the interface was created. */ + if (virStorageBackendIQNFound(conn, pool, ifacename) != IQN_FOUND) {
Actually, here we leak *ifacename, as virStorageBackendIQNFound() will just overwrite it not even read it. Clearly to me temp_ifacename should not be strdup'ed, and *ifacename should be initialized only by virStorageBackendIQNFound
+ VIR_DEBUG("Failed to find interface '%s' with IQN '%s' " + "after attempting to create it", + *ifacename, pool->def->source.initiator.iqn);
The problem here is that we don't check that *ifacename is equal to temp_ifacename but it's not a problem I think.
+ goto out; + } else { + VIR_DEBUG("Interface '%s' with IQN '%s' was created successfully", + *ifacename, pool->def->source.initiator.iqn); + } + + ret = 0; + +out: + return ret; +}
So I had to seriously revamp that function w.r.t. string usage
+ +static int +virStorageBackendISCSIConnectionIQN(virConnectPtr conn, + virStoragePoolObjPtr pool, + const char *portal, + const char *action) +{ + int ret = -1; + char *ifacename = NULL; + + switch (virStorageBackendIQNFound(conn, pool, &ifacename)) { + case IQN_FOUND: + VIR_DEBUG("ifacename: '%s'", ifacename); + break; + case IQN_MISSING: + if (virStorageBackendCreateIface(conn, pool, &ifacename) != 0) { + goto out; + } + break; + case IQN_ERROR: + default: + goto out; + } + + const char *const sendtargets[] = { + ISCSIADM, "--mode", "discovery", "--type", "sendtargets", "--portal", portal, NULL + }; + if (virRun(conn, sendtargets, NULL) < 0) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to run %s to get target list"), + sendtargets[0]); + goto out; + } + + const char *const cmdargv[] = { + ISCSIADM, "--mode", "node", "--portal", portal, + "--targetname", pool->def->source.devices[0].path, "--interface", + ifacename, action, NULL + }; + + VIR_DEBUG("cmdargv: '%s'", virArgvToString(cmdargv));
removed per second patch,
+ if (virRun(conn, cmdargv, NULL) < 0) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to run command '%s' with action '%s'"), + cmdargv[0], action); + goto out; + } + + ret = 0; + +out:
seems ifacename is leaked here, needs a VIR_FREE(ifacename); though I'm suprised it's not stored.
+ return ret; +} + + static int virStorageBackendISCSIConnection(virConnectPtr conn, virStoragePoolObjPtr pool, const char *portal, const char *action) { - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, action, NULL - }; + int ret = 0;
- if (virRun(conn, cmdargv, NULL) < 0) - return -1; + if (pool->def->source.initiator.iqn != NULL) {
- return 0; + ret = virStorageBackendISCSIConnectionIQN(conn, pool, portal, action); + + } 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) { + ret = -1; + } + + } + + return ret; }
diff --git a/src/storage/storage_backend_iscsi.h b/src/storage/storage_backend_iscsi.h index 665ed13..8878342 100644 --- a/src/storage/storage_backend_iscsi.h +++ b/src/storage/storage_backend_iscsi.h @@ -28,4 +28,8 @@
extern virStorageBackend virStorageBackendISCSI;
+#define IQN_FOUND 1 +#define IQN_MISSING 0 +#define IQN_ERROR -1 + #endif /* __VIR_STORAGE_BACKEND_ISCSI_H__ */
So I made a number of changes, and finally commited the patch enclosed. There is still need for: - documentation of the new option - I'm surprized the IQN name parsed seems never saved, I'm sure we dump the XML for pools somewere and that seems missing - and functional testing of course thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/