| Message ID | 20201113231655.139948-3-acardace@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | None | expand |
On Sat, Nov 14, 2020 at 12:16:54AM +0100, Antonio Cardace wrote: > Factor out some useful functions so that they can be reused > by other ethtool-netdevsim scripts. > > Signed-off-by: Antonio Cardace <acardace@redhat.com> Reviewed-by: Michal Kubecek <mkubecek@suse.cz> Just one comment: [...] > +function get_netdev_name { > + local -n old=$1 > + > + new=$(ls /sys/class/net) > + > + for netdev in $new; do > + for check in $old; do > + [ $netdev == $check ] && break > + done > + > + if [ $netdev != $check ]; then > + echo $netdev > + break > + fi > + done > +} [...] > +function make_netdev { > + # Make a netdevsim > + old_netdevs=$(ls /sys/class/net) > + > + if ! $(lsmod | grep -q netdevsim); then > + modprobe netdevsim > + fi > + > + echo $NSIM_ID > /sys/bus/netdevsim/new_device > + echo `get_netdev_name old_netdevs` > +} This would be rather unpredictable if someone ran another selftest (or anything else that would create a network device) in parallel. IMHO it would be safer (and easier) to get the name of the new device from /sys/bus/netdevsim/devices/netdevsim${NSIM_ID}/net/ But as this is not new code and you are just moving existing code, it can be done in a separate patch. Michal
On Mon, Nov 16, 2020 at 05:17:02PM +0100, Michal Kubecek wrote: > On Sat, Nov 14, 2020 at 12:16:54AM +0100, Antonio Cardace wrote: > > Factor out some useful functions so that they can be reused > > by other ethtool-netdevsim scripts. > > > > Signed-off-by: Antonio Cardace <acardace@redhat.com> > > Reviewed-by: Michal Kubecek <mkubecek@suse.cz> > > Just one comment: > > [...] > > +function get_netdev_name { > > + local -n old=$1 > > + > > + new=$(ls /sys/class/net) > > + > > + for netdev in $new; do > > + for check in $old; do > > + [ $netdev == $check ] && break > > + done > > + > > + if [ $netdev != $check ]; then > > + echo $netdev > > + break > > + fi > > + done > > +} > [...] > > +function make_netdev { > > + # Make a netdevsim > > + old_netdevs=$(ls /sys/class/net) > > + > > + if ! $(lsmod | grep -q netdevsim); then > > + modprobe netdevsim > > + fi > > + > > + echo $NSIM_ID > /sys/bus/netdevsim/new_device > > + echo `get_netdev_name old_netdevs` > > +} > > This would be rather unpredictable if someone ran another selftest (or > anything else that would create a network device) in parallel. IMHO it > would be safer (and easier) to get the name of the new device from > > /sys/bus/netdevsim/devices/netdevsim${NSIM_ID}/net/ > > But as this is not new code and you are just moving existing code, it > can be done in a separate patch. Yes it does make sense, I can send a patch for this once this is merged. Thanks for the review. Antonio
diff --git a/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh b/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh new file mode 100644 index 000000000000..fa44cf6e732c --- /dev/null +++ b/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh @@ -0,0 +1,69 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0-only + +NSIM_ID=$((RANDOM % 1024)) +NSIM_DEV_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_ID +NSIM_DEV_DFS=/sys/kernel/debug/netdevsim/netdevsim$NSIM_ID/ports/0 +NSIM_NETDEV= +num_passes=0 +num_errors=0 + +function cleanup_nsim { + if [ -e $NSIM_DEV_SYS ]; then + echo $NSIM_ID > /sys/bus/netdevsim/del_device + fi +} + +function cleanup { + cleanup_nsim +} + +trap cleanup EXIT + +function get_netdev_name { + local -n old=$1 + + new=$(ls /sys/class/net) + + for netdev in $new; do + for check in $old; do + [ $netdev == $check ] && break + done + + if [ $netdev != $check ]; then + echo $netdev + break + fi + done +} + +function check { + local code=$1 + local str=$2 + local exp_str=$3 + + if [ $code -ne 0 ]; then + ((num_errors++)) + return + fi + + if [ "$str" != "$exp_str" ]; then + echo -e "Expected: '$exp_str', got '$str'" + ((num_errors++)) + return + fi + + ((num_passes++)) +} + +function make_netdev { + # Make a netdevsim + old_netdevs=$(ls /sys/class/net) + + if ! $(lsmod | grep -q netdevsim); then + modprobe netdevsim + fi + + echo $NSIM_ID > /sys/bus/netdevsim/new_device + echo `get_netdev_name old_netdevs` +} diff --git a/tools/testing/selftests/drivers/net/netdevsim/ethtool-pause.sh b/tools/testing/selftests/drivers/net/netdevsim/ethtool-pause.sh index 25c896b9e2eb..b4a7abfe5454 100755 --- a/tools/testing/selftests/drivers/net/netdevsim/ethtool-pause.sh +++ b/tools/testing/selftests/drivers/net/netdevsim/ethtool-pause.sh @@ -1,60 +1,7 @@ #!/bin/bash # SPDX-License-Identifier: GPL-2.0-only -NSIM_ID=$((RANDOM % 1024)) -NSIM_DEV_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_ID -NSIM_DEV_DFS=/sys/kernel/debug/netdevsim/netdevsim$NSIM_ID/ports/0 -NSIM_NETDEV= -num_passes=0 -num_errors=0 - -function cleanup_nsim { - if [ -e $NSIM_DEV_SYS ]; then - echo $NSIM_ID > /sys/bus/netdevsim/del_device - fi -} - -function cleanup { - cleanup_nsim -} - -trap cleanup EXIT - -function get_netdev_name { - local -n old=$1 - - new=$(ls /sys/class/net) - - for netdev in $new; do - for check in $old; do - [ $netdev == $check ] && break - done - - if [ $netdev != $check ]; then - echo $netdev - break - fi - done -} - -function check { - local code=$1 - local str=$2 - local exp_str=$3 - - if [ $code -ne 0 ]; then - ((num_errors++)) - return - fi - - if [ "$str" != "$exp_str" ]; then - echo -e "Expected: '$exp_str', got '$str'" - ((num_errors++)) - return - fi - - ((num_passes++)) -} +source ethtool-common.sh # Bail if ethtool is too old if ! ethtool -h | grep include-stat 2>&1 >/dev/null; then @@ -62,13 +9,7 @@ if ! ethtool -h | grep include-stat 2>&1 >/dev/null; then exit 4 fi -# Make a netdevsim -old_netdevs=$(ls /sys/class/net) - -modprobe netdevsim -echo $NSIM_ID > /sys/bus/netdevsim/new_device - -NSIM_NETDEV=`get_netdev_name old_netdevs` +NSIM_NETDEV=$(make_netdev) set -o pipefail
Factor out some useful functions so that they can be reused by other ethtool-netdevsim scripts. Signed-off-by: Antonio Cardace <acardace@redhat.com> --- .../drivers/net/netdevsim/ethtool-common.sh | 69 +++++++++++++++++++ .../drivers/net/netdevsim/ethtool-pause.sh | 63 +---------------- 2 files changed, 71 insertions(+), 61 deletions(-) create mode 100644 tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh