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

[John Ferlan]

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 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:
         ^~~~

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