[libvirt] [PATCH v2 10/10] viriscsitest: Extend virISCSIConnectionLogin test

[John Ferlan]

On 07/24/2018 05:48 AM, Michal Privoznik wrote:
> On 07/23/2018 03:10 PM, John Ferlan wrote:
>>
>>
>> On 07/23/2018 04:01 AM, Michal Prívozník wrote:
>>> On 07/17/2018 09:15 PM, John Ferlan wrote:
>>>>
>>>>
>>>> On 07/04/2018 05:23 AM, Michal Privoznik wrote:
>>>>> Extend this existing test so that a case when IQN is provided is
>>>>> tested too. Since a special iSCSI interface is created and its
>>>>> name is randomly generated at runtime we need to link with
>>>>> virrandommock to have predictable names.
>>>>>
>>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>>> ---
>>>>>  tests/viriscsitest.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>>>  1 file changed, 73 insertions(+), 2 deletions(-)
>>>>>
>>>>
>>>> Should testIscsiadmCbData initialization get { false, false } now (and I
>>>> should have mention in patch 9 that :
>>>>
>>>> +    struct testIscsiadmCbData cbData = { 0 };
>>>>
>>>> should be
>>>>
>>>> +    struct testIscsiadmCbData cbData = { false };>
>>>> I know that 0 and false are synonymous, but syntactical correctness is
>>>> in play here ;-)
>>>
>>> No. So { 0 } is basically a shortcut for:
>>>
>>> struct testIscsiadmCbData cbData;
>>>
>>> memset(&cbData, 0, sizeof(cbData));
>>
>> Oh yeah, right <facepalm> for literal interpretation of data.
>>
>>>
>>> It has nothing to do with the struct having only two bools (for now). It
>>> is used widely in our code, because it has one great advantage: even if
>>> we add/remove/rearrange items in the struct it is always going to be
>>> zeroed out whereas if I initialized it like this:
>>>
>>> struct testIscsiadmCbData cbData = {false, false};
>>>
>>> and then somebody would append yet another member to the struct they
>>> would either:
>>>
>>> a) have to change all the lines where the struct is initialized
>>> (possibly touching functions that has nothing to do with actual change), or
>>> b) forget to initialize it completely.
>>>
>>>
>>> TL;DR It's just coincidence that the first member is a bool.
>>>
>>>>
>>>> I also think you're doing two things here:
>>>>
>>>> 1. Generating a dryRun output for "existing" test without initiator.iqn
>>>>
>>>> 2. Adding tests to test initiator.iqn
>>>>
>>>
>>> Sure. That's how login works. Firstly it lists output of iscsi
>>> interfaces, then it creates a new one and then use that to log in. I
>>> don't see a problem here.
>>>
>>
>> OK, right... Maybe it would have been useful from the literal review POV
>> to be reminded of this...  While sometimes it's a pain to document the
>> reason for something - it perhaps prevents this back and forth later on
>> especially when it's "unique case" code paths.  IIQN isn't really well
>> documented by any stretch...
> 
> Well, I usually read the code when trying to understand how something
> works as our comments/documentation is often incorrect.
> 

If the comments/documentation is often incorrect, then that should be
rectified. Help the next poor soul trying to read what someone thought
was self documenting code. To me that almost reaches the level of a
trivial patch.

>>
>>>>> diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
>>>>> index c6e0e3b8ff..55889d04a3 100644
>>>>> --- a/tests/viriscsitest.c
>>>>> +++ b/tests/viriscsitest.c
>>>>> @@ -60,8 +60,19 @@ const char *iscsiadmSendtargetsOutput =
>>>>>      "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n"
>>>>>      "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
>>>>>  
>>>>> +const char *iscsiadmIfaceDefaultOutput =
>>>>> +    "default tcp,<empty>,<empty>,<empty>,<empty>\n"
>>>>> +    "iser iser,<empty>,<empty>,<empty>,<empty>\n";
>>>>> +
>>>>> +const char *iscsiadmIfaceIfaceOutput =
>>>>> +    "default tcp,<empty>,<empty>,<empty>,<empty>\n"
>>>>> +    "iser iser,<empty>,<empty>,<empty>,<empty>\n"
>>>>> +    "libvirt-iface-03020100 tcp,<empty>,<empty>,<empty>,iqn.2004-06.example:example1:initiator\n";
>>>>> +
>>>>> +
>>>>>  struct testIscsiadmCbData {
>>>>>      bool output_version;
>>>>> +    bool iface_created;
>>>>>  };
>>>>>  
>>>>>  static void testIscsiadmCb(const char *const*args,
>>>>> @@ -103,6 +114,62 @@ static void testIscsiadmCb(const char *const*args,
>>>>>                 args[7] && STREQ(args[7], "--login") &&
>>>>>                 args[8] == NULL) {
>>>>>          /* nada */
>>>>
>>>> Is this "nada"
>>>
>>> Yes, this is "nada" ;-)
>>>
>>
>> Maybe the explanation as to why it's nada and not just leaving nada
>> there would have helped.
> 
> As I say in the other reply, how about s/nada/no output/?
> 
>>
>> In some instances we're doing output comparison (eventually) and in
>> others we're not. In the long run it's mainly a matter of being reminded
>> what the processing is and what (if any) the expected output is.
>> Sometimes that expected output only occurs on the next query, IIRC.
> 
> Yes, this is right.
> 
>>
>> Creating IIQN's or iSCSI targets is not something I do all that often -
>> I wonder if you came back to this code 6 months or longer from now
>> whether you'd (instantly) recall what type of output was generated ;-).
>> If you would then your short term memory is better than mine! I have to
>> keep some cheat notes for iSCSI processing letting me know which
>> commands to use and essentially what they return so I know they
>> completed as expected.
> 
> Of course I would not. But seeing this pattern would help definitely:
> 
> if (some args) {
>   VIR_STRDUP(output, expectedOutput1);
> } else if (some other args) {
>   /* nada */
> } else {
>   exitstatus = -1;
> }
> 
> It suggest to me that for "some args" an output is generated while for
> "some other args" it isn't.
> 
>>
>> BTW: This is my favorite from f28 recently:
>>
>> # iscsiadm -m session -P 3
>> iSCSI Transport Class version 2.0-870
>> version 6.2.0.874
>> Segmentation fault (core dumped)
>> #
>>
>> Oy!  at least it's getting further now than at one time where the first
>> line wasn't even printed.
>>
>> Don't even get me started about the 'targetcli' command which I find to
>> be absolutely awful. It's supposed to be better, but I feel like I
>> stepped back into the 1990's and I'm using OpenVMS NCP/NCL (that'd
>> require some googling for you I'm sure, but suffice to say it was an
>> awful^^n syntax).
> 
> Yes, iscsi CLI isn't one of the best CLIs I've seen either.
> 
>>
>>>>> +    } else if (args[0] && STREQ(args[0], ISCSIADM) &&
>>>>> +               args[1] && STREQ(args[1], "--mode") &&
>>>>> +               args[2] && STREQ(args[2], "iface") &&
>>>>> +               args[3] == NULL) {
>>>>> +        if (data->iface_created)
>>>>
>>>> How would iface_created be set by this point if...
>>>>
>>>
>>> I suggest opening virISCSIConnection() in another window and looking
>>> what it does (assuming that @initiatoriqn is set). So the first function
>>> that is called is virStorageBackendIQNFound() which execs `ISCSIADM
>>> --mode iface` to list all iscsi interfaces. If no libvirt interface is
>>> found IQN_MISSING is returned and virStorageBackendCreateIfaceIQN() is
>>> called. This function creates the interface and then calls
>>> virStorageBackendIQNFound() again to confirm the interface is created.
>>>
>>> Long story short, `ISCSIADM --mode iface` is called twice - before and
>>> after iface creation. Therefore we have to return different outputs to
>>> mimic the iface creation.
>>>
>>
>> Seems like something worthy to note in the code, but maybe that's just me...
> 
> Well, all that is needed to learn this is to open virISCSIConnection()
> and it's immediately visible there. Documenting it is no good IMO. IOW,
> it would be like this:
> 
> /* add A to B and store the result in C */
> C = A + B;
> /* Then call func() over it */
> func(C);
> 
> As soon as you see the code it's visible what is being called.
> 
> Putting a comment somewhere is worth it when the logic is unclear. The
> big comment (with a picture) in virNetDevBandwidthSet() is great
> example.
> 

Suffice to say I don't think A+B=C is the same as noting that creating
an Initiator IQN is a multi-command process especially when the non IIQN
is a single step process.

>>
>>>>> +            ignore_value(VIR_STRDUP(*output, iscsiadmIfaceIfaceOutput));
>>>>> +        else
>>>>> +            ignore_value(VIR_STRDUP(*output, iscsiadmIfaceDefaultOutput));
>>>>> +    } else if (args[0] && STREQ(args[0], ISCSIADM) &&
>>>>> +               args[1] && STREQ(args[1], "--mode") &&
>>>>> +               args[2] && STREQ(args[2], "iface") &&
>>>>> +               args[3] && STREQ(args[3], "--interface") &&
>>>>> +               args[4] && STREQ(args[4], "libvirt-iface-03020100") &&
>>>>> +               args[5] && STREQ(args[5], "--op") &&
>>>>> +               args[6] && STREQ(args[6], "new") &&
>>>>> +               args[7] == NULL) {
>>>>> +        data->iface_created = true;
>>>>
>>>> ... it's being set unconditionally here?
>>>>
>>>> See note below, shouldn't the caller be doing this?
>>>
>>> What caller?
>>>
>>
>> testISCSIConnectionLogin
> 
> No. I view @data as place for testIscsiadmCb to store its state between
> several runs. Also, if it was testISCSIConnectionLogin who sets it, what
> is the place to do so? It can't be before calling
> virISCSIConnectionLogin() because we want the code to go the extra mile
> and test iface creation too. But it can't be after
> virISCSIConnectionLogin() neither - at that point the iface must be
> already created.
> 

OK - this I guess makes sense since it's a multi-command process. I just
find it "odd" that callback routine is setting the boolean. I'd comment
that fact, but you'd say read the called API to find out what happens.

>>
>>>>
>>>>> +    } else if (args[0] && STREQ(args[0], ISCSIADM) &&
>>>>> +               args[1] && STREQ(args[1], "--mode") &&
>>>>> +               args[2] && STREQ(args[2], "iface") &&
>>>>> +               args[3] && STREQ(args[3], "--interface") &&
>>>>> +               args[4] && STREQ(args[4], "libvirt-iface-03020100") &&
>>>>> +               args[5] && STREQ(args[5], "--op") &&
>>>>> +               args[6] && STREQ(args[6], "update") &&
>>>>> +               args[7] && STREQ(args[7], "--name") &&
>>>>> +               args[8] && STREQ(args[8], "iface.initiatorname") &&
>>>>> +               args[9] && STREQ(args[9], "--value") &&
>>>>> +               args[10] && STREQ(args[10], "iqn.2004-06.example:example1:initiator") &&
>>>>> +               args[11] == NULL &&
>>>>> +               data->iface_created) {
>>>>> +        /* nada */
>>>>
>>>> ?? Why nada (now you know where my 7/10 requery came from)
>>>
>>> Maybe you are confused what this callback is doing? The whole point of
>>> this callback is to avoid calling real iscsiadm binary AND having to
>>> write our own binary. We can write a function that is called instead of
>>> spawning real binary. But the function has to act like real binary -
>>> otherwise our code declares an erroneous binary. For instance:
>>>
>>> calling `iscsiadm --mode iface --op new --interface myInterface` creates
>>> new iscsi interface. Therefore, if I call `iscsiadm --mode iface`
>>> afterwards I have to see it in the list of the interfaces. And this is
>>> what the callback is doing. It doesn't have to take action for every
>>> combination of cmd line arguments in order to successfully mimic real
>>> iscsiadm binary behaviour. This fact is then noted as 'nada'.
>>>
>>
>> Maybe it is just confusing to have some tests generate sample output
>> while others don't for "nada" (a/k/a no apparent) reason. Ones with far
>> more complex command options, too! Of course that's is the difference
>> between fetching status and performing an action perhaps.
>>
>> IMO: nada provides no details as to the reason why there's no output to
>> compare against (eventually).
>>
>>>>
>>>>> +    } else if (args[0] && STREQ(args[0], ISCSIADM) &&
>>>>> +               args[1] && STREQ(args[1], "--mode") &&
>>>>> +               args[2] && STREQ(args[2], "discovery") &&
>>>>> +               args[3] && STREQ(args[3], "--type") &&
>>>>> +               args[4] && STREQ(args[4], "sendtargets") &&
>>>>> +               args[5] && STREQ(args[5], "--portal") &&
>>>>> +               args[6] && STREQ(args[6], "10.20.30.40:3260,1") &&
>>>>> +               args[7] && STREQ(args[7], "--interface") &&
>>>>> +               args[8] && STREQ(args[8], "libvirt-iface-03020100") &&
>>>>> +               args[9] == NULL &&
>>>>> +               data->iface_created) {
>>>>> +        ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
>>>>> +    } 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] && STREQ(args[8], "--interface") &&
>>>>> +               args[9] && STREQ(args[9], "libvirt-iface-03020100") &&
>>>>> +               args[10] == NULL &&
>>>>> +               data->iface_created) {
>>>>> +        /* nada */
>>>>
>>>> Similar, why nada?
>>>>
>>>>
>>>>>      } else {
>>>>>          *status = -1;
>>>>>      }
>>>>> @@ -204,9 +271,10 @@ static int
>>>>>  testISCSIConnectionLogin(const void *data)
>>>>>  {
>>>>>      const struct testConnectionInfoLogin *info = data;
>>>>> +    struct testIscsiadmCbData cbData = { 0 };
>>>>
>>>> s/0/false, false/
>>>>
>>>>>      int ret = -1;
>>>>>  
>>>>
>>>> Why wouldn't initialization be:
>>>>
>>>>     cbData.iface_create = info->initiatoriqn != NULL;
>>>
>>> Because I want to test more than a single successful path in the code.
>>> If the interface doesn't exist initially, it is created and only then
>>> login is attempted. If it existed only login would be tested.
>>>
>>
>> It seems to me that 'logic' is hidden behind setting iface_create in the
>> callback routine.  Why not two tests?  One that tests ensuring the
>> interface is created and one that uses a created interface - if that's
>> even possible.
> 
> Because it's one function. Okay, step aside for a moment and consider
> the following function:
> 
>   int f(bool extra)
>   {
>     if (extra)
>       take_extra_action();
> 
>     take_some_action();
>   }
> 
> Sure, you can write two tests, one to call f(false) the other to call
> f(true). BUT the latter already tests the former so I don't see much
> value added (in fact I see no added value) in having two tests. I do see
> value in having f(true) test though.
> 
>>
>> So after all that, beyond the weirdess I find with iface_create, perhaps
>> a few extra words to document what's happening would help. I'm fine with
>> a here's a diff to the patch type patch. I don't think a v3 is necessary.
> 
> 
> The only diff I can think of is 's/nada/no output/'. I don't see any
> other problem with the patches :-P
> 
> Michal
> 

See patch7 for my feelings on "nada" and "no output"

Again, for the code/logic:

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

do whatever you want for comments

John