[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Introduce virnetdevtest



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]