From: Omar Polo <op@omarpolo.com>
Subject: Re: gotd: defer some checks while starting
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Tue, 17 Jan 2023 16:15:14 +0100

On 2023/01/17 15:27:51 +0100, Omar Polo <op@omarpolo.com> wrote:
> I'm a bit afraid to send this diff right after 0.80 has been tagged,
> but I've noticed this stuff only now that I was playing with the rc
> script.  (since now i've only ever run gotd from the command line with
> -v.)
> 
> The absolute path check is quite annoying when one just wants to see
> if the configuration is fine (i often do this manually.)  log_init
> could also be called a bit later, so that we always show the error for
> the configuration:
> 
> before the diff
> 
> 	% doas /usr/local/sbin/gotd -n
> 	% echo $?
> 	1
> 	% doas /usr/local/sbin/gotd -n -d
> 	gotd: /etc/gotd.conf:1: syntax error
> 
> after:
> 
> 	% make && doas ./gotd -n
> 	gotd: /etc/gotd.conf:1: syntax error
> 
> While here I was tempted to also print a message if the configuration
> is OK (like httpd does for example) and to tweak an error message:
> 
> % doas /usr/local/sbin/gotd -d -n
> gotd: : cannot run gotd as root: the user running gotd must not be the superuser
> % make && doas ./gotd -n
> gotd: : cannot run gotd as the superuser
> 
> ok?

a slightly better diff.  it's equivalent to the previous but by moving
the block where we set gotd.argv = argv0 below too makes easier to see
that it gets assigned to 'valid' data.

diff /tmp/got
commit - f08fa56da9b67c42d726230b936ee664a5ce0e42
path + /tmp/got
blob - 717b4729fac0259057de6bd02cadff694a709e40
file + gotd/gotd.c
--- gotd/gotd.c
+++ gotd/gotd.c
@@ -1758,24 +1758,12 @@ main(int argc, char **argv)
 	if (argc != 0)
 		usage();
 
-	/* Require an absolute path in argv[0] for reliable re-exec. */
-	if (!got_path_is_absolute(argv0))
-		fatalx("bad path \"%s\": must be an absolute path", argv0);
-
 	if (geteuid() && (proc_id == PROC_GOTD || proc_id == PROC_LISTEN))
 		fatalx("need root privileges");
 
-	log_init(daemonize ? 0 : 1, LOG_DAEMON);
-	log_setverbose(verbosity);
-
 	if (parse_config(confpath, proc_id, &gotd) != 0)
 		return 1;
 
-	gotd.argv0 = argv0;
-	gotd.daemonize = daemonize;
-	gotd.verbosity = verbosity;
-	gotd.confpath = confpath;
-
 	if (proc_id == PROC_GOTD &&
 	    (gotd.nrepos == 0 || TAILQ_EMPTY(&gotd.repos)))
 		fatalx("no repository defined in configuration file");
@@ -1784,20 +1772,31 @@ main(int argc, char **argv)
 	if (pw == NULL)
 		fatalx("user %s not found", gotd.user_name);
 
-	if (pw->pw_uid == 0) {
-		fatalx("cannot run %s as %s: the user running %s "
-		    "must not be the superuser",
-		    getprogname(), pw->pw_name, getprogname());
-	}
+	if (pw->pw_uid == 0)
+		fatalx("cannot run %s as the superuser", getprogname());
 
 	if (proc_id == PROC_LISTEN &&
 	    !got_path_is_absolute(gotd.unix_socket_path))
 		fatalx("bad unix socket path \"%s\": must be an absolute path",
 		    gotd.unix_socket_path);
 
-	if (noaction)
+	if (noaction) {
+		fprintf(stderr, "configuration OK\n");
 		return 0;
+	}
 
+	gotd.argv0 = argv0;
+	gotd.daemonize = daemonize;
+	gotd.verbosity = verbosity;
+	gotd.confpath = confpath;
+
+	/* Require an absolute path in argv[0] for reliable re-exec. */
+	if (!got_path_is_absolute(argv0))
+		fatalx("bad path \"%s\": must be an absolute path", argv0);
+
+	log_init(daemonize ? 0 : 1, LOG_DAEMON);
+	log_setverbose(verbosity);
+
 	if (proc_id == PROC_GOTD) {
 		gotd.pid = getpid();
 		snprintf(title, sizeof(title), "%s", gotd_proc_names[proc_id]);