[libvirt] [PATCH v2 10/10] viriscsitest: Extend virISCSIConnectionLogin test
John Ferlan
jferlan at redhat.com
Tue Jul 17 19:15:21 UTC 2018
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 ;-)
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
> 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"
> + } 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...
> + 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?
> + } 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)
> + } 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;
John
> - virCommandSetDryRun(NULL, testIscsiadmCb, NULL);
> + virCommandSetDryRun(NULL, testIscsiadmCb, &cbData);
>
> if (virISCSIConnectionLogin(info->portal, info->initiatoriqn, info->target) < 0)
> goto cleanup;
> @@ -265,11 +333,14 @@ mymain(void)
> } while (0)
>
> DO_LOGIN_TEST("10.20.30.40:3260,1", NULL, "iqn.2004-06.example:example1:iscsi.test");
> + DO_LOGIN_TEST("10.20.30.40:3260,1", "iqn.2004-06.example:example1:initiator",
> + "iqn.2004-06.example:example1:iscsi.test");
>
> if (rv < 0)
> return EXIT_FAILURE;
> return EXIT_SUCCESS;
> }
>
> -VIR_TEST_MAIN(mymain)
> +VIR_TEST_MAIN_PRELOAD(mymain,
> + abs_builddir "/.libs/virrandommock.so")
> #endif /* WIN32 */
>
More information about the libvir-list
mailing list