[libvirt] [PATCH v2 10/10] viriscsitest: Extend virISCSIConnectionLogin test
John Ferlan
jferlan at redhat.com
Mon Jul 23 13:10:25 UTC 2018
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...
>>> 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.
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.
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.
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).
>>> + } 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...
>>> + 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
>>
>>> + } 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.
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.
John
More information about the libvir-list
mailing list