[libvirt] [PATCH v2 10/10] viriscsitest: Extend virISCSIConnectionLogin test
Michal Prívozník
mprivozn at redhat.com
Mon Jul 23 08:01:27 UTC 2018
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));
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.
>> 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" ;-)
>> + } 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.
>> + 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?
>
>> + } 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'.
>
>> + } 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.
Michal
More information about the libvir-list
mailing list