[libvirt] [PATCH 0/6] Don't disable auto_login for non libvirt managed iSCSI targets
https://bugzilla.redhat.com/show_bug.cgi?id=1331552 This is based on : http://www.redhat.com/archives/libvir-list/2016-May/msg00002.html which I had to first "hand massage" to get to apply and then had to make a couple of adjustments in order to compile. Since the review, Fritz responded directly to me and I've exchanged some thoughts back to him as I dug deeper into the issue. Patches 1 & 2 are related to the problem insomuch as the iscsi code will first try to "see" if a client connection already exists and if so, it won't try to recreate one. If one doesn't exist, then it will try to create one. So in a way it actually expects an error for a couple of paths. That error would be splatted out (in my case in the window I was running libvirtd in debug mode). So the patches make it so one can get an exitstatus and error buffer if they so choose when running the virCommandRunRegex Patches 3-6 are essentially a step by step rework of Fritz's patch with one change. In Fritz's patch the model was to remove the big hammer approach taken to set the "node.startup" to "manual" for every target found for the portal (e.g. the <ipaddr:port>). My approach alters that by only setting "node.startup" to "manual" for any target which libvirt manages via an iSCSI storage pool. I think that keeps the spirit of the original change (commit id '3c12b654') in tact while also making sure to not set "manual" mode on targets libvirt doesn't manage. Fritz Elfert (1): util: Add "--op nonpersistent" to iSCSI sendtargets John Ferlan (5): util: Add exitstatus parameter to virCommandRunRegex iscsi: Add exit status checking for virISCSIGetSession iscsi: Export virISCSITargetAutologin iscsi: Inhibit autologin for only libvirt managed targets iscsi: Remove initiatoriqn from virISCSIScanTargets src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_iscsi.c | 19 +++++++++----- src/storage/storage_backend_logical.c | 10 ++++--- src/util/vircommand.c | 9 ++++--- src/util/vircommand.h | 3 ++- src/util/viriscsi.c | 49 ++++++++++++++++++++--------------- src/util/viriscsi.h | 8 +++++- tests/viriscsitest.c | 7 ++--- 9 files changed, 68 insertions(+), 40 deletions(-) -- 2.5.5
Rather than have virCommandRun just spit out the error, allow callers to decide to pass the exitstatus so the caller can make intelligent decisions based on the error. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_logical.c | 10 ++++++---- src/util/vircommand.c | 9 ++++++--- src/util/vircommand.h | 3 ++- src/util/viriscsi.c | 4 ++-- 5 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 02a129e..45474cb 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -267,7 +267,7 @@ virStorageBackendFileSystemNetFindNFSPoolSources(virNetfsDiscoverState *state) if (virCommandRunRegex(cmd, 1, regexes, vars, virStorageBackendFileSystemNetFindPoolSourcesFunc, - state, NULL) < 0) + state, NULL, NULL) < 0) goto cleanup; ret = 0; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 90a194e..ca05fe1 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -398,7 +398,8 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, vars, virStorageBackendLogicalMakeVol, &cbdata, - "lvs") < 0) + "lvs", + NULL) < 0) goto cleanup; ret = 0; @@ -511,10 +512,10 @@ virStorageBackendLogicalGetPoolSources(virStoragePoolSourceListPtr sourceList) cmd = virCommandNewArgList(PVS, "--noheadings", "-o", "pv_name,vg_name", - NULL); + NULL, NULL); if (virCommandRunRegex(cmd, 1, regexes, vars, virStorageBackendLogicalFindPoolSourcesFunc, - sourceList, "pvs") < 0) + sourceList, "pvs", NULL) < 0) goto cleanup; ret = 0; @@ -799,7 +800,8 @@ virStorageBackendLogicalRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, vars, virStorageBackendLogicalRefreshPoolFunc, pool, - "vgs") < 0) + "vgs", + NULL) < 0) goto cleanup; ret = 0; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 027cb64..f5bd7af 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2900,6 +2900,7 @@ virCommandSetDryRun(virBufferPtr buf, * needs to return 0 on success * @data: additional data that will be passed to the callback function * @prefix: prefix that will be skipped at the beginning of each line + * @exitstatus: allows the caller to handle command run exit failures * * Run an external program. * @@ -2917,7 +2918,8 @@ virCommandRunRegex(virCommandPtr cmd, int *nvars, virCommandRunRegexFunc func, void *data, - const char *prefix) + const char *prefix, + int *exitstatus) { int err; regex_t *reg; @@ -2959,7 +2961,7 @@ virCommandRunRegex(virCommandPtr cmd, goto cleanup; virCommandSetOutputBuffer(cmd, &outbuf); - if (virCommandRun(cmd, NULL) < 0) + if (virCommandRun(cmd, exitstatus) < 0) goto cleanup; if (!outbuf) { @@ -3114,7 +3116,8 @@ virCommandRunRegex(virCommandPtr cmd ATTRIBUTE_UNUSED, int *nvars ATTRIBUTE_UNUSED, virCommandRunRegexFunc func ATTRIBUTE_UNUSED, void *data ATTRIBUTE_UNUSED, - const char *prefix ATTRIBUTE_UNUSED) + const char *prefix ATTRIBUTE_UNUSED, + int *exitstatus ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, _("%s not implemented on Win32"), __FUNCTION__); diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 198da2f..44818ef 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -205,7 +205,8 @@ int virCommandRunRegex(virCommandPtr cmd, int *nvars, virCommandRunRegexFunc func, void *data, - const char *cmd_to_ignore); + const char *cmd_to_ignore, + int *exitstatus); int virCommandRunNul(virCommandPtr cmd, size_t n_columns, diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index bd34fea..846ea68 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -87,7 +87,7 @@ virISCSIGetSession(const char *devpath, regexes, vars, virISCSIExtractSession, - &cbdata, NULL) < 0) + &cbdata, NULL, NULL) < 0) goto cleanup; if (cbdata.session == NULL && !probe) { @@ -437,7 +437,7 @@ virISCSIScanTargets(const char *portal, regexes, vars, virISCSIGetTargets, - &list, NULL) < 0) + &list, NULL, NULL) < 0) goto cleanup; for (i = 0; i < list.ntargets; i++) { -- 2.5.5
Utilize the exit status parameter for virCommandRunRegex in order to check the return error from the 'iscsiadm --mode session' command. Without this enabled, if there are no sessions running then virCommandRun would have displayed an error such as: 2016-05-13 15:17:15.165+0000: 10920: error : virCommandWait:2553 : internal error: Child process (iscsiadm --mode session) unexpected exit status 21: iscsiadm: No active sessions. It is possible that for certain paths (when probe is true) we only care whether it's running or not to make certain decisions. Spitting out the error for those paths is unnecessary. If we do have a situation where probe = false and there's an error, then display the error from iscsiadm if it's there; otherwise, default to the non descript error. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/viriscsi.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 846ea68..be296d7 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -32,6 +32,7 @@ #include "virerror.h" #include "virfile.h" #include "virlog.h" +#include "virprocess.h" #include "virrandom.h" #include "virstring.h" @@ -79,21 +80,38 @@ virISCSIGetSession(const char *devpath, .session = NULL, .devpath = devpath, }; + char *error; + int exitstatus = 0; - virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL); + virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", + "session", NULL); + virCommandSetErrorBuffer(cmd, &error); if (virCommandRunRegex(cmd, 1, regexes, vars, virISCSIExtractSession, - &cbdata, NULL, NULL) < 0) + &cbdata, NULL, &exitstatus) < 0) goto cleanup; if (cbdata.session == NULL && !probe) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot find session")); - goto cleanup; + /* If the command failed, let's give some information as to why */ + if (exitstatus != 0) { + char *str = virCommandToString(cmd); + char *st = virProcessTranslateStatus(exitstatus); + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%s --mode session' unexpected %s%s%s"), + ISCSIADM, NULLSTR(st), + error ? ": " : "", + error ? error : ""); + VIR_FREE(str); + VIR_FREE(st); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot find session")); + } } cleanup: -- 2.5.5
On Fri, May 13, 2016 at 17:29:22 -0400, John Ferlan wrote:
Utilize the exit status parameter for virCommandRunRegex in order to check the return error from the 'iscsiadm --mode session' command. Without this enabled, if there are no sessions running then virCommandRun would have displayed an error such as:
2016-05-13 15:17:15.165+0000: 10920: error : virCommandWait:2553 : internal error: Child process (iscsiadm --mode session) unexpected exit status 21: iscsiadm: No active sessions.
^^^^^^^^^^[1]
It is possible that for certain paths (when probe is true) we only care whether it's running or not to make certain decisions. Spitting out the error for those paths is unnecessary.
If we do have a situation where probe = false and there's an error, then display the error from iscsiadm if it's there; otherwise, default to the non descript error.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/viriscsi.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 846ea68..be296d7 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c
@@ -79,21 +80,38 @@ virISCSIGetSession(const char *devpath, .session = NULL, .devpath = devpath, }; + char *error; + int exitstatus = 0;
- virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL); + virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", + "session", NULL); + virCommandSetErrorBuffer(cmd, &error);
if (virCommandRunRegex(cmd, 1, regexes, vars, virISCSIExtractSession, - &cbdata, NULL, NULL) < 0) + &cbdata, NULL, &exitstatus) < 0) goto cleanup;
if (cbdata.session == NULL && !probe) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot find session")); - goto cleanup; + /* If the command failed, let's give some information as to why */ + if (exitstatus != 0) { + char *str = virCommandToString(cmd); + char *st = virProcessTranslateStatus(exitstatus); + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%s --mode session' unexpected %s%s%s"), + ISCSIADM, NULLSTR(st), + error ? ": " : "", + error ? error : "");
The original code where this was copied from uses 'str' to be part of the error message. Here it's unused. Is it necessary to report the error as virCommandWait() would report? Isn't it enough just to notify the user that the command failed. Especially since error code 21 [1] is not really an unexpected error code in this code path (it's even documented in the man page for iscsiadm). In such case the error message in the "else" section is better than the raw output. Also is INTERNAL_ERROR really necessary in case where the session wasn't found. [2].
+ VIR_FREE(str); + VIR_FREE(st); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR,
[2]. Ah that's pre-existing.
+ "%s", _("cannot find session")); + } }
cleanup:
Buffer for 'error' is leaked. Peter
On 05/16/2016 07:57 AM, Peter Krempa wrote:
On Fri, May 13, 2016 at 17:29:22 -0400, John Ferlan wrote:
Utilize the exit status parameter for virCommandRunRegex in order to check the return error from the 'iscsiadm --mode session' command. Without this enabled, if there are no sessions running then virCommandRun would have displayed an error such as:
2016-05-13 15:17:15.165+0000: 10920: error : virCommandWait:2553 : internal error: Child process (iscsiadm --mode session) unexpected exit status 21: iscsiadm: No active sessions.
^^^^^^^^^^[1]
It is possible that for certain paths (when probe is true) we only care whether it's running or not to make certain decisions. Spitting out the error for those paths is unnecessary.
If we do have a situation where probe = false and there's an error, then display the error from iscsiadm if it's there; otherwise, default to the non descript error.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/viriscsi.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 846ea68..be296d7 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c
@@ -79,21 +80,38 @@ virISCSIGetSession(const char *devpath, .session = NULL, .devpath = devpath, }; + char *error; + int exitstatus = 0;
- virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL); + virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", + "session", NULL); + virCommandSetErrorBuffer(cmd, &error);
if (virCommandRunRegex(cmd, 1, regexes, vars, virISCSIExtractSession, - &cbdata, NULL, NULL) < 0) + &cbdata, NULL, &exitstatus) < 0) goto cleanup;
if (cbdata.session == NULL && !probe) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot find session")); - goto cleanup; + /* If the command failed, let's give some information as to why */ + if (exitstatus != 0) { + char *str = virCommandToString(cmd); + char *st = virProcessTranslateStatus(exitstatus); + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%s --mode session' unexpected %s%s%s"), + ISCSIADM, NULLSTR(st), + error ? ": " : "", + error ? error : "");
The original code where this was copied from uses 'str' to be part of the error message. Here it's unused.
dang - it was used, then I removed the need and forgot to remove it. simple enough to remove.
Is it necessary to report the error as virCommandWait() would report?
virCommandWait won't report the error if you pass exitstatus AFAICT, so that means the caller would need to report it. I figure that's the reason one passes exitstatus and sets the Error buffer. This code doesn't set the rawStatus, but the WIFEXITED(status) will return something if the command failed. If it didn't then, the else in virCommandWait doesn't report anything and thus we have the else here about cannot find session.
Isn't it enough just to notify the user that the command failed. Especially since error code 21 [1] is not really an unexpected error code in this code path (it's even documented in the man page for iscsiadm). In such case the error message in the "else" section is better than the raw output.
Error only printed with probe = false. Yes, error code 21 means no active sessions, which 3 out of 4 callers are happy to deal with it. For the 1 which doesn't want that, then an error code of 21 should be reported. Prior to this patch - the "error" gets displayed for probe's (e.g. check, start, and stop). After it only gets displayed from the refresh path for any error.
Also is INTERNAL_ERROR really necessary in case where the session wasn't found. [2].
Better suggestion for the refresh path that expects to find the session available? I know it's somewhat of a catch-all and well over-used; however, still no different that if virCommandWait reported it.
+ VIR_FREE(str); + VIR_FREE(st); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR,
[2]. Ah that's pre-existing.
+ "%s", _("cannot find session")); + } }
cleanup:
Buffer for 'error' is leaked.
Oh right and not initialized... Easy enough to fix. John
Peter
Make it accessible for future patches Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/viriscsi.c | 2 +- src/util/viriscsi.h | 7 +++++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fb24808..719d753 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1675,6 +1675,7 @@ virISCSIGetSession; virISCSINodeUpdate; virISCSIRescanLUNs; virISCSIScanTargets; +virISCSITargetAutologin; # util/virjson.h diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index be296d7..ea01b3d 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -404,7 +404,7 @@ virISCSIGetTargets(char **const groups, } -static int +int virISCSITargetAutologin(const char *portal, const char *initiatoriqn, const char *target, diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h index f4093f7..3e7ea68 100644 --- a/src/util/viriscsi.h +++ b/src/util/viriscsi.h @@ -48,6 +48,13 @@ virISCSIRescanLUNs(const char *session) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int +virISCSITargetAutologin(const char *portal, + const char *initiatoriqn, + const char *target, + bool enable) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; + +int virISCSIScanTargets(const char *portal, const char *initiatoriqn, size_t *ntargetsret, -- 2.5.5
On Fri, May 13, 2016 at 17:29:23 -0400, John Ferlan wrote:
Make it accessible for future patches
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/viriscsi.c | 2 +- src/util/viriscsi.h | 7 +++++++ 3 files changed, 9 insertions(+), 1 deletion(-)
ACK
From: Fritz Elfert <fritz@fritz-elfert.de> The root cause of commit id '3c12b654' was to ensure that an iSCSI sendtargets command to discover all the available targets on the host didn't reset the "node.startup" value for a future restarts on any target that libvirt is managing. However, by adding the "--op nonpersistent" inhibits the iSCSI initiator from recording/modifying in the bowels /var/lib/iscsi of any target. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/viriscsi.c | 1 + tests/viriscsitest.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index ea01b3d..36612c5 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -446,6 +446,7 @@ virISCSIScanTargets(const char *portal, "--mode", "discovery", "--type", "sendtargets", "--portal", portal, + "--op", "nonpersistent", NULL); memset(&list, 0, sizeof(list)); diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index c697a4a..b5b0e20 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -90,7 +90,9 @@ static void testIscsiadmCb(const char *const*args, args[4] && STREQ(args[4], "sendtargets") && args[5] && STREQ(args[5], "--portal") && args[6] && STREQ(args[6], "10.20.30.40:3260,1") && - args[7] == NULL) { + args[7] && STREQ(args[7], "--op") && + args[8] && STREQ(args[8], "nonpersistent") && + args[9] == NULL) { ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput)); } else { *status = -1; -- 2.5.5
https://bugzilla.redhat.com/show_bug.cgi?id=1331552 Based on code originally posted by Fritz Elfert <fritz@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@redhat.com> --- src/storage/storage_backend_iscsi.c | 11 +++++++++++ src/util/viriscsi.c | 15 ++------------- 2 files changed, 13 insertions(+), 13 deletions(-) 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; -- 2.5.5
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@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@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;
On Mon, May 16, 2016 at 08:49:17 -0400, John Ferlan wrote:
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@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@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
Well, if you create them in the mode without adding host config as of the previous patch this indeed should not be necessary. In addition the first hunk of this patch then doesn't make much sense.
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
Excactly.
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
Indeed. If you modify your host from outside of libvirt we can't really do much.
be overridden. So what's the point of setting it then <sigh>. Was really trying to follow the spirit of the original change...
Exactly. I don't think we need to set it at all. Peter
No longer necessary to have it, so remove it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_iscsi.c | 8 ++------ src/util/viriscsi.c | 3 +-- src/util/viriscsi.h | 1 - tests/viriscsitest.c | 3 +-- 4 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 5fbf390..b6b23d3 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -197,9 +197,7 @@ virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, if (!(portal = virStorageBackendISCSIPortal(source))) goto cleanup; - if (virISCSIScanTargets(portal, - source->initiator.iqn, - &ntargets, &targets) < 0) + if (virISCSIScanTargets(portal, &ntargets, &targets) < 0) goto cleanup; if (VIR_ALLOC_N(list.sources, ntargets) < 0) @@ -399,9 +397,7 @@ virStorageBackendISCSIStartPool(virConnectPtr conn, * iscsiadm doesn't let you login to a target, unless you've * first issued a 'sendtargets' command to the portal :-( */ - if (virISCSIScanTargets(portal, - pool->def->source.initiator.iqn, - NULL, NULL) < 0) + if (virISCSIScanTargets(portal, NULL, NULL) < 0) goto cleanup; /* Inhibit our autologin for our managed source device */ diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 3133d88..a2acb67 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -321,7 +321,7 @@ virISCSIConnection(const char *portal, * portal. Without the sendtargets all that is received is a * "iscsiadm: No records found" */ - if (virISCSIScanTargets(portal, initiatoriqn, NULL, NULL) < 0) + if (virISCSIScanTargets(portal, NULL, NULL) < 0) goto cleanup; break; @@ -422,7 +422,6 @@ virISCSITargetAutologin(const char *portal, int virISCSIScanTargets(const char *portal, - const char *initiatoriqn ATTRIBUTE_UNUSED, size_t *ntargetsret, char ***targetsret) { diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h index 3e7ea68..44fb1f4 100644 --- a/src/util/viriscsi.h +++ b/src/util/viriscsi.h @@ -56,7 +56,6 @@ virISCSITargetAutologin(const char *portal, 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 b5b0e20..40e4d10 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -145,8 +145,7 @@ testISCSIScanTargets(const void *data) virCommandSetDryRun(NULL, testIscsiadmCb, NULL); - if (virISCSIScanTargets(info->portal, NULL, - &ntargets, &targets) < 0) + if (virISCSIScanTargets(info->portal, &ntargets, &targets) < 0) goto cleanup; if (info->nexpected != ntargets) { -- 2.5.5
participants (2)
-
John Ferlan -
Peter Krempa