| Message ID | 20201117152015.142089-6-acardace@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netdevsim: add ethtool coalesce and ring settings | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 30 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Nov 17, 2020 at 04:20:14PM +0100, Antonio Cardace wrote: > As pointed out by Michal Kubecek, getting the name > with the previous approach was racy, it's better > and easier to get the name of the device with this > patch's approach. > > Essentialy the function doesn't need to exist > anymore as it's a simple 'ls' command. > > Signed-off-by: Antonio Cardace <acardace@redhat.com> > --- > .../drivers/net/netdevsim/ethtool-common.sh | 20 ++----------------- > 1 file changed, 2 insertions(+), 18 deletions(-) > > diff --git a/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh b/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh > index fa44cf6e732c..3c287ac78117 100644 > --- a/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh > +++ b/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh > @@ -20,23 +20,6 @@ function cleanup { > > 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 > @@ -65,5 +48,6 @@ function make_netdev { > fi > > echo $NSIM_ID > /sys/bus/netdevsim/new_device > - echo `get_netdev_name old_netdevs` > + # get new device name > + echo $(ls /sys/bus/netdevsim/devices/netdevsim${NSIM_ID}/net/) Is there a reason for combining command substitution with echo? Couldn't we use one of ls /sys/bus/netdevsim/devices/netdevsim${NSIM_ID}/net/ echo /sys/bus/netdevsim/devices/netdevsim${NSIM_ID}/net/* instead? Michal > } > -- > 2.28.0 >
On Tue, Nov 17, 2020 at 06:35:20PM +0100, Michal Kubecek wrote: > On Tue, Nov 17, 2020 at 04:20:14PM +0100, Antonio Cardace wrote: > > As pointed out by Michal Kubecek, getting the name > > with the previous approach was racy, it's better > > and easier to get the name of the device with this > > patch's approach. > > > > Essentialy the function doesn't need to exist > > anymore as it's a simple 'ls' command. > > > > Signed-off-by: Antonio Cardace <acardace@redhat.com> > > --- > > .../drivers/net/netdevsim/ethtool-common.sh | 20 ++----------------- > > 1 file changed, 2 insertions(+), 18 deletions(-) > > > > diff --git a/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh b/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh > > index fa44cf6e732c..3c287ac78117 100644 > > --- a/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh > > +++ b/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh > > @@ -20,23 +20,6 @@ function cleanup { > > > > 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 > > @@ -65,5 +48,6 @@ function make_netdev { > > fi > > > > echo $NSIM_ID > /sys/bus/netdevsim/new_device > > - echo `get_netdev_name old_netdevs` > > + # get new device name > > + echo $(ls /sys/bus/netdevsim/devices/netdevsim${NSIM_ID}/net/) > > Is there a reason for combining command substitution with echo? Couldn't > we use one of > > ls /sys/bus/netdevsim/devices/netdevsim${NSIM_ID}/net/ > echo /sys/bus/netdevsim/devices/netdevsim${NSIM_ID}/net/* > > instead? > > Michal > Ouch, no that's just a mistake. I'll fix it. Do I have to resend the whole serie as a new version or is there a quicker way to just resend a single patch? Thanks, Antonio
On Wed, 18 Nov 2020 10:03:20 +0100 Antonio Cardace wrote: > Do I have to resend the whole serie as a new version or is there a > quicker way to just resend a single patch? Just repost the series as v5, it's the least confusing way. Changelog from version to version would be good. You can put it in the cover letter.
On Wed, Nov 18, 2020 at 08:56:10AM -0800, Jakub Kicinski wrote: > On Wed, 18 Nov 2020 10:03:20 +0100 Antonio Cardace wrote: > > Do I have to resend the whole serie as a new version or is there a > > quicker way to just resend a single patch? > > Just repost the series as v5, it's the least confusing way. > Changelog from version to version would be good. You can put > it in the cover letter. > Ok thanks, will do. Antonio
diff --git a/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh b/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh index fa44cf6e732c..3c287ac78117 100644 --- a/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh +++ b/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh @@ -20,23 +20,6 @@ function cleanup { 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 @@ -65,5 +48,6 @@ function make_netdev { fi echo $NSIM_ID > /sys/bus/netdevsim/new_device - echo `get_netdev_name old_netdevs` + # get new device name + echo $(ls /sys/bus/netdevsim/devices/netdevsim${NSIM_ID}/net/) }
As pointed out by Michal Kubecek, getting the name with the previous approach was racy, it's better and easier to get the name of the device with this patch's approach. Essentialy the function doesn't need to exist anymore as it's a simple 'ls' command. Signed-off-by: Antonio Cardace <acardace@redhat.com> --- .../drivers/net/netdevsim/ethtool-common.sh | 20 ++----------------- 1 file changed, 2 insertions(+), 18 deletions(-)