[libvirt] [PATCHv2] nodedev: Fix gfeature size to be according to running kernel
John Ferlan
jferlan at redhat.com
Thu Aug 13 14:41:19 UTC 2015
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)
>
More information about the libvir-list
mailing list