Re: git: cf6356fd471c - main - daemon: repace goto exit with daemon_terminate()

On 18 Mar 2023, at 05:53, Kyle Evans <kevans@FreeBSD.org> wrote:
> 
> The branch main has been updated by kevans:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=cf6356fd471cdf4e5c52550f63599881d722962c
> 
> commit cf6356fd471cdf4e5c52550f63599881d722962c
> Author:     Ihor Antonov <ihor@antonovs.family>
> AuthorDate: 2023-03-18 05:31:12 +0000
> Commit:     Kyle Evans <kevans@FreeBSD.org>
> CommitDate: 2023-03-18 05:52:59 +0000
> 
>    daemon: repace goto exit with daemon_terminate()
> 
>    Start breaking down big main()
>    Remove goto exit label and replace it with a function that does cleanup.
> 
>    Comment re-worded by kevans@.
> 
>    Pull Request:   https://github.com/freebsd/freebsd-src/pull/694
> ---
> usr.sbin/daemon/daemon.c | 65 +++++++++++++++++++++++++++++-------------------
> 1 file changed, 40 insertions(+), 25 deletions(-)
> 
> diff --git a/usr.sbin/daemon/daemon.c b/usr.sbin/daemon/daemon.c
> index b215803cf3c6..2a57d021258e 100644
> --- a/usr.sbin/daemon/daemon.c
> +++ b/usr.sbin/daemon/daemon.c
> @@ -95,6 +95,8 @@ static void open_pid_files(struct daemon_state *);
> static void do_output(const unsigned char *, size_t, struct daemon_state *);
> static void daemon_sleep(time_t, long);
> static void daemon_state_init(struct daemon_state *);
> +static void daemon_terminate(struct daemon_state *);
> +

What’s with the whitespace?

> 
> static volatile sig_atomic_t terminate = 0;
> static volatile sig_atomic_t child_gone = 0;
> @@ -305,7 +307,7 @@ main(int argc, char *argv[])
> 	open_pid_files(&state);
> 	if (daemon(state.keep_cur_workdir, state.keep_fds_open) == -1) {
> 		warn("daemon");
> -		goto exit;
> +		daemon_terminate(&state);
> 	}
> 	/* Write out parent pidfile if needed. */
> 	pidfile_write(state.parent_pidfh);
> @@ -341,15 +343,15 @@ main(int argc, char *argv[])
> 		/* Block SIGTERM to avoid racing until we have forked. */
> 		if (sigprocmask(SIG_BLOCK, &mask_term, &mask_orig)) {
> 			warn("sigprocmask");
> -			goto exit;
> +			daemon_terminate(&state);
> 		}
> 		if (sigaction(SIGTERM, &act_term, NULL) == -1) {
> 			warn("sigaction");
> -			goto exit;
> +			daemon_terminate(&state);
> 		}
> 		if (sigaction(SIGCHLD, &act_chld, NULL) == -1) {
> 			warn("sigaction");
> -			goto exit;
> +			daemon_terminate(&state);
> 		}
> 		/*
> 		 * Try to protect against pageout kill. Ignore the
> @@ -360,7 +362,7 @@ main(int argc, char *argv[])
> 		if (state.log_reopen && state.output_fd >= 0 &&
> 		    sigaction(SIGHUP, &act_hup, NULL) == -1) {
> 			warn("sigaction");
> -			goto exit;
> +			daemon_terminate(&state);
> 		}
> restart:
> 		if (pipe(state.pipe_fd)) {
> @@ -376,7 +378,7 @@ restart:
> 	/* fork failed, this can only happen when supervision is enabled */
> 	if (pid == -1) {
> 		warn("fork");
> -		goto exit;
> +		daemon_terminate(&state);
> 	}
> 
> 
> @@ -425,7 +427,7 @@ restart:
> 	 */
> 	if (sigprocmask(SIG_UNBLOCK, &mask_term, NULL)) {
> 		warn("sigprocmask");
> -		goto exit;
> +		daemon_terminate(&state);
> 	}
> 	close(state.pipe_fd[1]);
> 	state.pipe_fd[1] = -1;
> @@ -458,34 +460,34 @@ restart:
> 		}
> 
> 		if (terminate) {
> -			goto exit;
> +			daemon_terminate(&state);
> 		}
> 
> 		if (state.child_eof) {
> 			if (sigprocmask(SIG_BLOCK, &mask_susp, NULL)) {
> 				warn("sigprocmask");
> -				goto exit;
> +				daemon_terminate(&state);
> 			}
> 			while (!terminate && !child_gone) {
> 				sigsuspend(&mask_orig);
> 			}
> 			if (sigprocmask(SIG_UNBLOCK, &mask_susp, NULL)) {
> 				warn("sigprocmask");
> -				goto exit;
> +				daemon_terminate(&state);
> 			}
> 			continue;
> 		}
> 
> 		if (sigprocmask(SIG_BLOCK, &mask_read, NULL)) {
> 			warn("sigprocmask");
> -			goto exit;
> +			daemon_terminate(&state);
> 		}
> 
> 		state.child_eof = !listen_child(state.pipe_fd[0], &state);
> 
> 		if (sigprocmask(SIG_UNBLOCK, &mask_read, NULL)) {
> 			warn("sigprocmask");
> -			goto exit;
> +			daemon_terminate(&state);
> 		}
> 
> 	}
> @@ -494,23 +496,14 @@ restart:
> 	}
> 	if (sigprocmask(SIG_BLOCK, &mask_term, NULL)) {
> 		warn("sigprocmask");
> -		goto exit;
> +		daemon_terminate(&state);
> 	}
> 	if (state.restart_enabled && !terminate) {
> 		close(state.pipe_fd[0]);
> 		state.pipe_fd[0] = -1;
> 		goto restart;
> 	}
> -exit:
> -	close(state.output_fd);
> -	close(state.pipe_fd[0]);
> -	close(state.pipe_fd[1]);
> -	if (state.syslog_enabled) {
> -		closelog();
> -	}
> -	pidfile_remove(state.child_pidfh);
> -	pidfile_remove(state.parent_pidfh);
> -	exit(1); /* If daemon(3) succeeded exit status does not matter. */
> +	daemon_terminate(&state);
> }
> 
> static void
> @@ -601,7 +594,7 @@ listen_child(int fd, struct daemon_state *state)
> 	static size_t bytes_read = 0;
> 	int rv;
> 
> -	assert(state);
> +	assert(state != NULL);
> 	assert(bytes_read < LBUF_SIZE - 1);
> 
> 	if (do_log_reopen) {
> @@ -659,7 +652,7 @@ static void
> do_output(const unsigned char *buf, size_t len, struct daemon_state *state)
> {
> 	assert(len <= LBUF_SIZE);
> -	assert(state);
> +	assert(state != NULL);
> 
> 	if (len < 1) {
> 		return;
> @@ -762,3 +755,25 @@ daemon_state_init(struct daemon_state *state)
> 		.output_filename = NULL,
> 	};
> }
> +
> +

Ditto.

Jess

> +static _Noreturn void
> +daemon_terminate(struct daemon_state *state)
> +{
> +	assert(state != NULL);
> +	close(state->output_fd);
> +	close(state->pipe_fd[0]);
> +	close(state->pipe_fd[1]);
> +	if (state->syslog_enabled) {
> +		closelog();
> +	}
> +	pidfile_remove(state->child_pidfh);
> +	pidfile_remove(state->parent_pidfh);
> +
> +	/*
> +	 * Note that the exit value here doesn't matter in the case of a clean
> +	 * exit; daemon(3) already detached us from the caller, nothing is left
> +	 * to care about this one.
> +	 */
> +	exit(1);
> +}