On 06/05/2014 11:39 AM, Michal Privoznik wrote:
The purpose of this function is to fetch link state and link speed for given NIC name from the SYSFS.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 6 +++ 3 files changed, 111 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 394c086..6d08ee9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1540,6 +1540,7 @@ virNetDevClearIPv4Address; virNetDevExists; virNetDevGetIndex; virNetDevGetIPv4Address; +virNetDevGetLinkInfo; virNetDevGetMAC; virNetDevGetMTU; virNetDevGetPhysicalFunction; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 3a60cf7..9d2c0d2 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1832,3 +1832,107 @@ virNetDevRestoreNetConfig(const char *linkdev ATTRIBUTE_UNUSED, }
#endif /* defined(__linux__) && defined(HAVE_LIBNL) */ + +#ifdef __linux__ +# define SYSFS_PREFIX "/sys/class/net/"
There is a 'NET_SYSFS' definition earlier to the same path - seems it could/should be reused... NIT: Your definition will create a double slash below for operstate and speed... Whether that's desired or not is up to you... There's a mix of usage within libvirt sources.
+int +virNetDevGetLinkInfo(const char *ifname, + virInterfaceState *state, + unsigned int *speed)
Could there ever be any other stats/data to fetch? Perhaps consider passing a virInterfaceLinkPtr instead and then reference state/speed off of it. Then in the future you don't have to keep adding params to the API. None of the current callers pass NULL for either parameter. I also wonder if we really want to fail out if we something goes wrong. It's not like we're affecting the state/speed of the device if something did go wrong - we're just outputting data if it's there. I guess I'm thinking more about the callers, but that's a design decision you have to make and live with. Considering the comment below regarding the bug with speed - one wonders what else lurks in former formats that the following code can error out on.
+{ + int ret = -1; + char *path = NULL; + char *buf = NULL; + char *tmp; + int tmp_state;
s/int/virInterfaceState/ Seems we should be able to set up a dummy /sys/class/net environment in order to test the new API's... Something I attempted with my recently posted scsi_host changes.
+ unsigned int tmp_speed; + + if (state) { + if (virAsprintf(&path, SYSFS_PREFIX "%s/operstate", ifname) < 0) + goto cleanup; + + if (virFileReadAll(path, 1024, &buf) < 0) { + virReportSystemError(errno, + _("unable to read: %s"), + path); + goto cleanup; + } + + if (!(tmp = strchr(buf, '\n'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse: %s"), + buf); + goto cleanup; + } + + *tmp = '\0'; + + /* We shouldn't allow 0 here, because + * virInterfaceState enum starts from 1. */ + if ((tmp_state = virInterfaceStateTypeFromString(buf)) <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse: %s"), + buf); + goto cleanup; + } + + *state = tmp_state; + + VIR_FREE(path); + VIR_FREE(buf); + } + + if (speed) { + if (virAsprintf(&path, SYSFS_PREFIX "%s/speed", ifname) < 0) + goto cleanup; + + if (virFileReadAll(path, 1024, &buf) < 0) { + /* Some devices doesn't report speed, in which case we get EINVAL */ + if (errno == EINVAL) { + ret = 0; + goto cleanup; + } + virReportSystemError(errno, + _("unable to read: %s"), + path); + goto cleanup; + } + + if (virStrToLong_ui(buf, &tmp, 10, &tmp_speed) < 0 || + *tmp != '\n') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse: %s"), + buf); + goto cleanup; + } + + /* Workaround broken kernel API. If the link is unplugged then + * depending on the NIC driver, link speed can be reported as -1. + * However, the value is printed out as unsigned integer instead of + * signed one. Terrifying but true. */ + *speed = (int) tmp_speed == -1 ? 0 : tmp_speed; + } + + ret = 0; + cleanup: + VIR_FREE(buf); + VIR_FREE(path); + return ret; +} + +#else + +int +virNetDevGetLinkInfo(const char *ifname, + virInterfaceState *state, + unsigned int *speed)
In virutil.c each of the paremters has a "ATTRIBUTE_UNUSED" and the code is: virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); return -1; Your call on how do do this ACK - in general though based on fixing up the nits/minor issues. I trust if you change the parameters that it'll be done right or the compiler will complain! John
+{ + /* Port me */ + VIR_DEBUG("Getting link info on %s is not implemented on this platform"); + if (state) + *state = 0; + if (speed) + *speed = 0; + return 0; +} +#endif /* defined(__linux__) */ diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 00c82e0..de33323 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -29,6 +29,7 @@ # include "virnetlink.h" # include "virmacaddr.h" # include "virpci.h" +# include "device_conf.h"
# ifdef HAVE_STRUCT_IFREQ typedef struct ifreq virIfreq; @@ -145,4 +146,9 @@ int virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname, int *vf) ATTRIBUTE_NONNULL(1);
+int virNetDevGetLinkInfo(const char *ifname, + virInterfaceState *state, + unsigned int *speed) + ATTRIBUTE_NONNULL(1); + #endif /* __VIR_NETDEV_H__ */