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

[Osier Yang <jyang@xxxxxxxxxx>]

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list