Index | Thread | Search

From:
Rafael Sadowski <rafael@sizeofvoid.org>
Subject:
Re: {relayd,httpd}/proc.c: sync trivial bits
To:
Martijn van Duren <openbsd+tech@list.imperialat.at>
Cc:
tech@openbsd.org
Date:
Thu, 2 Jul 2026 19:18:16 +0200

Download raw body.

Thread
On Thu Jul 02, 2026 at 10:04:35AM +0200, Martijn van Duren wrote:
> Hello tech@,
> 
> This syncs mostly whitespace, comments, and fatal() messages between
> httpd and relayd. The only functional change is a conditional inside
> proc_kill(), which is also present in iked/snmpd.
> 
> With this diff in addition to the control diff send earlier the only
> relevant difference between the two is a conditional preventing sharing
> of socketpairs.
> 
> OK?

Hi Martijn

The most recent formatting changes were made by knfmt(1) in relayd. I
would prefer to keep them, as I think they make sense and I would like a
knfmt(1) workflow that allows me(tm) to simply avoid formatting errors and
achieve clean, consistent formatting.

In my view, therefore, only the __func__ fatal make sense.

On the other hand I know where you're going with this.

> 
> martijn@
> 
> diff /usr/src
> path + /usr/src
> commit - fb32c8576ecb0ad5dc3bca5db208b3b6d6f92346
> blob - 5b952631d4d6f44cda5c614c89f4effe5ae99f31
> file + usr.sbin/httpd/proc.c
> --- usr.sbin/httpd/proc.c
> +++ usr.sbin/httpd/proc.c
> @@ -94,7 +94,7 @@ proc_exec(struct privsep *ps, struct privsep_proc *pro
>  	/* Point process instance arg to stack and copy the original args. */
>  	nargv[nargc++] = "-I";
>  	nargv[nargc++] = num;
> -	for (i = 1; i < (unsigned int) argc; i++)
> +	for (i = 1; i < (unsigned int)argc; i++)
>  		nargv[nargc++] = argv[i];
>  
>  	nargv[nargc] = NULL;
> @@ -158,7 +158,7 @@ proc_connect(struct privsep *ps)
>  			iev = &ps->ps_ievs[dst][inst];
>  			if (imsgbuf_init(&iev->ibuf,
>  			    ps->ps_pp->pp_pipes[dst][inst]) == -1)
> -				fatal(NULL);
> +				fatal("%s: imsgbuf_init", __func__);
>  			imsgbuf_allow_fdpass(&iev->ibuf);
>  			event_set(&iev->ev, iev->ibuf.fd, iev->events,
>  			    iev->handler, iev->data);
> @@ -271,7 +271,7 @@ proc_accept(struct privsep *ps, int fd, enum privsep_p
>  
>  	iev = &ps->ps_ievs[dst][n];
>  	if (imsgbuf_init(&iev->ibuf, fd) == -1)
> -		fatal(NULL);
> +		fatal("%s: imsgbuf_init", __func__);
>  	imsgbuf_allow_fdpass(&iev->ibuf);
>  	event_set(&iev->ev, iev->ibuf.fd, iev->events, iev->handler, iev->data);
>  	event_add(&iev->ev, NULL);
> @@ -385,7 +385,7 @@ proc_kill(struct privsep *ps)
>  			free(cause);
>  		} else
>  			log_warnx("lost child: pid %u", pid);
> -	} while (pid != -1 || errno == EINTR);
> +	} while (pid != -1 || (pid == -1 && errno == EINTR));
>  }
>  
>  void
> @@ -435,7 +435,7 @@ proc_open(struct privsep *ps, int src, int dst)
>  			 */
>  			if (proc_flush_imsg(ps, src, i) == -1 ||
>  			    proc_flush_imsg(ps, dst, j) == -1)
> -				fatal("%s: imsgbuf_flush", __func__);
> +				fatal("%s: proc_flush_imsg", __func__);
>  		}
>  	}
>  }
> @@ -603,13 +603,12 @@ proc_dispatch(int fd, short event, void *arg)
>  
>  	if (event & EV_WRITE) {
>  		if (imsgbuf_write(ibuf) == -1) {
> -			if (errno == EPIPE) {	/* connection closed */
> -				/* remove the event handler */
> +			if (errno == EPIPE) {	/* Connection closed. */
>  				event_del(&iev->ev);
>  				event_loopexit(NULL);
>  				return;
> -			} else
> -				fatal("%s: imsgbuf_write", __func__);
> +			}
> +			fatal("%s: imsgbuf_write", __func__);
>  		}
>  	}
>  
> @@ -641,7 +640,7 @@ proc_dispatch(int fd, short event, void *arg)
>  		switch (imsg_get_type(&imsg)) {
>  		case IMSG_CTL_VERBOSE:
>  			if (imsg_get_data(&imsg, &ver, sizeof(ver)) == -1)
> -			       fatalx("%s: imsg_get_data", __func__);
> +				fatalx("%s: imsg_get_data", __func__);
>  
>  			log_setverbose(ver);
>  			break;
> @@ -656,7 +655,7 @@ proc_dispatch(int fd, short event, void *arg)
>  				fatalx("%s: imsg_get_data", __func__);
>  
>  			proc_accept(ps, imsg_get_fd(&imsg), pf.pf_procid,
> -				    pf.pf_instance);
> +			    pf.pf_instance);
>  			break;
>  		default:
>  			fatalx("%s: %s %d got invalid imsg %d peerid %d "
> commit - fb32c8576ecb0ad5dc3bca5db208b3b6d6f92346
> blob - 3c03720d4ce022039937a283eeacc9ebbb12e2a2
> file + usr.sbin/relayd/proc.c
> --- usr.sbin/relayd/proc.c
> +++ usr.sbin/relayd/proc.c
> @@ -38,11 +38,11 @@
>  #include "log.h"
>  
>  void	 proc_exec(struct privsep *, struct privsep_proc *, unsigned int, int,
> -    char **);
> +	    char **);
>  void	 proc_setup(struct privsep *, struct privsep_proc *, unsigned int);
>  void	 proc_open(struct privsep *, int, int);
>  void	 proc_accept(struct privsep *, int, enum privsep_procid,
> -    unsigned int);
> +	    unsigned int);
>  void	 proc_close(struct privsep *);
>  void	 proc_shutdown(struct privsep_proc *);
>  void	 proc_sig_handler(int, short, void *);
> @@ -71,11 +71,11 @@ void
>  proc_exec(struct privsep *ps, struct privsep_proc *procs, unsigned int nproc,
>      int argc, char **argv)
>  {
> -	unsigned int		  proc, nargc, i, proc_i;
> +	unsigned int		 proc, nargc, i, proc_i;
>  	char			**nargv;
> -	struct privsep_proc	 *p;
> -	char			  num[32];
> -	int			  fd;
> +	struct privsep_proc	*p;
> +	char			 num[32];
> +	int			 fd;
>  
>  	/* Prepare the new process argv. */
>  	nargv = calloc(argc + 5, sizeof(char *));
> @@ -120,8 +120,8 @@ proc_exec(struct privsep *ps, struct privsep_proc *pro
>  			case 0:
>  				/* Prepare parent socket. */
>  				if (fd != PROC_PARENT_SOCK_FILENO) {
> -					if (dup2(fd, PROC_PARENT_SOCK_FILENO) ==
> -					    -1)
> +					if (dup2(fd, PROC_PARENT_SOCK_FILENO)
> +					    == -1)
>  						fatal("dup2");
>  				} else if (fcntl(fd, F_SETFD, 0) == -1)
>  					fatal("fcntl");
> @@ -158,7 +158,7 @@ proc_connect(struct privsep *ps)
>  			iev = &ps->ps_ievs[dst][inst];
>  			if (imsgbuf_init(&iev->ibuf,
>  			    ps->ps_pp->pp_pipes[dst][inst]) == -1)
> -				fatal("imsgbuf_init");
> +				fatal("%s: imsgbuf_init", __func__);
>  			imsgbuf_allow_fdpass(&iev->ibuf);
>  			event_set(&iev->ev, iev->ibuf.fd, iev->events,
>  			    iev->handler, iev->data);
> @@ -271,7 +271,7 @@ proc_accept(struct privsep *ps, int fd, enum privsep_p
>  
>  	iev = &ps->ps_ievs[dst][n];
>  	if (imsgbuf_init(&iev->ibuf, fd) == -1)
> -		fatal("imsgbuf_init");
> +		fatal("%s: imsgbuf_init", __func__);
>  	imsgbuf_allow_fdpass(&iev->ibuf);
>  	event_set(&iev->ev, iev->ibuf.fd, iev->events, iev->handler, iev->data);
>  	event_add(&iev->ev, NULL);
> @@ -616,8 +616,7 @@ proc_dispatch(int fd, short event, void *arg)
>  
>  	if (event & EV_WRITE) {
>  		if (imsgbuf_write(ibuf) == -1) {
> -			if (errno == EPIPE) {
> -				/* this pipe is dead, remove the handler */
> +			if (errno == EPIPE) {	/* Connection closed. */
>  				event_del(&iev->ev);
>  				event_loopexit(NULL);
>  				return;
>