[libvirt] [PATCH v2 00/10] viriscsi: Couple of fixes
v2 of: https://www.redhat.com/archives/libvir-list/2018-June/msg01861.html diff to v1: - fixed misleading while() loop in 03/10 - Introduced some tests - patch 06/10 is new, while writing the tests I've noticed that errors from testIscsiadmCb were not propagated properly. Patches 01,02,04 and 05 are ACKed already. Even though John suggested merging 04 and 05 together I haven't done that because I believe "iscsi --interface" and "iscsi --op nonpersistent" are orthogonal and even though we currently treat them as mutually exclusive (in storage backend) the util/viriscsi.c API should not suffer because of that. Michal Privoznik (10): virStorageBackendIQNFound: Fix ret value assignment virStorageBackendIQNFound: Rename out label virStorageBackendIQNFound: Rework iscsiadm output parsing virISCSIScanTargets: Honour iSCSI interface virISCSIScanTargets: Allow making targets persistent virCommandWait: Propagate dryRunCallback return value properly viriscsitest: Test virISCSIConnectionLogin viriscsitest: Move testSessionInfo struct viriscsitest: Introduce testIscsiadmCbData struct viriscsitest: Extend virISCSIConnectionLogin test src/storage/storage_backend_iscsi.c | 5 +- src/util/vircommand.c | 2 + src/util/viriscsi.c | 196 ++++++++++++++++++++++++------------ src/util/viriscsi.h | 2 + tests/viriscsitest.c | 145 ++++++++++++++++++++++++-- 5 files changed, 272 insertions(+), 78 deletions(-) -- 2.16.4
Perform some method clean-up to follow more accepted coding standards: * Initialize @ret to error value and prove otherwise. * Initialize *ifacename to NULL Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/util/viriscsi.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index d4c745a1af..ad857f8cac 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -117,15 +117,16 @@ static int virStorageBackendIQNFound(const char *initiatoriqn, char **ifacename) { - int ret = IQN_MISSING, fd = -1; + int ret = IQN_ERROR, fd = -1; char ebuf[64]; FILE *fp = NULL; char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL; virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "iface", NULL); + *ifacename = NULL; + if (VIR_ALLOC_N(line, LINE_SIZE) != 0) { - ret = IQN_ERROR; virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not allocate memory for output of '%s'"), ISCSIADM); @@ -135,24 +136,20 @@ virStorageBackendIQNFound(const char *initiatoriqn, memset(line, 0, LINE_SIZE); virCommandSetOutputFD(cmd, &fd); - if (virCommandRunAsync(cmd, NULL) < 0) { - ret = IQN_ERROR; + if (virCommandRunAsync(cmd, NULL) < 0) goto out; - } if ((fp = VIR_FDOPEN(fd, "r")) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open stream for file descriptor " "when reading output from '%s': '%s'"), ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf))); - ret = IQN_ERROR; goto out; } while (fgets(line, LINE_SIZE, fp) != NULL) { newline = strrchr(line, '\n'); if (newline == NULL) { - ret = IQN_ERROR; virReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected line > %d characters " "when parsing output of '%s'"), @@ -169,24 +166,24 @@ virStorageBackendIQNFound(const char *initiatoriqn, if (STREQ(iqn, initiatoriqn)) { token = strchr(line, ' '); if (!token) { - ret = IQN_ERROR; virReportError(VIR_ERR_INTERNAL_ERROR, _("Missing space when parsing output " "of '%s'"), ISCSIADM); goto out; } - if (VIR_STRNDUP(*ifacename, line, token - line) < 0) { - ret = IQN_ERROR; + + if (VIR_STRNDUP(*ifacename, line, token - line) < 0) goto out; - } + VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn); - ret = IQN_FOUND; break; } } if (virCommandWait(cmd, NULL) < 0) - ret = IQN_ERROR; + goto out; + + ret = *ifacename ? IQN_FOUND : IQN_MISSING; out: if (ret == IQN_MISSING) -- 2.16.4
This is in fact 'cleanup' label and it should be named as such. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/util/viriscsi.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index ad857f8cac..01b5b4be68 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -130,21 +130,21 @@ virStorageBackendIQNFound(const char *initiatoriqn, virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not allocate memory for output of '%s'"), ISCSIADM); - goto out; + goto cleanup; } memset(line, 0, LINE_SIZE); virCommandSetOutputFD(cmd, &fd); if (virCommandRunAsync(cmd, NULL) < 0) - goto out; + goto cleanup; if ((fp = VIR_FDOPEN(fd, "r")) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open stream for file descriptor " "when reading output from '%s': '%s'"), ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf))); - goto out; + goto cleanup; } while (fgets(line, LINE_SIZE, fp) != NULL) { @@ -154,7 +154,7 @@ virStorageBackendIQNFound(const char *initiatoriqn, _("Unexpected line > %d characters " "when parsing output of '%s'"), LINE_SIZE, ISCSIADM); - goto out; + goto cleanup; } *newline = '\0'; @@ -169,11 +169,11 @@ virStorageBackendIQNFound(const char *initiatoriqn, virReportError(VIR_ERR_INTERNAL_ERROR, _("Missing space when parsing output " "of '%s'"), ISCSIADM); - goto out; + goto cleanup; } if (VIR_STRNDUP(*ifacename, line, token - line) < 0) - goto out; + goto cleanup; VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn); break; @@ -181,11 +181,11 @@ virStorageBackendIQNFound(const char *initiatoriqn, } if (virCommandWait(cmd, NULL) < 0) - goto out; + goto cleanup; ret = *ifacename ? IQN_FOUND : IQN_MISSING; - out: + cleanup: if (ret == IQN_MISSING) VIR_DEBUG("Could not find interface with IQN '%s'", iqn); -- 2.16.4
Firstly, we can utilize virCommandSetOutputBuffer() API which will collect the command output for us. Secondly, sscanf()-ing through each line is easier to understand (and more robust) than jumping over a string with strchr(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viriscsi.c | 85 +++++++++++++++++++++-------------------------------- 1 file changed, 34 insertions(+), 51 deletions(-) diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 01b5b4be68..1ddf00aa4c 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath, -#define LINE_SIZE 4096 #define IQN_FOUND 1 #define IQN_MISSING 0 #define IQN_ERROR -1 @@ -117,71 +116,56 @@ static int virStorageBackendIQNFound(const char *initiatoriqn, char **ifacename) { - int ret = IQN_ERROR, fd = -1; - char ebuf[64]; - FILE *fp = NULL; - char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL; + int ret = IQN_ERROR; + char *outbuf = NULL; + char *line = NULL; + char *iface = NULL; + char *iqn = NULL; virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "iface", NULL); *ifacename = NULL; - if (VIR_ALLOC_N(line, LINE_SIZE) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not allocate memory for output of '%s'"), - ISCSIADM); + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, NULL) < 0) goto cleanup; - } - memset(line, 0, LINE_SIZE); + /* Example of data we are dealing with: + * default tcp,<empty>,<empty>,<empty>,<empty> + * iser iser,<empty>,<empty>,<empty>,<empty> + * libvirt-iface-253db048 tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client + */ - virCommandSetOutputFD(cmd, &fd); - if (virCommandRunAsync(cmd, NULL) < 0) - goto cleanup; + line = outbuf; + while (line && *line) { + char *newline; + int num; - if ((fp = VIR_FDOPEN(fd, "r")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to open stream for file descriptor " - "when reading output from '%s': '%s'"), - ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf))); - goto cleanup; - } + if (!(newline = strchr(line, '\n'))) + break; - while (fgets(line, LINE_SIZE, fp) != NULL) { - newline = strrchr(line, '\n'); - if (newline == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected line > %d characters " - "when parsing output of '%s'"), - LINE_SIZE, ISCSIADM); - goto cleanup; - } *newline = '\0'; - iqn = strrchr(line, ','); - if (iqn == NULL) - continue; - iqn++; + VIR_FREE(iface); + VIR_FREE(iqn); + num = sscanf(line, "%ms %*[^,],%*[^,],%*[^,],%*[^,],%ms", &iface, &iqn); + + if (num != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("malformed output of %s: %s"), + ISCSIADM, line); + goto cleanup; + } if (STREQ(iqn, initiatoriqn)) { - token = strchr(line, ' '); - if (!token) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing space when parsing output " - "of '%s'"), ISCSIADM); - goto cleanup; - } - - if (VIR_STRNDUP(*ifacename, line, token - line) < 0) - goto cleanup; + VIR_STEAL_PTR(*ifacename, iface); VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn); break; } - } - if (virCommandWait(cmd, NULL) < 0) - goto cleanup; + line = newline + 1; + } ret = *ifacename ? IQN_FOUND : IQN_MISSING; @@ -189,11 +173,10 @@ virStorageBackendIQNFound(const char *initiatoriqn, if (ret == IQN_MISSING) VIR_DEBUG("Could not find interface with IQN '%s'", iqn); - VIR_FREE(line); - VIR_FORCE_FCLOSE(fp); - VIR_FORCE_CLOSE(fd); + VIR_FREE(iqn); + VIR_FREE(iface); + VIR_FREE(outbuf); virCommandFree(cmd); - return ret; } -- 2.16.4
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Firstly, we can utilize virCommandSetOutputBuffer() API which will collect the command output for us. Secondly, sscanf()-ing through each line is easier to understand (and more robust) than jumping over a string with strchr().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viriscsi.c | 85 +++++++++++++++++++++-------------------------------- 1 file changed, 34 insertions(+), 51 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 01b5b4be68..1ddf00aa4c 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
-#define LINE_SIZE 4096 #define IQN_FOUND 1 #define IQN_MISSING 0 #define IQN_ERROR -1 @@ -117,71 +116,56 @@ static int virStorageBackendIQNFound(const char *initiatoriqn, char **ifacename) { - int ret = IQN_ERROR, fd = -1; - char ebuf[64]; - FILE *fp = NULL; - char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL; + int ret = IQN_ERROR; + char *outbuf = NULL; + char *line = NULL; + char *iface = NULL; + char *iqn = NULL; virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "iface", NULL);
*ifacename = NULL;
- if (VIR_ALLOC_N(line, LINE_SIZE) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not allocate memory for output of '%s'"), - ISCSIADM); + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, NULL) < 0) goto cleanup; - }
- memset(line, 0, LINE_SIZE); + /* Example of data we are dealing with: + * default tcp,<empty>,<empty>,<empty>,<empty> + * iser iser,<empty>,<empty>,<empty>,<empty> + * libvirt-iface-253db048 tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client + */
- virCommandSetOutputFD(cmd, &fd); - if (virCommandRunAsync(cmd, NULL) < 0) - goto cleanup; + line = outbuf; + while (line && *line) { + char *newline; + int num;
- if ((fp = VIR_FDOPEN(fd, "r")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to open stream for file descriptor " - "when reading output from '%s': '%s'"), - ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf))); - goto cleanup; - } + if (!(newline = strchr(line, '\n'))) + break;
The next hunk is is going to pick up a non libvirt generated initiator.iqn, is that something we want? Or should we : if (!STRPREFIX(line, "libvirt-iface-")) continue; where "libvirt-iface-" could be made into a #define constant for usage by both this and virStorageBackendCreateIfaceIQN
- while (fgets(line, LINE_SIZE, fp) != NULL) { - newline = strrchr(line, '\n'); - if (newline == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected line > %d characters " - "when parsing output of '%s'"), - LINE_SIZE, ISCSIADM); - goto cleanup; - } *newline = '\0';
- iqn = strrchr(line, ','); - if (iqn == NULL) - continue; - iqn++; + VIR_FREE(iface); + VIR_FREE(iqn); + num = sscanf(line, "%ms %*[^,],%*[^,],%*[^,],%*[^,],%ms", &iface, &iqn);
Reading up on sscanf, the '%ms' seems to imply a GNU lib C mechanism vs. a C standard mechanism. So the query/concern being all our various build environments acceptance of the 'm' qualifier (at least coverity didn't choke on it ;-)). Reviewed-by: John Ferlan <jferlan@redhat.com> (regardless of the decision to add the libivrt-iface- filter John
+ + if (num != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("malformed output of %s: %s"), + ISCSIADM, line); + goto cleanup; + }
if (STREQ(iqn, initiatoriqn)) { - token = strchr(line, ' '); - if (!token) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing space when parsing output " - "of '%s'"), ISCSIADM); - goto cleanup; - } - - if (VIR_STRNDUP(*ifacename, line, token - line) < 0) - goto cleanup; + VIR_STEAL_PTR(*ifacename, iface);
VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn); break; } - }
- if (virCommandWait(cmd, NULL) < 0) - goto cleanup; + line = newline + 1; + }
ret = *ifacename ? IQN_FOUND : IQN_MISSING;
@@ -189,11 +173,10 @@ virStorageBackendIQNFound(const char *initiatoriqn, if (ret == IQN_MISSING) VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
- VIR_FREE(line); - VIR_FORCE_FCLOSE(fp); - VIR_FORCE_CLOSE(fd); + VIR_FREE(iqn); + VIR_FREE(iface); + VIR_FREE(outbuf); virCommandFree(cmd); - return ret; }
On 07/17/2018 08:42 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Firstly, we can utilize virCommandSetOutputBuffer() API which will collect the command output for us. Secondly, sscanf()-ing through each line is easier to understand (and more robust) than jumping over a string with strchr().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viriscsi.c | 85 +++++++++++++++++++++-------------------------------- 1 file changed, 34 insertions(+), 51 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 01b5b4be68..1ddf00aa4c 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
-#define LINE_SIZE 4096 #define IQN_FOUND 1 #define IQN_MISSING 0 #define IQN_ERROR -1 @@ -117,71 +116,56 @@ static int virStorageBackendIQNFound(const char *initiatoriqn, char **ifacename) { - int ret = IQN_ERROR, fd = -1; - char ebuf[64]; - FILE *fp = NULL; - char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL; + int ret = IQN_ERROR; + char *outbuf = NULL; + char *line = NULL; + char *iface = NULL; + char *iqn = NULL; virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "iface", NULL);
*ifacename = NULL;
- if (VIR_ALLOC_N(line, LINE_SIZE) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not allocate memory for output of '%s'"), - ISCSIADM); + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, NULL) < 0) goto cleanup; - }
- memset(line, 0, LINE_SIZE); + /* Example of data we are dealing with: + * default tcp,<empty>,<empty>,<empty>,<empty> + * iser iser,<empty>,<empty>,<empty>,<empty> + * libvirt-iface-253db048 tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client + */
- virCommandSetOutputFD(cmd, &fd); - if (virCommandRunAsync(cmd, NULL) < 0) - goto cleanup; + line = outbuf; + while (line && *line) { + char *newline; + int num;
- if ((fp = VIR_FDOPEN(fd, "r")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to open stream for file descriptor " - "when reading output from '%s': '%s'"), - ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf))); - goto cleanup; - } + if (!(newline = strchr(line, '\n'))) + break;
The next hunk is is going to pick up a non libvirt generated initiator.iqn, is that something we want? Or should we :
if (!STRPREFIX(line, "libvirt-iface-")) continue;
I don't think we need this. Firstly, the code as is now doesn't care about that either. Secondly, even though we currently call the function only to learn about libvirt-iface-* it is generic enough to be called over any interface.
where "libvirt-iface-" could be made into a #define constant for usage by both this and virStorageBackendCreateIfaceIQN
- while (fgets(line, LINE_SIZE, fp) != NULL) { - newline = strrchr(line, '\n'); - if (newline == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected line > %d characters " - "when parsing output of '%s'"), - LINE_SIZE, ISCSIADM); - goto cleanup; - } *newline = '\0';
- iqn = strrchr(line, ','); - if (iqn == NULL) - continue; - iqn++; + VIR_FREE(iface); + VIR_FREE(iqn); + num = sscanf(line, "%ms %*[^,],%*[^,],%*[^,],%*[^,],%ms", &iface, &iqn);
Reading up on sscanf, the '%ms' seems to imply a GNU lib C mechanism vs. a C standard mechanism. So the query/concern being all our various build environments acceptance of the 'm' qualifier (at least coverity didn't choke on it ;-)).
Not really. From sscanf(3): The use of the letter a for this purpose was problematic, since a is also specified by the ISO C standard as a synonym for f (floating-point input). POSIX.1-2008 instead specifies the m modifier for assignment allocation (as documented in DESCRIPTION, above). So it's mandated by POSIX which should be good enough for us to use.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Thanks, Michal
When scanning for targets, iSCSI might give different results depending on the interface used. This is basically just name of config file under /etc/iscsi/ifaces to use. The file contains initiator IQN thus different results claim. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_iscsi.c | 4 +- src/util/viriscsi.c | 79 ++++++++++++++++++++++++++++++++++--- src/util/viriscsi.h | 1 + tests/viriscsitest.c | 2 +- 4 files changed, 78 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 7871d1915b..3b9dddb4fd 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -194,7 +194,9 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec, if (!(portal = virStorageBackendISCSIPortal(source))) goto cleanup; - if (virISCSIScanTargets(portal, &ntargets, &targets) < 0) + if (virISCSIScanTargets(portal, + source->initiator.iqn, + &ntargets, &targets) < 0) goto cleanup; if (VIR_ALLOC_N(list.sources, ntargets) < 0) diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 1ddf00aa4c..549f75b434 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -40,6 +40,13 @@ VIR_LOG_INIT("util.iscsi"); +static int +virISCSIScanTargetsInternal(const char *portal, + const char *ifacename, + size_t *ntargetsret, + char ***targetsret); + + struct virISCSISessionData { char *session; const char *devpath; @@ -286,9 +293,10 @@ virISCSIConnection(const char *portal, * iscsiadm doesn't let you send commands to the Interface IQN, * unless you've first issued a 'sendtargets' command to the * portal. Without the sendtargets all that is received is a - * "iscsiadm: No records found" + * "iscsiadm: No records found". However, we must ensure that + * the command is issued over interface name we invented above. */ - if (virISCSIScanTargets(portal, NULL, NULL) < 0) + if (virISCSIScanTargetsInternal(portal, ifacename, NULL, NULL) < 0) goto cleanup; break; @@ -371,10 +379,11 @@ virISCSIGetTargets(char **const groups, } -int -virISCSIScanTargets(const char *portal, - size_t *ntargetsret, - char ***targetsret) +static int +virISCSIScanTargetsInternal(const char *portal, + const char *ifacename, + size_t *ntargetsret, + char ***targetsret) { /** * @@ -400,6 +409,12 @@ virISCSIScanTargets(const char *portal, "--op", "nonpersistent", NULL); + if (ifacename) { + virCommandAddArgList(cmd, + "--interface", ifacename, + NULL); + } + memset(&list, 0, sizeof(list)); if (virCommandRunRegex(cmd, @@ -425,6 +440,58 @@ virISCSIScanTargets(const char *portal, return ret; } + +/** + * virISCSIScanTargets: + * @portal: iSCSI portal + * @initiatoriqn: Initiator IQN + * @ntargets: number of items in @targetsret array + * @targets: array of targets + * + * For given @portal issue sendtargets command. Optionally, + * @initiatoriqn can be set to override default configuration. + * The targets are stored into @targets array and the size of + * the array is stored into @ntargets. + * + * Returns: 0 on success, + * -1 otherwise (with error reported) + */ +int +virISCSIScanTargets(const char *portal, + const char *initiatoriqn, + size_t *ntargets, + char ***targets) +{ + char *ifacename = NULL; + int ret = -1; + + if (ntargets) + *ntargets = 0; + if (targets) + *targets = NULL; + + if (initiatoriqn) { + switch ((virStorageBackendIQNFound(initiatoriqn, &ifacename))) { + case IQN_FOUND: + break; + + case IQN_MISSING: + virReportError(VIR_ERR_OPERATION_FAILED, + _("no iSCSI interface defined for IQN %s"), + initiatoriqn); + ATTRIBUTE_FALLTHROUGH; + case IQN_ERROR: + default: + return -1; + } + } + + ret = virISCSIScanTargetsInternal(portal, ifacename, ntargets, targets); + VIR_FREE(ifacename); + return ret; +} + + /* * virISCSINodeNew: * @portal: address for iSCSI target diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h index a44beeaf67..31b589dbf9 100644 --- a/src/util/viriscsi.h +++ b/src/util/viriscsi.h @@ -49,6 +49,7 @@ virISCSIRescanLUNs(const char *session) int virISCSIScanTargets(const char *portal, + const char *initiatoriqn, size_t *ntargetsret, char ***targetsret) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index aa4ba57707..4bdb782e36 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -145,7 +145,7 @@ testISCSIScanTargets(const void *data) virCommandSetDryRun(NULL, testIscsiadmCb, NULL); - if (virISCSIScanTargets(info->portal, &ntargets, &targets) < 0) + if (virISCSIScanTargets(info->portal, NULL, &ntargets, &targets) < 0) goto cleanup; if (info->nexpected != ntargets) { -- 2.16.4
After a new iSCSI interface is successfully set up, we issue a sendtargets command. However, after 56057900dc53df490d we don't update the host config which in turn makes login fail because iscsiadm is unable to find any matching record for the interface. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_iscsi.c | 1 + src/util/viriscsi.c | 23 +++++++++++++++++++---- src/util/viriscsi.h | 1 + tests/viriscsitest.c | 3 ++- 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 3b9dddb4fd..6242cd0fac 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -196,6 +196,7 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec, if (virISCSIScanTargets(portal, source->initiator.iqn, + false, &ntargets, &targets) < 0) goto cleanup; diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 549f75b434..e2c80acaaa 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -43,6 +43,7 @@ VIR_LOG_INIT("util.iscsi"); static int virISCSIScanTargetsInternal(const char *portal, const char *ifacename, + bool persist, size_t *ntargetsret, char ***targetsret); @@ -294,9 +295,11 @@ virISCSIConnection(const char *portal, * unless you've first issued a 'sendtargets' command to the * portal. Without the sendtargets all that is received is a * "iscsiadm: No records found". However, we must ensure that - * the command is issued over interface name we invented above. + * the command is issued over interface name we invented above + * and that targets are made persistent. */ - if (virISCSIScanTargetsInternal(portal, ifacename, NULL, NULL) < 0) + if (virISCSIScanTargetsInternal(portal, ifacename, + true, NULL, NULL) < 0) goto cleanup; break; @@ -382,6 +385,7 @@ virISCSIGetTargets(char **const groups, static int virISCSIScanTargetsInternal(const char *portal, const char *ifacename, + bool persist, size_t *ntargetsret, char ***targetsret) { @@ -406,9 +410,14 @@ virISCSIScanTargetsInternal(const char *portal, "--mode", "discovery", "--type", "sendtargets", "--portal", portal, - "--op", "nonpersistent", NULL); + if (!persist) { + virCommandAddArgList(cmd, + "--op", "nonpersistent", + NULL); + } + if (ifacename) { virCommandAddArgList(cmd, "--interface", ifacename, @@ -445,6 +454,7 @@ virISCSIScanTargetsInternal(const char *portal, * virISCSIScanTargets: * @portal: iSCSI portal * @initiatoriqn: Initiator IQN + * @persists: whether scanned targets should be saved * @ntargets: number of items in @targetsret array * @targets: array of targets * @@ -453,12 +463,16 @@ virISCSIScanTargetsInternal(const char *portal, * The targets are stored into @targets array and the size of * the array is stored into @ntargets. * + * If @persist is true, then targets returned by iSCSI portal are + * made persistent on the host (their config is saved). + * * Returns: 0 on success, * -1 otherwise (with error reported) */ int virISCSIScanTargets(const char *portal, const char *initiatoriqn, + bool persist, size_t *ntargets, char ***targets) { @@ -486,7 +500,8 @@ virISCSIScanTargets(const char *portal, } } - ret = virISCSIScanTargetsInternal(portal, ifacename, ntargets, targets); + ret = virISCSIScanTargetsInternal(portal, ifacename, + persist, ntargets, targets); VIR_FREE(ifacename); return ret; } diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h index 31b589dbf9..4da9becfb2 100644 --- a/src/util/viriscsi.h +++ b/src/util/viriscsi.h @@ -50,6 +50,7 @@ virISCSIRescanLUNs(const char *session) int virISCSIScanTargets(const char *portal, const char *initiatoriqn, + bool persist, size_t *ntargetsret, char ***targetsret) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 4bdb782e36..3bb3993196 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -145,7 +145,8 @@ testISCSIScanTargets(const void *data) virCommandSetDryRun(NULL, testIscsiadmCb, NULL); - if (virISCSIScanTargets(info->portal, NULL, &ntargets, &targets) < 0) + if (virISCSIScanTargets(info->portal, NULL, + false, &ntargets, &targets) < 0) goto cleanup; if (info->nexpected != ntargets) { -- 2.16.4
The documentation to virCommandWait() function states that if @exitstatus is NULL and command finished with error -1 is returned. In other words, if @dryRunCallback is set and returns an error (by setting its @status argument to a nonzero value) we must propagate this error properly honouring the documentation (and also regular run). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircommand.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 6dab105f56..13f75967fa 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2553,6 +2553,8 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) dryRunStatus); if (exitstatus) *exitstatus = dryRunStatus; + else if (dryRunStatus) + return -1; return 0; } -- 2.16.4
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
The documentation to virCommandWait() function states that if @exitstatus is NULL and command finished with error -1 is returned. In other words, if @dryRunCallback is set and returns an error (by setting its @status argument to a nonzero value) we must propagate this error properly honouring the documentation (and also regular run).
That's not how I read virCommandWait: * Wait for the command previously started with virCommandRunAsync() * to complete. Return -1 on any error waiting for * completion. Returns 0 if the command * finished with the exit status set. If @exitstatus is NULL, then the * child must exit with status 0 for this to succeed. By default, * a non-NULL @exitstatus contains the normal exit status of the child * (death from a signal is treated as execution error); but if * virCommandRawStatus() was used, it instead contains the raw exit * status that the caller must then decipher using WIFEXITED() and friends. perhaps the author (danpb) of commit id 7b3f1f8c3 would be able to say for sure... I only see -1 being returned "on any error waiting for completion". Filling @exitstatus with @dryRunStatus is reasonable since it is initialized to 0 in virCommandRunAsync and is what is passed to @dryRunCallback and thus only changed as a result of running @dryRunCallback. It has nothing to do with virCommandWait AFAICT. John
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircommand.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 6dab105f56..13f75967fa 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2553,6 +2553,8 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) dryRunStatus); if (exitstatus) *exitstatus = dryRunStatus; + else if (dryRunStatus) + return -1; return 0; }
On 07/17/2018 08:43 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
The documentation to virCommandWait() function states that if @exitstatus is NULL and command finished with error -1 is returned. In other words, if @dryRunCallback is set and returns an error (by setting its @status argument to a nonzero value) we must propagate this error properly honouring the documentation (and also regular run).
That's not how I read virCommandWait:
* Wait for the command previously started with virCommandRunAsync() * to complete. Return -1 on any error waiting for * completion. Returns 0 if the command * finished with the exit status set. If @exitstatus is NULL, then the * child must exit with status 0 for this to succeed. By default, * a non-NULL @exitstatus contains the normal exit status of the child * (death from a signal is treated as execution error); but if * virCommandRawStatus() was used, it instead contains the raw exit * status that the caller must then decipher using WIFEXITED() and friends.
perhaps the author (danpb) of commit id 7b3f1f8c3 would be able to say for sure...
I only see -1 being returned "on any error waiting for completion". Filling @exitstatus with @dryRunStatus is reasonable since it is initialized to 0 in virCommandRunAsync and is what is passed to @dryRunCallback and thus only changed as a result of running @dryRunCallback.
It has nothing to do with virCommandWait AFAICT.
So there are two ways how virCommandWait() can be called. The first is with @exitstatus being non-NULL. In this case, error is returned iff there was an error fetching command's exit status (e.g. because virProcessWait() failed). The second way is to call virCommandWait() with NULL in which case the function fails for all the cases in the first case plus if the command exit status is not zero. This is documented in docs/internals/command.html#async: As with virCommandRun, the status arg for virCommandWait can be omitted, in which case it will validate that exit status is zero and raise an error if not. Let's put aside dry run case for a while. Imagine /bin/false was started asynchronously and control now reaches virCommandWait(cmd, NULL). What do you think should be expected return value? I'd expect "Child process (%) unexpected..." error message and return -1. However, this is not the case if dry run callback sets an error. Michal
On 07/23/2018 04:01 AM, Michal Prívozník wrote:
On 07/17/2018 08:43 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
The documentation to virCommandWait() function states that if @exitstatus is NULL and command finished with error -1 is returned. In other words, if @dryRunCallback is set and returns an error (by setting its @status argument to a nonzero value) we must propagate this error properly honouring the documentation (and also regular run).
That's not how I read virCommandWait:
* Wait for the command previously started with virCommandRunAsync() * to complete. Return -1 on any error waiting for * completion. Returns 0 if the command * finished with the exit status set. If @exitstatus is NULL, then the * child must exit with status 0 for this to succeed. By default, * a non-NULL @exitstatus contains the normal exit status of the child * (death from a signal is treated as execution error); but if * virCommandRawStatus() was used, it instead contains the raw exit * status that the caller must then decipher using WIFEXITED() and friends.
perhaps the author (danpb) of commit id 7b3f1f8c3 would be able to say for sure...
I only see -1 being returned "on any error waiting for completion". Filling @exitstatus with @dryRunStatus is reasonable since it is initialized to 0 in virCommandRunAsync and is what is passed to @dryRunCallback and thus only changed as a result of running @dryRunCallback.
It has nothing to do with virCommandWait AFAICT.
So there are two ways how virCommandWait() can be called. The first is with @exitstatus being non-NULL. In this case, error is returned iff there was an error fetching command's exit status (e.g. because virProcessWait() failed). The second way is to call virCommandWait() with NULL in which case the function fails for all the cases in the first case plus if the command exit status is not zero. This is documented in docs/internals/command.html#async:
As with virCommandRun, the status arg for virCommandWait can be omitted, in which case it will validate that exit status is zero and raise an error if not.
Let's put aside dry run case for a while. Imagine /bin/false was started asynchronously and control now reaches virCommandWait(cmd, NULL). What do you think should be expected return value? I'd expect "Child process (%) unexpected..." error message and return -1. However, this is not the case if dry run callback sets an error.
Michal
Was /bin/false run successfully? It returns 1 (non zero). Isn't that expected? Did virCommandWait fail to wait for /bin/false to return? If someone wants the status from virCommandRunAsync, then they need to pass the @exitstatus in; otherwise, the command itself actually ran to completion and returned the expected result. If I want to know that result, then I should use the proper mechanism which is to pass @exitstatus. The virCommandWait didn't fail (regardless of DryRun or not) to wait for completion, so returning -1 because the underlying command "failed" seems to be outside it's scope of purpose. AFAICT, @DryRunStatus is meant to mimic @exitstatus. John
On 07/23/2018 02:07 PM, John Ferlan wrote:
On 07/23/2018 04:01 AM, Michal Prívozník wrote:
On 07/17/2018 08:43 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
The documentation to virCommandWait() function states that if @exitstatus is NULL and command finished with error -1 is returned. In other words, if @dryRunCallback is set and returns an error (by setting its @status argument to a nonzero value) we must propagate this error properly honouring the documentation (and also regular run).
That's not how I read virCommandWait:
* Wait for the command previously started with virCommandRunAsync() * to complete. Return -1 on any error waiting for * completion. Returns 0 if the command * finished with the exit status set. If @exitstatus is NULL, then the * child must exit with status 0 for this to succeed. By default, * a non-NULL @exitstatus contains the normal exit status of the child * (death from a signal is treated as execution error); but if * virCommandRawStatus() was used, it instead contains the raw exit * status that the caller must then decipher using WIFEXITED() and friends.
perhaps the author (danpb) of commit id 7b3f1f8c3 would be able to say for sure...
I only see -1 being returned "on any error waiting for completion". Filling @exitstatus with @dryRunStatus is reasonable since it is initialized to 0 in virCommandRunAsync and is what is passed to @dryRunCallback and thus only changed as a result of running @dryRunCallback.
It has nothing to do with virCommandWait AFAICT.
So there are two ways how virCommandWait() can be called. The first is with @exitstatus being non-NULL. In this case, error is returned iff there was an error fetching command's exit status (e.g. because virProcessWait() failed). The second way is to call virCommandWait() with NULL in which case the function fails for all the cases in the first case plus if the command exit status is not zero. This is documented in docs/internals/command.html#async:
As with virCommandRun, the status arg for virCommandWait can be omitted, in which case it will validate that exit status is zero and raise an error if not.
Let's put aside dry run case for a while. Imagine /bin/false was started asynchronously and control now reaches virCommandWait(cmd, NULL). What do you think should be expected return value? I'd expect "Child process (%) unexpected..." error message and return -1. However, this is not the case if dry run callback sets an error.
Michal
Was /bin/false run successfully? It returns 1 (non zero). Isn't that expected? Did virCommandWait fail to wait for /bin/false to return?
Yes. Yes. No.
If someone wants the status from virCommandRunAsync, then they need to pass the @exitstatus in; otherwise, the command itself actually ran to completion and returned the expected result. If I want to know that result, then I should use the proper mechanism which is to pass @exitstatus.
The virCommandWait didn't fail (regardless of DryRun or not) to wait for completion, so returning -1 because the underlying command "failed" seems to be outside it's scope of purpose.
Okay. I believe picture is more than words. So try this example: diff --git i/tools/virsh.c w/tools/virsh.c index 62226eea4c..2544fe0d74 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -867,6 +867,18 @@ main(int argc, char **argv) virshControl virshCtl; bool ret = true; + virCommandPtr cmd = virCommandNewArgList("/bin/false", NULL); + + if (virCommandRunAsync(cmd, NULL) < 0) { + fprintf(stderr, "async()\n"); + return -1; + } + + if (virCommandWait(cmd, NULL) < 0) { + fprintf(stderr, "wait()\n"); + return -1; + } + memset(ctl, 0, sizeof(vshControl)); memset(&virshCtl, 0, sizeof(virshControl)); ctl->name = "virsh"; /* hardcoded name of the binary */ And try replacing /bin/false with /bin/true. Also, try passing an int to virCommandWait() instead of NULL. You'll see what I mean then. And the whole point is that if we have a dry run callback set and it indicates an error we have to make virCommandWait(, NULL) fail - just like when it's failing with real execution. Michal
On 07/23/2018 08:27 AM, Michal Prívozník wrote:
On 07/23/2018 02:07 PM, John Ferlan wrote:
On 07/23/2018 04:01 AM, Michal Prívozník wrote:
On 07/17/2018 08:43 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
The documentation to virCommandWait() function states that if @exitstatus is NULL and command finished with error -1 is returned. In other words, if @dryRunCallback is set and returns an error (by setting its @status argument to a nonzero value) we must propagate this error properly honouring the documentation (and also regular run).
That's not how I read virCommandWait:
* Wait for the command previously started with virCommandRunAsync() * to complete. Return -1 on any error waiting for * completion. Returns 0 if the command * finished with the exit status set. If @exitstatus is NULL, then the * child must exit with status 0 for this to succeed. By default, * a non-NULL @exitstatus contains the normal exit status of the child * (death from a signal is treated as execution error); but if * virCommandRawStatus() was used, it instead contains the raw exit * status that the caller must then decipher using WIFEXITED() and friends.
perhaps the author (danpb) of commit id 7b3f1f8c3 would be able to say for sure...
I only see -1 being returned "on any error waiting for completion". Filling @exitstatus with @dryRunStatus is reasonable since it is initialized to 0 in virCommandRunAsync and is what is passed to @dryRunCallback and thus only changed as a result of running @dryRunCallback.
It has nothing to do with virCommandWait AFAICT.
So there are two ways how virCommandWait() can be called. The first is with @exitstatus being non-NULL. In this case, error is returned iff there was an error fetching command's exit status (e.g. because virProcessWait() failed). The second way is to call virCommandWait() with NULL in which case the function fails for all the cases in the first case plus if the command exit status is not zero. This is documented in docs/internals/command.html#async:
As with virCommandRun, the status arg for virCommandWait can be omitted, in which case it will validate that exit status is zero and raise an error if not.
Let's put aside dry run case for a while. Imagine /bin/false was started asynchronously and control now reaches virCommandWait(cmd, NULL). What do you think should be expected return value? I'd expect "Child process (%) unexpected..." error message and return -1. However, this is not the case if dry run callback sets an error.
Michal
Was /bin/false run successfully? It returns 1 (non zero). Isn't that expected? Did virCommandWait fail to wait for /bin/false to return?
Yes. Yes. No.
If someone wants the status from virCommandRunAsync, then they need to pass the @exitstatus in; otherwise, the command itself actually ran to completion and returned the expected result. If I want to know that result, then I should use the proper mechanism which is to pass @exitstatus.
The virCommandWait didn't fail (regardless of DryRun or not) to wait for completion, so returning -1 because the underlying command "failed" seems to be outside it's scope of purpose.
Okay. I believe picture is more than words. So try this example:
diff --git i/tools/virsh.c w/tools/virsh.c index 62226eea4c..2544fe0d74 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -867,6 +867,18 @@ main(int argc, char **argv) virshControl virshCtl; bool ret = true;
+ virCommandPtr cmd = virCommandNewArgList("/bin/false", NULL); + + if (virCommandRunAsync(cmd, NULL) < 0) { + fprintf(stderr, "async()\n"); + return -1; + } + + if (virCommandWait(cmd, NULL) < 0) { + fprintf(stderr, "wait()\n"); + return -1; + } + memset(ctl, 0, sizeof(vshControl)); memset(&virshCtl, 0, sizeof(virshControl)); ctl->name = "virsh"; /* hardcoded name of the binary */
And try replacing /bin/false with /bin/true. Also, try passing an int to virCommandWait() instead of NULL. You'll see what I mean then.
What you expect me to WORK for the answer ;-) Code as written: $ ./run tools/virsh wait() $ echo $? 255 $ <change from /bin/false to /bin/true> $ ./run tools/virsh virCommandWait exitstatus=1 virsh # quit $ <go back to /bin/false, add &exitstatus to virCommandWait, *and* a print of status> $ ./run tools/virsh virCommandWait exitstatus=0 virsh # quit $ <change from /bin/false to /bin/true> $ ./run tools/virsh virCommandWait exitstatus=0 virsh # quit $ I don't see the problem if you decide to pass @exitstatus to virCommandWait and then choose to ignore actually checking the status, then you WYSIWYG. If in the /bin/false case you checked "if (exitstatus != 0)" and fail, then you'd get what I think you're hinting should happen.
And the whole point is that if we have a dry run callback set and it indicates an error we have to make virCommandWait(, NULL) fail - just like when it's failing with real execution.
If the caller passes @exitstatus to virCommandWait, but doesn't check it's value, then who's coding error is that? I see virCommandWait documented as: "If @exitstatus is NULL, then the child must exit with status 0 for this to succeed." IOW, AIUI, usage of @exitstatus gives one finer grained control over knowing whether the command run by virCommandRunAsync failed or whether virCommandWait failed. If exitstatus == NULL, then if either fails, you get a -1 returned; otherwise, using exitstatus you can determine whether the command failed or the wait for the command to run failed. John
On 07/23/2018 03:49 PM, John Ferlan wrote:
On 07/23/2018 08:27 AM, Michal Prívozník wrote:
On 07/23/2018 02:07 PM, John Ferlan wrote:
On 07/23/2018 04:01 AM, Michal Prívozník wrote:
On 07/17/2018 08:43 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
The documentation to virCommandWait() function states that if @exitstatus is NULL and command finished with error -1 is returned. In other words, if @dryRunCallback is set and returns an error (by setting its @status argument to a nonzero value) we must propagate this error properly honouring the documentation (and also regular run).
That's not how I read virCommandWait:
* Wait for the command previously started with virCommandRunAsync() * to complete. Return -1 on any error waiting for * completion. Returns 0 if the command * finished with the exit status set. If @exitstatus is NULL, then the * child must exit with status 0 for this to succeed. By default, * a non-NULL @exitstatus contains the normal exit status of the child * (death from a signal is treated as execution error); but if * virCommandRawStatus() was used, it instead contains the raw exit * status that the caller must then decipher using WIFEXITED() and friends.
perhaps the author (danpb) of commit id 7b3f1f8c3 would be able to say for sure...
I only see -1 being returned "on any error waiting for completion". Filling @exitstatus with @dryRunStatus is reasonable since it is initialized to 0 in virCommandRunAsync and is what is passed to @dryRunCallback and thus only changed as a result of running @dryRunCallback.
It has nothing to do with virCommandWait AFAICT.
So there are two ways how virCommandWait() can be called. The first is with @exitstatus being non-NULL. In this case, error is returned iff there was an error fetching command's exit status (e.g. because virProcessWait() failed). The second way is to call virCommandWait() with NULL in which case the function fails for all the cases in the first case plus if the command exit status is not zero. This is documented in docs/internals/command.html#async:
As with virCommandRun, the status arg for virCommandWait can be omitted, in which case it will validate that exit status is zero and raise an error if not.
Let's put aside dry run case for a while. Imagine /bin/false was started asynchronously and control now reaches virCommandWait(cmd, NULL). What do you think should be expected return value? I'd expect "Child process (%) unexpected..." error message and return -1. However, this is not the case if dry run callback sets an error.
Michal
Was /bin/false run successfully? It returns 1 (non zero). Isn't that expected? Did virCommandWait fail to wait for /bin/false to return?
Yes. Yes. No.
If someone wants the status from virCommandRunAsync, then they need to pass the @exitstatus in; otherwise, the command itself actually ran to completion and returned the expected result. If I want to know that result, then I should use the proper mechanism which is to pass @exitstatus.
The virCommandWait didn't fail (regardless of DryRun or not) to wait for completion, so returning -1 because the underlying command "failed" seems to be outside it's scope of purpose.
Okay. I believe picture is more than words. So try this example:
diff --git i/tools/virsh.c w/tools/virsh.c index 62226eea4c..2544fe0d74 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -867,6 +867,18 @@ main(int argc, char **argv) virshControl virshCtl; bool ret = true;
+ virCommandPtr cmd = virCommandNewArgList("/bin/false", NULL); + + if (virCommandRunAsync(cmd, NULL) < 0) { + fprintf(stderr, "async()\n"); + return -1; + } + + if (virCommandWait(cmd, NULL) < 0) { + fprintf(stderr, "wait()\n"); + return -1; + } + memset(ctl, 0, sizeof(vshControl)); memset(&virshCtl, 0, sizeof(virshControl)); ctl->name = "virsh"; /* hardcoded name of the binary */
And try replacing /bin/false with /bin/true. Also, try passing an int to virCommandWait() instead of NULL. You'll see what I mean then.
What you expect me to WORK for the answer ;-)
Code as written:
$ ./run tools/virsh wait()
So virCommandWait() failed. As expected.
$ echo $? 255 $
<change from /bin/false to /bin/true>
$ ./run tools/virsh virCommandWait exitstatus=1
It did not fail here, because /bin/true returns 0 (in contrast to /bin/false which returns -1).
virsh # quit $
<go back to /bin/false, add &exitstatus to virCommandWait, *and* a print of status>
$ ./run tools/virsh virCommandWait exitstatus=0 virsh # quit $
<change from /bin/false to /bin/true>
$ ./run tools/virsh virCommandWait exitstatus=0 virsh # quit $
I don't see the problem if you decide to pass @exitstatus to virCommandWait and then choose to ignore actually checking the status, then you WYSIWYG.
Sure, but that is not the point. The point is, caller can pass NULL which is equivalent to saying "I don't care what the actual problem was, if anything fails (be it failing to wait, or command returning non zero, or OOM, or anything else) I want you to return -1". It can be written in dozen other ways, sure. But we already have chosen one. And if we use dry run there is a bug because it behaves differently than actual command execution.
If in the /bin/false case you checked "if (exitstatus != 0)" and fail, then you'd get what I think you're hinting should happen.
This is not the point.
And the whole point is that if we have a dry run callback set and it indicates an error we have to make virCommandWait(, NULL) fail - just like when it's failing with real execution.
If the caller passes @exitstatus to virCommandWait, but doesn't check it's value, then who's coding error is that?
No one's. This is equivalent to saying: "I don't care what command returned (whether it succeeded or not), I'm only interested whether it finished, i.e. we successfully waited for it". This is allowed also. But then again, I'm trying to fix case where @exitstatus is NULL.
I see virCommandWait documented as:
"If @exitstatus is NULL, then the child must exit with status 0 for this to succeed."
IOW, AIUI, usage of @exitstatus gives one finer grained control over knowing whether the command run by virCommandRunAsync failed or whether virCommandWait failed. If exitstatus == NULL, then if either fails, you get a -1 returned; otherwise, using exitstatus you can determine whether the command failed or the wait for the command to run failed.
Exactly! But, this is not the case when dry run callback returns an error but caller waits with @exitstatus = NULL. IOW, now that we've seen how virCommandWait() behaves with real binaries, we can take that behaviour for granted and have to make sure that using dry run callbacks behaves the same. Does it? I don't think so. Michal
Let's just go back to the start on this one as light has dawned on marblehead and it's now clear what the technicality I was missing on the reinterpretation of the API docs. On 07/04/2018 05:23 AM, Michal Privoznik wrote:
The documentation to virCommandWait() function states that if @exitstatus is NULL and command finished with error -1 is returned. In other words, if @dryRunCallback is set and returns an error (by setting its @status argument to a nonzero value) we must propagate this error properly honouring the documentation (and also regular run).
The documentation to virCommandWait() functions states "If @exitstatus is NULL, then the child must exit with status 0 for this to succeed." Thus if a dryRunCallback environment doesn't pass @exitstatus, then when @dryRunStatus is not zero we must return -1. ... So the technicality is that dryRunCallback is always set - whether it's set to 0 in virCommandRunAsync or (inexplicably at this point) to some other value in some as yet to be created mocked/fictitious environment. My first gut instinct on this was why not just return dryRunStatus, but that conflicts with the exitstatus rules and well I just couldn't get past that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircommand.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
On 07/24/2018 04:53 PM, John Ferlan wrote:
Let's just go back to the start on this one as light has dawned on marblehead and it's now clear what the technicality I was missing on the reinterpretation of the API docs.
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
The documentation to virCommandWait() function states that if @exitstatus is NULL and command finished with error -1 is returned. In other words, if @dryRunCallback is set and returns an error (by setting its @status argument to a nonzero value) we must propagate this error properly honouring the documentation (and also regular run).
The documentation to virCommandWait() functions states "If @exitstatus is NULL, then the child must exit with status 0 for this to succeed."
Thus if a dryRunCallback environment doesn't pass @exitstatus, then when @dryRunStatus is not zero we must return -1.
...
So the technicality is that dryRunCallback is always set - whether it's set to 0 in virCommandRunAsync or (inexplicably at this point) to some other value in some as yet to be created mocked/fictitious environment.
My first gut instinct on this was why not just return dryRunStatus, but that conflicts with the exitstatus rules and well I just couldn't get past that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircommand.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com>
I've pushed these. Thanks! Michal
Introduce one basic test that tests the simplest case: logging into portal without any IQN. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 3bb3993196..a7287069ba 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args, args[8] && STREQ(args[8], "nonpersistent") && args[9] == NULL) { ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "node") && + args[3] && STREQ(args[3], "--portal") && + args[4] && STREQ(args[4], "10.20.30.40:3260,1") && + args[5] && STREQ(args[5], "--targetname") && + args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") && + args[7] && STREQ(args[7], "--login") && + args[8] == NULL) { + /* nada */ } else { *status = -1; } @@ -175,6 +185,32 @@ testISCSIScanTargets(const void *data) return ret; } + +struct testConnectionInfoLogin { + const char *portal; + const char *initiatoriqn; + const char *target; +}; + + +static int +testISCSIConnectionLogin(const void *data) +{ + const struct testConnectionInfoLogin *info = data; + int ret = -1; + + virCommandSetDryRun(NULL, testIscsiadmCb, NULL); + + if (virISCSIConnectionLogin(info->portal, info->initiatoriqn, info->target) < 0) + goto cleanup; + + ret = 0; + cleanup: + virCommandSetDryRun(NULL, NULL, NULL); + return ret; +} + + static int mymain(void) { @@ -213,6 +249,16 @@ mymain(void) if (virTestRun("ISCSI scan targets", testISCSIScanTargets, &infoTargets) < 0) rv = -1; +# define DO_LOGIN_TEST(portal, iqn, target) \ + do { \ + struct testConnectionInfoLogin info = {portal, iqn, target }; \ + if (virTestRun("ISCSI login " portal, \ + testISCSIConnectionLogin, &info) < 0) \ + rv = -1; \ + } while (0) + + DO_LOGIN_TEST("10.20.30.40:3260,1", NULL, "iqn.2004-06.example:example1:iscsi.test"); + if (rv < 0) return EXIT_FAILURE; return EXIT_SUCCESS; -- 2.16.4
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Introduce one basic test that tests the simplest case: logging into portal without any IQN.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John FWIW: This is successful for me w/o patch 6
On 07/17/2018 08:46 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Introduce one basic test that tests the simplest case: logging into portal without any IQN.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
FWIW: This is successful for me w/o patch 6
Of course it is. The testIscsiadmCb() is not failing because of hunk 1. But try removing both patch 6 AND the first hunk (so that testIscsiadmCb() sets *status = -1;). I guess now you can see my reasoning for patch 6. Michal
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Introduce one basic test that tests the simplest case: logging into portal without any IQN.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 3bb3993196..a7287069ba 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args, args[8] && STREQ(args[8], "nonpersistent") && args[9] == NULL) { ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "node") && + args[3] && STREQ(args[3], "--portal") && + args[4] && STREQ(args[4], "10.20.30.40:3260,1") && + args[5] && STREQ(args[5], "--targetname") && + args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") && + args[7] && STREQ(args[7], "--login") && + args[8] == NULL) { + /* nada */
Hmm.. can we place a hold on that R-By - I missed this gem "nada" - why is that? If we have *output because we've sent the dryRun, then why not compare? John
} else { *status = -1; } @@ -175,6 +185,32 @@ testISCSIScanTargets(const void *data) return ret; }
+ +struct testConnectionInfoLogin { + const char *portal; + const char *initiatoriqn; + const char *target; +}; + + +static int +testISCSIConnectionLogin(const void *data) +{ + const struct testConnectionInfoLogin *info = data; + int ret = -1; + + virCommandSetDryRun(NULL, testIscsiadmCb, NULL); + + if (virISCSIConnectionLogin(info->portal, info->initiatoriqn, info->target) < 0) + goto cleanup; + + ret = 0; + cleanup: + virCommandSetDryRun(NULL, NULL, NULL); + return ret; +} + + static int mymain(void) { @@ -213,6 +249,16 @@ mymain(void) if (virTestRun("ISCSI scan targets", testISCSIScanTargets, &infoTargets) < 0) rv = -1;
+# define DO_LOGIN_TEST(portal, iqn, target) \ + do { \ + struct testConnectionInfoLogin info = {portal, iqn, target }; \ + if (virTestRun("ISCSI login " portal, \ + testISCSIConnectionLogin, &info) < 0) \ + rv = -1; \ + } while (0) + + DO_LOGIN_TEST("10.20.30.40:3260,1", NULL, "iqn.2004-06.example:example1:iscsi.test"); + if (rv < 0) return EXIT_FAILURE; return EXIT_SUCCESS;
On 07/17/2018 09:14 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Introduce one basic test that tests the simplest case: logging into portal without any IQN.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 3bb3993196..a7287069ba 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args, args[8] && STREQ(args[8], "nonpersistent") && args[9] == NULL) { ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "node") && + args[3] && STREQ(args[3], "--portal") && + args[4] && STREQ(args[4], "10.20.30.40:3260,1") && + args[5] && STREQ(args[5], "--targetname") && + args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") && + args[7] && STREQ(args[7], "--login") && + args[8] == NULL) { + /* nada */
Hmm.. can we place a hold on that R-By - I missed this gem "nada" - why is that?
If we have *output because we've sent the dryRun, then why not compare?
I'm not quite sure what you mean. What should be compared? Michal
On 07/23/2018 04:01 AM, Michal Prívozník wrote:
On 07/17/2018 09:14 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Introduce one basic test that tests the simplest case: logging into portal without any IQN.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 3bb3993196..a7287069ba 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args, args[8] && STREQ(args[8], "nonpersistent") && args[9] == NULL) { ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
here we do some sort of comparison
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "node") && + args[3] && STREQ(args[3], "--portal") && + args[4] && STREQ(args[4], "10.20.30.40:3260,1") && + args[5] && STREQ(args[5], "--targetname") && + args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") && + args[7] && STREQ(args[7], "--login") && + args[8] == NULL) { + /* nada */
Hmm.. can we place a hold on that R-By - I missed this gem "nada" - why is that?
If we have *output because we've sent the dryRun, then why not compare?
I'm not quite sure what you mean. What should be compared?
here we do not - instead we just use nada. So after reading patch10 where nada is again used, I began to doubt/wonder about using it here especially since there are comparisons for all other cases. John
On 07/23/2018 02:12 PM, John Ferlan wrote:
On 07/23/2018 04:01 AM, Michal Prívozník wrote:
On 07/17/2018 09:14 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Introduce one basic test that tests the simplest case: logging into portal without any IQN.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 3bb3993196..a7287069ba 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args, args[8] && STREQ(args[8], "nonpersistent") && args[9] == NULL) { ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
here we do some sort of comparison
What comparison? The only comparison I see is "args[X] && STREQ(args[X], ...)". If you're referring to VIR_STRDUP() that is setting the pretended output of the iscsiadm command. It's not comparing anything.
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "node") && + args[3] && STREQ(args[3], "--portal") && + args[4] && STREQ(args[4], "10.20.30.40:3260,1") && + args[5] && STREQ(args[5], "--targetname") && + args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") && + args[7] && STREQ(args[7], "--login") && + args[8] == NULL) { + /* nada */
Hmm.. can we place a hold on that R-By - I missed this gem "nada" - why is that?
If we have *output because we've sent the dryRun, then why not compare?
I'm not quite sure what you mean. What should be compared?
here we do not - instead we just use nada.
That is correct because we do need to accept that cmd line but we do not need to take any extra step to mimic iscsiadm behaviour we're after.
So after reading patch10 where nada is again used, I began to doubt/wonder about using it here especially since there are comparisons for all other cases.
John
Michal
On 07/23/2018 08:34 AM, Michal Prívozník wrote:
On 07/23/2018 02:12 PM, John Ferlan wrote:
On 07/23/2018 04:01 AM, Michal Prívozník wrote:
On 07/17/2018 09:14 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Introduce one basic test that tests the simplest case: logging into portal without any IQN.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 3bb3993196..a7287069ba 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args, args[8] && STREQ(args[8], "nonpersistent") && args[9] == NULL) { ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
here we do some sort of comparison
What comparison? The only comparison I see is "args[X] && STREQ(args[X], ...)".
If you're referring to VIR_STRDUP() that is setting the pretended output of the iscsiadm command. It's not comparing anything.
Can you just indicate rather than "nada" what the actual deal is here? That is, in our mocked environment when running this command we don't expect to get any output from iscsiadm. If this were a real command then the following would be returned: ... In my saved iSCSI output file, I have for example: iscsiadm -m node -T iqn.2013-12.com.example:iscsi-chap-pool -p 192.168.122.1 --login returning: Logging in to [iface: default, target: iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260] (multiple) Login to [iface: default, target: iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260] successful. Hopefully this helps resolves the "confusion" over this. Like I said for patch10, no need for a v3, just a diff would be fine, although at this point it only helps for my really short term memory. John
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "node") && + args[3] && STREQ(args[3], "--portal") && + args[4] && STREQ(args[4], "10.20.30.40:3260,1") && + args[5] && STREQ(args[5], "--targetname") && + args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") && + args[7] && STREQ(args[7], "--login") && + args[8] == NULL) { + /* nada */
Hmm.. can we place a hold on that R-By - I missed this gem "nada" - why is that?
If we have *output because we've sent the dryRun, then why not compare?
I'm not quite sure what you mean. What should be compared?
here we do not - instead we just use nada.
That is correct because we do need to accept that cmd line but we do not need to take any extra step to mimic iscsiadm behaviour we're after.
So after reading patch10 where nada is again used, I began to doubt/wonder about using it here especially since there are comparisons for all other cases.
John
Michal
On 07/23/2018 03:55 PM, John Ferlan wrote:
On 07/23/2018 08:34 AM, Michal Prívozník wrote:
On 07/23/2018 02:12 PM, John Ferlan wrote:
On 07/23/2018 04:01 AM, Michal Prívozník wrote:
On 07/17/2018 09:14 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Introduce one basic test that tests the simplest case: logging into portal without any IQN.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 3bb3993196..a7287069ba 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args, args[8] && STREQ(args[8], "nonpersistent") && args[9] == NULL) { ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
here we do some sort of comparison
What comparison? The only comparison I see is "args[X] && STREQ(args[X], ...)".
If you're referring to VIR_STRDUP() that is setting the pretended output of the iscsiadm command. It's not comparing anything.
Can you just indicate rather than "nada" what the actual deal is here?
Okay, how about "/* no output */" instead of "nada"?
That is, in our mocked environment when running this command we don't expect to get any output from iscsiadm. If this were a real command then the following would be returned:
...
In my saved iSCSI output file, I have for example:
iscsiadm -m node -T iqn.2013-12.com.example:iscsi-chap-pool -p 192.168.122.1 --login
returning:
Logging in to [iface: default, target: iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260] (multiple) Login to [iface: default, target: iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260] successful.
But this output is never processed by our code, therefore it doesn't make much sense for our 'mock' to produce any. Michal
On 07/24/2018 04:25 AM, Michal Privoznik wrote:
On 07/23/2018 03:55 PM, John Ferlan wrote:
On 07/23/2018 08:34 AM, Michal Prívozník wrote:
On 07/23/2018 02:12 PM, John Ferlan wrote:
On 07/23/2018 04:01 AM, Michal Prívozník wrote:
On 07/17/2018 09:14 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote: > Introduce one basic test that tests the simplest case: > logging into portal without any IQN. > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > tests/viriscsitest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c > index 3bb3993196..a7287069ba 100644 > --- a/tests/viriscsitest.c > +++ b/tests/viriscsitest.c > @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args, > args[8] && STREQ(args[8], "nonpersistent") && > args[9] == NULL) { > ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
here we do some sort of comparison
What comparison? The only comparison I see is "args[X] && STREQ(args[X], ...)".
If you're referring to VIR_STRDUP() that is setting the pretended output of the iscsiadm command. It's not comparing anything.
Can you just indicate rather than "nada" what the actual deal is here?
Okay, how about "/* no output */" instead of "nada"?
Not convinced that's better than nada? Mainly because it's not true and the example below I believe proves that point. As poor of an API to configure things as iscsiadm is, it can be quite chatty for output. Replace -T with --targetname and -p with --portal. Not only is there visual output, but (at least on my host) there's a sequence of clicks that happens on login and logout.
That is, in our mocked environment when running this command we don't expect to get any output from iscsiadm. If this were a real command then the following would be returned:
...
In my saved iSCSI output file, I have for example:
iscsiadm -m node -T iqn.2013-12.com.example:iscsi-chap-pool -p 192.168.122.1 --login
returning:
Logging in to [iface: default, target: iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260] (multiple) Login to [iface: default, target: iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260] successful.
But this output is never processed by our code, therefore it doesn't make much sense for our 'mock' to produce any.
Not exactly the point compared to the 3 strings already present in the code that are in a way processed, but I do understand what you're stating. Remove yourself from the authorship of the code and you don't find it strange that some commands aren't producing mock output? Reviewing such code would you just be satisfied with "/* nada */"? Wouldn't you want to read the code, try an example or two, and then make a decision how to proceed. If during your examples you found output, then where do you stand on "nada" or "no output"? I really don't think it's that much of an ask or a reach to supply information that would instantaneously help someone looking at this code to know what is going on. Reading "nada" or "no output" doesn't help. Reading something like: /* mocking real environment output is not feasible for [creating | updating | logging into], example of real environment is: xxx */ I believe is better - it's not difficult to add, but I'm at the point of not caring right now because this truly has gone on too long. For the code/logic: Reviewed-by: John Ferlan <jferlan@redhat.com> Do whatever you want for the comments. I disagree that "nada" or "no output" is good enough, but it's not worth holding this up. John
On 07/24/2018 04:08 PM, John Ferlan wrote:
<snip/>> Reading something like:
/* mocking real environment output is not feasible for [creating | updating | logging into], example of real environment is:
xxx
*/
I believe is better - it's not difficult to add, but I'm at the point of not caring right now because this truly has gone on too long.
For the code/logic:
Reviewed-by: John Ferlan <jferlan@redhat.com>
Do whatever you want for the comments. I disagree that "nada" or "no output" is good enough, but it's not worth holding this up.
Thanks. I'll add the real output into comments as you're suggesting. Michal
This struct has nothing to do with testIscsiadmCb() rather than testISCSIGetSession(). Move it closer to the latter. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index a7287069ba..361bf6a8da 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -60,12 +60,6 @@ const char *iscsiadmSendtargetsOutput = "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n"; -struct testSessionInfo { - const char *device_path; - int output_version; - const char *expected_session; -}; - static void testIscsiadmCb(const char *const*args, const char *const*env ATTRIBUTE_UNUSED, const char *input ATTRIBUTE_UNUSED, @@ -109,6 +103,12 @@ static void testIscsiadmCb(const char *const*args, } } +struct testSessionInfo { + const char *device_path; + int output_version; + const char *expected_session; +}; + static int testISCSIGetSession(const void *data) { -- 2.16.4
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
This struct has nothing to do with testIscsiadmCb() rather than testISCSIGetSession(). Move it closer to the latter.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Some tests will want to pass their own callback data into the testIscsiadmCbData callback. Introduce testIscsiadmCbData struct to give this some form and order. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 361bf6a8da..c6e0e3b8ff 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -60,6 +60,10 @@ const char *iscsiadmSendtargetsOutput = "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n"; +struct testIscsiadmCbData { + bool output_version; +}; + static void testIscsiadmCb(const char *const*args, const char *const*env ATTRIBUTE_UNUSED, const char *input ATTRIBUTE_UNUSED, @@ -68,12 +72,13 @@ static void testIscsiadmCb(const char *const*args, int *status, void *opaque) { - int *output_version = opaque; + struct testIscsiadmCbData *data = opaque; + if (args[0] && STREQ(args[0], ISCSIADM) && args[1] && STREQ(args[1], "--mode") && args[2] && STREQ(args[2], "session") && args[3] == NULL) { - if (*output_version == 1) + if (data->output_version) ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutputNonFlash)); else ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutput)); @@ -113,11 +118,13 @@ static int testISCSIGetSession(const void *data) { const struct testSessionInfo *info = data; - int ver = info->output_version; + struct testIscsiadmCbData cbData = { 0 }; char *actual_session = NULL; int ret = -1; - virCommandSetDryRun(NULL, testIscsiadmCb, &ver); + cbData.output_version = info->output_version; + + virCommandSetDryRun(NULL, testIscsiadmCb, &cbData); actual_session = virISCSIGetSession(info->device_path, true); -- 2.16.4
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Some tests will want to pass their own callback data into the testIscsiadmCbData callback. Introduce testIscsiadmCbData struct to give this some form and order.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 361bf6a8da..c6e0e3b8ff 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -60,6 +60,10 @@ const char *iscsiadmSendtargetsOutput = "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
+struct testIscsiadmCbData { + bool output_version; +}; + static void testIscsiadmCb(const char *const*args, const char *const*env ATTRIBUTE_UNUSED, const char *input ATTRIBUTE_UNUSED, @@ -68,12 +72,13 @@ static void testIscsiadmCb(const char *const*args, int *status, void *opaque) { - int *output_version = opaque; + struct testIscsiadmCbData *data = opaque; + if (args[0] && STREQ(args[0], ISCSIADM) && args[1] && STREQ(args[1], "--mode") && args[2] && STREQ(args[2], "session") && args[3] == NULL) { - if (*output_version == 1) + if (data->output_version) ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutputNonFlash)); else ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutput)); @@ -113,11 +118,13 @@ static int testISCSIGetSession(const void *data) { const struct testSessionInfo *info = data; - int ver = info->output_version; + struct testIscsiadmCbData cbData = { 0 }; char *actual_session = NULL; int ret = -1;
- virCommandSetDryRun(NULL, testIscsiadmCb, &ver); + cbData.output_version = info->output_version;
Should this be "= info->output_version == 1" instead? Alternatively you could change testSessionInfo->output_version to be a bool and then DO_SESSION_TEST to use 'False' and 'True' appropriately. One way or the other, Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ + virCommandSetDryRun(NULL, testIscsiadmCb, &cbData);
actual_session = virISCSIGetSession(info->device_path, true);
Extend this existing test so that a case when IQN is provided is tested too. Since a special iSCSI interface is created and its name is randomly generated at runtime we need to link with virrandommock to have predictable names. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 2 deletions(-) diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index c6e0e3b8ff..55889d04a3 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -60,8 +60,19 @@ const char *iscsiadmSendtargetsOutput = "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n"; +const char *iscsiadmIfaceDefaultOutput = + "default tcp,<empty>,<empty>,<empty>,<empty>\n" + "iser iser,<empty>,<empty>,<empty>,<empty>\n"; + +const char *iscsiadmIfaceIfaceOutput = + "default tcp,<empty>,<empty>,<empty>,<empty>\n" + "iser iser,<empty>,<empty>,<empty>,<empty>\n" + "libvirt-iface-03020100 tcp,<empty>,<empty>,<empty>,iqn.2004-06.example:example1:initiator\n"; + + struct testIscsiadmCbData { bool output_version; + bool iface_created; }; static void testIscsiadmCb(const char *const*args, @@ -103,6 +114,62 @@ static void testIscsiadmCb(const char *const*args, args[7] && STREQ(args[7], "--login") && args[8] == NULL) { /* nada */ + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] == NULL) { + if (data->iface_created) + ignore_value(VIR_STRDUP(*output, iscsiadmIfaceIfaceOutput)); + else + ignore_value(VIR_STRDUP(*output, iscsiadmIfaceDefaultOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] && STREQ(args[3], "--interface") && + args[4] && STREQ(args[4], "libvirt-iface-03020100") && + args[5] && STREQ(args[5], "--op") && + args[6] && STREQ(args[6], "new") && + args[7] == NULL) { + data->iface_created = true; + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] && STREQ(args[3], "--interface") && + args[4] && STREQ(args[4], "libvirt-iface-03020100") && + args[5] && STREQ(args[5], "--op") && + args[6] && STREQ(args[6], "update") && + args[7] && STREQ(args[7], "--name") && + args[8] && STREQ(args[8], "iface.initiatorname") && + args[9] && STREQ(args[9], "--value") && + args[10] && STREQ(args[10], "iqn.2004-06.example:example1:initiator") && + args[11] == NULL && + data->iface_created) { + /* nada */ + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "discovery") && + args[3] && STREQ(args[3], "--type") && + 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] && STREQ(args[7], "--interface") && + args[8] && STREQ(args[8], "libvirt-iface-03020100") && + args[9] == NULL && + data->iface_created) { + ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "node") && + args[3] && STREQ(args[3], "--portal") && + args[4] && STREQ(args[4], "10.20.30.40:3260,1") && + args[5] && STREQ(args[5], "--targetname") && + args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") && + args[7] && STREQ(args[7], "--login") && + args[8] && STREQ(args[8], "--interface") && + args[9] && STREQ(args[9], "libvirt-iface-03020100") && + args[10] == NULL && + data->iface_created) { + /* nada */ } else { *status = -1; } @@ -204,9 +271,10 @@ static int testISCSIConnectionLogin(const void *data) { const struct testConnectionInfoLogin *info = data; + struct testIscsiadmCbData cbData = { 0 }; int ret = -1; - virCommandSetDryRun(NULL, testIscsiadmCb, NULL); + virCommandSetDryRun(NULL, testIscsiadmCb, &cbData); if (virISCSIConnectionLogin(info->portal, info->initiatoriqn, info->target) < 0) goto cleanup; @@ -265,11 +333,14 @@ mymain(void) } while (0) DO_LOGIN_TEST("10.20.30.40:3260,1", NULL, "iqn.2004-06.example:example1:iscsi.test"); + DO_LOGIN_TEST("10.20.30.40:3260,1", "iqn.2004-06.example:example1:initiator", + "iqn.2004-06.example:example1:iscsi.test"); if (rv < 0) return EXIT_FAILURE; return EXIT_SUCCESS; } -VIR_TEST_MAIN(mymain) +VIR_TEST_MAIN_PRELOAD(mymain, + abs_builddir "/.libs/virrandommock.so") #endif /* WIN32 */ -- 2.16.4
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Extend this existing test so that a case when IQN is provided is tested too. Since a special iSCSI interface is created and its name is randomly generated at runtime we need to link with virrandommock to have predictable names.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 2 deletions(-)
Should testIscsiadmCbData initialization get { false, false } now (and I should have mention in patch 9 that : + struct testIscsiadmCbData cbData = { 0 }; should be + struct testIscsiadmCbData cbData = { false }; I know that 0 and false are synonymous, but syntactical correctness is in play here ;-) I also think you're doing two things here: 1. Generating a dryRun output for "existing" test without initiator.iqn 2. Adding tests to test initiator.iqn
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index c6e0e3b8ff..55889d04a3 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -60,8 +60,19 @@ const char *iscsiadmSendtargetsOutput = "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
+const char *iscsiadmIfaceDefaultOutput = + "default tcp,<empty>,<empty>,<empty>,<empty>\n" + "iser iser,<empty>,<empty>,<empty>,<empty>\n"; + +const char *iscsiadmIfaceIfaceOutput = + "default tcp,<empty>,<empty>,<empty>,<empty>\n" + "iser iser,<empty>,<empty>,<empty>,<empty>\n" + "libvirt-iface-03020100 tcp,<empty>,<empty>,<empty>,iqn.2004-06.example:example1:initiator\n"; + + struct testIscsiadmCbData { bool output_version; + bool iface_created; };
static void testIscsiadmCb(const char *const*args, @@ -103,6 +114,62 @@ static void testIscsiadmCb(const char *const*args, args[7] && STREQ(args[7], "--login") && args[8] == NULL) { /* nada */
Is this "nada"
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] == NULL) { + if (data->iface_created)
How would iface_created be set by this point if...
+ ignore_value(VIR_STRDUP(*output, iscsiadmIfaceIfaceOutput)); + else + ignore_value(VIR_STRDUP(*output, iscsiadmIfaceDefaultOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] && STREQ(args[3], "--interface") && + args[4] && STREQ(args[4], "libvirt-iface-03020100") && + args[5] && STREQ(args[5], "--op") && + args[6] && STREQ(args[6], "new") && + args[7] == NULL) { + data->iface_created = true;
... it's being set unconditionally here? See note below, shouldn't the caller be doing this?
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] && STREQ(args[3], "--interface") && + args[4] && STREQ(args[4], "libvirt-iface-03020100") && + args[5] && STREQ(args[5], "--op") && + args[6] && STREQ(args[6], "update") && + args[7] && STREQ(args[7], "--name") && + args[8] && STREQ(args[8], "iface.initiatorname") && + args[9] && STREQ(args[9], "--value") && + args[10] && STREQ(args[10], "iqn.2004-06.example:example1:initiator") && + args[11] == NULL && + data->iface_created) { + /* nada */
?? Why nada (now you know where my 7/10 requery came from)
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "discovery") && + args[3] && STREQ(args[3], "--type") && + 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] && STREQ(args[7], "--interface") && + args[8] && STREQ(args[8], "libvirt-iface-03020100") && + args[9] == NULL && + data->iface_created) { + ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "node") && + args[3] && STREQ(args[3], "--portal") && + args[4] && STREQ(args[4], "10.20.30.40:3260,1") && + args[5] && STREQ(args[5], "--targetname") && + args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") && + args[7] && STREQ(args[7], "--login") && + args[8] && STREQ(args[8], "--interface") && + args[9] && STREQ(args[9], "libvirt-iface-03020100") && + args[10] == NULL && + data->iface_created) { + /* nada */
Similar, why nada?
} else { *status = -1; } @@ -204,9 +271,10 @@ static int testISCSIConnectionLogin(const void *data) { const struct testConnectionInfoLogin *info = data; + struct testIscsiadmCbData cbData = { 0 };
s/0/false, false/
int ret = -1;
Why wouldn't initialization be: cbData.iface_create = info->initiatoriqn != NULL; John
- virCommandSetDryRun(NULL, testIscsiadmCb, NULL); + virCommandSetDryRun(NULL, testIscsiadmCb, &cbData);
if (virISCSIConnectionLogin(info->portal, info->initiatoriqn, info->target) < 0) goto cleanup; @@ -265,11 +333,14 @@ mymain(void) } while (0)
DO_LOGIN_TEST("10.20.30.40:3260,1", NULL, "iqn.2004-06.example:example1:iscsi.test"); + DO_LOGIN_TEST("10.20.30.40:3260,1", "iqn.2004-06.example:example1:initiator", + "iqn.2004-06.example:example1:iscsi.test");
if (rv < 0) return EXIT_FAILURE; return EXIT_SUCCESS; }
-VIR_TEST_MAIN(mymain) +VIR_TEST_MAIN_PRELOAD(mymain, + abs_builddir "/.libs/virrandommock.so") #endif /* WIN32 */
On 07/17/2018 09:15 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Extend this existing test so that a case when IQN is provided is tested too. Since a special iSCSI interface is created and its name is randomly generated at runtime we need to link with virrandommock to have predictable names.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 2 deletions(-)
Should testIscsiadmCbData initialization get { false, false } now (and I should have mention in patch 9 that :
+ struct testIscsiadmCbData cbData = { 0 };
should be
+ struct testIscsiadmCbData cbData = { false };> I know that 0 and false are synonymous, but syntactical correctness is in play here ;-)
No. So { 0 } is basically a shortcut for: struct testIscsiadmCbData cbData; memset(&cbData, 0, sizeof(cbData)); It has nothing to do with the struct having only two bools (for now). It is used widely in our code, because it has one great advantage: even if we add/remove/rearrange items in the struct it is always going to be zeroed out whereas if I initialized it like this: struct testIscsiadmCbData cbData = {false, false}; and then somebody would append yet another member to the struct they would either: a) have to change all the lines where the struct is initialized (possibly touching functions that has nothing to do with actual change), or b) forget to initialize it completely. TL;DR It's just coincidence that the first member is a bool.
I also think you're doing two things here:
1. Generating a dryRun output for "existing" test without initiator.iqn
2. Adding tests to test initiator.iqn
Sure. That's how login works. Firstly it lists output of iscsi interfaces, then it creates a new one and then use that to log in. I don't see a problem here.
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index c6e0e3b8ff..55889d04a3 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -60,8 +60,19 @@ const char *iscsiadmSendtargetsOutput = "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
+const char *iscsiadmIfaceDefaultOutput = + "default tcp,<empty>,<empty>,<empty>,<empty>\n" + "iser iser,<empty>,<empty>,<empty>,<empty>\n"; + +const char *iscsiadmIfaceIfaceOutput = + "default tcp,<empty>,<empty>,<empty>,<empty>\n" + "iser iser,<empty>,<empty>,<empty>,<empty>\n" + "libvirt-iface-03020100 tcp,<empty>,<empty>,<empty>,iqn.2004-06.example:example1:initiator\n"; + + struct testIscsiadmCbData { bool output_version; + bool iface_created; };
static void testIscsiadmCb(const char *const*args, @@ -103,6 +114,62 @@ static void testIscsiadmCb(const char *const*args, args[7] && STREQ(args[7], "--login") && args[8] == NULL) { /* nada */
Is this "nada"
Yes, this is "nada" ;-)
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] == NULL) { + if (data->iface_created)
How would iface_created be set by this point if...
I suggest opening virISCSIConnection() in another window and looking what it does (assuming that @initiatoriqn is set). So the first function that is called is virStorageBackendIQNFound() which execs `ISCSIADM --mode iface` to list all iscsi interfaces. If no libvirt interface is found IQN_MISSING is returned and virStorageBackendCreateIfaceIQN() is called. This function creates the interface and then calls virStorageBackendIQNFound() again to confirm the interface is created. Long story short, `ISCSIADM --mode iface` is called twice - before and after iface creation. Therefore we have to return different outputs to mimic the iface creation.
+ ignore_value(VIR_STRDUP(*output, iscsiadmIfaceIfaceOutput)); + else + ignore_value(VIR_STRDUP(*output, iscsiadmIfaceDefaultOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] && STREQ(args[3], "--interface") && + args[4] && STREQ(args[4], "libvirt-iface-03020100") && + args[5] && STREQ(args[5], "--op") && + args[6] && STREQ(args[6], "new") && + args[7] == NULL) { + data->iface_created = true;
... it's being set unconditionally here?
See note below, shouldn't the caller be doing this?
What caller?
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] && STREQ(args[3], "--interface") && + args[4] && STREQ(args[4], "libvirt-iface-03020100") && + args[5] && STREQ(args[5], "--op") && + args[6] && STREQ(args[6], "update") && + args[7] && STREQ(args[7], "--name") && + args[8] && STREQ(args[8], "iface.initiatorname") && + args[9] && STREQ(args[9], "--value") && + args[10] && STREQ(args[10], "iqn.2004-06.example:example1:initiator") && + args[11] == NULL && + data->iface_created) { + /* nada */
?? Why nada (now you know where my 7/10 requery came from)
Maybe you are confused what this callback is doing? The whole point of this callback is to avoid calling real iscsiadm binary AND having to write our own binary. We can write a function that is called instead of spawning real binary. But the function has to act like real binary - otherwise our code declares an erroneous binary. For instance: calling `iscsiadm --mode iface --op new --interface myInterface` creates new iscsi interface. Therefore, if I call `iscsiadm --mode iface` afterwards I have to see it in the list of the interfaces. And this is what the callback is doing. It doesn't have to take action for every combination of cmd line arguments in order to successfully mimic real iscsiadm binary behaviour. This fact is then noted as 'nada'.
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "discovery") && + args[3] && STREQ(args[3], "--type") && + 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] && STREQ(args[7], "--interface") && + args[8] && STREQ(args[8], "libvirt-iface-03020100") && + args[9] == NULL && + data->iface_created) { + ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "node") && + args[3] && STREQ(args[3], "--portal") && + args[4] && STREQ(args[4], "10.20.30.40:3260,1") && + args[5] && STREQ(args[5], "--targetname") && + args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") && + args[7] && STREQ(args[7], "--login") && + args[8] && STREQ(args[8], "--interface") && + args[9] && STREQ(args[9], "libvirt-iface-03020100") && + args[10] == NULL && + data->iface_created) { + /* nada */
Similar, why nada?
} else { *status = -1; } @@ -204,9 +271,10 @@ static int testISCSIConnectionLogin(const void *data) { const struct testConnectionInfoLogin *info = data; + struct testIscsiadmCbData cbData = { 0 };
s/0/false, false/
int ret = -1;
Why wouldn't initialization be:
cbData.iface_create = info->initiatoriqn != NULL;
Because I want to test more than a single successful path in the code. If the interface doesn't exist initially, it is created and only then login is attempted. If it existed only login would be tested. Michal
On 07/23/2018 04:01 AM, Michal Prívozník wrote:
On 07/17/2018 09:15 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Extend this existing test so that a case when IQN is provided is tested too. Since a special iSCSI interface is created and its name is randomly generated at runtime we need to link with virrandommock to have predictable names.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 2 deletions(-)
Should testIscsiadmCbData initialization get { false, false } now (and I should have mention in patch 9 that :
+ struct testIscsiadmCbData cbData = { 0 };
should be
+ struct testIscsiadmCbData cbData = { false };> I know that 0 and false are synonymous, but syntactical correctness is in play here ;-)
No. So { 0 } is basically a shortcut for:
struct testIscsiadmCbData cbData;
memset(&cbData, 0, sizeof(cbData));
Oh yeah, right <facepalm> for literal interpretation of data.
It has nothing to do with the struct having only two bools (for now). It is used widely in our code, because it has one great advantage: even if we add/remove/rearrange items in the struct it is always going to be zeroed out whereas if I initialized it like this:
struct testIscsiadmCbData cbData = {false, false};
and then somebody would append yet another member to the struct they would either:
a) have to change all the lines where the struct is initialized (possibly touching functions that has nothing to do with actual change), or b) forget to initialize it completely.
TL;DR It's just coincidence that the first member is a bool.
I also think you're doing two things here:
1. Generating a dryRun output for "existing" test without initiator.iqn
2. Adding tests to test initiator.iqn
Sure. That's how login works. Firstly it lists output of iscsi interfaces, then it creates a new one and then use that to log in. I don't see a problem here.
OK, right... Maybe it would have been useful from the literal review POV to be reminded of this... While sometimes it's a pain to document the reason for something - it perhaps prevents this back and forth later on especially when it's "unique case" code paths. IIQN isn't really well documented by any stretch...
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index c6e0e3b8ff..55889d04a3 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -60,8 +60,19 @@ const char *iscsiadmSendtargetsOutput = "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
+const char *iscsiadmIfaceDefaultOutput = + "default tcp,<empty>,<empty>,<empty>,<empty>\n" + "iser iser,<empty>,<empty>,<empty>,<empty>\n"; + +const char *iscsiadmIfaceIfaceOutput = + "default tcp,<empty>,<empty>,<empty>,<empty>\n" + "iser iser,<empty>,<empty>,<empty>,<empty>\n" + "libvirt-iface-03020100 tcp,<empty>,<empty>,<empty>,iqn.2004-06.example:example1:initiator\n"; + + struct testIscsiadmCbData { bool output_version; + bool iface_created; };
static void testIscsiadmCb(const char *const*args, @@ -103,6 +114,62 @@ static void testIscsiadmCb(const char *const*args, args[7] && STREQ(args[7], "--login") && args[8] == NULL) { /* nada */
Is this "nada"
Yes, this is "nada" ;-)
Maybe the explanation as to why it's nada and not just leaving nada there would have helped. In some instances we're doing output comparison (eventually) and in others we're not. In the long run it's mainly a matter of being reminded what the processing is and what (if any) the expected output is. Sometimes that expected output only occurs on the next query, IIRC. Creating IIQN's or iSCSI targets is not something I do all that often - I wonder if you came back to this code 6 months or longer from now whether you'd (instantly) recall what type of output was generated ;-). If you would then your short term memory is better than mine! I have to keep some cheat notes for iSCSI processing letting me know which commands to use and essentially what they return so I know they completed as expected. BTW: This is my favorite from f28 recently: # iscsiadm -m session -P 3 iSCSI Transport Class version 2.0-870 version 6.2.0.874 Segmentation fault (core dumped) # Oy! at least it's getting further now than at one time where the first line wasn't even printed. Don't even get me started about the 'targetcli' command which I find to be absolutely awful. It's supposed to be better, but I feel like I stepped back into the 1990's and I'm using OpenVMS NCP/NCL (that'd require some googling for you I'm sure, but suffice to say it was an awful^^n syntax).
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] == NULL) { + if (data->iface_created)
How would iface_created be set by this point if...
I suggest opening virISCSIConnection() in another window and looking what it does (assuming that @initiatoriqn is set). So the first function that is called is virStorageBackendIQNFound() which execs `ISCSIADM --mode iface` to list all iscsi interfaces. If no libvirt interface is found IQN_MISSING is returned and virStorageBackendCreateIfaceIQN() is called. This function creates the interface and then calls virStorageBackendIQNFound() again to confirm the interface is created.
Long story short, `ISCSIADM --mode iface` is called twice - before and after iface creation. Therefore we have to return different outputs to mimic the iface creation.
Seems like something worthy to note in the code, but maybe that's just me...
+ ignore_value(VIR_STRDUP(*output, iscsiadmIfaceIfaceOutput)); + else + ignore_value(VIR_STRDUP(*output, iscsiadmIfaceDefaultOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] && STREQ(args[3], "--interface") && + args[4] && STREQ(args[4], "libvirt-iface-03020100") && + args[5] && STREQ(args[5], "--op") && + args[6] && STREQ(args[6], "new") && + args[7] == NULL) { + data->iface_created = true;
... it's being set unconditionally here?
See note below, shouldn't the caller be doing this?
What caller?
testISCSIConnectionLogin
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] && STREQ(args[3], "--interface") && + args[4] && STREQ(args[4], "libvirt-iface-03020100") && + args[5] && STREQ(args[5], "--op") && + args[6] && STREQ(args[6], "update") && + args[7] && STREQ(args[7], "--name") && + args[8] && STREQ(args[8], "iface.initiatorname") && + args[9] && STREQ(args[9], "--value") && + args[10] && STREQ(args[10], "iqn.2004-06.example:example1:initiator") && + args[11] == NULL && + data->iface_created) { + /* nada */
?? Why nada (now you know where my 7/10 requery came from)
Maybe you are confused what this callback is doing? The whole point of this callback is to avoid calling real iscsiadm binary AND having to write our own binary. We can write a function that is called instead of spawning real binary. But the function has to act like real binary - otherwise our code declares an erroneous binary. For instance:
calling `iscsiadm --mode iface --op new --interface myInterface` creates new iscsi interface. Therefore, if I call `iscsiadm --mode iface` afterwards I have to see it in the list of the interfaces. And this is what the callback is doing. It doesn't have to take action for every combination of cmd line arguments in order to successfully mimic real iscsiadm binary behaviour. This fact is then noted as 'nada'.
Maybe it is just confusing to have some tests generate sample output while others don't for "nada" (a/k/a no apparent) reason. Ones with far more complex command options, too! Of course that's is the difference between fetching status and performing an action perhaps. IMO: nada provides no details as to the reason why there's no output to compare against (eventually).
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "discovery") && + args[3] && STREQ(args[3], "--type") && + 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] && STREQ(args[7], "--interface") && + args[8] && STREQ(args[8], "libvirt-iface-03020100") && + args[9] == NULL && + data->iface_created) { + ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "node") && + args[3] && STREQ(args[3], "--portal") && + args[4] && STREQ(args[4], "10.20.30.40:3260,1") && + args[5] && STREQ(args[5], "--targetname") && + args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") && + args[7] && STREQ(args[7], "--login") && + args[8] && STREQ(args[8], "--interface") && + args[9] && STREQ(args[9], "libvirt-iface-03020100") && + args[10] == NULL && + data->iface_created) { + /* nada */
Similar, why nada?
} else { *status = -1; } @@ -204,9 +271,10 @@ static int testISCSIConnectionLogin(const void *data) { const struct testConnectionInfoLogin *info = data; + struct testIscsiadmCbData cbData = { 0 };
s/0/false, false/
int ret = -1;
Why wouldn't initialization be:
cbData.iface_create = info->initiatoriqn != NULL;
Because I want to test more than a single successful path in the code. If the interface doesn't exist initially, it is created and only then login is attempted. If it existed only login would be tested.
It seems to me that 'logic' is hidden behind setting iface_create in the callback routine. Why not two tests? One that tests ensuring the interface is created and one that uses a created interface - if that's even possible. So after all that, beyond the weirdess I find with iface_create, perhaps a few extra words to document what's happening would help. I'm fine with a here's a diff to the patch type patch. I don't think a v3 is necessary. John
On 07/23/2018 03:10 PM, John Ferlan wrote:
On 07/23/2018 04:01 AM, Michal Prívozník wrote:
On 07/17/2018 09:15 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Extend this existing test so that a case when IQN is provided is tested too. Since a special iSCSI interface is created and its name is randomly generated at runtime we need to link with virrandommock to have predictable names.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 2 deletions(-)
Should testIscsiadmCbData initialization get { false, false } now (and I should have mention in patch 9 that :
+ struct testIscsiadmCbData cbData = { 0 };
should be
+ struct testIscsiadmCbData cbData = { false };> I know that 0 and false are synonymous, but syntactical correctness is in play here ;-)
No. So { 0 } is basically a shortcut for:
struct testIscsiadmCbData cbData;
memset(&cbData, 0, sizeof(cbData));
Oh yeah, right <facepalm> for literal interpretation of data.
It has nothing to do with the struct having only two bools (for now). It is used widely in our code, because it has one great advantage: even if we add/remove/rearrange items in the struct it is always going to be zeroed out whereas if I initialized it like this:
struct testIscsiadmCbData cbData = {false, false};
and then somebody would append yet another member to the struct they would either:
a) have to change all the lines where the struct is initialized (possibly touching functions that has nothing to do with actual change), or b) forget to initialize it completely.
TL;DR It's just coincidence that the first member is a bool.
I also think you're doing two things here:
1. Generating a dryRun output for "existing" test without initiator.iqn
2. Adding tests to test initiator.iqn
Sure. That's how login works. Firstly it lists output of iscsi interfaces, then it creates a new one and then use that to log in. I don't see a problem here.
OK, right... Maybe it would have been useful from the literal review POV to be reminded of this... While sometimes it's a pain to document the reason for something - it perhaps prevents this back and forth later on especially when it's "unique case" code paths. IIQN isn't really well documented by any stretch...
Well, I usually read the code when trying to understand how something works as our comments/documentation is often incorrect.
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index c6e0e3b8ff..55889d04a3 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -60,8 +60,19 @@ const char *iscsiadmSendtargetsOutput = "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
+const char *iscsiadmIfaceDefaultOutput = + "default tcp,<empty>,<empty>,<empty>,<empty>\n" + "iser iser,<empty>,<empty>,<empty>,<empty>\n"; + +const char *iscsiadmIfaceIfaceOutput = + "default tcp,<empty>,<empty>,<empty>,<empty>\n" + "iser iser,<empty>,<empty>,<empty>,<empty>\n" + "libvirt-iface-03020100 tcp,<empty>,<empty>,<empty>,iqn.2004-06.example:example1:initiator\n"; + + struct testIscsiadmCbData { bool output_version; + bool iface_created; };
static void testIscsiadmCb(const char *const*args, @@ -103,6 +114,62 @@ static void testIscsiadmCb(const char *const*args, args[7] && STREQ(args[7], "--login") && args[8] == NULL) { /* nada */
Is this "nada"
Yes, this is "nada" ;-)
Maybe the explanation as to why it's nada and not just leaving nada there would have helped.
As I say in the other reply, how about s/nada/no output/?
In some instances we're doing output comparison (eventually) and in others we're not. In the long run it's mainly a matter of being reminded what the processing is and what (if any) the expected output is. Sometimes that expected output only occurs on the next query, IIRC.
Yes, this is right.
Creating IIQN's or iSCSI targets is not something I do all that often - I wonder if you came back to this code 6 months or longer from now whether you'd (instantly) recall what type of output was generated ;-). If you would then your short term memory is better than mine! I have to keep some cheat notes for iSCSI processing letting me know which commands to use and essentially what they return so I know they completed as expected.
Of course I would not. But seeing this pattern would help definitely: if (some args) { VIR_STRDUP(output, expectedOutput1); } else if (some other args) { /* nada */ } else { exitstatus = -1; } It suggest to me that for "some args" an output is generated while for "some other args" it isn't.
BTW: This is my favorite from f28 recently:
# iscsiadm -m session -P 3 iSCSI Transport Class version 2.0-870 version 6.2.0.874 Segmentation fault (core dumped) #
Oy! at least it's getting further now than at one time where the first line wasn't even printed.
Don't even get me started about the 'targetcli' command which I find to be absolutely awful. It's supposed to be better, but I feel like I stepped back into the 1990's and I'm using OpenVMS NCP/NCL (that'd require some googling for you I'm sure, but suffice to say it was an awful^^n syntax).
Yes, iscsi CLI isn't one of the best CLIs I've seen either.
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] == NULL) { + if (data->iface_created)
How would iface_created be set by this point if...
I suggest opening virISCSIConnection() in another window and looking what it does (assuming that @initiatoriqn is set). So the first function that is called is virStorageBackendIQNFound() which execs `ISCSIADM --mode iface` to list all iscsi interfaces. If no libvirt interface is found IQN_MISSING is returned and virStorageBackendCreateIfaceIQN() is called. This function creates the interface and then calls virStorageBackendIQNFound() again to confirm the interface is created.
Long story short, `ISCSIADM --mode iface` is called twice - before and after iface creation. Therefore we have to return different outputs to mimic the iface creation.
Seems like something worthy to note in the code, but maybe that's just me...
Well, all that is needed to learn this is to open virISCSIConnection() and it's immediately visible there. Documenting it is no good IMO. IOW, it would be like this: /* add A to B and store the result in C */ C = A + B; /* Then call func() over it */ func(C); As soon as you see the code it's visible what is being called. Putting a comment somewhere is worth it when the logic is unclear. The big comment (with a picture) in virNetDevBandwidthSet() is great example.
+ ignore_value(VIR_STRDUP(*output, iscsiadmIfaceIfaceOutput)); + else + ignore_value(VIR_STRDUP(*output, iscsiadmIfaceDefaultOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] && STREQ(args[3], "--interface") && + args[4] && STREQ(args[4], "libvirt-iface-03020100") && + args[5] && STREQ(args[5], "--op") && + args[6] && STREQ(args[6], "new") && + args[7] == NULL) { + data->iface_created = true;
... it's being set unconditionally here?
See note below, shouldn't the caller be doing this?
What caller?
testISCSIConnectionLogin
No. I view @data as place for testIscsiadmCb to store its state between several runs. Also, if it was testISCSIConnectionLogin who sets it, what is the place to do so? It can't be before calling virISCSIConnectionLogin() because we want the code to go the extra mile and test iface creation too. But it can't be after virISCSIConnectionLogin() neither - at that point the iface must be already created.
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] && STREQ(args[3], "--interface") && + args[4] && STREQ(args[4], "libvirt-iface-03020100") && + args[5] && STREQ(args[5], "--op") && + args[6] && STREQ(args[6], "update") && + args[7] && STREQ(args[7], "--name") && + args[8] && STREQ(args[8], "iface.initiatorname") && + args[9] && STREQ(args[9], "--value") && + args[10] && STREQ(args[10], "iqn.2004-06.example:example1:initiator") && + args[11] == NULL && + data->iface_created) { + /* nada */
?? Why nada (now you know where my 7/10 requery came from)
Maybe you are confused what this callback is doing? The whole point of this callback is to avoid calling real iscsiadm binary AND having to write our own binary. We can write a function that is called instead of spawning real binary. But the function has to act like real binary - otherwise our code declares an erroneous binary. For instance:
calling `iscsiadm --mode iface --op new --interface myInterface` creates new iscsi interface. Therefore, if I call `iscsiadm --mode iface` afterwards I have to see it in the list of the interfaces. And this is what the callback is doing. It doesn't have to take action for every combination of cmd line arguments in order to successfully mimic real iscsiadm binary behaviour. This fact is then noted as 'nada'.
Maybe it is just confusing to have some tests generate sample output while others don't for "nada" (a/k/a no apparent) reason. Ones with far more complex command options, too! Of course that's is the difference between fetching status and performing an action perhaps.
IMO: nada provides no details as to the reason why there's no output to compare against (eventually).
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "discovery") && + args[3] && STREQ(args[3], "--type") && + 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] && STREQ(args[7], "--interface") && + args[8] && STREQ(args[8], "libvirt-iface-03020100") && + args[9] == NULL && + data->iface_created) { + ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "node") && + args[3] && STREQ(args[3], "--portal") && + args[4] && STREQ(args[4], "10.20.30.40:3260,1") && + args[5] && STREQ(args[5], "--targetname") && + args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") && + args[7] && STREQ(args[7], "--login") && + args[8] && STREQ(args[8], "--interface") && + args[9] && STREQ(args[9], "libvirt-iface-03020100") && + args[10] == NULL && + data->iface_created) { + /* nada */
Similar, why nada?
} else { *status = -1; } @@ -204,9 +271,10 @@ static int testISCSIConnectionLogin(const void *data) { const struct testConnectionInfoLogin *info = data; + struct testIscsiadmCbData cbData = { 0 };
s/0/false, false/
int ret = -1;
Why wouldn't initialization be:
cbData.iface_create = info->initiatoriqn != NULL;
Because I want to test more than a single successful path in the code. If the interface doesn't exist initially, it is created and only then login is attempted. If it existed only login would be tested.
It seems to me that 'logic' is hidden behind setting iface_create in the callback routine. Why not two tests? One that tests ensuring the interface is created and one that uses a created interface - if that's even possible.
Because it's one function. Okay, step aside for a moment and consider the following function: int f(bool extra) { if (extra) take_extra_action(); take_some_action(); } Sure, you can write two tests, one to call f(false) the other to call f(true). BUT the latter already tests the former so I don't see much value added (in fact I see no added value) in having two tests. I do see value in having f(true) test though.
So after all that, beyond the weirdess I find with iface_create, perhaps a few extra words to document what's happening would help. I'm fine with a here's a diff to the patch type patch. I don't think a v3 is necessary.
The only diff I can think of is 's/nada/no output/'. I don't see any other problem with the patches :-P Michal
On 07/24/2018 05:48 AM, Michal Privoznik wrote:
On 07/23/2018 03:10 PM, John Ferlan wrote:
On 07/23/2018 04:01 AM, Michal Prívozník wrote:
On 07/17/2018 09:15 PM, John Ferlan wrote:
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
Extend this existing test so that a case when IQN is provided is tested too. Since a special iSCSI interface is created and its name is randomly generated at runtime we need to link with virrandommock to have predictable names.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/viriscsitest.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 2 deletions(-)
Should testIscsiadmCbData initialization get { false, false } now (and I should have mention in patch 9 that :
+ struct testIscsiadmCbData cbData = { 0 };
should be
+ struct testIscsiadmCbData cbData = { false };> I know that 0 and false are synonymous, but syntactical correctness is in play here ;-)
No. So { 0 } is basically a shortcut for:
struct testIscsiadmCbData cbData;
memset(&cbData, 0, sizeof(cbData));
Oh yeah, right <facepalm> for literal interpretation of data.
It has nothing to do with the struct having only two bools (for now). It is used widely in our code, because it has one great advantage: even if we add/remove/rearrange items in the struct it is always going to be zeroed out whereas if I initialized it like this:
struct testIscsiadmCbData cbData = {false, false};
and then somebody would append yet another member to the struct they would either:
a) have to change all the lines where the struct is initialized (possibly touching functions that has nothing to do with actual change), or b) forget to initialize it completely.
TL;DR It's just coincidence that the first member is a bool.
I also think you're doing two things here:
1. Generating a dryRun output for "existing" test without initiator.iqn
2. Adding tests to test initiator.iqn
Sure. That's how login works. Firstly it lists output of iscsi interfaces, then it creates a new one and then use that to log in. I don't see a problem here.
OK, right... Maybe it would have been useful from the literal review POV to be reminded of this... While sometimes it's a pain to document the reason for something - it perhaps prevents this back and forth later on especially when it's "unique case" code paths. IIQN isn't really well documented by any stretch...
Well, I usually read the code when trying to understand how something works as our comments/documentation is often incorrect.
If the comments/documentation is often incorrect, then that should be rectified. Help the next poor soul trying to read what someone thought was self documenting code. To me that almost reaches the level of a trivial patch.
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index c6e0e3b8ff..55889d04a3 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -60,8 +60,19 @@ const char *iscsiadmSendtargetsOutput = "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
+const char *iscsiadmIfaceDefaultOutput = + "default tcp,<empty>,<empty>,<empty>,<empty>\n" + "iser iser,<empty>,<empty>,<empty>,<empty>\n"; + +const char *iscsiadmIfaceIfaceOutput = + "default tcp,<empty>,<empty>,<empty>,<empty>\n" + "iser iser,<empty>,<empty>,<empty>,<empty>\n" + "libvirt-iface-03020100 tcp,<empty>,<empty>,<empty>,iqn.2004-06.example:example1:initiator\n"; + + struct testIscsiadmCbData { bool output_version; + bool iface_created; };
static void testIscsiadmCb(const char *const*args, @@ -103,6 +114,62 @@ static void testIscsiadmCb(const char *const*args, args[7] && STREQ(args[7], "--login") && args[8] == NULL) { /* nada */
Is this "nada"
Yes, this is "nada" ;-)
Maybe the explanation as to why it's nada and not just leaving nada there would have helped.
As I say in the other reply, how about s/nada/no output/?
In some instances we're doing output comparison (eventually) and in others we're not. In the long run it's mainly a matter of being reminded what the processing is and what (if any) the expected output is. Sometimes that expected output only occurs on the next query, IIRC.
Yes, this is right.
Creating IIQN's or iSCSI targets is not something I do all that often - I wonder if you came back to this code 6 months or longer from now whether you'd (instantly) recall what type of output was generated ;-). If you would then your short term memory is better than mine! I have to keep some cheat notes for iSCSI processing letting me know which commands to use and essentially what they return so I know they completed as expected.
Of course I would not. But seeing this pattern would help definitely:
if (some args) { VIR_STRDUP(output, expectedOutput1); } else if (some other args) { /* nada */ } else { exitstatus = -1; }
It suggest to me that for "some args" an output is generated while for "some other args" it isn't.
BTW: This is my favorite from f28 recently:
# iscsiadm -m session -P 3 iSCSI Transport Class version 2.0-870 version 6.2.0.874 Segmentation fault (core dumped) #
Oy! at least it's getting further now than at one time where the first line wasn't even printed.
Don't even get me started about the 'targetcli' command which I find to be absolutely awful. It's supposed to be better, but I feel like I stepped back into the 1990's and I'm using OpenVMS NCP/NCL (that'd require some googling for you I'm sure, but suffice to say it was an awful^^n syntax).
Yes, iscsi CLI isn't one of the best CLIs I've seen either.
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] == NULL) { + if (data->iface_created)
How would iface_created be set by this point if...
I suggest opening virISCSIConnection() in another window and looking what it does (assuming that @initiatoriqn is set). So the first function that is called is virStorageBackendIQNFound() which execs `ISCSIADM --mode iface` to list all iscsi interfaces. If no libvirt interface is found IQN_MISSING is returned and virStorageBackendCreateIfaceIQN() is called. This function creates the interface and then calls virStorageBackendIQNFound() again to confirm the interface is created.
Long story short, `ISCSIADM --mode iface` is called twice - before and after iface creation. Therefore we have to return different outputs to mimic the iface creation.
Seems like something worthy to note in the code, but maybe that's just me...
Well, all that is needed to learn this is to open virISCSIConnection() and it's immediately visible there. Documenting it is no good IMO. IOW, it would be like this:
/* add A to B and store the result in C */ C = A + B; /* Then call func() over it */ func(C);
As soon as you see the code it's visible what is being called.
Putting a comment somewhere is worth it when the logic is unclear. The big comment (with a picture) in virNetDevBandwidthSet() is great example.
Suffice to say I don't think A+B=C is the same as noting that creating an Initiator IQN is a multi-command process especially when the non IIQN is a single step process.
+ ignore_value(VIR_STRDUP(*output, iscsiadmIfaceIfaceOutput)); + else + ignore_value(VIR_STRDUP(*output, iscsiadmIfaceDefaultOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] && STREQ(args[3], "--interface") && + args[4] && STREQ(args[4], "libvirt-iface-03020100") && + args[5] && STREQ(args[5], "--op") && + args[6] && STREQ(args[6], "new") && + args[7] == NULL) { + data->iface_created = true;
... it's being set unconditionally here?
See note below, shouldn't the caller be doing this?
What caller?
testISCSIConnectionLogin
No. I view @data as place for testIscsiadmCb to store its state between several runs. Also, if it was testISCSIConnectionLogin who sets it, what is the place to do so? It can't be before calling virISCSIConnectionLogin() because we want the code to go the extra mile and test iface creation too. But it can't be after virISCSIConnectionLogin() neither - at that point the iface must be already created.
OK - this I guess makes sense since it's a multi-command process. I just find it "odd" that callback routine is setting the boolean. I'd comment that fact, but you'd say read the called API to find out what happens.
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "iface") && + args[3] && STREQ(args[3], "--interface") && + args[4] && STREQ(args[4], "libvirt-iface-03020100") && + args[5] && STREQ(args[5], "--op") && + args[6] && STREQ(args[6], "update") && + args[7] && STREQ(args[7], "--name") && + args[8] && STREQ(args[8], "iface.initiatorname") && + args[9] && STREQ(args[9], "--value") && + args[10] && STREQ(args[10], "iqn.2004-06.example:example1:initiator") && + args[11] == NULL && + data->iface_created) { + /* nada */
?? Why nada (now you know where my 7/10 requery came from)
Maybe you are confused what this callback is doing? The whole point of this callback is to avoid calling real iscsiadm binary AND having to write our own binary. We can write a function that is called instead of spawning real binary. But the function has to act like real binary - otherwise our code declares an erroneous binary. For instance:
calling `iscsiadm --mode iface --op new --interface myInterface` creates new iscsi interface. Therefore, if I call `iscsiadm --mode iface` afterwards I have to see it in the list of the interfaces. And this is what the callback is doing. It doesn't have to take action for every combination of cmd line arguments in order to successfully mimic real iscsiadm binary behaviour. This fact is then noted as 'nada'.
Maybe it is just confusing to have some tests generate sample output while others don't for "nada" (a/k/a no apparent) reason. Ones with far more complex command options, too! Of course that's is the difference between fetching status and performing an action perhaps.
IMO: nada provides no details as to the reason why there's no output to compare against (eventually).
+ } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "discovery") && + args[3] && STREQ(args[3], "--type") && + 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] && STREQ(args[7], "--interface") && + args[8] && STREQ(args[8], "libvirt-iface-03020100") && + args[9] == NULL && + data->iface_created) { + ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "node") && + args[3] && STREQ(args[3], "--portal") && + args[4] && STREQ(args[4], "10.20.30.40:3260,1") && + args[5] && STREQ(args[5], "--targetname") && + args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") && + args[7] && STREQ(args[7], "--login") && + args[8] && STREQ(args[8], "--interface") && + args[9] && STREQ(args[9], "libvirt-iface-03020100") && + args[10] == NULL && + data->iface_created) { + /* nada */
Similar, why nada?
} else { *status = -1; } @@ -204,9 +271,10 @@ static int testISCSIConnectionLogin(const void *data) { const struct testConnectionInfoLogin *info = data; + struct testIscsiadmCbData cbData = { 0 };
s/0/false, false/
int ret = -1;
Why wouldn't initialization be:
cbData.iface_create = info->initiatoriqn != NULL;
Because I want to test more than a single successful path in the code. If the interface doesn't exist initially, it is created and only then login is attempted. If it existed only login would be tested.
It seems to me that 'logic' is hidden behind setting iface_create in the callback routine. Why not two tests? One that tests ensuring the interface is created and one that uses a created interface - if that's even possible.
Because it's one function. Okay, step aside for a moment and consider the following function:
int f(bool extra) { if (extra) take_extra_action();
take_some_action(); }
Sure, you can write two tests, one to call f(false) the other to call f(true). BUT the latter already tests the former so I don't see much value added (in fact I see no added value) in having two tests. I do see value in having f(true) test though.
So after all that, beyond the weirdess I find with iface_create, perhaps a few extra words to document what's happening would help. I'm fine with a here's a diff to the patch type patch. I don't think a v3 is necessary.
The only diff I can think of is 's/nada/no output/'. I don't see any other problem with the patches :-P
Michal
See patch7 for my feelings on "nada" and "no output" Again, for the code/logic: Reviewed-by: John Ferlan <jferlan@redhat.com> do whatever you want for comments John
On 07/04/2018 05:23 AM, Michal Privoznik wrote:
v2 of:
https://www.redhat.com/archives/libvir-list/2018-June/msg01861.html
diff to v1: - fixed misleading while() loop in 03/10 - Introduced some tests - patch 06/10 is new, while writing the tests I've noticed that errors from testIscsiadmCb were not propagated properly.
Patches 01,02,04 and 05 are ACKed already. Even though John suggested merging 04 and 05 together I haven't done that because I believe "iscsi --interface" and "iscsi --op nonpersistent" are orthogonal and even though we currently treat them as mutually exclusive (in storage backend) the util/viriscsi.c API should not suffer because of that.
Michal Privoznik (10): virStorageBackendIQNFound: Fix ret value assignment virStorageBackendIQNFound: Rename out label virStorageBackendIQNFound: Rework iscsiadm output parsing virISCSIScanTargets: Honour iSCSI interface virISCSIScanTargets: Allow making targets persistent virCommandWait: Propagate dryRunCallback return value properly viriscsitest: Test virISCSIConnectionLogin viriscsitest: Move testSessionInfo struct viriscsitest: Introduce testIscsiadmCbData struct viriscsitest: Extend virISCSIConnectionLogin test
src/storage/storage_backend_iscsi.c | 5 +- src/util/vircommand.c | 2 + src/util/viriscsi.c | 196 ++++++++++++++++++++++++------------ src/util/viriscsi.h | 2 + tests/viriscsitest.c | 145 ++++++++++++++++++++++++-- 5 files changed, 272 insertions(+), 78 deletions(-)
I think a release note to indicate fixing some iSCSI Initiator IQN issues would be appropriate... John
participants (3)
-
John Ferlan -
Michal Privoznik -
Michal Prívozník