> -----Original Message----- > From: sendmail [mailto:justsendmailnothingelse@xxxxxxxxx] On Behalf Of > Laine Stump > Sent: Tuesday, August 11, 2015 9:27 AM > To: libvir-list@xxxxxxxxxx > Cc: Moshe Levi > Subject: Re: [PATCH] nodedev: Fix gfeature size to be according to > running kernel > > 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). Ok I will rebase once you patch is merged. > > > > 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? It is define in ethtool-copy.h > > > > # 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? I printed the value when I run ethtool command moshe index 25 tx-udp_tnl-segmentation: on moshe index 26 tx-mpls-segmentation: off [fixed] ethtool modified code static void dump_one_feature(const char *indent, const char *name, const struct feature_state *state, const struct feature_state *ref_state, u32 index) { if (ref_state && !(FEATURE_BIT_IS_SET(state->features.features, index, active) ^ FEATURE_BIT_IS_SET(ref_state->features.features, index, active))) return; printf("moshe index %d\n",index); printf("%s%s: %s%s\n", indent, name, FEATURE_BIT_IS_SET(state->features.features, index, active) ? "on" : "off", (!FEATURE_BIT_IS_SET(state->features.features, index, available) || FEATURE_BIT_IS_SET(state->features.features, index, never_changed)) ? " [fixed]" : (FEATURE_BIT_IS_SET(state->features.features, index, requested) ^ FEATURE_BIT_IS_SET(state->features.features, index, active)) ? (FEATURE_BIT_IS_SET(state->features.features, index, requested) ? " [requested on]" : " [requested off]") : ""); } Also this how I got the GFEATURES_SIZE 2 > > > > -# 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. This was comment by John Ferlan he preferred the method will return size 0 if not supported or error. My previous patch which I send to him directly and not the ML return -1 on failure. But thinking about this again I don't want to swallow if error occur. What do you think? > > > > + 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? So I guess only if I will return -1 on failure we should not ignore it (like I mention in the comment above ) and the unsupported will be size 0 And that can be ignored. > > > > + else > > + if (virNetDevGFeatureAvailable(ifname, &g_cmd)) > > + ignore_value(virBitmapSetBit(*out, > > + VIR_NET_DEV_FEAT_TXUDPTNL)); > > # endif > > > > if (virNetDevRDMAFeature(ifname, out) < 0) -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list