Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 18 Mar 2023 05:58:04 +0000
From:      Jessica Clarke <jrtc27@freebsd.org>
To:        Kyle Evans <kevans@FreeBSD.org>
Cc:        "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org>
Subject:   Re: git: cf6356fd471c - main - daemon: repace goto exit with daemon_terminate()
Message-ID:  <FC6F996B-A761-4E1B-BF25-A950D60A44D7@freebsd.org>
In-Reply-To: <202303180553.32I5rfRu088924@gitrepo.freebsd.org>
References:  <202303180553.32I5rfRu088924@gitrepo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On 18 Mar 2023, at 05:53, Kyle Evans <kevans@FreeBSD.org> wrote:
>=20
> The branch main has been updated by kevans:
>=20
> URL: =
https://cgit.FreeBSD.org/src/commit/?id=3Dcf6356fd471cdf4e5c52550f63599881=
d722962c
>=20
> 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
>=20
>    daemon: repace goto exit with daemon_terminate()
>=20
>    Start breaking down big main()
>    Remove goto exit label and replace it with a function that does =
cleanup.
>=20
>    Comment re-worded by kevans@.
>=20
>    Pull Request:   https://github.com/freebsd/freebsd-src/pull/694
> ---
> usr.sbin/daemon/daemon.c | 65 =
+++++++++++++++++++++++++++++-------------------
> 1 file changed, 40 insertions(+), 25 deletions(-)
>=20
> 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=E2=80=99s with the whitespace?

>=20
> static volatile sig_atomic_t terminate =3D 0;
> static volatile sig_atomic_t child_gone =3D 0;
> @@ -305,7 +307,7 @@ main(int argc, char *argv[])
> 	open_pid_files(&state);
> 	if (daemon(state.keep_cur_workdir, state.keep_fds_open) =3D=3D =
-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) =3D=3D -1) {
> 			warn("sigaction");
> -			goto exit;
> +			daemon_terminate(&state);
> 		}
> 		if (sigaction(SIGCHLD, &act_chld, NULL) =3D=3D -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 >=3D 0 &&
> 		    sigaction(SIGHUP, &act_hup, NULL) =3D=3D -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 =3D=3D -1) {
> 		warn("fork");
> -		goto exit;
> +		daemon_terminate(&state);
> 	}
>=20
>=20
> @@ -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] =3D -1;
> @@ -458,34 +460,34 @@ restart:
> 		}
>=20
> 		if (terminate) {
> -			goto exit;
> +			daemon_terminate(&state);
> 		}
>=20
> 		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;
> 		}
>=20
> 		if (sigprocmask(SIG_BLOCK, &mask_read, NULL)) {
> 			warn("sigprocmask");
> -			goto exit;
> +			daemon_terminate(&state);
> 		}
>=20
> 		state.child_eof =3D !listen_child(state.pipe_fd[0], =
&state);
>=20
> 		if (sigprocmask(SIG_UNBLOCK, &mask_read, NULL)) {
> 			warn("sigprocmask");
> -			goto exit;
> +			daemon_terminate(&state);
> 		}
>=20
> 	}
> @@ -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] =3D -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);
> }
>=20
> static void
> @@ -601,7 +594,7 @@ listen_child(int fd, struct daemon_state *state)
> 	static size_t bytes_read =3D 0;
> 	int rv;
>=20
> -	assert(state);
> +	assert(state !=3D NULL);
> 	assert(bytes_read < LBUF_SIZE - 1);
>=20
> 	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 <=3D LBUF_SIZE);
> -	assert(state);
> +	assert(state !=3D NULL);
>=20
> 	if (len < 1) {
> 		return;
> @@ -762,3 +755,25 @@ daemon_state_init(struct daemon_state *state)
> 		.output_filename =3D NULL,
> 	};
> }
> +
> +

Ditto.

Jess

> +static _Noreturn void
> +daemon_terminate(struct daemon_state *state)
> +{
> +	assert(state !=3D 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);
> +}




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?FC6F996B-A761-4E1B-BF25-A950D60A44D7>