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

[John Ferlan]

On 07/03/2018 01:08 AM, Michal Prívozník wrote:
> 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.
> 

Understood - it's probably something really early on when I first
started w/ libvirt and using sscanf was discouraged for something that
has stuck in my head. Another part for me is readability - similar to
the various [i]scsi code that uses regex's in order to parse output.
That format is just a mish-mash of search patterns that causes my eyes
to complain.

Perhaps it's best to see this along with the combined 4&5 "again".  Also
add a non initiatoriqn to the mix (even the comment above) just so show
the various formats.

Hopefully using libiscsi makes all the personally displeasing regex and
sscanf searching code disappear.

John