[libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing

[John Ferlan]

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