Introduced by commit 244ce462e29, which refactored the helper for wwn reading, however, it forgot to change the old "strndup" and "sizeof(buf)", "sizeof(buf)" operates on the fixed length array ("buf") in the old code, but now "buf" is a pointer. Before the fix: % virsh nodedev-dumpxml scsi_host5 <device> <name>scsi_host5</name> <parent>pci_0000_04_00_1</parent> <capability type='scsi_host'> <host>5</host> <capability type='fc_host'> <wwnn>2001001b</wwnn> <wwpn>2101001b</wwpn> <fabric_wwn>2001000d</fabric_wwn> </capability> </capability> </device> With the fix: % virsh nodedev-dumpxml scsi_host5 <device> <name>scsi_host5</name> <parent>pci_0000_04_00_1</parent> <capability type='scsi_host'> <host>5</host> <capability type='fc_host'> <wwnn>0x2001001b32a9da4e</wwnn> <wwpn>0x2101001b32a9da4e</wwpn> <fabric_wwn>0x2001000dec9877c1</fabric_wwn> </capability> </capability> </device> --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virutil.c b/src/util/virutil.c index 982d4a3..7773d5c 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -3121,7 +3121,7 @@ virReadFCHost(const char *sysfs_prefix, else p = buf; - if (!(*result = strndup(p, sizeof(buf)))) { + if (!(*result = strdup(p))) { virReportOOMError(); goto cleanup; }

Introduced by commit 244ce462e29, which refactored the helper for wwn reading, however, it forgot to change the old "strndup" and "sizeof(buf)", "sizeof(buf)" operates on the fixed length array ("buf") in the old code, but now "buf" is a pointer.

+++ b/src/util/virutil.c @@ -3121,7 +3121,7 @@ virReadFCHost(const char *sysfs_prefix, else p = buf; - if (!(*result = strndup(p, sizeof(buf)))) { + if (!(*result = strdup(p))) { virReportOOMError();

Which refactored the old code, and introduced new helper virIsCapableVport, but the path for checking with access() is not correctly constructed.

--- src/util/virutil.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index 7773d5c..ebf902d 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -3163,7 +3163,7 @@ virIsCapableVport(const char *sysfs_prefix, int ret = -1; if (virAsprintf(&fc_host_path, - "%shost%d%s", + "%shost%d/%s", sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH, host, "vport_create") < 0) { @@ -3172,7 +3172,7 @@ virIsCapableVport(const char *sysfs_prefix, } if (virAsprintf(&scsi_host_path, - "%shost%d%s", + "%shost%d/%s", sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_HOST_PATH, host, "vport_create") < 0) {

On 05/06/2013 08:45 AM, Osier Yang wrote: > Function name with "aIsB" generally means its return value is > in Bi-state (true/false). > --- > src/node_device/node_device_linux_sysfs.c | 4 ++-- > src/util/virutil.c | 18 +++++++++--------- > src/util/virutil.h | 4 ++-- > 3 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c > index 5228a01..cb2f86e 100644 > --- a/src/node_device/node_device_linux_sysfs.c > +++ b/src/node_device/node_device_linux_sysfs.c > @@ -47,7 +47,7 @@ detect_scsi_host_caps(union _virNodeDevCapData *d) > > VIR_DEBUG("Checking if host%d is an FC HBA", d->scsi_host.host); > > - if (virIsCapableFCHost(NULL, d->scsi_host.host) == 0) { > + if (virIsCapableFCHost(NULL, d->scsi_host.host)) { > d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST; > > if (virReadFCHost(NULL, > @@ -76,7 +76,7 @@ detect_scsi_host_caps(union _virNodeDevCapData *d) > } > } > > - if (virIsCapableVport(NULL, d->scsi_host.host) == 0) { > + if (virIsCapableVport(NULL, d->scsi_host.host)) { > d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS; > > if (virReadFCHost(NULL, > diff --git a/src/util/virutil.c b/src/util/virutil.c > index da87897..dfbef06 100644 > --- a/src/util/virutil.c > +++ b/src/util/virutil.c > @@ -3133,12 +3133,12 @@ cleanup: > return ret; > } > > -int > +bool > virIsCapableFCHost(const char *sysfs_prefix, > int host) > { > char *sysfs_path = NULL; > - int ret = -1; > + bool ret = false; > > if (virAsprintf(&sysfs_path, "%s/host%d", > sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH, Doesn't this return -1 here? after the virReportOOMError() So you'd need a return ret; instead > @@ -3148,19 +3148,19 @@ virIsCapableFCHost(const char *sysfs_prefix, > } > > if (access(sysfs_path, F_OK) == 0) > - ret = 0; > + ret = true; > > VIR_FREE(sysfs_path); > return ret; > } > > -int > +bool > virIsCapableVport(const char *sysfs_prefix, > int host) > { > char *scsi_host_path = NULL; > char *fc_host_path = NULL; > - int ret = -1; > + int ret = false; > > if (virAsprintf(&fc_host_path, > "%s/host%d/%s", > @@ -3168,7 +3168,7 @@ virIsCapableVport(const char *sysfs_prefix, > host, > "vport_create") < 0) { > virReportOOMError(); > - return -1; > + return false; s/false/ret or goto cleanup; instead

> } > > if (virAsprintf(&scsi_host_path, > @@ -3182,7 +3182,7 @@ virIsCapableVport(const char *sysfs_prefix, > > if ((access(fc_host_path, F_OK) == 0) || > (access(scsi_host_path, F_OK) == 0)) > - ret = 0; > + ret = true; > > cleanup: > VIR_FREE(fc_host_path); > @@ -3458,7 +3458,7 @@ virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, > return -1; > } > > -int > +bool > virIsCapableFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, > int host ATTRIBUTE_UNUSED) > { You forgot the: s/return -1;/return false;

> @@ -3466,7 +3466,7 @@ virIsCapableFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED, > return -1; > } > > -int > +bool > virIsCapableVport(const char *sysfs_prefix ATTRIBUTE_UNUSED, > int host ATTRIBUTE_UNUSED) > { You forgot the: s/return -1;/return false;

> diff --git a/src/util/virutil.h b/src/util/virutil.h > index 8a2d25c..0e45ac7 100644 > --- a/src/util/virutil.h > +++ b/src/util/virutil.h > @@ -254,8 +254,8 @@ int virReadFCHost(const char *sysfs_prefix, > char **result) > ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); > > -int virIsCapableFCHost(const char *sysfs_prefix, int host); > -int virIsCapableVport(const char *sysfs_prefix, int host); > +bool virIsCapableFCHost(const char *sysfs_prefix, int host); > +bool virIsCapableVport(const char *sysfs_prefix, int host); > > enum { > VPORT_CREATE, > ACK w/ changes John -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

The helper works for default sysfs_prefix, but for user specified prefix, it doesn't work. (Detected when writing test cases. A later patch will add the test cases for fc_host). --- src/util/virutil.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/util/virutil.c b/src/util/virutil.c index c83996d..2373f1f 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -3367,7 +3367,8 @@ cleanup: /* virFindFCHostCapableVport: * * Iterate over the sysfs and find out the first online HBA which - * supports vport, and not saturated,. + * supports vport, and not saturated. Returns the host name (e.g. + * host5) on success, or NULL on failure. */ char * virFindFCHostCapableVport(const char *sysfs_prefix) @@ -3401,10 +3402,10 @@ virFindFCHostCapableVport(const char *sysfs_prefix) continue; } - if (!virIsCapableVport(NULL, host)) + if (!virIsCapableVport(prefix, host)) continue; - if (virReadFCHost(NULL, host, "port_state", &state) < 0) { + if (virReadFCHost(prefix, host, "port_state", &state) < 0) { VIR_DEBUG("Failed to read port_state for host%d", host); continue; } @@ -3416,12 +3417,12 @@ virFindFCHostCapableVport(const char *sysfs_prefix) } VIR_FREE(state); - if (virReadFCHost(NULL, host, "max_npiv_vports", &max_vports) < 0) { + if (virReadFCHost(prefix, host, "max_npiv_vports", &max_vports) < 0) { VIR_DEBUG("Failed to read max_npiv_vports for host%d", host); continue; } - if (virReadFCHost(NULL, host, "npiv_vports_inuse", &vports) < 0) { + if (virReadFCHost(prefix, host, "npiv_vports_inuse", &vports) < 0) { VIR_DEBUG("Failed to read npiv_vports_inuse for host%d", host); VIR_FREE(max_vports); continue;

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.

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.

On 15/05/13 00:35, Gene Czarcinski wrote: > 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. Fixed by commit 1cc8259bfe17be51eb1