On 08/13/2015 06:23 AM, Moshe Levi wrote:

This patch add virNetDevGetGFeaturesSize to get the supported gfeature size from the kernel --- src/util/virnetdev.c | 79 ++++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 71 insertions(+), 8 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 2f3690e..582efda 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -90,7 +90,8 @@ VIR_LOG_INIT("util.netdev"); #define RESOURCE_FILE_LEN 4096 #if HAVE_DECL_ETHTOOL_GFEATURES # define TX_UDP_TNL 25 -# define GFEATURES_SIZE 2 +# define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))

Maybe we should follow the convention laine noted in one of his earlier patches and use "GDIV_ROUND_UP" and "GFEATURE_BITS..."... Although perhaps DIV_ROUND_UP seems superfluous if it's only used once. I'd even be fine with changing the names of any of the following that are G* related to make it clearer...

+# 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 +3111,59 @@ 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, 0 not supported and -1 on failure. + */ +static int + virNetDevGetGFeaturesSize(const char *ifname) +{ + struct { + struct ethtool_sset_info hdr; + uint32_t buf[1]; + } sset_info; + struct ethtool_gstrings *strings;

s/*strings;/*strings = NULL;/

+ uint32_t size = 0; + int ret = -1; + + 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)

s/== -1/< 0/

+ return ret;

s/return ret;/goto cleanup;/ [1] Although this may also be a case where you want to set "ret = 0"... As I've read further into this patch and kept Laine's comments about the Ioctl API filtering certain return values - I'm becoming less convinced that the filtering is a good idea. I know the following comments are "beyond" the scope of this patch, but I'm not sure it was a "good idea"... I can perhaps understand filtering EOPNOTSUPP, but it really should be the caller that decides if that errno is "OK" at the point in time it's used in the code. EPERM being returned usually indicates something else. Perhaps this had to do with the session mode connection - maybe for that option it's a better idea to fail and indicate why than to just ignore the error... Similarly for EINVAL - what does that really mean for the ioctl() failure. I'm too lazy right now to check out why the ETHTOOL_* commands would return that... ;-) Since you seem to know the ioctl()'s better than I do - maybe a patch prior to this one to adjust that logic is in order. Could make some decision points here a bit easier. I'm not going to require changing the *Ioctl() API, but I do hope you can consider it.

+ size = sset_info.hdr.sset_mask ? sset_info.hdr.data[0] : 0;

Why not? if (size == 0) { ret = 0; goto cleanup; } You're not calling the Ioctl() if size == 0 anyway, so no need to alloc something, go through the steps of filling in data fields, and then just not use it... followed by a free...

+ if (VIR_ALLOC_VAR(strings, struct ethtool_gstrings, sizeof(*strings) + size * ETH_GSTRING_LEN))

s/LEN))/LEN) < 0)

+ return ret;

s/return ret;/goto cleanup;/

+ + strings->cmd = ETHTOOL_GSTRINGS; + strings->string_set = ETH_SS_FEATURES; + strings->len = size; + if (size != 0 && virNetDevSendEthtoolIoctl(ifname, strings) == -1) {

Since size cannot be 0, that check can be removed s/== -1/< 0/ Add a comment as to what the decision point here is... EG why is it ok to ignore all these errors... I know the called function is ignoring them and not generating a virReportError message... You're just setting ret = 0 (or size = 0) so that the caller of this function will consider the feature unsupported. Of course if you decide to create a patch to allow the caller to decide what's an appropriate error message, then things will be clearer...

+ if (errno == EOPNOTSUPP || errno == EINVAL || errno == EPERM) { + ret = 0; + size = 0;

Depending on how you proceed - a "filtered" errno (eg. EOPNOTSUPP) could set "ret = 0"... while unfiltered messages would keep ret = -1. Also, if there's only one line after an if, you won't need the { }

+ } + goto cleanup; + } + + ret = 0; + size = FEATURE_BITS_TO_BLOCKS(size);

why not just ret = FEATURE_BITS_TO_BLOCKS(size);

+ + cleanup: + VIR_FREE(strings); + if (ret != -1) + ret = size;

These two lines won't be necessary as ret should either be -1, 0, or the translated size at this point.

+ return ret; +} + # endif

@@ -3131,6 +3185,7 @@ virNetDevGetFeatures(const char *ifname, struct ethtool_value cmd = { 0 }; # if HAVE_DECL_ETHTOOL_GFEATURES struct ethtool_gfeatures *g_cmd; + uint32_t size = 0; # endif struct elem{ const int cmd; @@ -3188,14 +3243,22 @@ virNetDevGetFeatures(const char *ifname, # endif

# if HAVE_DECL_ETHTOOL_GFEATURES - if (VIR_ALLOC_VAR(g_cmd, - struct ethtool_get_features_block, GFEATURES_SIZE) < 0) + size = virNetDevGetGFeaturesSize(ifname); + if (size == -1) { + VIR_DEBUG("Failure to get GFeatures size on ifname %s", ifname); return -1;

As I read the code, there are three ways to get -1 returned 1. virNetDevSendEthtoolIoctl fails on ETHTOOL_GSSET_INFO 2. VIR_ALLOC_VAR fails 3. virNetDevSendEthtoolIoctl fails on ETHTOOL_GSTRINGS [1] from above... If 1. fails, then is that perhaps is an indicator that getting the size is not supported, so perhaps it'd be better to return a 0 and allow continuance to virNetDevRDMAFeature. Additionally, you may need to virResetLastError() to clear the possible error. If 2. fails, then not much is going to happen as we're probably memory constrained and not much more is going to happen anyway. The good news here is libvirt should set a message on error (virReportOOMErrorFull) in order to let the user know why it failed. If 3. fails, then the ethtool ioctl failed error message is displayed. John

- g_cmd->cmd = ETHTOOL_GFEATURES; - g_cmd->size = GFEATURES_SIZE; - if (virNetDevGFeatureAvailable(ifname, g_cmd)) - ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL)); - VIR_FREE(g_cmd); + } else if (size == 0) { + VIR_DEBUG("GFeatures unsupported on ifname %s", ifname); + } else { + if (VIR_ALLOC_VAR(g_cmd, + struct ethtool_get_features_block, size) < 0) + return -1; + g_cmd->cmd = ETHTOOL_GFEATURES; + g_cmd->size = size; + if (virNetDevGFeatureAvailable(ifname, g_cmd)) + ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL)); + VIR_FREE(g_cmd); + } # endif

if (virNetDevRDMAFeature(ifname, out) < 0)