6:18 p.m.
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(a)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;
}