[libvirt] [PATCH 0/2] Implement interface stats for BSD
This series implements support for querying network interface stats on (Free)BSD. It's more of an RFC, because I'm uncertain about few things: - It feels a little strange to have a source file that implements only a single function like this. I am wondering if it would be better to just move it to something like util/virnetdev.c? - FreeBSD stores interface data in the if_data struct and a number of outgoing packet drops is stored in a field 'ifi_oqdrops'. This field was added in -CURRENT and later merged back to 10-STABLE. In order not to break the ABI, it's available only if _IFI_OQDROPS is defined. I've added a configure.ac check which adds -D_IFI_OQDROPS before checking this field and resetting it back if it is not present. This way, this flag will present when the field is available even if the flag is not needed (e.g. on -CURRENT). Is there a better way of doing it? I was thinking about trying to check this field without the flag and if it fails check one more time with the flag, but it looks a little messy. - Did I get it right that the stats reported are from the guest POV, e.g. when downloading a large file from guest, it should look like: vnet0 rx_bytes 731603341 vnet0 rx_packets 518354 vnet0 rx_errs 0 vnet0 rx_drop 0 vnet0 tx_bytes 17577834 vnet0 tx_packets 264226 vnet0 tx_errs 0 vnet0 tx_drop 0 Roman Bogorodskiy (2): util: virstatslinux: make more generic Implement interface stats for BSD configure.ac | 13 ++++- po/POTFILES.in | 2 +- src/Makefile.am | 2 +- src/libvirt_linux.syms | 3 -- src/libvirt_private.syms | 2 + src/lxc/lxc_driver.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/qemu/qemu_driver.c | 16 +----- src/uml/uml_driver.c | 2 +- src/util/{virstatslinux.c => virstats.c} | 93 +++++++++++++++++++++++++------- src/util/{virstatslinux.h => virstats.h} | 12 ++--- src/xen/xen_hypervisor.c | 2 +- tests/statstest.c | 2 +- 13 files changed, 102 insertions(+), 51 deletions(-) rename src/util/{virstatslinux.c => virstats.c} (61%) rename src/util/{virstatslinux.h => virstats.h} (77%) -- 1.9.0
Rename linuxDomainInterfaceStats to virNetInterfaceStats in order to allow adding platform specific implementations without making consumer worrying about specific implementation to be used. Also, rename util/virstatslinux.c to util/virstats.c so placing other platform specific implementations into this file don't look unexpected from the file name. --- po/POTFILES.in | 2 +- src/Makefile.am | 2 +- src/libvirt_linux.syms | 3 --- src/libvirt_private.syms | 2 ++ src/lxc/lxc_driver.c | 4 +-- src/openvz/openvz_driver.c | 4 +-- src/qemu/qemu_driver.c | 16 ++---------- src/uml/uml_driver.c | 2 +- src/util/{virstatslinux.c => virstats.c} | 44 ++++++++++++++++++-------------- src/util/{virstatslinux.h => virstats.h} | 12 +++------ src/xen/xen_hypervisor.c | 4 +-- tests/statstest.c | 2 +- 12 files changed, 43 insertions(+), 54 deletions(-) rename src/util/{virstatslinux.c => virstats.c} (82%) rename src/util/{virstatslinux.h => virstats.h} (77%) diff --git a/po/POTFILES.in b/po/POTFILES.in index 64a987e..fd4b56e 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -198,7 +198,7 @@ src/util/virrandom.c src/util/virsexpr.c src/util/virscsi.c src/util/virsocketaddr.c -src/util/virstatslinux.c +src/util/virstats.c src/util/virstorageencryption.c src/util/virstoragefile.c src/util/virstring.c diff --git a/src/Makefile.am b/src/Makefile.am index e2f76a7..a40e63f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -149,7 +149,7 @@ UTIL_SOURCES = \ util/virseclabel.c util/virseclabel.h \ util/virsexpr.c util/virsexpr.h \ util/virsocketaddr.h util/virsocketaddr.c \ - util/virstatslinux.c util/virstatslinux.h \ + util/virstats.c util/virstats.h \ util/virstorageencryption.c util/virstorageencryption.h \ util/virstoragefile.c util/virstoragefile.h \ util/virstring.h util/virstring.c \ diff --git a/src/libvirt_linux.syms b/src/libvirt_linux.syms index b3b2384..1a7f263 100644 --- a/src/libvirt_linux.syms +++ b/src/libvirt_linux.syms @@ -6,9 +6,6 @@ linuxNodeGetCPUStats; linuxNodeInfoCPUPopulate; -# util/virstatslinux.h -linuxDomainInterfaceStats; - # Let emacs know we want case-insensitive sorting # Local Variables: # sort-fold-case: t diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 29e9db9..1aca260 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1880,6 +1880,8 @@ virSocketAddrPrefixToNetmask; virSocketAddrSetIPv4Addr; virSocketAddrSetPort; +# util/virstats.h +virNetInterfaceStats; # util/virstorageencryption.h virStorageEncryptionFormat; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index fce16f2..fe1e555 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -51,7 +51,7 @@ #include "virnetdevveth.h" #include "nodeinfo.h" #include "viruuid.h" -#include "virstatslinux.h" +#include "virstats.h" #include "virhook.h" #include "virfile.h" #include "virpidfile.h" @@ -3092,7 +3092,7 @@ lxcDomainInterfaceStats(virDomainPtr dom, } if (ret == 0) - ret = linuxDomainInterfaceStats(path, stats); + ret = virNetInterfaceStats(path, stats); else virReportError(VIR_ERR_INVALID_ARG, _("Invalid path, '%s' is not a known interface"), path); diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index baa7256..851ed30 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -56,7 +56,7 @@ #include "virlog.h" #include "vircommand.h" #include "viruri.h" -#include "virstatslinux.h" +#include "virstats.h" #include "virstring.h" #define VIR_FROM_THIS VIR_FROM_OPENVZ @@ -2010,7 +2010,7 @@ openvzDomainInterfaceStats(virDomainPtr dom, } if (ret == 0) - ret = linuxDomainInterfaceStats(path, stats); + ret = virNetInterfaceStats(path, stats); else virReportError(VIR_ERR_INVALID_ARG, _("invalid path, '%s' is not a known interface"), path); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2d1aa9e..027a7ef 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -61,7 +61,7 @@ #include "datatypes.h" #include "virbuffer.h" #include "nodeinfo.h" -#include "virstatslinux.h" +#include "virstats.h" #include "capabilities.h" #include "viralloc.h" #include "viruuid.h" @@ -9760,7 +9760,6 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, return ret; } -#ifdef __linux__ static int qemuDomainInterfaceStats(virDomainPtr dom, const char *path, @@ -9792,7 +9791,7 @@ qemuDomainInterfaceStats(virDomainPtr dom, } if (ret == 0) - ret = linuxDomainInterfaceStats(path, stats); + ret = virNetInterfaceStats(path, stats); else virReportError(VIR_ERR_INVALID_ARG, _("invalid path, '%s' is not a known interface"), path); @@ -9802,17 +9801,6 @@ qemuDomainInterfaceStats(virDomainPtr dom, virObjectUnlock(vm); return ret; } -#else -static int -qemuDomainInterfaceStats(virDomainPtr dom ATTRIBUTE_UNUSED, - const char *path ATTRIBUTE_UNUSED, - struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) -{ - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("interface stats not implemented on this platform")); - return -1; -} -#endif static int qemuDomainSetInterfaceParameters(virDomainPtr dom, diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 62d1afe..7039afc 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -47,7 +47,7 @@ #include "uml_conf.h" #include "virbuffer.h" #include "nodeinfo.h" -#include "virstatslinux.h" +#include "virstats.h" #include "capabilities.h" #include "viralloc.h" #include "viruuid.h" diff --git a/src/util/virstatslinux.c b/src/util/virstats.c similarity index 82% rename from src/util/virstatslinux.c rename to src/util/virstats.c index 60b72dc..17ef5b6 100644 --- a/src/util/virstatslinux.c +++ b/src/util/virstats.c @@ -1,5 +1,5 @@ /* - * virstatslinux.c: Linux block and network stats. + * virstats.c: Block and network stats. * * Copyright (C) 2007-2010 Red Hat, Inc. * @@ -22,23 +22,20 @@ #include <config.h> -/* This file only applies on Linux. */ -#ifdef __linux__ - -# include <stdio.h> -# include <stdlib.h> -# include <fcntl.h> -# include <string.h> -# include <unistd.h> -# include <regex.h> +#include <stdio.h> +#include <stdlib.h> +#include <fcntl.h> +#include <string.h> +#include <unistd.h> +#include <regex.h> -# include "virerror.h" -# include "datatypes.h" -# include "virstatslinux.h" -# include "viralloc.h" -# include "virfile.h" +#include "virerror.h" +#include "datatypes.h" +#include "virstats.h" +#include "viralloc.h" +#include "virfile.h" -# define VIR_FROM_THIS VIR_FROM_STATS_LINUX +#define VIR_FROM_THIS VIR_FROM_STATS_LINUX /*-------------------- interface stats --------------------*/ @@ -46,10 +43,10 @@ * NB. Caller must check that libvirt user is trying to query * the interface of a domain they own. We do no such checking. */ - +#ifdef __linux__ int -linuxDomainInterfaceStats(const char *path, - struct _virDomainInterfaceStats *stats) +virNetInterfaceStats(const char *path, + struct _virDomainInterfaceStats *stats) { int path_len; FILE *fp; @@ -117,5 +114,14 @@ linuxDomainInterfaceStats(const char *path, _("/proc/net/dev: Interface not found")); return -1; } +#else +int +virNetInterfaceStats(const char *path ATTRIBUTE_UNUSED, + struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("interface stats not implemented on this platform")); + return -1; +} #endif /* __linux__ */ diff --git a/src/util/virstatslinux.h b/src/util/virstats.h similarity index 77% rename from src/util/virstatslinux.h rename to src/util/virstats.h index d5900ed..9724d8e 100644 --- a/src/util/virstatslinux.h +++ b/src/util/virstats.h @@ -1,5 +1,5 @@ /* - * virstatslinux.h: Linux block and network stats. + * virstats.h: Block and network stats. * * Copyright (C) 2007 Red Hat, Inc. * @@ -23,13 +23,9 @@ #ifndef __STATS_LINUX_H__ # define __STATS_LINUX_H__ -# ifdef __linux__ +# include "internal.h" -# include "internal.h" - -extern int linuxDomainInterfaceStats(const char *path, - struct _virDomainInterfaceStats *stats); - -# endif /* __linux__ */ +extern int virNetInterfaceStats(const char *path, + struct _virDomainInterfaceStats *stats); #endif /* __STATS_LINUX_H__ */ diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 524b21f..af24e2e 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -68,7 +68,7 @@ #include "xen_driver.h" #include "xen_hypervisor.h" #include "xs_internal.h" -#include "virstatslinux.h" +#include "virstats.h" #include "block_stats.h" #include "xend_internal.h" #include "virbuffer.h" @@ -1470,7 +1470,7 @@ xenHypervisorDomainInterfaceStats(virDomainDefPtr def, return -1; } - return linuxDomainInterfaceStats(path, stats); + return virNetInterfaceStats(path, stats); #else virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("/proc/net/dev: Interface not found")); diff --git a/tests/statstest.c b/tests/statstest.c index ce1567c..f9b4573 100644 --- a/tests/statstest.c +++ b/tests/statstest.c @@ -5,7 +5,7 @@ #include <string.h> #include <sys/utsname.h> -#include "virstatslinux.h" +#include "virstats.h" #include "internal.h" #include "xen/block_stats.h" #include "testutils.h" -- 1.9.0
--- configure.ac | 13 ++++++++++++- src/util/virstats.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index a12186b..8001e24 100644 --- a/configure.ac +++ b/configure.ac @@ -275,7 +275,7 @@ dnl and various less common threadsafe functions AC_CHECK_FUNCS_ONCE([cfmakeraw fallocate geteuid getgid getgrnam_r \ getmntent_r getpwuid_r getuid kill mmap newlocale posix_fallocate \ posix_memalign prlimit regexec sched_getaffinity setgroups setns \ - setrlimit symlink sysctlbyname]) + setrlimit symlink sysctlbyname getifaddrs]) dnl Availability of pthread functions. Because of $LIB_PTHREAD, we dnl cannot use AC_CHECK_FUNCS_ONCE. LIB_PTHREAD and LIBMULTITHREAD @@ -2691,6 +2691,17 @@ if test $with_freebsd = yes; then ) fi +# FreeBSD 10-STABLE requires _IFI_OQDROPS to be defined for if_data.ifi_oqdrops +# field be available +old_CFLAGS="$CFLAGS" +CFLAGS="$CFLAGS -D_IFI_OQDROPS" + +AC_CHECK_MEMBERS([struct if_data.ifi_oqdrops], + [], + [CFLAGS="$old_CFLAGS"], + [#include <net/if.h> + ]) + # Check if we need to look for ifconfig if test "$want_ifconfig" = "yes"; then AC_PATH_PROG([IFCONFIG_PATH], [ifconfig]) diff --git a/src/util/virstats.c b/src/util/virstats.c index 17ef5b6..910803f 100644 --- a/src/util/virstats.c +++ b/src/util/virstats.c @@ -29,6 +29,11 @@ #include <unistd.h> #include <regex.h> +#ifdef HAVE_GETIFADDRS +# include <net/if.h> +# include <ifaddrs.h> +#endif + #include "virerror.h" #include "datatypes.h" #include "virstats.h" @@ -114,6 +119,52 @@ virNetInterfaceStats(const char *path, _("/proc/net/dev: Interface not found")); return -1; } +#elif defined(HAVE_GETIFADDRS) +int +virNetInterfaceStats(const char *path, + struct _virDomainInterfaceStats *stats) +{ + struct ifaddrs *ifap, *ifa; + struct if_data *ifd; + int ret = -1; + + if (getifaddrs(&ifap) < 0) { + virReportSystemError(errno, "%s", + _("Could not get interface list")); + return -1; + } + + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { + if (ifa->ifa_addr->sa_family != AF_LINK) + continue; + + if (STREQ(ifa->ifa_name, path)) { + ifd = (struct if_data *)ifa->ifa_data; + stats->tx_bytes = ifd->ifi_ibytes; + stats->tx_packets = ifd->ifi_ipackets; + stats->tx_errs = ifd->ifi_ierrors; + stats->tx_drop = ifd->ifi_iqdrops; + stats->rx_bytes = ifd->ifi_obytes; + stats->rx_packets = ifd->ifi_opackets; + stats->rx_errs = ifd->ifi_oerrors; +# ifdef HAVE_STRUCT_IF_DATA_IFI_OQDROPS + stats->rx_drop = ifd->ifi_oqdrops; +# else + stats->rx_drop = 0; +# endif + + ret = 0; + break; + } + } + + if (ret < 0) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Interface not found")); + + freeifaddrs(ifap); + return ret; +} #else int virNetInterfaceStats(const char *path ATTRIBUTE_UNUSED, -- 1.9.0
On 06.07.2014 18:28, Roman Bogorodskiy wrote:
This series implements support for querying network interface stats on (Free)BSD.
It's more of an RFC, because I'm uncertain about few things:
- It feels a little strange to have a source file that implements only a single function like this. I am wondering if it would be better to just move it to something like util/virnetdev.c? - FreeBSD stores interface data in the if_data struct and a number of outgoing packet drops is stored in a field 'ifi_oqdrops'. This field was added in -CURRENT and later merged back to 10-STABLE. In order not to break the ABI, it's available only if _IFI_OQDROPS is defined. I've added a configure.ac check which adds -D_IFI_OQDROPS before checking this field and resetting it back if it is not present. This way, this flag will present when the field is available even if the flag is not needed (e.g. on -CURRENT). Is there a better way of doing it? I was thinking about trying to check this field without the flag and if it fails check one more time with the flag, but it looks a little messy. - Did I get it right that the stats reported are from the guest POV, e.g. when downloading a large file from guest, it should look like:
vnet0 rx_bytes 731603341 vnet0 rx_packets 518354 vnet0 rx_errs 0 vnet0 rx_drop 0 vnet0 tx_bytes 17577834 vnet0 tx_packets 264226 vnet0 tx_errs 0 vnet0 tx_drop 0
Roman Bogorodskiy (2): util: virstatslinux: make more generic Implement interface stats for BSD
configure.ac | 13 ++++- po/POTFILES.in | 2 +- src/Makefile.am | 2 +- src/libvirt_linux.syms | 3 -- src/libvirt_private.syms | 2 + src/lxc/lxc_driver.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/qemu/qemu_driver.c | 16 +----- src/uml/uml_driver.c | 2 +- src/util/{virstatslinux.c => virstats.c} | 93 +++++++++++++++++++++++++------- src/util/{virstatslinux.h => virstats.h} | 12 ++--- src/xen/xen_hypervisor.c | 2 +- tests/statstest.c | 2 +- 13 files changed, 102 insertions(+), 51 deletions(-) rename src/util/{virstatslinux.c => virstats.c} (61%) rename src/util/{virstatslinux.h => virstats.h} (77%)
ACK to both patches. Michal
participants (2)
-
Michal Privoznik -
Roman Bogorodskiy