On 06/29/2018 11:01 AM, Michal Privoznik wrote:

The return value of this function is overwritten more times than I am comfortable with.

True, but let's actually say what you're doing... 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> --- src/util/viriscsi.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)

Touching Dave's 8+ year old code (commit id 6aabcb5), lucky you. I believe this "feature" was never documented too - now that your name is on top you can own it :-P!

diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index d4c745a1af..a308ee4bac 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,27 +166,27 @@ 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:

While you're at it s/out/cleanup

- if (ret == IQN_MISSING) + if (ret != IQN_FOUND)

IDC since it's VIR_DEBUG, but you'll get this message on IQN_ERROR too as well as the virReportError... I would think that's fairly obvious on the error paths With some minor cleanups... Reviewed-by: John Ferlan <jferlan@redhat.com> John

VIR_DEBUG("Could not find interface with IQN '%s'", iqn);

VIR_FREE(line);

On Fri, Jun 29, 2018 at 17:01:49 +0200, 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 2e55b3c10b..44788056fd 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c

[...]

@@ -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) {

This is severely misleading and makes this loop technically infinite. Or just checks that output buffer was not null. Both operations should be made explicit. Or you meant *line or line[0]

+ 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;

This is ther reason why it's working at this point.

- while (fgets(line, LINE_SIZE, fp) != NULL) { - newline = strrchr(line, '\n');

On 06/29/2018 11:01 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(-)

I assume virCommandSetOutputBuffer didn't exist when this code was created. Also, why do I have this recollection related to portability of sscanf? I know we use it elsewhere, but some oddball issue that someone like Eric may recollect or know about.

diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 2e55b3c10b..44788056fd 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 + */

Nice to have an example especially since this is a bit of a edge case... Another option - virStringSplitCount on the "\n" to get the number of lines and then on each line again using "," to tear apart the pieces. This I assume works as I don't have a initiatoriqn set up to test. Any thoughts/recollections about sscanf? I assume it'll be felt that virStringSplit might be overkill, but at least it's for other purposes and perhaps less susceptible to the line && *line adjustment. IDC - let's see if sscanf causes any other recollections before R-by'ing. John

- virCommandSetOutputFD(cmd, &fd); - if (virCommandRunAsync(cmd, NULL) < 0) - goto cleanup; + line = outbuf; + while (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_FOUND) 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/03/2018 01:08 AM, Michal Prívozník wrote:

On 07/03/2018 01:18 AM, John Ferlan wrote:

...

On 06/29/2018 11:01 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(-)

I assume virCommandSetOutputBuffer didn't exist when this code was created.

Perhaps. Let me check git log. .. .. And yes, you are right. virCommandSetOutputBuffer was introduced among with other basic virCommand* APIs by Dan in late 2010 (f16ad06fb2aeb5e5c9974). But this function was introduced by Dave in Jan 2010 so he couldn't have used it.

...

Also, why do I have this recollection related to portability of sscanf? I know we use it elsewhere, but some oddball issue that someone like Eric may recollect or know about.

I don't know about anything. But since we use it in our code pretty freely I assumed there is no problem with it.

...

...

diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 2e55b3c10b..44788056fd 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 + */

Nice to have an example especially since this is a bit of a edge case...

Another option - virStringSplitCount on the "\n" to get the number of lines and then on each line again using "," to tear apart the pieces. This I assume works as I don't have a initiatoriqn set up to test.

Any thoughts/recollections about sscanf? I assume it'll be felt that virStringSplit might be overkill,

Indeed.

...

but at least it's for other purposes and perhaps less susceptible to the line && *line adjustment.

I like sscanf() more because it's fewer lines of code, the variables I need are assigned value immediately and also memory footprint is smaller (I guess) since there's no need to store multiple arrays.

Understood - it's probably something really early on when I first started w/ libvirt and using sscanf was discouraged for something that has stuck in my head. Another part for me is readability - similar to the various [i]scsi code that uses regex's in order to parse output. That format is just a mish-mash of search patterns that causes my eyes to complain. Perhaps it's best to see this along with the combined 4&5 "again". Also add a non initiatoriqn to the mix (even the comment above) just so show the various formats. Hopefully using libiscsi makes all the personally displeasing regex and sscanf searching code disappear. John

On 07/03/2018 01:38 AM, John Ferlan wrote:

On 06/29/2018 11:01 AM, Michal Privoznik wrote:

...

When scanning for targets, iSCSI might give different results depending on the interface used. This is basically just name of

assuming this means whether the initiatoriqn interface was used

Yes.

...

config file under /etc/iscsi/ifaces to use. The file contains initiator IQN thus different results claim.

Strange to have activity here - was there a bz? Or something found by the (I assume) GSoC project:

https://www.redhat.com/archives/libvir-list/2018-June/msg00249.html

Touching something that's been avoided for 8 years is always scary, but if it's broken, then sure it ought to be fixed.

There is no BZ. And yes, the GSoC project got me experimenting (because for libiscsi backend we will have to make IQN required since libiscsi does not parse anything from /etc/iscsi nor initiatorname.iscsi). And while experimenting I've tired to put my own IQN into iscsi pool we already have and found this bug.

...

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_iscsi.c | 4 +- src/util/viriscsi.c | 78 ++++++++++++++++++++++++++++++++++--- src/util/viriscsi.h | 1 + tests/viriscsitest.c | 2 +- 4 files changed, 77 insertions(+), 8 deletions(-)

This patch fails to compile for me:

In file included from util/viriscsi.c:32:0: util/viriscsi.c: In function 'virISCSIScanTargets': util/virerror.h:187:5: error: this statement may fall through [-Werror=implicit-fallthrough=] virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ __FUNCTION__, __LINE__, __VA_ARGS__) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ util/viriscsi.c:479:13: note: in expansion of macro 'virReportError' virReportError(VIR_ERR_OPERATION_FAILED, ^~~~~~~~~~~~~~ util/viriscsi.c:482:9: note: here case IQN_ERROR:

Ah, okay. My gcc doesn't warn me about this. Wonder if I have something misconfigured.

^~~~

...

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,

NIT: This could go on the previous line

...

+ &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 44788056fd..365669aac2 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.

"invented" - again is this the make sure it's issued over the initiatoriqn interface?

Yes. And we construct the interface name at the beginning of this function. Michal

On 07/03/2018 02:38 PM, John Ferlan wrote:

On 07/03/2018 01:08 AM, Michal Prívozník wrote:

...

On 07/03/2018 01:38 AM, John Ferlan wrote:

...

On 06/29/2018 11:01 AM, Michal Privoznik wrote:

...

When scanning for targets, iSCSI might give different results depending on the interface used. This is basically just name of

assuming this means whether the initiatoriqn interface was used

Yes.

...

...

config file under /etc/iscsi/ifaces to use. The file contains initiator IQN thus different results claim.

Strange to have activity here - was there a bz? Or something found by the (I assume) GSoC project:

https://www.redhat.com/archives/libvir-list/2018-June/msg00249.html

Touching something that's been avoided for 8 years is always scary, but if it's broken, then sure it ought to be fixed.

There is no BZ. And yes, the GSoC project got me experimenting (because for libiscsi backend we will have to make IQN required since libiscsi does not parse anything from /etc/iscsi nor initiatorname.iscsi). And while experimenting I've tired to put my own IQN into iscsi pool we already have and found this bug.

FWIW: To a degree this is restoring the initiatoriqn argument that commit 027986f5 removed...

Yes, but I did not think it is worth mentioning anywhere. We change code all the time, and if we were even to blame which commits caused what .. it's a endless spiral IMO.

[...]

...

...

...

+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.

"invented" - again is this the make sure it's issued over the initiatoriqn interface?

Yes. And we construct the interface name at the beginning of this function.

Oh right, the virStorageBackendCreateIfaceIQN call which creates that libvirt-iface-* name. That's just one of those context things that you know when you've been working through the code ;-).

Still without the next patch, does this work?

Not really because even though targets are refreshed their configs are not saved and thus login still fails. Michal

On 06/29/2018 11:01 AM, Michal Privoznik wrote:

After new iSCSI interface is successfully set up, we issue

s/new/a new/ s/issue/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> --- src/storage/storage_backend_iscsi.c | 1 + src/util/viriscsi.c | 21 ++++++++++++++++++--- src/util/viriscsi.h | 1 + tests/viriscsitest.c | 3 ++- 4 files changed, 22 insertions(+), 4 deletions(-)

Like the previous patch - is there a specific bug or something that led you down this path? Can you show an example of the existing code that's creating a bad command line and generating an error and then how this fixes things. It's not like we have tests and for this stuff it's really nice to have plenty of examples.

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 365669aac2..baf41c5be1 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);

@@ -295,8 +296,10 @@ virISCSIConnection(const char *portal, * 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.

s/above./above/

+ * AND that targets are made persistent.

s/AND/and/ Similar to previous - with minor adjustments and explanation, Reviewed-by: John Ferlan <jferlan@redhat.com> John

*/ - if (virISCSIScanTargetsInternal(portal, ifacename, NULL, NULL) < 0) + if (virISCSIScanTargetsInternal(portal, ifacename, + true, NULL, NULL) < 0) goto cleanup;

break;

[...]