8:10 a.m.
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(a)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