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

[Michal Prívozník]

On 07/17/2018 08:42 PM, John Ferlan wrote:
> 
> 
> 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 at 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 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;

I don't think we need this. Firstly, the code as is now doesn't care
about that either. Secondly, even though we currently call the function
only to learn about libvirt-iface-* it is generic enough to be called
over any interface.

> 
> 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 ;-)).
> 

Not really. From sscanf(3):

  The  use  of  the  letter  a  for  this purpose was problematic, since
  a is also specified by the ISO C standard as a synonym for f
  (floating-point input).  POSIX.1-2008 instead specifies the m modifier
  for assignment allocation (as documented in DESCRIPTION, above).

So it's mandated by POSIX which should be good enough for us to use.

> Reviewed-by: John Ferlan <jferlan at redhat.com>

Thanks,
Michal