| Message ID | 20240606212714.27472-1-jain.abhinav177@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | selftests: net: Add on/off checks for network interface non fixed features | expand |
On Thu, Jun 06, 2024 at 09:27:14PM +0000, Abhinav Jain wrote: > This patch addresses the present TODO in the file. > I have tested it manually on my system and added relevant filtering to > ensure that the correct feature list is being checked. > > Signed-off-by: Abhinav Jain <jain.abhinav177@gmail.com> > --- > tools/testing/selftests/net/netdevice.sh | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/net/netdevice.sh b/tools/testing/selftests/net/netdevice.sh > index e3afcb424710..cbe2573c3827 100755 > --- a/tools/testing/selftests/net/netdevice.sh > +++ b/tools/testing/selftests/net/netdevice.sh > @@ -117,14 +117,31 @@ kci_netdev_ethtool() > return 1 > fi > > - ethtool -k "$netdev" > "$TMP_ETHTOOL_FEATURES" > + ethtool -k "$netdev" | tail -n +2 > "$TMP_ETHTOOL_FEATURES" > if [ $? -ne 0 ];then Hi Abhinav, I suspect this will now only report a failure if tail fails, but ignore ethtool failures. > echo "FAIL: $netdev: ethtool list features" > rm "$TMP_ETHTOOL_FEATURES" > return 1 > fi > echo "PASS: $netdev: ethtool list features" > - #TODO for each non fixed features, try to turn them on/off > + > + for feature in $(grep -v fixed "$TMP_ETHTOOL_FEATURES" | \ > + awk '{print $1}' | sed 's/://'); do Shellcheck warns that the above reads words rather than lines, and recommends using read instead. I think that is ok, because the construction reduces lines to single words. But it does seem a bit awkward to call grep, awk and sed for this. I wonder if the following construction nicer: while read -r FEATURE VALUE FIXED; do [ "$FEAT" != "Features" ] || continue # Skip "Features" line [ "$FIXED" != "[fixed]" ] || continue # Skip fixed features feature="${FEATURE%:*}" ... done < "$TMP_ETHTOOL_FEATURES" > + ethtool --offload "$netdev" "$feature" off > + if [ $? -eq 0 ]; then > + echo "PASS: $netdev: Turned off feature: $feature" > + else > + echo "FAIL: $netdev: Failed to turn off feature: $feature" > + fi > + > + ethtool --offload "$netdev" "$feature" on > + if [ $? -eq 0 ]; then > + echo "PASS: $netdev: Turned on feature: $feature" > + else > + echo "FAIL: $netdev: Failed to turn on feature: $feature" > + fi > + done > + > rm "$TMP_ETHTOOL_FEATURES" > > kci_netdev_ethtool_test 74 'dump' "ethtool -d $netdev" > -- > 2.34.1 > >
On Fri, 7 Jun 2024 19:01:27 +0100, Simon Horman wrote: > Hi Abhinav, > > I suspect this will now only report a failure if tail fails, > but ignore ethtool failures. Hi Simon, I agree, I missed this part earlier. After taking other suggestion into account, we don't need this tail and I have removed it. > Shellcheck warns that the above reads words rather than lines, > and recommends using read instead. > > I think that is ok, because the construction reduces lines to single words. > But it does seem a bit awkward to call grep, awk and sed for this. > > I wonder if the following construction nicer: > > while read -r FEATURE VALUE FIXED; do > [ "$FEAT" != "Features" ] || continue # Skip "Features" line > [ "$FIXED" != "[fixed]" ] || continue # Skip fixed features > feature="${FEATURE%:*}" > ... > done < "$TMP_ETHTOOL_FEATURES" I have re-submitted a v2 of patch here keeping the above change: https://lore.kernel.org/all/20240609132124.51683-1-jain.abhinav177@gmail.com/ Please review. Thank you.
diff --git a/tools/testing/selftests/net/netdevice.sh b/tools/testing/selftests/net/netdevice.sh index e3afcb424710..cbe2573c3827 100755 --- a/tools/testing/selftests/net/netdevice.sh +++ b/tools/testing/selftests/net/netdevice.sh @@ -117,14 +117,31 @@ kci_netdev_ethtool() return 1 fi - ethtool -k "$netdev" > "$TMP_ETHTOOL_FEATURES" + ethtool -k "$netdev" | tail -n +2 > "$TMP_ETHTOOL_FEATURES" if [ $? -ne 0 ];then echo "FAIL: $netdev: ethtool list features" rm "$TMP_ETHTOOL_FEATURES" return 1 fi echo "PASS: $netdev: ethtool list features" - #TODO for each non fixed features, try to turn them on/off + + for feature in $(grep -v fixed "$TMP_ETHTOOL_FEATURES" | \ + awk '{print $1}' | sed 's/://'); do + ethtool --offload "$netdev" "$feature" off + if [ $? -eq 0 ]; then + echo "PASS: $netdev: Turned off feature: $feature" + else + echo "FAIL: $netdev: Failed to turn off feature: $feature" + fi + + ethtool --offload "$netdev" "$feature" on + if [ $? -eq 0 ]; then + echo "PASS: $netdev: Turned on feature: $feature" + else + echo "FAIL: $netdev: Failed to turn on feature: $feature" + fi + done + rm "$TMP_ETHTOOL_FEATURES" kci_netdev_ethtool_test 74 'dump' "ethtool -d $netdev"
This patch addresses the present TODO in the file. I have tested it manually on my system and added relevant filtering to ensure that the correct feature list is being checked. Signed-off-by: Abhinav Jain <jain.abhinav177@gmail.com> --- tools/testing/selftests/net/netdevice.sh | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)