This is in fact 'cleanup' label and it should be named as such. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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

On 07/04/2018 05:23 AM, Michal Privoznik wrote: 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 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(a)redhat.com&gt; (regardless of the decision to add the libivrt-iface- filter John

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(a)redhat.com&gt; Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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/23/2018 02:07 PM, John Ferlan wrote: Yes. Yes. No. 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 03:49 PM, John Ferlan wrote: So virCommandWait() failed. As expected. It did not fail here, because /bin/true returns 0 (in contrast to /bin/false which returns -1). 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. This is not the point. 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. 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

On 07/04/2018 05:23 AM, Michal Privoznik wrote: 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

On 07/23/2018 04:01 AM, Michal Prívozník wrote: here we do some sort of comparison 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 08:34 AM, Michal Prívozník wrote: 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

On 07/24/2018 04:25 AM, Michal Privoznik wrote: 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. 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(a)redhat.com&gt; 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

This struct has nothing to do with testIscsiadmCb() rather than testISCSIGetSession(). Move it closer to the latter. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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/17/2018 09:15 PM, John Ferlan wrote: 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. 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. Yes, this is "nada" ;-) 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. What caller? 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'. 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 03:10 PM, John Ferlan wrote: Well, I usually read the code when trying to understand how something works as our comments/documentation is often incorrect. As I say in the other reply, how about s/nada/no output/? Yes, this is right. 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. Yes, iscsi CLI isn't one of the best CLIs I've seen either. 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. 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. 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. The only diff I can think of is 's/nada/no output/'. I don't see any other problem with the patches :-P Michal