Exit with 0 on SIGTERM/SIGHUP/SIGINT

[davidBar-On]

• Version of iperf3 (or development branch, such as master or
  3.1-STABLE) to which this pull request applies:
  master

• Issues fixed (if any): Better daemon mode #1009

• Brief description of code changes (suitable for use as a commit message):

Exit with code 0 on termination because of SIGTERM. Termination because of all other signals remain with exit code 1.

[bjornfor]

I confirm that this fixes the issue -- when I run sudo systemctl stop iperf3 it stops cleanly now:

Feb 06 23:18:56 srv1 systemd[1]: Started iperf3 daemon.
Feb 06 23:19:05 srv1 iperf3[2413983]: iperf3: interrupt - the server has terminated by signal Terminated(15)
Feb 06 23:19:05 srv1 iperf3[2413983]: -----------------------------------------------------------
Feb 06 23:19:05 srv1 iperf3[2413983]: Server listening on 5201 (test #1)
Feb 06 23:19:05 srv1 iperf3[2413983]: -----------------------------------------------------------
Feb 06 23:19:05 srv1 systemd[1]: Stopping iperf3 daemon...
Feb 06 23:19:05 srv1 systemd[1]: iperf3.service: Deactivated successfully.
Feb 06 23:19:05 srv1 systemd[1]: Stopped iperf3 daemon.

[bmah888]

Thanks for the PR! One comment (well with two parts) in-line.

[bmah888]

Couple comments on this part:

1. iperf3 does identical signal handling for SIGINT, SIGTERM, and SIGHUP (per the function iperf_catch_sigend()). If we want to treat an exit because of a SIGTERM as a "non-error" exit, would it make sense to do the same for SIGINT and SIGHUP also?
2. In commit 0eb370d, we added conditional compilation to be sure these signal names were defined before using them (these changes were added as a part of PR Minor fix to improve the portability #935). Something similar might be necessary here as well. That PR has some more details on the environment where this was necessary, which seems somewhat different from a typical POSIX.1 environment.

[davidBar-On]

1. Added SIGHUP to the SIGTERM for the "non-error" exit. As I understand, SIGHUP is sent when the controlling process is terminated, so I assume any error conditions may be handled there. Regarding SIGINT I don't know what it the right handling. As its common use (if I understand correctly) is by the CLI "control-C" then the exit code may not be important. If you think it should be added I will do that. (Another approach could be adding an iperf3 option with the list of these signals, but it seems to be an overkill.)

2. Added #ifdef to SIGHUP and SIGTERM per commit 0eb370d.

[davidBar-On]

Added also SIGINT per @rathann comment.

[bmah888]

Thanks for the changes...I think this is probably the best behavior because iperf3 explicitly has code to gracefully exit for all three of those signals. So regardless of what causes something to send a signal to an iperf3 process, it'll behave consistently in all of those cases.

I'm got a question in to the perfSONAR team (our primary constituency) to confirm this isn't going to cause any problems for that use case before merging. I'm not expecting any problems.

[mfeit-internet2]

Greetings from the perfSONAR constituency. :-)

If perfSONAR sends iperf3 a signal, we already know the program is going to stop and don't care about how it exits, although the exit code might be noted in the diagnostics. If we run iperf3 and it stops before completing the job we asked of it, that should be reported as an error.

I can appreciate that SIGTERM is the accepted way to ask a program to end itself, but it's good for the parent to know how it was terminated rather than disguising it with a 0. The convention for uncaught signals is 128 plus the signal number, and I think iperf3 should follow that.

If the goal here is to make a Systemd unit happy with this kind of exit, the SuccessExitStatus directive can take a list of exit codes that are considered successful, e.g.:

SuccessExitStatus=0 SIGTERM

[bmah888]

Greetings from the perfSONAR constituency. :-)

If perfSONAR sends iperf3 a signal, we already know the program is going to stop and don't care about how it exits, although the exit code might be noted in the diagnostics. If we run iperf3 and it stops before completing the job we asked of it, that should be reported as an error.

I can appreciate that SIGTERM is the accepted way to ask a program to end itself, but it's good for the parent to know how it was terminated rather than disguising it with a 0. The convention for uncaught signals is 128 plus the signal number, and I think iperf3 should follow that.

If the goal here is to make a Systemd unit happy with this kind of exit, the SuccessExitStatus directive can take a list of exit codes that are considered successful, e.g.:

SuccessExitStatus=0 SIGTERM

Thanks for the feedback! I'm glad to know that perfSONAR (or rather pScheduler I guess) doesn't care.

iperf3 catches three signals so that it can exit cleanly. I think this behavior was intended to handle the user sending a SIGINT. As iperf3 stands now, the exit value in these cases is 1, which is the same as iperf3 flagging a user error of some kind (like an invalid parameter). It feels to me like 0 is better than 1 because iperf3 is explicitly handling those specific signals, exiting gracefully, and basically treating the situation as a normal exit. But I understand that it might be useful to tell the difference between "iperf3 finished on its own" and "iperf3 got a signal and gracefully terminated".

iperf3 follows the correct behavior for all other (uncaught) signals, it exits with 128 + signal number, and this PR doesn't change that AFAIK.

I think I've done the tweak to SuccessExitStatus in some other projects. It might not work 100% right in this case because as I indicated above, the iperf3 signal handler currently exits with the same exit code as a user error.

Argh. I admit I'm still fuzzy on what exactly the correct behavior should be. It feels to me like the latest of this PR makes things better though?

[bjornfor]

If the goal here is to make a Systemd unit happy with this kind of exit, the SuccessExitStatus directive can take a list of exit codes that are considered successful, e.g.:

SuccessExitStatus=0 SIGTERM

That doesn't work because iperf3 currently exits with 1 on SIGTERM, making it impossible to distinguish between successful and erronous exit.

Argh. I admit I'm still fuzzy on what exactly the correct behavior should be. It feels to me like the latest of this PR makes things better though?

Yes, this makes it better, because now a clean exit isn't mapped to error code '1'. But I'm fine with the exit code being 128 + 15 when receiving SIGTERM (15) too.