From: Mark Jamsek <mark@jamsek.com>
Subject: Re: gotd: defer some checks while starting
To: Omar Polo <op@omarpolo.com>
Cc: Stefan Sperling <stsp@stsp.name>, gameoftrees@openbsd.org
Date: Wed, 18 Jan 2023 10:13:45 +1100

On 23-01-17 06:55PM, Omar Polo wrote:
> On 2023/01/17 17:43:25 +0100, Omar Polo <op@omarpolo.com> wrote:
> > since i was there, what about moving some checks inside parse_config?
> > 
> > it slightly improves the error message if `listen on socket' is not an
> > absolute path.
> 
> i can't send a diff on the first try today it seems.
> 
> after walking away from the keyboard i thought that having the
> got_path_is_absolute check only in the listen process was a waste: if
> the value is wrong, report it directly from the main process without
> waiting to spawning the listener.  It's also nicer because it's caught
> at `gotd -n' time:
> 
> 	% cat /etc/gotd.conf
> 	user op
> 	listen on 'gotd.sock'
> 	repository 'ports' {
> 	        path '/home/op/git/ports.git'
> 	        permit ro gotdev
> 	}
> 	% doas gotd -n
> 	configuration OK
> 	% make && doas ./obj/gotd -n
> 	gotd: /etc/gotd.conf:2: bad unix socket path "gotd.sock": \
> 	    must be an absolute path
> 
> ok?

yes, ok

I also really like the config OK hint you added, thanks!

> diff /tmp/got
> commit - f4e8c21cb2dc6a468bae32a4dcf3a9e18f269527
> path + /tmp/got
> blob - 070e37a9d1342993266222f1483916a70450a206
> file + gotd/gotd.c
> --- gotd/gotd.c
> +++ gotd/gotd.c
> @@ -1764,10 +1764,6 @@ main(int argc, char **argv)
>  	if (parse_config(confpath, proc_id, &gotd) != 0)
>  		return 1;
>  
> -	if (proc_id == PROC_GOTD &&
> -	    (gotd.nrepos == 0 || TAILQ_EMPTY(&gotd.repos)))
> -		fatalx("no repository defined in configuration file");
> -
>  	pw = getpwnam(gotd.user_name);
>  	if (pw == NULL)
>  		fatalx("user %s not found", gotd.user_name);
> @@ -1775,11 +1771,6 @@ main(int argc, char **argv)
>  	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) {
>  		fprintf(stderr, "configuration OK\n");
>  		return 0;
> blob - 9be73f985cd4b495f70ba089cee603ee8afc861d
> file + gotd/parse.y
> --- gotd/parse.y
> +++ gotd/parse.y
> @@ -195,6 +195,10 @@ main		: LISTEN ON STRING {
>  		;
>  
>  main		: LISTEN ON STRING {
> +			if (!got_path_is_absolute($3))
> +				yyerror("bad unix socket path \"%s\": "
> +				    "must be an absolute path", $3);
> +
>  			if (gotd_proc_id == PROC_LISTEN) {
>  				if (strlcpy(gotd->unix_socket_path, $3,
>  				    sizeof(gotd->unix_socket_path)) >=
> @@ -744,6 +748,11 @@ parse_config(const char *filename, enum gotd_procid pr
>  		}
>  	}
>  
> +	if (proc_id == PROC_GOTD && TAILQ_EMPTY(&gotd->repos)) {
> +		log_warnx("no repository defined in configuration file");
> +		return (-1);
> +	}
> +
>  	return (0);
>  }
>  
> 

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68