[libvirt] [PATCH 4/5] virISCSIScanTargets: Honour iSCSI interface

[Michal Prívozník]

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 at 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