Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exit with 0 on SIGTERM/SIGHUP/SIGINT #1829

Merged
merged 3 commits into from
Feb 21, 2025
Merged

Exit with 0 on SIGTERM/SIGHUP/SIGINT #1829

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4bab9bc
Exit with 0 on SIGTERM
davidBar-On Feb 6, 2025
7e8a19f
Per reviwer comment added SIGHUP to the non-error signals and added #…
davidBar-On Feb 8, 2025
46af7e9
Add also SIGINT to the non-error signals
davidBar-On Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/iperf_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4845,7 +4845,7 @@ iperf_catch_sigend(void (*handler)(int))
* before cleaning up and exiting.
*/
void
iperf_got_sigend(struct iperf_test *test)
iperf_got_sigend(struct iperf_test *test, int sig)
{
/*
* If we're the client, or if we're a server and running a test,
Expand All @@ -4868,7 +4868,11 @@ iperf_got_sigend(struct iperf_test *test)
(void) Nwrite(test->ctrl_sck, (char*) &test->state, sizeof(signed char), Ptcp);
}
i_errno = (test->role == 'c') ? IECLIENTTERM : IESERVERTERM;
iperf_errexit(test, "interrupt - %s", iperf_strerror(i_errno));
if (sig == SIGTERM) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added also SIGINT per @rathann comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

iperf_signormalexit(test, "interrupt - %s by signal %s(%d)", iperf_strerror(i_errno), strsignal(sig), sig);
} else {
iperf_errexit(test, "interrupt - %s by signal %s(%d)", iperf_strerror(i_errno), strsignal(sig), sig);
}
}

/* Try to write a PID file if requested, return -1 on an error. */
Expand Down
4 changes: 3 additions & 1 deletion src/iperf_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ void iperf_check_throttle(struct iperf_stream *sp, struct iperf_time *nowP);
int iperf_send_mt(struct iperf_stream *) /* __attribute__((hot)) */;
int iperf_recv_mt(struct iperf_stream *);
void iperf_catch_sigend(void (*handler)(int));
void iperf_got_sigend(struct iperf_test *test) __attribute__ ((noreturn));
void iperf_got_sigend(struct iperf_test *test, int sig) __attribute__ ((noreturn));
void usage(void);
void usage_long(FILE * f);
void warning(const char *);
Expand Down Expand Up @@ -380,6 +380,8 @@ int iflush(struct iperf_test *test);
/* Error routines. */
void iperf_err(struct iperf_test *test, const char *format, ...) __attribute__ ((format(printf,2,3)));
void iperf_errexit(struct iperf_test *test, const char *format, ...) __attribute__ ((format(printf,2,3),noreturn));
void iperf_signormalexit(struct iperf_test *test, const char *format, ...) __attribute__ ((format(printf,2,3),noreturn));
void iperf_exit(struct iperf_test *test, int exit_code, const char *format, va_list argp) __attribute__ ((noreturn));
char *iperf_strerror(int);
extern int i_errno;
enum {
Expand Down
24 changes: 21 additions & 3 deletions src/iperf_error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,30 @@ iperf_err(struct iperf_test *test, const char *format, ...)
va_end(argp);
}

/* Do a printf to stderr or log file as appropriate, then exit. */
/* Do a printf to stderr or log file as appropriate, then exit(0). */
void
iperf_signormalexit(struct iperf_test *test, const char *format, ...)
{
va_list argp;

va_start(argp, format);
iperf_exit(test, 0, format, argp);
}

/* Do a printf to stderr or log file as appropriate, then exit(1). */
void
iperf_errexit(struct iperf_test *test, const char *format, ...)
{
va_list argp;

va_start(argp, format);
iperf_exit(test, 1, format, argp);
}

/* Do a printf to stderr or log file as appropriate, then exit. */
void
iperf_exit(struct iperf_test *test, int exit_code, const char *format, va_list argp)
{
char str[1000];
time_t now;
struct tm *ltm = NULL;
Expand All @@ -103,7 +122,6 @@ iperf_errexit(struct iperf_test *test, const char *format, ...)
ct = iperf_timestrerr;
}

va_start(argp, format);
vsnprintf(str, sizeof(str), format, argp);
if (test != NULL && test->json_output) {
if (test->json_top != NULL) {
Expand Down Expand Up @@ -136,7 +154,7 @@ iperf_errexit(struct iperf_test *test, const char *format, ...)
va_end(argp);
if (test)
iperf_delete_pidfile(test);
exit(1);
exit(exit_code);
}

int i_errno;
Expand Down
4 changes: 3 additions & 1 deletion src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,12 @@ main(int argc, char **argv)


static jmp_buf sigend_jmp_buf;
static int signed_sig;

static void __attribute__ ((noreturn))
sigend_handler(int sig)
{
signed_sig = sig;
longjmp(sigend_jmp_buf, 1);
}

Expand All @@ -145,7 +147,7 @@ run(struct iperf_test *test)
/* Termination signals. */
iperf_catch_sigend(sigend_handler);
if (setjmp(sigend_jmp_buf))
iperf_got_sigend(test);
iperf_got_sigend(test, signed_sig);

/* Ignore SIGPIPE to simplify error handling */
signal(SIGPIPE, SIG_IGN);
Expand Down