When scanning for targets, iSCSI might give different results
depending on the interface used. This is basically just name of
config file under /etc/iscsi/ifaces to use. The file contains
initiator IQN thus different results claim.
Signed-off-by: Michal Privoznik <mprivozn@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(-)
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,
+ &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.
*/
- 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);
+ }
+
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);
+ 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) {
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@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) {
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/03/2018 01:38 AM, John Ferlan wrote:
>
>
> 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
Yes.
>
>> 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.
There is no BZ. And yes, the GSoC project got me experimenting (because
for libiscsi backend we will have to make IQN required since libiscsi
does not parse anything from /etc/iscsi nor initiatorname.iscsi). And
while experimenting I've tired to put my own IQN into iscsi pool we
already have and found this bug.
>
>> Signed-off-by: Michal Privoznik <mprivozn@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:
Ah, okay. My gcc doesn't warn me about this. Wonder if I have something
misconfigured.
> ^~~~
>
>> 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?
Yes. And we construct the interface name at the beginning of this function.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/03/2018 01:08 AM, Michal Prívozník wrote:
> On 07/03/2018 01:38 AM, John Ferlan wrote:
>>
>>
>> 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
>
> Yes.
>
>>
>>> 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.
>
> There is no BZ. And yes, the GSoC project got me experimenting (because
> for libiscsi backend we will have to make IQN required since libiscsi
> does not parse anything from /etc/iscsi nor initiatorname.iscsi). And
> while experimenting I've tired to put my own IQN into iscsi pool we
> already have and found this bug.
>
FWIW: To a degree this is restoring the initiatoriqn argument that
commit 027986f5 removed...
[...]
>>> +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?
>
> Yes. And we construct the interface name at the beginning of this function.
>
Oh right, the virStorageBackendCreateIfaceIQN call which creates that
libvirt-iface-* name. That's just one of those context things that you
know when you've been working through the code ;-).
Still without the next patch, does this work?
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 07/03/2018 02:38 PM, John Ferlan wrote:
>
>
> On 07/03/2018 01:08 AM, Michal Prívozník wrote:
>> On 07/03/2018 01:38 AM, John Ferlan wrote:
>>>
>>>
>>> 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
>>
>> Yes.
>>
>>>
>>>> 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.
>>
>> There is no BZ. And yes, the GSoC project got me experimenting (because
>> for libiscsi backend we will have to make IQN required since libiscsi
>> does not parse anything from /etc/iscsi nor initiatorname.iscsi). And
>> while experimenting I've tired to put my own IQN into iscsi pool we
>> already have and found this bug.
>>
>
> FWIW: To a degree this is restoring the initiatoriqn argument that
> commit 027986f5 removed...
Yes, but I did not think it is worth mentioning anywhere. We change code
all the time, and if we were even to blame which commits caused what ..
it's a endless spiral IMO.
>
> [...]
>
>>>> +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?
>>
>> Yes. And we construct the interface name at the beginning of this function.
>>
>
> Oh right, the virStorageBackendCreateIfaceIQN call which creates that
> libvirt-iface-* name. That's just one of those context things that you
> know when you've been working through the code ;-).
>
> Still without the next patch, does this work?
Not really because even though targets are refreshed their configs are
not saved and thus login still fails.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.