10:03 a.m.
v1: http://www.redhat.com/archives/libvir-list/2016-May/msg01027.html
Changes since v2:
* Patch 2
* Remove the 'str' fetch/free - it was unnecessary
* Initialize and free 'error'
* Did not adjust reported error since with the usage of exitstatus
the call to virCommandRunRegex, that means the virCommandWait code
will return the error rather than reporting it. So the reporting is
left to this code to handle. Kept the same error code (INTERNAL) as
virCommandWait
Patch 3-5
* Remove 3, combine 4 & 5, but remove the setting of the autologin for
libvirt managed targets. This is essentially Fritz's initial patch:
http://www.redhat.com/archives/libvir-list/2016-May/msg00002.html
but with the adjustment to add the "ATTRIBUTE_UNUSED" for the
initiatoriqn parameter (it'll be removed in the next patch as
it is a bit more invasive).
Fritz Elfert (1):
util: Remove disabling of autologin for iscsi-targets
John Ferlan (3):
util: Add exitstatus parameter to virCommandRunRegex
iscsi: Add exit status checking for virISCSIGetSession
iscsi: Remove initiatoriqn from virISCSIScanTargets
src/storage/storage_backend_fs.c | 2 +-
src/storage/storage_backend_iscsi.c | 8 ++---
src/storage/storage_backend_logical.c | 10 +++---
src/util/vircommand.c | 9 ++++--
src/util/vircommand.h | 3 +-
src/util/viriscsi.c | 60 +++++++++++++++--------------------
src/util/viriscsi.h | 1 -
tests/viriscsitest.c | 7 ++--
8 files changed, 46 insertions(+), 54 deletions(-)
--
2.5.5
10:03 a.m.
New subject: [libvirt] [PATCH v2 1/4] 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
10:03 a.m.
New subject: [libvirt] [PATCH v2 2/4] 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 | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 846ea68..3627eed 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,24 +80,40 @@ virISCSIGetSession(const char *devpath,
.session = NULL,
.devpath = devpath,
};
+ char *error = NULL;
+ 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 *st = virProcessTranslateStatus(exitstatus);
+
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("'%s --mode session' unexpected %s%s%s"),
+ ISCSIADM, NULLSTR(st),
+ error ? ": " : "",
+ error ? error : "");
+ VIR_FREE(st);
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("cannot find session"));
+ }
}
cleanup:
+ VIR_FREE(error);
virCommandFree(cmd);
return cbdata.session;
}
--
2.5.5
10:04 a.m.
New subject: [libvirt] [PATCH v2 2/4] iscsi: Add exit status checking for virISCSIGetSession
On Mon, May 16, 2016 at 11:03:09 -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.
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 | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 846ea68..3627eed 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
[...]
@@ -79,24 +80,40 @@ virISCSIGetSession(const char *devpath,
[....]
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 *st = virProcessTranslateStatus(exitstatus);
+
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("'%s --mode session' unexpected %s%s%s"),
I still don't think this message is okay. As the error message above
suggests error code 21 is returned if no active session is found which
is definitely not unexpected here ... [1]
Also it's quite unlikely that iscsiadm would die of any "unnatural"
reasons and thus virProcessTranslateStatus would provide any helpful
data.
+ ISCSIADM, NULLSTR(st),
+ error ? ": " : "",
+ error ? error : "");
+ VIR_FREE(st);
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("cannot find session"));
[1] ... as this suggests.
+ }
}
The direction of this patch is okay, but the error could be improved. Do
you have any strong reason for keeping the message?
Peter
10:31 a.m.
New subject: [libvirt] [PATCH v2 2/4] iscsi: Add exit status checking for virISCSIGetSession
On 05/17/2016 11:04 AM, Peter Krempa wrote:
On Mon, May 16, 2016 at 11:03:09 -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.
>
> 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 | 27 ++++++++++++++++++++++-----
> 1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 846ea68..3627eed 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
[...]
> @@ -79,24 +80,40 @@ virISCSIGetSession(const char *devpath,
[....]
>
> 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 *st = virProcessTranslateStatus(exitstatus);
> +
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("'%s --mode session' unexpected
%s%s%s"),
I still don't think this message is okay. As the error message above
suggests error code 21 is returned if no active session is found which
is definitely not unexpected here ... [1]
Also it's quite unlikely that iscsiadm would die of any "unnatural"
reasons and thus virProcessTranslateStatus would provide any helpful
data.
> + ISCSIADM, NULLSTR(st),
> + error ? ": " : "",
> + error ? error : "");
> + VIR_FREE(st);
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("cannot find session"));
[1] ... as this suggests.
> + }
> }
>
The direction of this patch is okay, but the error could be improved. Do
you have any strong reason for keeping the message?
No - beyond keeping the output the same as previously except that this
message would only be displayed for the Refresh path; whereas, currently
it's displayed for check, startup, and shutdown as well.
I suppose all error paths could use the less than perfect second message
perhaps with a tweak to add the error code to at least indicate what the
error from the executed iscsiadm was. Just figured it'd be nice to
actually translate that error (which just doesn't work the same when
exitstatus = 0).
John
12:55 a.m.
New subject: [libvirt] [PATCH v2 2/4] iscsi: Add exit status checking for virISCSIGetSession
On Tue, May 17, 2016 at 11:31:07 -0400, John Ferlan wrote:
On 05/17/2016 11:04 AM, Peter Krempa wrote:
> On Mon, May 16, 2016 at 11:03:09 -0400, John Ferlan wrote:
[...]
>> +
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("'%s --mode session' unexpected
%s%s%s"),
>
> I still don't think this message is okay. As the error message above
> suggests error code 21 is returned if no active session is found which
> is definitely not unexpected here ... [1]
>
> Also it's quite unlikely that iscsiadm would die of any "unnatural"
> reasons and thus virProcessTranslateStatus would provide any helpful
> data.
>
>> + ISCSIADM, NULLSTR(st),
>> + error ? ": " : "",
>> + error ? error : "");
>> + VIR_FREE(st);
>> + } else {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + "%s", _("cannot find
session"));
>
> [1] ... as this suggests.
>
>> + }
>> }
>>
>
> The direction of this patch is okay, but the error could be improved. Do
> you have any strong reason for keeping the message?
>
No - beyond keeping the output the same as previously except that this
message would only be displayed for the Refresh path; whereas, currently
it's displayed for check, startup, and shutdown as well.
I suppose all error paths could use the less than perfect second message
perhaps with a tweak to add the error code to at least indicate what the
error from the executed iscsiadm was.
IMO that would be better than the current message.
Just figured it'd be nice to
actually translate that error (which just doesn't work the same when
exitstatus = 0).
I don't object to add the error message from iscsiadm itself to the
message. That might help debugging stuff. I object that the resulting
message isn't any better in case when you want to report it to the user.
As I've written above. Error 21 with the message is not unexpected in
this case.
In terms of transatability the error message string is extremely wrong.
If you as a translator see just "'%s --mode session' unexpected %s%s%s"
you definitely need to go check what's happening there.
The original message in the command code is wrong in terms of
translatability but it's justified by being a catch-all case for every
possible command execution that doesn't want to handle errors.
In addition as the error string isn't actually exactly the same as in
the command code (by partially adding the arguments verbatim to the
message) it would need to be re-translated.
"cannot find session iscsiadm session" followed by the message and/or
containing the code will be probably the best case here. The function
you are modifying is meant to find the session so any error is basically
that it couldn't find it. The message from iscsiadm then helps
debugging.
Peter
6:15 a.m.
New subject: [libvirt] [PATCH v2 2/4] iscsi: Add exit status checking for virISCSIGetSession
On 05/18/2016 01:55 AM, Peter Krempa wrote:
On Tue, May 17, 2016 at 11:31:07 -0400, John Ferlan wrote:
> On 05/17/2016 11:04 AM, Peter Krempa wrote:
>> On Mon, May 16, 2016 at 11:03:09 -0400, John Ferlan wrote:
[...]
>>> +
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("'%s --mode session' unexpected
%s%s%s"),
>>
>> I still don't think this message is okay. As the error message above
>> suggests error code 21 is returned if no active session is found which
>> is definitely not unexpected here ... [1]
>>
>> Also it's quite unlikely that iscsiadm would die of any
"unnatural"
>> reasons and thus virProcessTranslateStatus would provide any helpful
>> data.
>>
>>> + ISCSIADM, NULLSTR(st),
>>> + error ? ": " : "",
>>> + error ? error : "");
>>> + VIR_FREE(st);
>>> + } else {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + "%s", _("cannot find
session"));
>>
>> [1] ... as this suggests.
>>
>>> + }
>>> }
>>>
>>
>> The direction of this patch is okay, but the error could be improved. Do
>> you have any strong reason for keeping the message?
>>
>
> No - beyond keeping the output the same as previously except that this
> message would only be displayed for the Refresh path; whereas, currently
> it's displayed for check, startup, and shutdown as well.
> I suppose all error paths could use the less than perfect second message
> perhaps with a tweak to add the error code to at least indicate what the
> error from the executed iscsiadm was.
IMO that would be better than the current message.
> Just figured it'd be nice to
> actually translate that error (which just doesn't work the same when
> exitstatus = 0).
I don't object to add the error message from iscsiadm itself to the
message. That might help debugging stuff. I object that the resulting
message isn't any better in case when you want to report it to the user.
As I've written above. Error 21 with the message is not unexpected in
this case.
In terms of transatability the error message string is extremely wrong.
If you as a translator see just "'%s --mode session' unexpected
%s%s%s"
you definitely need to go check what's happening there.
The original message in the command code is wrong in terms of
translatability but it's justified by being a catch-all case for every
possible command execution that doesn't want to handle errors.
In addition as the error string isn't actually exactly the same as in
the command code (by partially adding the arguments verbatim to the
message) it would need to be re-translated.
"cannot find session iscsiadm session" followed by the message and/or
containing the code will be probably the best case here. The function
you are modifying is meant to find the session so any error is basically
that it couldn't find it. The message from iscsiadm then helps
debugging.
Fair enough... Primary thought was to be consistent with previous error,
but generating/using a unique error is fine... So rather than the if
then else based on exit status, how about:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot find iscsiadm session: %s"),
NULLSTR(error));
John
6:48 a.m.
New subject: [libvirt] [PATCH v2 2/4] iscsi: Add exit status checking for virISCSIGetSession
On Wed, May 18, 2016 at 07:15:54 -0400, John Ferlan wrote:
On 05/18/2016 01:55 AM, Peter Krempa wrote:
[...]
> "cannot find session iscsiadm session" followed by the
message and/or
> containing the code will be probably the best case here. The function
> you are modifying is meant to find the session so any error is basically
> that it couldn't find it. The message from iscsiadm then helps
> debugging.
>
Fair enough... Primary thought was to be consistent with previous error,
but generating/using a unique error is fine... So rather than the if
then else based on exit status, how about:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot find iscsiadm session: %s"),
NULLSTR(error));
That is reasonable. ACK to that including 1/4 if I didn't ACK it before.
10:03 a.m.
New subject: [libvirt] [PATCH v2 3/4] util: Remove disabling of autologin for iscsi-targets
From: Fritz Elfert <fritz(a)fritz-elfert.de>
https://bugzilla.redhat.com/show_bug.cgi?id=1331552
Instead of disabling auto-login of all scsi targets (even those
that do not "belong" to libvirt), use iscsiadm's "--op
nonpersistent"
during discovery of iSCSI targets (e.g. "iscsiadm --mode discovery
--type sendtargets") in order to avoid the node database being altered
which led to the need for the "large hammer" approach taken by
commit id '3c12b654'.
This commit removes the virISCSITargetAutologin adjustment (eg. the setting
of node.startup to "manual"). The iscsiadm command has supported this mode
of operation as of commit id 'ad873767' to open-iscsi.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/viriscsi.c | 30 ++----------------------------
tests/viriscsitest.c | 4 +++-
2 files changed, 5 insertions(+), 29 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 3627eed..8a0f559 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -403,24 +403,9 @@ virISCSIGetTargets(char **const groups,
}
-static int
-virISCSITargetAutologin(const char *portal,
- const char *initiatoriqn,
- const char *target,
- bool enable)
-{
- const char *extraargv[] = { "--op", "update",
- "--name", "node.startup",
- "--value", enable ? "automatic" :
"manual",
- NULL };
-
- return virISCSIConnection(portal, initiatoriqn, target, extraargv);
-}
-
-
int
virISCSIScanTargets(const char *portal,
- const char *initiatoriqn,
+ const char *initiatoriqn ATTRIBUTE_UNUSED,
size_t *ntargetsret,
char ***targetsret)
{
@@ -445,6 +430,7 @@ virISCSIScanTargets(const char *portal,
"--mode", "discovery",
"--type",
"sendtargets",
"--portal", portal,
+ "--op",
"nonpersistent",
NULL);
memset(&list, 0, sizeof(list));
@@ -457,18 +443,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;
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
3:22 a.m.
New subject: [libvirt] [PATCH v2 3/4] util: Remove disabling of autologin for iscsi-targets
On Mon, May 16, 2016 at 11:03:10 -0400, John Ferlan wrote:
From: Fritz Elfert <fritz(a)fritz-elfert.de>
https://bugzilla.redhat.com/show_bug.cgi?id=1331552
Instead of disabling auto-login of all scsi targets (even those
that do not "belong" to libvirt), use iscsiadm's "--op
nonpersistent"
during discovery of iSCSI targets (e.g. "iscsiadm --mode discovery
--type sendtargets") in order to avoid the node database being altered
which led to the need for the "large hammer" approach taken by
commit id '3c12b654'.
commit 3c12b6542cfc994f6b9cf3aef623767bf441c3ea
Author: Daniel P. Berrange <berrange(a)redhat.com>
Date: Fri Nov 12 15:21:48 2010 +0000
This commit removes the virISCSITargetAutologin adjustment (eg. the
setting
of node.startup to "manual"). The iscsiadm command has supported this mode
of operation as of commit id 'ad873767' to open-iscsi.
commit ad873767436f1cc242f0d4a522a2fce7133795c1
Author: Mike Christie <michaelc(a)cs.wisc.edu>
Date: Mon Mar 22 17:28:41 2010 -0500
iscsiadm: add nonpersistent mode to discovery mode
Looks like this is already around for a while so I'm okay with using the
new approach now.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/viriscsi.c | 30 ++----------------------------
tests/viriscsitest.c | 4 +++-
2 files changed, 5 insertions(+), 29 deletions(-)
ACK
10:03 a.m.
New subject: [libvirt] [PATCH v2 4/4] 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 9e2d01e..bccfba3 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;
if (virStorageBackendISCSISetAuth(portal, conn, &pool->def->source)
< 0)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 8a0f559..2b452c5 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -320,7 +320,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;
@@ -405,7 +405,6 @@ virISCSIGetTargets(char **const groups,
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 f4093f7..459249e 100644
--- a/src/util/viriscsi.h
+++ b/src/util/viriscsi.h
@@ -49,7 +49,6 @@ 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 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
3107
days inactive
3109
days old
11 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa