6:38 p.m.
On 06/29/2018 11:01 AM, Michal Privoznik wrote:
When scanning for targets, iSCSI might give different results
depending on the interface used. This is basically just name of
assuming this means whether the initiatoriqn interface was used
config file under /etc/iscsi/ifaces to use. The file contains
initiator IQN thus different results claim.
Strange to have activity here - was there a bz? Or something found by
the (I assume) GSoC project:
https://www.redhat.com/archives/libvir-list/2018-June/msg00249.html
Touching something that's been avoided for 8 years is always scary, but
if it's broken, then sure it ought to be fixed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 4 +-
src/util/viriscsi.c | 78 ++++++++++++++++++++++++++++++++++---
src/util/viriscsi.h | 1 +
tests/viriscsitest.c | 2 +-
4 files changed, 77 insertions(+), 8 deletions(-)
This patch fails to compile for me:
In file included from util/viriscsi.c:32:0:
util/viriscsi.c: In function 'virISCSIScanTargets':
util/virerror.h:187:5: error: this statement may fall through
[-Werror=implicit-fallthrough=]
virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
__FUNCTION__, __LINE__, __VA_ARGS__)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
util/viriscsi.c:479:13: note: in expansion of macro 'virReportError'
virReportError(VIR_ERR_OPERATION_FAILED,
^~~~~~~~~~~~~~
util/viriscsi.c:482:9: note: here
case IQN_ERROR:
^~~~
diff --git a/src/storage/storage_backend_iscsi.c
b/src/storage/storage_backend_iscsi.c
index 7871d1915b..3b9dddb4fd 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -194,7 +194,9 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec,
if (!(portal = virStorageBackendISCSIPortal(source)))
goto cleanup;
- if (virISCSIScanTargets(portal, &ntargets, &targets) < 0)
+ if (virISCSIScanTargets(portal,
+ source->initiator.iqn,
NIT: This could go on the previous line
+ &ntargets, &targets) < 0)
goto cleanup;
if (VIR_ALLOC_N(list.sources, ntargets) < 0)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 44788056fd..365669aac2 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -40,6 +40,13 @@
VIR_LOG_INIT("util.iscsi");
+static int
+virISCSIScanTargetsInternal(const char *portal,
+ const char *ifacename,
+ size_t *ntargetsret,
+ char ***targetsret);
+
+
struct virISCSISessionData {
char *session;
const char *devpath;
@@ -286,9 +293,10 @@ virISCSIConnection(const char *portal,
* iscsiadm doesn't let you send commands to the Interface IQN,
* unless you've first issued a 'sendtargets' command to the
* portal. Without the sendtargets all that is received is a
- * "iscsiadm: No records found"
+ * "iscsiadm: No records found". However, we must ensure that
+ * the command is issued over interface name we invented above.
"invented" - again is this the make sure it's issued over the
initiatoriqn interface?
*/
- if (virISCSIScanTargets(portal, NULL, NULL) < 0)
+ if (virISCSIScanTargetsInternal(portal, ifacename, NULL, NULL) < 0)
goto cleanup;
break;
@@ -371,10 +379,11 @@ virISCSIGetTargets(char **const groups,
}
-int
-virISCSIScanTargets(const char *portal,
- size_t *ntargetsret,
- char ***targetsret)
+static int
+virISCSIScanTargetsInternal(const char *portal,
+ const char *ifacename,
+ size_t *ntargetsret,
+ char ***targetsret)
{
/**
*
@@ -400,6 +409,12 @@ virISCSIScanTargets(const char *portal,
"--op",
"nonpersistent",
NULL);
+ if (ifacename) {
+ virCommandAddArgList(cmd,
+ "--interface", ifacename,
+ NULL);
+ }
+
Could just be one line to avoid the { }
memset(&list, 0, sizeof(list));
if (virCommandRunRegex(cmd,
@@ -425,6 +440,57 @@ virISCSIScanTargets(const char *portal,
return ret;
}
+
+/**
+ * virISCSIScanTargets:
+ * @portal: iSCSI portal
+ * @initiatoriqn: Initiator IQN
+ * @ntargets: number of items in @targetsret array
+ * @targets: array of targets
+ *
+ * For given @portal issue sendtargets command. Optionally,
+ * @initiatoriqn can be set to override default configuration.
+ * The targets are stored into @targets array and the size of
+ * the array is stored into @ntargets.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (with error reported)
+ */
+int
+virISCSIScanTargets(const char *portal,
+ const char *initiatoriqn,
+ size_t *ntargets,
+ char ***targets)
+{
+ char *ifacename = NULL;
+ int ret = -1;
+
+ if (ntargets)
+ *ntargets = 0;
+ if (targets)
+ *targets = NULL;
+
+ if (initiatoriqn) {
+ switch ((virStorageBackendIQNFound(initiatoriqn, &ifacename))) {
+ case IQN_FOUND:
+ break;
+
+ case IQN_MISSING:
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("no iSCSI interface defined for IQN %s"),
+ initiatoriqn);
ATTRIBUTE_FALLTHROUGH;
With a couple of adjustments and some more explanation why we're here -
as in what is failing in the current scheme,
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
+ case IQN_ERROR:
+ default:
+ return -1;
+ }
+ }
+
+ ret = virISCSIScanTargetsInternal(portal, ifacename, ntargets, targets);
+ VIR_FREE(ifacename);
+ return ret;
+}
+
+
/*
* virISCSINodeNew:
* @portal: address for iSCSI target
diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h
index a44beeaf67..31b589dbf9 100644
--- a/src/util/viriscsi.h
+++ b/src/util/viriscsi.h
@@ -49,6 +49,7 @@ virISCSIRescanLUNs(const char *session)
int
virISCSIScanTargets(const char *portal,
+ const char *initiatoriqn,
size_t *ntargetsret,
char ***targetsret)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
index aa4ba57707..4bdb782e36 100644
--- a/tests/viriscsitest.c
+++ b/tests/viriscsitest.c
@@ -145,7 +145,7 @@ testISCSIScanTargets(const void *data)
virCommandSetDryRun(NULL, testIscsiadmCb, NULL);
- if (virISCSIScanTargets(info->portal, &ntargets, &targets) < 0)
+ if (virISCSIScanTargets(info->portal, NULL, &ntargets, &targets) < 0)
goto cleanup;
if (info->nexpected != ntargets) {