Re: [libvirt] [PATCH v2 03/10] virStorageBackendIQNFound: Rework iscsiadm output parsing

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