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