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@xxxxxxxxxx>
>>> ---
>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
