[libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel

[Laine Stump]

On 08/08/2015 05:34 AM, Moshe Levi wrote:
> This patch add virNetDevGetGFeaturesSize to get the supported
> gfeature size from the kernel
> ---

This is interesting/possibly useful, but it doesn't fix the crash that
users are experiencing. Here is a patch that should fix the crash:

https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html

I would rather have that patch pushed before this one (which will mean
rebasing and resolving some merge conflicts).


>  src/util/virnetdev.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 56 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 1e20789..3fdf4bb 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -89,8 +89,10 @@ VIR_LOG_INIT("util.netdev");
>  
>  #define RESOURCE_FILE_LEN 4096
>  #if HAVE_DECL_ETHTOOL_GFEATURES
> +#define ETH_GSTRING_LEN 32

Where does the "32" come from? Is it from ETH_GSTRING_LEN (defined in
ethtool.h)? If so, why not just use that directly instead of #defining a
new one?


>  # define TX_UDP_TNL 25

I'll keep asking this every time I encounter it until I get a response -
when I look at the ethtool source files in the kernel, it looks to me
like the index for this capability should be 26 and not 25. Where/how
did you arrive at the value of 25?


> -# define GFEATURES_SIZE 2
> +# define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
> +# define FEATURE_BITS_TO_BLOCKS(n_bits)      DIV_ROUND_UP(n_bits, 32U)
>  # define FEATURE_WORD(blocks, index, field)  ((blocks)[(index) / 32U].field)
>  # define FEATURE_FIELD_FLAG(index)      (1U << (index) % 32U)
>  # define FEATURE_BIT_IS_SET(blocks, index, field)        \
> @@ -3110,6 +3112,53 @@ virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd)
>          ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active);
>      return ret;
>  }
> +/**
> + * virNetDevGetGFeaturesSize
> + * This function return the number of gfeatures supported in
> + * the kernel
> + *
> + * @ifname: name of the interface
> + *
> + * Returns gfeature size on success, and 0 on failure or not supported.
> + */
> + static int
> +virNetDevGetGFeaturesSize(const char *ifname)
> +{
> +    struct {
> +        struct ethtool_sset_info hdr;
> +        uint32_t buf[1];
> +    } sset_info;
> +    struct ethtool_gstrings *strings;
> +    uint32_t size = 0;
> +
> +    sset_info.hdr.cmd = ETHTOOL_GSSET_INFO;
> +    sset_info.hdr.reserved = 0;
> +    sset_info.hdr.sset_mask = 1ULL << ETH_SS_FEATURES;
> +
> +    if (virNetDevSendEthtoolIoctl(ifname, &sset_info) == -1)
> +        return size;

virNetDevSendEthtoolIoctl() logs an error message, but it looks like you
want for an error to be swallowed here (just returning size = 0). If
that's the case then virNetDevSendEthtoolIoctl() needs to be reworked to
not log errors, then every caller to it needs to log the error.


> +    size = sset_info.hdr.sset_mask ? sset_info.hdr.data[0] : 0;
> +
> +    if (VIR_ALLOC_VAR(strings, struct ethtool_gstrings, sizeof(*strings) + size * ETH_GSTRING_LEN)) {
> +        size = 0;
> +        return size;
> +    }
> +
> +    strings->cmd = ETHTOOL_GSTRINGS;
> +    strings->string_set = ETH_SS_FEATURES;
> +    strings->len = size;
> +    if (size != 0 && virNetDevSendEthtoolIoctl(ifname, strings) == -1) {
> +        size = 0;
> +        goto cleanup;
> +    }
> +
> +    size = FEATURE_BITS_TO_BLOCKS(size);
> +
> + cleanup:
> +    VIR_FREE(strings);
> +    return size;
> +}
> +
>  # endif
>  
>  
> @@ -3189,9 +3238,12 @@ virNetDevGetFeatures(const char *ifname,
>  
>  # if HAVE_DECL_ETHTOOL_GFEATURES
>      g_cmd.cmd = ETHTOOL_GFEATURES;
> -    g_cmd.size = GFEATURES_SIZE;
> -    if (virNetDevGFeatureAvailable(ifname, &g_cmd))
> -        ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
> +    g_cmd.size = virNetDevGetGFeaturesSize(ifname);
> +    if (g_cmd.size == 0)
> +        VIR_DEBUG("GFeatures unsupported or failure in getting GFeatures size on ifname %s", ifname);

Should this failure really be ignored? Or should it lead to failure of
the entire operation?


> +    else
> +        if (virNetDevGFeatureAvailable(ifname, &g_cmd))
> +            ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
>  # endif
>  
>      if (virNetDevRDMAFeature(ifname, out) < 0)