Re: [libvirt] [PATCH 7/7] tests: Add tests for fc_host

On 05/13/2013 05:32 AM, Osier Yang wrote: > 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. NOTE!! Your test adds data but DOES NOT update Makefile.am Therefore. when you create a dist tarball and run the tests, the fchosttest will fail because it cannot find its data because the data is NOT included in the tarball.