On 04/13/2015 11:11 AM, John Ferlan wrote:
On 04/03/2015 08:36 AM, Michal Privoznik wrote:
> This is yet another test for check of basic functionality of our
> NIC state handling code.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virnetdev.c | 4 +-
> src/util/virnetdev.h | 4 ++
> tests/Makefile.am | 15 +++++
> tests/virnetdevmock.c | 48 ++++++++++++++
> tests/virnetdevtest.c | 94 +++++++++++++++++++++++++++
> tests/virnetdevtestdata/eth0-broken/operstate | 1 +
> tests/virnetdevtestdata/eth0-broken/speed | 1 +
> tests/virnetdevtestdata/eth0/operstate | 1 +
> tests/virnetdevtestdata/eth0/speed | 1 +
> tests/virnetdevtestdata/lo/operstate | 1 +
> tests/virnetdevtestdata/lo/speed | 1 +
> 12 files changed, 170 insertions(+), 2 deletions(-)
> create mode 100644 tests/virnetdevmock.c
> create mode 100644 tests/virnetdevtest.c
> create mode 100644 tests/virnetdevtestdata/eth0-broken/operstate
> create mode 100644 tests/virnetdevtestdata/eth0-broken/speed
> create mode 100644 tests/virnetdevtestdata/eth0/operstate
> create mode 100644 tests/virnetdevtestdata/eth0/speed
> create mode 100644 tests/virnetdevtestdata/lo/operstate
> create mode 100644 tests/virnetdevtestdata/lo/speed
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 9f82926..0b42238 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1741,6 +1741,7 @@ virNetDevSetPromiscuous;
> virNetDevSetRcvAllMulti;
> virNetDevSetRcvMulti;
> virNetDevSetupControl;
> +virNetDevSysfsFile;
> virNetDevValidateConfig;
>
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 54d866e..a2d55a8 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1519,9 +1519,9 @@ int virNetDevValidateConfig(const char *ifname
ATTRIBUTE_UNUSED,
> #ifdef __linux__
> # define NET_SYSFS "/sys/class/net/"
>
> -static int
> +int
> virNetDevSysfsFile(char **pf_sysfs_device_link, const char *ifname,
> - const char *file)
> + const char *file)
> {
>
> if (virAsprintf(pf_sysfs_device_link, NET_SYSFS "%s/%s", ifname, file)
< 0)
Not related specifically to this change, but there seems to be four more
places which make up their own 'version' of this type of logic - two use
'SYSFS_NET_DIR' instead of 'NET_SYSFS' and two use the raw
'/sys/class/net' path... Might be "nice" to have them use this
function
now too.
Well, the other two uses have parameters that would need to be passed in
printf-style, so that complicates any standard function. Not that I
would mind such a thing, but an in-between solution could be to provide
the #define of the base directory in a .h file, or a function that
returns that string.
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index 856127b..999a89a 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -219,4 +219,8 @@ int virNetDevSetRcvAllMulti(const char *ifname, bool receive)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> int virNetDevGetRcvAllMulti(const char *ifname, bool *receive)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +int virNetDevSysfsFile(char **pf_sysfs_device_link,
> + const char *ifname,
> + const char *file)
> + ATTRIBUTE_NONNULL(1);
Seems (2) and (3) should be checked as well..
"checked" seems like an incorrect description to me - I thought that the
function of ATTRIBUTE_NONNULL ended up being exactly the opposite of
that - if you mark an arg as ATTRIBUTE_NONNULL then the code within the
function (and more importantly, static analyzers) can assume that the
arg will always be non-null, so no checking is required, i.e. it is a
more of an optimization aid than a debugging aid. And as a matter of
fact, if you turn on optimization in gcc, any code in a function that
checks for NULL in an ATTRIBUTE_NONNULL arg *will be removed* (or at
least it *would have* without the 2nd patch I point out below).
In my experience, ATTRIBUTE_NONNULL has done more to obscure failure to
check for non-null than to point it out:
https://www.redhat.com/archives/libvir-list/2012-April/msg01370.html
although I *think* this patch from 2012 eliminated that failing:
https://www.redhat.com/archives/libvir-list/2012-April/msg01376.html
so maybe I'm blathering on about nothing ;-)
Also, checked current callers and found all have whatever gets used as
arg (2) and (3) checked for non null, except for one...
The virNetDevGetVirtualFunctionInfo() prototype doesn't make sure that
it's arg (2) is non-null, but goes ahead and derefs it right away...
> #endif /* __VIR_NETDEV_H__ */
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 046cd08..9ebedc3 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -176,6 +176,7 @@ test_programs = virshtest sockettest \
> domainconftest \
> virhostdevtest \
> vircaps2xmltest \
> + virnetdevtest \
> $(NULL)
>
> if WITH_REMOTE
> @@ -402,6 +403,7 @@ test_libraries = libshunload.la \
> virnetserverclientmock.la \
> vircgroupmock.la \
> virpcimock.la \
> + virnetdevmock.la \
> $(NULL)
> if WITH_QEMU
> test_libraries += libqemumonitortestutils.la \
> @@ -1029,6 +1031,19 @@ virpcimock_la_LIBADD = $(GNULIB_LIBS) \
> virpcimock_la_LDFLAGS = -module -avoid-version \
> -rpath /evil/libtool/hack/to/force/shared/lib/creation
>
> +virnetdevtest_SOURCES = \
> + virnetdevtest.c testutils.h testutils.c
> +virnetdevtest_CFLAGS = $(AM_CFLAGS) $(LIBNL_CFLAGS)
> +virnetdevtest_LDADD = $(LDADDS)
> +
> +virnetdevmock_la_SOURCES = \
> + virnetdevmock.c
> +virnetdevmock_la_CFLAGS = $(AM_CFLAGS) $(LIBNL_CFLAGS)
> +virnetdevmock_la_LIBADD = $(GNULIB_LIBS) \
> + ../src/libvirt.la
> +virnetdevmock_la_LDFLAGS = -module -avoid-version \
> + -rpath /evil/libtool/hack/to/force/shared/lib/creation
> +
> if WITH_LINUX
> virusbtest_SOURCES = \
> virusbtest.c testutils.h testutils.c
> diff --git a/tests/virnetdevmock.c b/tests/virnetdevmock.c
> new file mode 100644
> index 0000000..681e5fe
> --- /dev/null
> +++ b/tests/virnetdevmock.c
> @@ -0,0 +1,48 @@
> +/*
> + * Copyright (C) 2013 Red Hat, Inc.
Been sitting on this awhile?
> + *
> + * 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/>.
> + *
> + * Author: Michal Privoznik <mprivozn(a)redhat.com>
> + */
> +
> +#include <config.h>
> +
> +#ifdef __linux__
> +# include "internal.h"
> +# include <stdlib.h>
> +# include <stdio.h>
> +# include "virstring.h"
> +# include "virnetdev.h"
> +
> +# define NET_DEV_TEST_DATA_PREFIX abs_srcdir "/virnetdevtestdata"
Maybe this could be abs_srcdir "/virnetdevtestdata/sys/class/net/" so
that when/if the tests are expanded to use anything outside of
/sys/class/net, all of the test data could be added under the same base
directory.
> +
> +int
> +virNetDevSysfsFile(char **pf_sysfs_device_link,
> + const char *ifname,
> + const char *file)
> +{
> +
> + if (virAsprintfQuiet(pf_sysfs_device_link, "%s/%s/%s",
> + NET_DEV_TEST_DATA_PREFIX, ifname, file) < 0) {
> + fprintf(stderr, "Out of memory\n");
> + abort();
> + }
> +
> + return 0;
> +}
> +#else
> +/* Nothing to override on non-__linux__ platforms */
> +#endif
> diff --git a/tests/virnetdevtest.c b/tests/virnetdevtest.c
> new file mode 100644
> index 0000000..8795bf1
> --- /dev/null
> +++ b/tests/virnetdevtest.c
> @@ -0,0 +1,94 @@
> +/*
> + * Copyright (C) 2014 Red Hat, Inc.
Ditto
ACK for what's here with some minor adjustments - extra points for the
additional changes to use the now non static function ;-)
John
> + *
> + * 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/>.
> + *
> + * Author: Michal Privoznik <mprivozn(a)redhat.com>
> + */
> +
> +#include <config.h>
> +
> +#include "testutils.h"
> +
> +#ifdef __linux__
> +
> +# include "virnetdev.h"
> +
> +# define VIR_FROM_THIS VIR_FROM_NONE
> +
> +struct testVirNetDevGetLinkInfoData {
> + const char *ifname; /* ifname to get info on */
> + virInterfaceState state; /* expected state */
> + unsigned int speed; /* expected speed */
> +};
> +
> +static int
> +testVirNetDevGetLinkInfo(const void *opaque)
> +{
> + int ret = -1;
> + const struct testVirNetDevGetLinkInfoData *data = opaque;
> + virInterfaceLink lnk;
> +
> + if (virNetDevGetLinkInfo(data->ifname, &lnk) < 0)
> + goto cleanup;
> +
> + if (lnk.state != data->state) {
> + fprintf(stderr,
> + "Fetched link state (%s) doesn't match the expected one
(%s)",
> + virInterfaceStateTypeToString(lnk.state),
> + virInterfaceStateTypeToString(data->state));
> + goto cleanup;
> + }
> +
> + if (lnk.speed != data->speed) {
> + fprintf(stderr,
> + "Fetched link speed (%u) doesn't match the expected one
(%u)",
> + lnk.speed, data->speed);
> + goto cleanup;
> + }
> +
> + ret = 0;
> + cleanup:
> + return ret;
> +}
> +
> +static int
> +mymain(void)
> +{
> + int ret = 0;
> +
> +# define DO_TEST_LINK(ifname, state, speed) \
> + do { \
> + struct testVirNetDevGetLinkInfoData data = {ifname, state, speed}; \
> + if (virtTestRun("Link info: " # ifname,
\
> + testVirNetDevGetLinkInfo, &data) < 0)
\
> + ret = -1; \
> + } while (0)
> +
> + DO_TEST_LINK("eth0", VIR_INTERFACE_STATE_UP, 1000);
> + DO_TEST_LINK("lo", VIR_INTERFACE_STATE_UNKNOWN, 0);
> + DO_TEST_LINK("eth0-broken", VIR_INTERFACE_STATE_DOWN, 0);
> +
> + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
> +}
> +
> +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virnetdevmock.so")
> +#else
> +int
> +main(void)
> +{
> + return EXIT_AM_SKIP;
> +}
> +#endif
> diff --git a/tests/virnetdevtestdata/eth0-broken/operstate
b/tests/virnetdevtestdata/eth0-broken/operstate
> new file mode 100644
> index 0000000..eb0e904
> --- /dev/null
> +++ b/tests/virnetdevtestdata/eth0-broken/operstate
> @@ -0,0 +1 @@
> +down
> diff --git a/tests/virnetdevtestdata/eth0-broken/speed
b/tests/virnetdevtestdata/eth0-broken/speed
> new file mode 100644
> index 0000000..4f6ff86
> --- /dev/null
> +++ b/tests/virnetdevtestdata/eth0-broken/speed
> @@ -0,0 +1 @@
> +4294967295
> diff --git a/tests/virnetdevtestdata/eth0/operstate
b/tests/virnetdevtestdata/eth0/operstate
> new file mode 100644
> index 0000000..e31ee94
> --- /dev/null
> +++ b/tests/virnetdevtestdata/eth0/operstate
> @@ -0,0 +1 @@
> +up
> diff --git a/tests/virnetdevtestdata/eth0/speed b/tests/virnetdevtestdata/eth0/speed
> new file mode 100644
> index 0000000..83b33d2
> --- /dev/null
> +++ b/tests/virnetdevtestdata/eth0/speed
> @@ -0,0 +1 @@
> +1000
> diff --git a/tests/virnetdevtestdata/lo/operstate
b/tests/virnetdevtestdata/lo/operstate
> new file mode 100644
> index 0000000..3546645
> --- /dev/null
> +++ b/tests/virnetdevtestdata/lo/operstate
> @@ -0,0 +1 @@
> +unknown
> diff --git a/tests/virnetdevtestdata/lo/speed b/tests/virnetdevtestdata/lo/speed
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/virnetdevtestdata/lo/speed
> @@ -0,0 +1 @@
> +0
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list