| Message ID | 20250219234956.520599-2-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 846742f7e32f632166e3b2a106304b2bec7541a9 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | selftests: drv-net: improve the queue test for XSK | expand |
On 02/19, Jakub Kicinski wrote: > Joe Damato reports that some shells will fork before running > the command when python does "sh -c $cmd", while bash does > an exec of $cmd directly. This will have implications for our > ability to terminate the child process on bash vs other shells. > Warn about using > > bkg(... shell=True, termininate=True) > > most background commands can hopefully exit cleanly (exit_wait). > > Link: https://lore.kernel.org/Z7Yld21sv_Ip3gQx@LQ3V64L9R2 > Signed-off-by: Jakub Kicinski <kuba@kernel.org> Acked-by: Stanislav Fomichev <sdf@fomichev.me>
On Wed, Feb 19, 2025 at 03:49:50PM -0800, Jakub Kicinski wrote: > Joe Damato reports that some shells will fork before running > the command when python does "sh -c $cmd", while bash does > an exec of $cmd directly. I'm not sure what's going on, but as I mentioned in the other thread and below, I am using bash as well. > This will have implications for our > ability to terminate the child process on bash vs other shells. > Warn about using > > bkg(... shell=True, termininate=True) > > most background commands can hopefully exit cleanly (exit_wait). > > Link: https://lore.kernel.org/Z7Yld21sv_Ip3gQx@LQ3V64L9R2 > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > v2: new > --- > tools/testing/selftests/net/lib/py/utils.py | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py > index 9e3bcddcf3e8..33b153767d89 100644 > --- a/tools/testing/selftests/net/lib/py/utils.py > +++ b/tools/testing/selftests/net/lib/py/utils.py > @@ -61,6 +61,10 @@ import time > self.terminate = not exit_wait > self.check_fail = fail > > + if shell and self.terminate: > + print("# Warning: combining shell and terminate is risky!") > + print("# SIGTERM may not reach the child on zsh/ksh!") I'm not opposed to putting a warning here, but just as a disclaimer and for anyone else following along -- I am using bash: $ echo $SHELL /bin/bash $ bash --version GNU bash, version 5.2.21(1)-release (x86_64-pc-linux-gnu)
On Wed, Feb 19, 2025 at 03:49:50PM -0800, Jakub Kicinski wrote: > Joe Damato reports that some shells will fork before running > the command when python does "sh -c $cmd", while bash does > an exec of $cmd directly. This will have implications for our > ability to terminate the child process on bash vs other shells. > Warn about using > > bkg(... shell=True, termininate=True) > > most background commands can hopefully exit cleanly (exit_wait). > > Link: https://lore.kernel.org/Z7Yld21sv_Ip3gQx@LQ3V64L9R2 > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > v2: new > --- > tools/testing/selftests/net/lib/py/utils.py | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py > index 9e3bcddcf3e8..33b153767d89 100644 > --- a/tools/testing/selftests/net/lib/py/utils.py > +++ b/tools/testing/selftests/net/lib/py/utils.py > @@ -61,6 +61,10 @@ import time > self.terminate = not exit_wait > self.check_fail = fail > > + if shell and self.terminate: > + print("# Warning: combining shell and terminate is risky!") > + print("# SIGTERM may not reach the child on zsh/ksh!") > + This warning did not print on my system, FWIW. Up to you if you want to respin and drop it or just leave it be. I am fine with this warning being added, although I disagree with the commit message as mentioned in my previous email. Since the code seems fine though: Acked-by: Joe Damato <jdamato@fastly.com>
On Thu, 20 Feb 2025 14:31:35 -0500 Joe Damato wrote: > > + if shell and self.terminate: > > + print("# Warning: combining shell and terminate is risky!") > > + print("# SIGTERM may not reach the child on zsh/ksh!") > > + > > This warning did not print on my system, FWIW. Up to you if you want > to respin and drop it or just leave it be. You mean when running this patchset? It shouldn't, the current test uses ksft_wait rather than terminate. The warning is for test developers trying to use the risky config in new tests. > I am fine with this warning being added, although I disagree with > the commit message as mentioned in my previous email. I'll edit the commit message when applying. Unless you want to dig deeper and find out why your bash/env is different than mine :( GNU bash, version 5.2.32(1)-release (x86_64-redhat-linux-gnu) We know that "some shells" fork, that's the important point.
On Thu, Feb 20, 2025 at 01:10:59PM -0800, Jakub Kicinski wrote: > On Thu, 20 Feb 2025 14:31:35 -0500 Joe Damato wrote: > > > + if shell and self.terminate: > > > + print("# Warning: combining shell and terminate is risky!") > > > + print("# SIGTERM may not reach the child on zsh/ksh!") > > > + > > > > This warning did not print on my system, FWIW. Up to you if you want > > to respin and drop it or just leave it be. > > You mean when running this patchset? It shouldn't, the current > test uses ksft_wait rather than terminate. The warning is for > test developers trying to use the risky config in new tests. > > > I am fine with this warning being added, although I disagree with > > the commit message as mentioned in my previous email. > > I'll edit the commit message when applying. Unless you want to dig > deeper and find out why your bash/env is different than mine :( No, no - no need for that much extra work. Just wanted to mention for anyone following along or who stumbles on this in the future that my shell is bash, is all :) Thanks again for the work on this.
diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py index 9e3bcddcf3e8..33b153767d89 100644 --- a/tools/testing/selftests/net/lib/py/utils.py +++ b/tools/testing/selftests/net/lib/py/utils.py @@ -61,6 +61,10 @@ import time self.terminate = not exit_wait self.check_fail = fail + if shell and self.terminate: + print("# Warning: combining shell and terminate is risky!") + print("# SIGTERM may not reach the child on zsh/ksh!") + def __enter__(self): return self
Joe Damato reports that some shells will fork before running the command when python does "sh -c $cmd", while bash does an exec of $cmd directly. This will have implications for our ability to terminate the child process on bash vs other shells. Warn about using bkg(... shell=True, termininate=True) most background commands can hopefully exit cleanly (exit_wait). Link: https://lore.kernel.org/Z7Yld21sv_Ip3gQx@LQ3V64L9R2 Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- v2: new --- tools/testing/selftests/net/lib/py/utils.py | 4 ++++ 1 file changed, 4 insertions(+)