4:32 a.m.
On 08/05/13 23:03, Osier Yang wrote:
On 08/05/13 21:53, John Ferlan wrote:
> On 05/06/2013 08:45 AM, Osier Yang wrote:
>> Since the NPIV machine is not easy to get, it's very likely to
>> introduce regressions when doing changes on the existing code.
>> This patch dumps part of the sysfs files (the necessary ones)
>> of fc_host as test input data, to test the related util functions.
>> It could be extended for more fc_host related testing in future.
>> ---
>> tests/Makefile.am | 5 +
>> tests/fchostdata/fc_host/host4/fabric_name | 1 +
>> tests/fchostdata/fc_host/host4/max_npiv_vports | 1 +
>> tests/fchostdata/fc_host/host4/node_name | 1 +
>> tests/fchostdata/fc_host/host4/npiv_vports_inuse | 1 +
>> tests/fchostdata/fc_host/host4/port_name | 1 +
>> tests/fchostdata/fc_host/host4/port_state | 1 +
>> tests/fchostdata/fc_host/host4/vport_create | 0
>> tests/fchostdata/fc_host/host4/vport_delete | 0
>> tests/fchostdata/fc_host/host5/fabric_name | 1 +
>> tests/fchostdata/fc_host/host5/max_npiv_vports | 1 +
>> tests/fchostdata/fc_host/host5/node_name | 1 +
>> tests/fchostdata/fc_host/host5/npiv_vports_inuse | 1 +
>> tests/fchostdata/fc_host/host5/port_name | 1 +
>> tests/fchostdata/fc_host/host5/port_state | 1 +
>> tests/fchostdata/fc_host/host5/vport_create | 0
>> tests/fchostdata/fc_host/host5/vport_delete | 0
>> tests/fchosttest.c | 175
>> +++++++++++++++++++++++
>> 18 files changed, 192 insertions(+)
>> create mode 100644 tests/fchostdata/fc_host/host4/fabric_name
>> create mode 100644 tests/fchostdata/fc_host/host4/max_npiv_vports
>> create mode 100644 tests/fchostdata/fc_host/host4/node_name
>> create mode 100644 tests/fchostdata/fc_host/host4/npiv_vports_inuse
>> create mode 100644 tests/fchostdata/fc_host/host4/port_name
>> create mode 100644 tests/fchostdata/fc_host/host4/port_state
>> create mode 100644 tests/fchostdata/fc_host/host4/vport_create
>> create mode 100644 tests/fchostdata/fc_host/host4/vport_delete
>> create mode 100644 tests/fchostdata/fc_host/host5/fabric_name
>> create mode 100644 tests/fchostdata/fc_host/host5/max_npiv_vports
>> create mode 100644 tests/fchostdata/fc_host/host5/node_name
>> create mode 100644 tests/fchostdata/fc_host/host5/npiv_vports_inuse
>> create mode 100644 tests/fchostdata/fc_host/host5/port_name
>> create mode 100644 tests/fchostdata/fc_host/host5/port_state
>> create mode 100644 tests/fchostdata/fc_host/host5/vport_create
>> create mode 100644 tests/fchostdata/fc_host/host5/vport_delete
>> create mode 100644 tests/fchosttest.c
>>
>> diff --git a/tests/Makefile.am b/tests/Makefile.am
>> index 68dbb27..48c4e4a 100644
>> --- a/tests/Makefile.am
>> +++ b/tests/Makefile.am
>> @@ -107,6 +107,7 @@ test_programs = virshtest sockettest \
>> virportallocatortest \
>> sysinfotest \
>> virstoragetest \
>> + fchosttest \
>> $(NULL)
>> if WITH_GNUTLS
>> @@ -525,6 +526,10 @@ nodeinfotest_SOURCES = \
>> nodeinfotest.c testutils.h testutils.c
>> nodeinfotest_LDADD = $(LDADDS)
>> +fchosttest_SOURCES = \
>> + fchosttest.c testutils.h testutils.c
>> +fchosttest_LDADD = $(LDADDS)
>> +
>> commandtest_SOURCES = \
>> commandtest.c testutils.h testutils.c
>> commandtest_LDADD = $(LDADDS)
>> diff --git a/tests/fchostdata/fc_host/host4/fabric_name
>> b/tests/fchostdata/fc_host/host4/fabric_name
>> new file mode 100644
>> index 0000000..f587b68
>> --- /dev/null
>> +++ b/tests/fchostdata/fc_host/host4/fabric_name
>> @@ -0,0 +1 @@
>> +0xffffffffffffffff
>> diff --git a/tests/fchostdata/fc_host/host4/max_npiv_vports
>> b/tests/fchostdata/fc_host/host4/max_npiv_vports
>> new file mode 100644
>> index 0000000..c75acbe
>> --- /dev/null
>> +++ b/tests/fchostdata/fc_host/host4/max_npiv_vports
>> @@ -0,0 +1 @@
>> +127
>> diff --git a/tests/fchostdata/fc_host/host4/node_name
>> b/tests/fchostdata/fc_host/host4/node_name
>> new file mode 100644
>> index 0000000..e8c1e1a
>> --- /dev/null
>> +++ b/tests/fchostdata/fc_host/host4/node_name
>> @@ -0,0 +1 @@
>> +0x2000001b3289da4e
>> diff --git a/tests/fchostdata/fc_host/host4/npiv_vports_inuse
>> b/tests/fchostdata/fc_host/host4/npiv_vports_inuse
>> new file mode 100644
>> index 0000000..573541a
>> --- /dev/null
>> +++ b/tests/fchostdata/fc_host/host4/npiv_vports_inuse
>> @@ -0,0 +1 @@
>> +0
>> diff --git a/tests/fchostdata/fc_host/host4/port_name
>> b/tests/fchostdata/fc_host/host4/port_name
>> new file mode 100644
>> index 0000000..ee2d399
>> --- /dev/null
>> +++ b/tests/fchostdata/fc_host/host4/port_name
>> @@ -0,0 +1 @@
>> +0x2100001b3289da4e
>> diff --git a/tests/fchostdata/fc_host/host4/port_state
>> b/tests/fchostdata/fc_host/host4/port_state
>> new file mode 100644
>> index 0000000..bd1e6d5
>> --- /dev/null
>> +++ b/tests/fchostdata/fc_host/host4/port_state
>> @@ -0,0 +1 @@
>> +Linkdown
>> diff --git a/tests/fchostdata/fc_host/host4/vport_create
>> b/tests/fchostdata/fc_host/host4/vport_create
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/tests/fchostdata/fc_host/host4/vport_delete
>> b/tests/fchostdata/fc_host/host4/vport_delete
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/tests/fchostdata/fc_host/host5/fabric_name
>> b/tests/fchostdata/fc_host/host5/fabric_name
>> new file mode 100644
>> index 0000000..4128070
>> --- /dev/null
>> +++ b/tests/fchostdata/fc_host/host5/fabric_name
>> @@ -0,0 +1 @@
>> +0x2001000dec9877c1
>> diff --git a/tests/fchostdata/fc_host/host5/max_npiv_vports
>> b/tests/fchostdata/fc_host/host5/max_npiv_vports
>> new file mode 100644
>> index 0000000..c75acbe
>> --- /dev/null
>> +++ b/tests/fchostdata/fc_host/host5/max_npiv_vports
>> @@ -0,0 +1 @@
>> +127
>> diff --git a/tests/fchostdata/fc_host/host5/node_name
>> b/tests/fchostdata/fc_host/host5/node_name
>> new file mode 100644
>> index 0000000..91a0a05
>> --- /dev/null
>> +++ b/tests/fchostdata/fc_host/host5/node_name
>> @@ -0,0 +1 @@
>> +0x2001001b32a9da4e
>> diff --git a/tests/fchostdata/fc_host/host5/npiv_vports_inuse
>> b/tests/fchostdata/fc_host/host5/npiv_vports_inuse
>> new file mode 100644
>> index 0000000..573541a
>> --- /dev/null
>> +++ b/tests/fchostdata/fc_host/host5/npiv_vports_inuse
>> @@ -0,0 +1 @@
>> +0
>> diff --git a/tests/fchostdata/fc_host/host5/port_name
>> b/tests/fchostdata/fc_host/host5/port_name
>> new file mode 100644
>> index 0000000..c37f618
>> --- /dev/null
>> +++ b/tests/fchostdata/fc_host/host5/port_name
>> @@ -0,0 +1 @@
>> +0x2101001b32a9da4e
>> diff --git a/tests/fchostdata/fc_host/host5/port_state
>> b/tests/fchostdata/fc_host/host5/port_state
>> new file mode 100644
>> index 0000000..b73dd46
>> --- /dev/null
>> +++ b/tests/fchostdata/fc_host/host5/port_state
>> @@ -0,0 +1 @@
>> +Online
>> diff --git a/tests/fchostdata/fc_host/host5/vport_create
>> b/tests/fchostdata/fc_host/host5/vport_create
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/tests/fchostdata/fc_host/host5/vport_delete
>> b/tests/fchostdata/fc_host/host5/vport_delete
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/tests/fchosttest.c b/tests/fchosttest.c
>> new file mode 100644
>> index 0000000..035f721
>> --- /dev/null
>> +++ b/tests/fchosttest.c
>> @@ -0,0 +1,175 @@
>> +/*
>> + * Copyright (C) 2013 Red Hat, Inc.
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library. If not, see
>> + * <http://www.gnu.org/licenses/>;.
>> + *
>> + */
>> +
>> +#include <config.h>
>> +
>> +#include "virutil.h"
>> +#include "testutils.h"
>> +
>> +#define TEST_FC_HOST_PREFIX "./fchostdata/fc_host/"
>> +#define TEST_FC_HOST_NUM 5
>> +
>> +/* Test virIsCapableFCHost */
>> +static int
>> +test1(const void *data ATTRIBUTE_UNUSED)
>> +{
>> + if (virIsCapableFCHost(TEST_FC_HOST_PREFIX,
>> + TEST_FC_HOST_NUM))
>> + return 0;
>> +
>> + return -1;
>> +}
>> +
>> +/* Test virIsCapableVport */
>> +static int
>> +test2(const void *data ATTRIBUTE_UNUSED)
>> +{
>> + if (virIsCapableVport(TEST_FC_HOST_PREFIX,
>> + TEST_FC_HOST_NUM))
>> + return 0;
>> +
>> + return -1;
>> +}
>> +
>> +/* Test virReadFCHost */
>> +static int
>> +test3(const void *data ATTRIBUTE_UNUSED)
>> +{
>> + const char *expect_wwnn = "2001001b32a9da4e";
>> + const char *expect_wwpn = "2101001b32a9da4e";
>> + const char *expect_fabric_wwn = "2001000dec9877c1";
>> + const char *expect_max_vports = "127";
>> + const char *expect_vports = "0";
>> + char *wwnn = NULL;
>> + char *wwpn = NULL;
>> + char *fabric_wwn = NULL;
>> + char *max_vports = NULL;
>> + char *vports = NULL;
>> + int ret = -1;
>> +
>> + if (virReadFCHost(TEST_FC_HOST_PREFIX,
>> + TEST_FC_HOST_NUM,
>> + "node_name",
>> + &wwnn) < 0)
>> + return -1;
> NIT: s/-1/ret or goto cleanup;
>
> Not sure if there's a norm though
>
>> +
>> + if (virReadFCHost(TEST_FC_HOST_PREFIX,
>> + TEST_FC_HOST_NUM,
>> + "port_name",
>> + &wwpn) < 0)
>> + goto cleanup;
>> +
>> + if (virReadFCHost(TEST_FC_HOST_PREFIX,
>> + TEST_FC_HOST_NUM,
>> + "fabric_name",
>> + &fabric_wwn) < 0)
>> + goto cleanup;
>> +
>> + if (virReadFCHost(TEST_FC_HOST_PREFIX,
>> + TEST_FC_HOST_NUM,
>> + "max_npiv_vports",
>> + &max_vports) < 0)
>> + goto cleanup;
>> +
>> +
>> + if (virReadFCHost(TEST_FC_HOST_PREFIX,
>> + TEST_FC_HOST_NUM,
>> + "npiv_vports_inuse",
>> + &vports) < 0)
>> + goto cleanup;
>> +
>> + if (STRNEQ(expect_wwnn, wwnn) ||
>> + STRNEQ(expect_wwpn, wwpn) ||
>> + STRNEQ(expect_fabric_wwn, fabric_wwn) ||
>> + STRNEQ(expect_max_vports, max_vports) ||
>> + STRNEQ(expect_vports, vports))
>> + goto cleanup;
>> +
>> + ret = 0;
>> +cleanup:
>> + VIR_FREE(wwnn);
>> + VIR_FREE(wwpn);
>> + VIR_FREE(fabric_wwn);
>> + VIR_FREE(max_vports);
>> + VIR_FREE(vports);
>> + return ret;
>> +}
>> +
>> +/* Test virGetFCHostNameByWWN */
>> +static int
>> +test4(const void *data ATTRIBUTE_UNUSED)
>> +{
>> + const char *expect_hostname = "host5";
>> + char *hostname = NULL;
>> + int ret = -1;
>> +
>> + if (!(hostname = virGetFCHostNameByWWN(TEST_FC_HOST_PREFIX,
>> + "2001001b32a9da4e",
>> + "2101001b32a9da4e")))
>> + return -1;
> Same NIT here.
>
>> +
>> + if (STRNEQ(hostname, expect_hostname))
>> + goto cleanup;
>> +
>> + ret = 0;
>> +cleanup:
>> + VIR_FREE(hostname);
>> + return ret;
>> +}
>> +
>> +/* Test virFindFCHostCapableVport (host4 is not Online) */
>> +static int
>> +test5(const void *data ATTRIBUTE_UNUSED)
>> +{
>> + const char *expect_hostname = "host5";
>> + char *hostname = NULL;
>> + int ret = -1;
>> +
>> + if (!(hostname = virFindFCHostCapableVport(TEST_FC_HOST_PREFIX)))
>> + return -1;
> Same NIT here.
>
>> +
>> + if (STRNEQ(hostname, expect_hostname))
>> + goto cleanup;
>> +
>> + ret = 0;
>> +cleanup:
>> + VIR_FREE(hostname);
>> + return ret;
>> +}
>> +
>> +static int
>> +mymain(void)
>> +{
>> + int ret = 0;
>> +
>> + if (virtTestRun("test1", 1, test1, NULL) < 0)
>> + ret = -1;
>> + if (virtTestRun("test2", 1, test2, NULL) < 0)
>> + ret = -1;
>> + if (virtTestRun("test3", 1, test3, NULL) < 0)
>> + ret = -1;
>> + if (virtTestRun("test4", 1, test4, NULL) < 0)
>> + ret = -1;
>> + if (virtTestRun("test5", 1, test5, NULL) < 0)
>> + ret = -1;
>> +
>> + return ret;
>> +}
>> +
>> +VIRT_TEST_MAIN(mymain)
>>
> ACK - again, not sure of "coding style" norm on those returns. They
> accomplish the same thing so I'm fine with things the way they are.
>
As replied in 4/7, personally I prefer to use the "return 0/-1" if
there is no need to goto cleanup, it's more clear to let one
known what's the return value is, especially when the code
body of a function is long enough.
I will send v2 to add the patches to cleanup the macros not
used anymore in src/node_device_driver.h, and use the enum
in the utils.
I misunderstood your meaning in 2/7, you meant the macros defined
for string, pushed the set and will post independant patches.