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

[Michal Prívozník]

On 07/03/2018 01:18 AM, John Ferlan wrote:
> 
> 
> 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.

Perhaps. Let me check git log. .. .. And yes, you are right.
virCommandSetOutputBuffer was introduced among with other basic
virCommand* APIs by Dan in late 2010 (f16ad06fb2aeb5e5c9974). But this
function was introduced by Dave in Jan 2010 so he couldn't have used it.

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

I don't know about anything. But since we use it in our code pretty
freely I assumed there is no problem with it.

> 
>> 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, 

Indeed.

> but at least it's for other purposes
> and perhaps less susceptible to the line && *line adjustment.

I like sscanf() more because it's fewer lines of code, the variables I
need are assigned value immediately and also memory footprint is smaller
(I guess) since there's no need to store multiple arrays.

Michal