Date: Tue, 25 Jul 2023 18:23:08 +0200
From: Mirsad Todorovac <mirsad.todorovac@....unizg.hr>
To: Petr Machata <petrm@...dia.com>
Cc: Ido Schimmel <idosch@...dia.com>, netdev@...r.kernel.org,
 linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH v1 01/11] selftests: forwarding: custom_multipath_hash.sh:
 add cleanup for SIGTERM sent by timeout

On 7/25/23 10:44, Petr Machata wrote:
> 
> Mirsad Todorovac <mirsad.todorovac@....unizg.hr> writes:
> 
>> Add trap and cleanup for SIGTERM sent by timeout and SIGINT from
>> keyboard, for the test times out and leaves incoherent network stack.
>>
>> Fixes: 511e8db54036c ("selftests: forwarding: Add test for custom multipath hash")
>> Cc: Ido Schimmel <idosch@...dia.com>
>> Cc: netdev@...r.kernel.org
>> ---
>>   tools/testing/selftests/net/forwarding/custom_multipath_hash.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh b/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh
>> index 56eb83d1a3bd..c7ab883d2515 100755
>> --- a/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh
>> +++ b/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh
>> @@ -363,7 +363,7 @@ custom_hash()
>>   	custom_hash_v6
>>   }
>>   
>> -trap cleanup EXIT
>> +trap cleanup INT TERM EXIT
>>   
>>   setup_prepare
>>   setup_wait
> 
> I believe the EXIT trap covers whatever the cause of the exit was, i.e.
> INT and TERM are implicitly covered:
> 
>      $ vim tmp/x.sh
>      $ cat tmp/x.sh
>      foo() { date; }
>      trap foo EXIT
>      read -p Ready.
>      $ bash tmp/x.sh
>      Ready.^CTue Jul 25 10:44:20 AM CEST 2023

Thank you Petr for going to the bottom of this.

This is probably specific to bash. I tried to figure out from the manual,
but it wasn't so precise as direct experiment.

> Also, the interrupt trap seems to prevent the exit actually:
> 
>      $ cat tmp/x.sh
>      foo() { date; }
>      trap foo INT TERM EXIT
>      read -p Ready.
>      [petr@...iefel ~]$ bash tmp/x.sh
>      Ready.^CTue Jul 25 10:43:35 AM CEST 2023
>      ^CTue Jul 25 10:43:35 AM CEST 2023
>      ^CTue Jul 25 10:43:36 AM CEST 2023
>      ^CTue Jul 25 10:43:36 AM CEST 2023
> 
> (I see the same when I kill -TERM the script.)
> 
> This would call cleanup, which would dismantle the configuration, but
> then would happilly proceed in the script. I might be missing something,
> but I don't see how this can work.

Certainly, an explicit 'exit' from the 'cleanup' function would do.

It is bound to exit in any case, so explicit exit can't hurt. But if 'trap cleanup EXIT'
catches all cases, then my patch set is clearly obsoleted.

I didn't see the logic in EXIT catching SIGINT and SIGTERM when there are explicit
traps, but that's bash issue, not  selftest/net/forwarding issue :-)

I should apologise, but my understanding of the manuals went after the 'ash' semanthics
of the trap.

The manual does say:

       trap [-lp] [[arg] sigspec ...]
              The command arg is to be read and executed when the shell receives signal(s) sigspec.  If arg is absent (and there is a  single  sigspec)  or  -,  each
              specified signal is reset to its original disposition (the value it had upon entrance to the shell).  If arg is the null string the signal specified by
              each sigspec is ignored by the shell and by the commands it invokes.  If arg is not present and -p has been supplied, then the trap commands associated
              with  each  sigspec  are displayed.  If no arguments are supplied or if only -p is given, trap prints the list of commands associated with each signal.
              The -l option causes the shell to print a list of signal names and their corresponding numbers.  Each sigspec is either a signal name defined in  <sig‐
              nal.h>, or a signal number.  Signal names are case insensitive and the SIG prefix is optional.

              If  a  sigspec  is EXIT (0) the command arg is executed on exit from the shell.  If a sigspec is DEBUG, the command arg is executed before every simple
              command, for command, case command, select command, every arithmetic for command, and before the first command executes in a shell function (see  SHELL
              GRAMMAR above).  Refer to the description of the extdebug option to the shopt builtin for details of its effect on the DEBUG trap.  If a sigspec is RE‐
              TURN, the command arg is executed each time a shell function or a script executed with the . or source builtins finishes executing.

              If a sigspec is ERR, the command arg is executed whenever a pipeline (which may consist of a single simple command), a list, or a compound command  re‐
              turns  a non-zero exit status, subject to the following conditions.  The ERR trap is not executed if the failed command is part of the command list im‐
              mediately following a while or until keyword, part of the test in an if statement, part of a command executed in a && or ||  list  except  the  command
              following  the  final && or ||, any command in a pipeline but the last, or if the command's return value is being inverted using !.  These are the same
              conditions obeyed by the errexit (-e) option.

              Signals ignored upon entry to the shell cannot be trapped or reset.  Trapped signals that are not being ignored are reset to their original values in a
              subshell or subshell environment when one is created.  The return status is false if any sigspec is invalid; otherwise trap returns true.

Maybe "If  a  sigspec  is EXIT (0) the command arg is executed on exit from the shell." should
have had less assumptions on what is obvious to the reader?

But from this it wasn't obvious to me that EXIT will catch exit by signals SIGINT and SIGTERM.

Perhaps mostly because of the leftovers after cleanup()?

Still it seems impossible to run two consecutive test runs without an intermediate reboot.
('systemctl restart networking' didn't help on ubuntu 22.04).

It doesn't seem impossible to fix these, but I think you as authors and knowers of the Linux
networking stack will do a better job at it.

Though I seem to benefit from these brainstorming exercises as well :-)

Thanks
Mirsad

Powered by blists - more mailing lists