[libvirt] [PATCH v2 2/4] virnetdev: Introduce virNetDevGetLinkInfo

[John Ferlan]

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 at 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__ */
>