[libvirt] [PATCH 5/6] iscsi: Inhibit autologin for only libvirt managed targets

[John Ferlan]

On 05/13/2016 05:29 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1331552
> 
> Based on code originally posted by Fritz Elfert <fritz at fritz-elfert.de>
> to remove the Autologin code entirely from libvirt, but reworked to only
> set Autologin for libvirt managed targets.
> 
> Commit id '3c12b654' took a "large hammer" approach to inhibiting logins
> which causes issues if there are iSCSI targets not being managed by libvirt.
> 
> Now that the previous commit ensures that the iscsi initiator doesn't update
> the /var/lib/iscsi tree with the results for a 'sendtargets' by using the
> "--op nonpersistent" option, let's remove the code from virISCSIScanTargets
> that disables autologin for every target, but add that same setting into
> the start pool code for each managed/started target to ensure that nothing
> else goes and tries to autologin.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/storage/storage_backend_iscsi.c | 11 +++++++++++
>  src/util/viriscsi.c                 | 15 ++-------------
>  2 files changed, 13 insertions(+), 13 deletions(-)
> 

Over the weekend I got more "data" on this from Fritz...

Seems with targets implemented by external SAN hardware, the host has a
single iSCSI initiator with multiple targets. The claim is libvirt
doesn't need the targets in manual mode. I'm still trying to process
that so patches 3-5 could need more adjustment. The adjustment being the
removal of the autologin code.  Since patch 4 would add the "--op
nonpersistent" to the listing command line, the adjustment made to the
/var/lib/iscsi doesn't happen.  Still trying to process that feedback. I
did find that even with this patch applied, if something runs the
"--mode discovery" without the "--op nonpersistent" (something other
than libvirt), then the mode in the /var/lib/iscsi goes *back to*
automatic.  So in a way, it doesn't matter what we set this to, it can
be overridden. So what's the point of setting it then <sigh>.  Was
really trying to follow the spirit of the original change...

John
> diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
> index 9e2d01e..5fbf390 100644
> --- a/src/storage/storage_backend_iscsi.c
> +++ b/src/storage/storage_backend_iscsi.c
> @@ -404,6 +404,17 @@ virStorageBackendISCSIStartPool(virConnectPtr conn,
>                                  NULL, NULL) < 0)
>              goto cleanup;
>  
> +        /* Inhibit our autologin for our managed source device */
> +        if (virISCSITargetAutologin(portal,
> +                                    pool->def->source.initiator.iqn,
> +                                    pool->def->source.devices[0].path,
> +                                    false) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("failed to inhibit autologin for target '%s'"),
> +                           pool->def->source.devices[0].path);
> +            goto cleanup;
> +        }
> +
>          if (virStorageBackendISCSISetAuth(portal, conn, &pool->def->source) < 0)
>              goto cleanup;
>  
> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 36612c5..3133d88 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
> @@ -415,13 +415,14 @@ virISCSITargetAutologin(const char *portal,
>                                  "--value", enable ? "automatic" : "manual",
>                                  NULL };
>  
> +    VIR_DEBUG("set autologin for '%s' to '%d'", target, enable);
>      return virISCSIConnection(portal, initiatoriqn, target, extraargv);
>  }
>  
>  
>  int
>  virISCSIScanTargets(const char *portal,
> -                    const char *initiatoriqn,
> +                    const char *initiatoriqn ATTRIBUTE_UNUSED,
>                      size_t *ntargetsret,
>                      char ***targetsret)
>  {
> @@ -459,18 +460,6 @@ virISCSIScanTargets(const char *portal,
>                             &list, NULL, NULL) < 0)
>          goto cleanup;
>  
> -    for (i = 0; i < list.ntargets; i++) {
> -        /* We have to ignore failure, because we can't undo
> -         * the results of 'sendtargets', unless we go scrubbing
> -         * around in the dirt in /var/lib/iscsi.
> -         */
> -        if (virISCSITargetAutologin(portal,
> -                                    initiatoriqn,
> -                                    list.targets[i], false) < 0)
> -            VIR_WARN("Unable to disable auto-login on iSCSI target %s: %s",
> -                     portal, list.targets[i]);
> -    }
> -
>      if (ntargetsret && targetsret) {
>          *ntargetsret = list.ntargets;
>          *targetsret = list.targets;
>