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)