[PATCH net] selftests: openvswitch: Use bash as interpreter

[PATCH net] selftests: openvswitch: Use bash as interpreter

[Simon Horman]

openvswitch.sh makes use of substitutions of the form ${ns:0:1}, to
obtain the first character of $ns. Empirically, this is works with bash
but not dash. When run with dash these evaluate to an empty string and
printing an error to stdout.

 # dash -c 'ns=client; echo "${ns:0:1}"' 2>error
 # cat error
 dash: 1: Bad substitution
 # bash -c 'ns=client; echo "${ns:0:1}"' 2>error
 c
 # cat error

This leads to tests that neither pass nor fail.
F.e.

 TEST: arp_ping                                                      [START]
 adding sandbox 'test_arp_ping'
 Adding DP/Bridge IF: sbx:test_arp_ping dp:arpping {, , }
 create namespaces
 ./openvswitch.sh: 282: eval: Bad substitution
 TEST: ct_connect_v4                                                 [START]
 adding sandbox 'test_ct_connect_v4'
 Adding DP/Bridge IF: sbx:test_ct_connect_v4 dp:ct4 {, , }
 ./openvswitch.sh: 322: eval: Bad substitution
 create namespaces

Resolve this by making openvswitch.sh a bash script.

Fixes: 918423fda910 ("selftests: openvswitch: add an initial flow programming case")
Signed-off-by: Simon Horman <horms@kernel.org>
---
 tools/testing/selftests/net/openvswitch/openvswitch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 5cae53543849..15bca0708717 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 #
 # OVS kernel module self tests

Re: [PATCH net] selftests: openvswitch: Use bash as interpreter

[Przemek Kitszel]

On 6/17/24 10:28, Simon Horman wrote:
> openvswitch.sh makes use of substitutions of the form ${ns:0:1}, to
> obtain the first character of $ns. Empirically, this is works with bash
> but not dash. When run with dash these evaluate to an empty string and
> printing an error to stdout.
> 
>   # dash -c 'ns=client; echo "${ns:0:1}"' 2>error
>   # cat error
>   dash: 1: Bad substitution
>   # bash -c 'ns=client; echo "${ns:0:1}"' 2>error
>   c
>   # cat error
> 
> This leads to tests that neither pass nor fail.
> F.e.
> 
>   TEST: arp_ping                                                      [START]
>   adding sandbox 'test_arp_ping'
>   Adding DP/Bridge IF: sbx:test_arp_ping dp:arpping {, , }
>   create namespaces
>   ./openvswitch.sh: 282: eval: Bad substitution
>   TEST: ct_connect_v4                                                 [START]
>   adding sandbox 'test_ct_connect_v4'
>   Adding DP/Bridge IF: sbx:test_ct_connect_v4 dp:ct4 {, , }
>   ./openvswitch.sh: 322: eval: Bad substitution
>   create namespaces
> 
> Resolve this by making openvswitch.sh a bash script.
> 
> Fixes: 918423fda910 ("selftests: openvswitch: add an initial flow programming case")
> Signed-off-by: Simon Horman <horms@kernel.org>

That's good fix,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

sidenote: I like very much the idea to use the least powerful tool, like
sh vs bash, awk vs gawk, but it breaks when we forget what is outside of
the scope of the former/standard.
Perhaps for shell, we could convert all the selftests at once?

> ---
>   tools/testing/selftests/net/openvswitch/openvswitch.sh | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index 5cae53543849..15bca0708717 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> @@ -1,4 +1,4 @@
> -#!/bin/sh
> +#!/bin/bash
>   # SPDX-License-Identifier: GPL-2.0
>   #
>   # OVS kernel module self tests
> 
> 

Re: [PATCH net] selftests: openvswitch: Use bash as interpreter

[Simon Horman]

On Mon, Jun 17, 2024 at 12:05:11PM +0200, Przemek Kitszel wrote:
> On 6/17/24 10:28, Simon Horman wrote:
> > openvswitch.sh makes use of substitutions of the form ${ns:0:1}, to
> > obtain the first character of $ns. Empirically, this is works with bash
> > but not dash. When run with dash these evaluate to an empty string and
> > printing an error to stdout.
> > 
> >   # dash -c 'ns=client; echo "${ns:0:1}"' 2>error
> >   # cat error
> >   dash: 1: Bad substitution
> >   # bash -c 'ns=client; echo "${ns:0:1}"' 2>error
> >   c
> >   # cat error
> > 
> > This leads to tests that neither pass nor fail.
> > F.e.
> > 
> >   TEST: arp_ping                                                      [START]
> >   adding sandbox 'test_arp_ping'
> >   Adding DP/Bridge IF: sbx:test_arp_ping dp:arpping {, , }
> >   create namespaces
> >   ./openvswitch.sh: 282: eval: Bad substitution
> >   TEST: ct_connect_v4                                                 [START]
> >   adding sandbox 'test_ct_connect_v4'
> >   Adding DP/Bridge IF: sbx:test_ct_connect_v4 dp:ct4 {, , }
> >   ./openvswitch.sh: 322: eval: Bad substitution
> >   create namespaces
> > 
> > Resolve this by making openvswitch.sh a bash script.
> > 
> > Fixes: 918423fda910 ("selftests: openvswitch: add an initial flow programming case")
> > Signed-off-by: Simon Horman <horms@kernel.org>
> 
> That's good fix,
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> 
> sidenote: I like very much the idea to use the least powerful tool, like
> sh vs bash, awk vs gawk, but it breaks when we forget what is outside of
> the scope of the former/standard.
> Perhaps for shell, we could convert all the selftests at once?

Thanks,

Now that you mention it, I have the same feelings.

Do we ever expect to use the minimal tools, when other
parts of the test suite depend on the enhanced ones?

...

Re: [PATCH net] selftests: openvswitch: Use bash as interpreter

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 17 Jun 2024 09:28:33 +0100 you wrote:
> openvswitch.sh makes use of substitutions of the form ${ns:0:1}, to
> obtain the first character of $ns. Empirically, this is works with bash
> but not dash. When run with dash these evaluate to an empty string and
> printing an error to stdout.
> 
>  # dash -c 'ns=client; echo "${ns:0:1}"' 2>error
>  # cat error
>  dash: 1: Bad substitution
>  # bash -c 'ns=client; echo "${ns:0:1}"' 2>error
>  c
>  # cat error
> 
> [...]

Here is the summary with links:
  - [net] selftests: openvswitch: Use bash as interpreter
    https://git.kernel.org/netdev/net/c/e2b447c9a1bb

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

RE: [PATCH net] selftests: openvswitch: Use bash as interpreter

[David Laight]

From: Simon Horman
> Sent: 17 June 2024 11:34
...
> > sidenote: I like very much the idea to use the least powerful tool, like
> > sh vs bash, awk vs gawk, but it breaks when we forget what is outside of
> > the scope of the former/standard.
> > Perhaps for shell, we could convert all the selftests at once?
> 
> Thanks,
> 
> Now that you mention it, I have the same feelings.
> 
> Do we ever expect to use the minimal tools, when other
> parts of the test suite depend on the enhanced ones?

Certainly trying to avoid bash-isms seems like a good idea.
Especially in scripts where it isn't really that hard.
OTOH avoiding posix features (so the script will run on a traditional SYSV /bin/sh)
is probably excessive.

I'd use "${foo%"${foo#?}"}" to get the first character without bash-isms.
But, IIRC, one version of ash/dash made a 'pig's breakfast' of the nested
pattern match.
(Most syntax highlighters don't get the quoting right either...)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)