On 05/06/2013 08:45 AM, Osier Yang wrote:

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(-)

ACK for technically right; however, since this problem is in 1.0.4 is there an "effect" where there is a written buffer that has the shorter (and wrong) wwnn/wwpn that could cause "issues" on the read (and possible compare) side now?? John

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; }

On 05/06/2013 06:45 AM, Osier Yang wrote:

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();

Now that we have it, I'd write this as: if (VIR_STRDUP(result, p) < 0) goto cleanup; Of course, when backporting to v1.0.5-maint, you'll have to use the long-hand (unless we backport VIR_STRDUP first). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 05/06/2013 08:45 AM, Osier Yang wrote:

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

I'd like to make a couple of suggestions... This issue is/was because "LINUX_SYSFS_VPORT_CREATE_POSTFIX" didn't get refactored into the new code (e.g. "/vport_create") So why not create a "SYSFS_VPORT_CREATE_POSTFIX" symbol in virutil.c like there is a refactored "SYSFS_FC_HOST_PATH" symbol... Then since the "LINUX_" versions of those symbols in src/node_device/node_device_driver.h are no longer used, they could be removed from there as well. There's also the "vport_delete" symbol that could get the same treatment. John

--- 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 08/05/13 21:38, John Ferlan wrote:

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

I'd think this is personal habit, and actually we use it across the source.

...

}

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;

Oops...

...

@@ -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;

Okay.

...

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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 05/06/2013 08:45 AM, Osier Yang wrote:

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(-)

Seems right, although it does make me wonder why the choice was made initially to use NULL instead... That is could some other assumption now be invalidated? Furthermore, are the calls from detect_scsi_host_caps() still valid with a NULL? If NULL ends up being an invalid value completely, then you should also make the change to add the ATTRIBUTE_NONNULL(1) to the definition. Should you "separate" the virIsCapableVport and virReadFCHost changes? John

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 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. Thanks for the reviewing. Osier

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. Gene

On 05/14/2013 01:42 PM, Osier Yang wrote:

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 Thanks!

Gene