4:29 p.m.
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
4:29 p.m.
New subject: [libvirt] [PATCH 1/6] util: Add exitstatus parameter to virCommandRunRegex
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(a)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
4:29 p.m.
New subject: [libvirt] [PATCH 2/6] iscsi: Add exit status checking for virISCSIGetSession
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(a)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
6:57 a.m.
New subject: [libvirt] [PATCH 2/6] iscsi: Add exit status checking for virISCSIGetSession
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(a)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
7:35 a.m.
New subject: [libvirt] [PATCH 2/6] iscsi: Add exit status checking for virISCSIGetSession
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(a)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
4:29 p.m.
New subject: [libvirt] [PATCH 3/6] iscsi: Export virISCSITargetAutologin
Make it accessible for future patches
Signed-off-by: John Ferlan <jferlan(a)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
7:15 a.m.
New subject: [libvirt] [PATCH 3/6] iscsi: Export virISCSITargetAutologin
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(a)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
4:29 p.m.
New subject: [libvirt] [PATCH 4/6] util: Add "--op nonpersistent" to iSCSI sendtargets
From: Fritz Elfert <fritz(a)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(a)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
4:29 p.m.
New subject: [libvirt] [PATCH 5/6] iscsi: Inhibit autologin for only libvirt managed targets
https://bugzilla.redhat.com/show_bug.cgi?id=1331552
Based on code originally posted by Fritz Elfert <fritz(a)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(a)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
7:49 a.m.
New subject: [libvirt] [PATCH 5/6] iscsi: Inhibit autologin for only libvirt managed targets
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(a)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(a)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;
8:44 a.m.
New subject: [libvirt] [PATCH 5/6] iscsi: Inhibit autologin for only libvirt managed 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(a)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(a)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
4:29 p.m.
New subject: [libvirt] [PATCH 6/6] iscsi: Remove initiatoriqn from virISCSIScanTargets
No longer necessary to have it, so remove it.
Signed-off-by: John Ferlan <jferlan(a)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
3145
days inactive
3148
days old
11 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa