[libvirt] [PATCH v2 07/10] viriscsitest: Test virISCSIConnectionLogin

[John Ferlan]

On 07/23/2018 08:34 AM, Michal Prívozník wrote:
> On 07/23/2018 02:12 PM, John Ferlan wrote:
>>
>>
>> On 07/23/2018 04:01 AM, Michal Prívozník wrote:
>>> On 07/17/2018 09:14 PM, John Ferlan wrote:
>>>>
>>>>
>>>> On 07/04/2018 05:23 AM, Michal Privoznik wrote:
>>>>> Introduce one basic test that tests the simplest case:
>>>>> logging into portal without any IQN.
>>>>>
>>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>>> ---
>>>>>  tests/viriscsitest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 46 insertions(+)
>>>>>
>>>>> diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
>>>>> index 3bb3993196..a7287069ba 100644
>>>>> --- a/tests/viriscsitest.c
>>>>> +++ b/tests/viriscsitest.c
>>>>> @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args,
>>>>>                 args[8] && STREQ(args[8], "nonpersistent") &&
>>>>>                 args[9] == NULL) {
>>>>>          ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
>>
>> here we do some sort of comparison
> 
> What comparison? The only comparison I see is "args[X] && STREQ(args[X],
> ...)".
> 
> If you're referring to VIR_STRDUP() that is setting the pretended output
> of the iscsiadm command. It's not comparing anything.
> 

Can you just indicate rather than "nada" what the actual deal is here?
That is, in our mocked environment when running this command we don't
expect to get any output from iscsiadm. If this were a real command then
the following would be returned:

...

In my saved iSCSI output file, I have for example:

iscsiadm -m node -T iqn.2013-12.com.example:iscsi-chap-pool -p
192.168.122.1 --login

returning:

Logging in to [iface: default, target:
iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260]
(multiple)
Login to [iface: default, target:
iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260]
successful.


Hopefully this helps resolves the "confusion" over this.  Like I said
for patch10, no need for a v3, just a diff would be fine, although at
this point it only helps for my really short term memory.

John

>>
>>>>> +    } else if (args[0] && STREQ(args[0], ISCSIADM) &&
>>>>> +               args[1] && STREQ(args[1], "--mode") &&
>>>>> +               args[2] && STREQ(args[2], "node") &&
>>>>> +               args[3] && STREQ(args[3], "--portal") &&
>>>>> +               args[4] && STREQ(args[4], "10.20.30.40:3260,1") &&
>>>>> +               args[5] && STREQ(args[5], "--targetname") &&
>>>>> +               args[6] && STREQ(args[6], "iqn.2004-06.example:example1:iscsi.test") &&
>>>>> +               args[7] && STREQ(args[7], "--login") &&
>>>>> +               args[8] == NULL) {
>>>>> +        /* nada */
>>>>
>>>>
>>>> Hmm.. can we place a hold on that R-By - I missed this gem "nada"  -
>>>> why is that?
>>>>
>>>> If we have *output because we've sent the dryRun, then why not compare?
>>>
>>> I'm not quite sure what you mean. What should be compared?
>>
>> here we do not - instead we just use nada. 
> 
> That is correct because we do need to accept that cmd line but we do not
> need to take any extra step to mimic iscsiadm behaviour we're after.
> 
>> So after reading patch10
>> where nada is again used, I began to doubt/wonder about using it here
>> especially since there are comparisons for all other cases.
>>
>> John
>>
> 
> Michal
>