Index | Thread | Search

From:
Martijn van Duren <openbsd+tech@list.imperialat.at>
Subject:
Re: iked/snmpd: Implement new imsg API for proc.c
To:
tech@openbsd.org
Date:
Thu, 2 Jul 2026 10:32:21 +0200

Download raw body.

Thread
On 7/2/26 10:21 AM, hshoexer wrote:
> On Wed, Jul 01, 2026 at 08:20:55PM +0200, Martijn van Duren wrote:
>> Hello tech@,
>>
>> Similar to a diff relayd's proc.c r1.54, and a pending diff for httpd by
>> rsadowski@, here's a diff to implement the new imsg API inside iked, and
>> snmpd, bringing the implementations a bit closer once again.
>>
>> OK?
> 
> see two white space comments inline below.  Otherwise ok hshoexer@

Thnx, these came from the httpd diff. I've already committed this diff
with the OK rsadowski@. I've fixed this for httpd in the sync diff I
just send out, and will be fixed when syncing iked/snmpd with
httpd/relayd.
> 
> fwiw: In snmpd proc_forward_imsg() is not used at all.

My goal is to have proc.c in sync as much as possible, so that fixes to
one can be applied to others without issue.
> 
>> martijn@
>>
>> diff d010102fe4efde3ee95164581695909c7c6b672a d99e3d210e154f2f1322afa2aa01cecfa7172ebf
>> commit - d010102fe4efde3ee95164581695909c7c6b672a
>> commit + d99e3d210e154f2f1322afa2aa01cecfa7172ebf
>> blob - 7bba4cba8ec28d87db99f7ca6a1b47495f86396b
>> blob + 35a34480c04d6125e64ec3ba2d623b392ab71146
>> --- sbin/iked/control.c
>> +++ sbin/iked/control.c
>> @@ -310,7 +310,7 @@ control_dispatch_imsg(int fd, short event, void *arg)
>>  			memcpy(&v, imsg.data, sizeof(v));
>>  			log_setverbose(v);
>>  
>> -			proc_forward_imsg(&env->sc_ps, &imsg, PROC_PARENT, -1);
>> +			proc_forward_imsg(&env->sc_ps, &imsg, PROC_PARENT);
>>  			break;
>>  		case IMSG_CTL_RELOAD:
>>  		case IMSG_CTL_RESET:
>> @@ -318,17 +318,17 @@ control_dispatch_imsg(int fd, short event, void *arg)
>>  		case IMSG_CTL_DECOUPLE:
>>  		case IMSG_CTL_ACTIVE:
>>  		case IMSG_CTL_PASSIVE:
>> -			proc_forward_imsg(&env->sc_ps, &imsg, PROC_PARENT, -1);
>> +			proc_forward_imsg(&env->sc_ps, &imsg, PROC_PARENT);
>>  			break;
>>  		case IMSG_CTL_RESET_ID:
>> -			proc_forward_imsg(&env->sc_ps, &imsg, PROC_IKEV2, -1);
>> +			proc_forward_imsg(&env->sc_ps, &imsg, PROC_IKEV2);
>>  			break;
>>  		case IMSG_CTL_SHOW_SA:
>>  		case IMSG_CTL_SHOW_STATS:
>> -			proc_forward_imsg(&env->sc_ps, &imsg, PROC_IKEV2, -1);
>> +			proc_forward_imsg(&env->sc_ps, &imsg, PROC_IKEV2);
>>  			break;
>>  		case IMSG_CTL_SHOW_CERTSTORE:
>> -			proc_forward_imsg(&env->sc_ps, &imsg, PROC_CERT, -1);
>> +			proc_forward_imsg(&env->sc_ps, &imsg, PROC_CERT);
>>  			break;
>>  		default:
>>  			log_debug("%s: error handling imsg %d",
>> blob - 8feaadef5073b9f8311e5f9226a320f3bbca624f
>> blob + d361d8f5abcbcdbdaa683330ef2702ab050c5be0
>> --- sbin/iked/iked.c
>> +++ sbin/iked/iked.c
>> @@ -434,7 +434,7 @@ parent_dispatch_ca(int fd, struct privsep_proc *p, str
>>  	switch (imsg->hdr.type) {
>>  	case IMSG_CTL_ACTIVE:
>>  	case IMSG_CTL_PASSIVE:
>> -		proc_forward_imsg(&env->sc_ps, imsg, PROC_IKEV2, -1);
>> +		proc_forward_imsg(&env->sc_ps, imsg, PROC_IKEV2);
>>  		break;
>>  	case IMSG_OCSP_FD:
>>  		ocsp_connect(env, imsg);
>> @@ -473,8 +473,8 @@ parent_dispatch_control(int fd, struct privsep_proc *p
>>  		free(str);
>>  		break;
>>  	case IMSG_CTL_VERBOSE:
>> -		proc_forward_imsg(&env->sc_ps, imsg, PROC_IKEV2, -1);
>> -		proc_forward_imsg(&env->sc_ps, imsg, PROC_CERT, -1);
>> +		proc_forward_imsg(&env->sc_ps, imsg, PROC_IKEV2);
>> +		proc_forward_imsg(&env->sc_ps, imsg, PROC_CERT);
>>  
>>  		/* return 1 to let proc.c handle it locally */
>>  		return (1);
>> blob - 367ea76047ae3d7ec009bd36cab0e3bbeb9ec36a
>> blob + 9d0aa6cbd06ad919737bf59c91a38dd91f956b9e
>> --- sbin/iked/iked.h
>> +++ sbin/iked/iked.h
>> @@ -1333,8 +1333,8 @@ int	 proc_composev_imsg(struct privsep *, enum privsep
>>  	    uint16_t, uint32_t, int, const struct iovec *, int);
>>  int	 proc_composev(struct privsep *, enum privsep_procid,
>>  	    uint16_t, const struct iovec *, int);
>> -int	 proc_forward_imsg(struct privsep *, struct imsg *,
>> -	    enum privsep_procid, int);
>> +void	 proc_forward_imsg(struct privsep *, struct imsg *,
>> +	    enum privsep_procid);
>>  struct imsgbuf *
>>  	 proc_ibuf(struct privsep *, enum privsep_procid, int);
>>  struct imsgev *
>> blob - 7db459f43bd5a9cdd3f1200f78543470ef2d57c7
>> blob + 56737effa0689ab1dd4221337bd2233974b49625
>> --- sbin/iked/proc.c
>> +++ sbin/iked/proc.c
>> @@ -615,7 +615,7 @@ proc_dispatch(int fd, short event, void *arg)
>>  	struct imsgbuf		*ibuf;
>>  	struct imsg		 imsg;
>>  	ssize_t			 n;
>> -	int			 verbose;
>> +	int			 ver;
>>  	const char		*title;
>>  	struct privsep_fd	 pf;
>>  
>> @@ -623,9 +623,11 @@ proc_dispatch(int fd, short event, void *arg)
>>  	ibuf = &iev->ibuf;
>>  
>>  	if (event & EV_READ) {
>> -		if ((n = imsgbuf_read(ibuf)) == -1)
>> +		switch (imsgbuf_read(ibuf)) {
>> +		case -1:
>>  			fatal("%s: imsgbuf_read", __func__);
>> -		if (n == 0) {
>> +			break;
>> +		case 0:
>>  			/* this pipe is dead, so remove the event handler */
>>  			event_del(&iev->ev);
>>  			event_loopexit(NULL);
>> @@ -645,15 +647,16 @@ proc_dispatch(int fd, short event, void *arg)
>>  	}
>>  
>>  	for (;;) {
>> -		if ((n = imsg_get(ibuf, &imsg)) == -1)
>> -			fatal("%s: imsg_get", __func__);
>> +		if ((n = imsgbuf_get(ibuf, &imsg)) == -1)
>> +			fatal("%s: imsgbuf_get", __func__);
>>  		if (n == 0)
>>  			break;
>>  
>>  #if DEBUG > 1
>>  		log_debug("%s: %s %d got imsg %d peerid %d from %s %d",
>>  		    __func__, title, ps->ps_instance + 1,
>> -		    imsg.hdr.type, imsg.hdr.peerid, p->p_title, imsg.hdr.pid);
>> +		    imsg_get_type(&imsg), imsg_get_id(&imsg),
>> +		    p->p_title, imsg_get_pid(&imsg));
>>  #endif
>>  
>>  		/*
>> @@ -668,11 +671,12 @@ proc_dispatch(int fd, short event, void *arg)
>>  		/*
>>  		 * Generic message handling
>>  		 */
>> -		switch (imsg.hdr.type) {
>> +		switch (imsg_get_type(&imsg)) {
>>  		case IMSG_CTL_VERBOSE:
>> -			IMSG_SIZE_CHECK(&imsg, &verbose);
>> -			memcpy(&verbose, imsg.data, sizeof(verbose));
>> -			log_setverbose(verbose);
>> +			if (imsg_get_data(&imsg, &ver, sizeof(ver)) == -1)
>> +			       fatalx("%s: imsg_get_data", __func__);
> 
> spaces instead of tab.
> 
>> +
>> +			log_setverbose(ver);
>>  			break;
>>  		case IMSG_CTL_PROCFD:
>>  			if (p->p_id != PROC_PARENT) {
>> @@ -680,8 +684,10 @@ proc_dispatch(int fd, short event, void *arg)
>>  				    "IMSG_CTL_PROCFD from %s",
>>  				    __func__, p->p_title);
>>  			}
>> -			IMSG_SIZE_CHECK(&imsg, &pf);
>> -			memcpy(&pf, imsg.data, sizeof(pf));
>> +
>> +			if (imsg_get_data(&imsg, &pf, sizeof(pf)) == -1)
>> +				fatalx("%s: imsg_get_data", __func__);
>> +
>>  			proc_accept(ps, imsg_get_fd(&imsg), pf.pf_procid,
>>  			    pf.pf_instance);
>>  			break;
>> @@ -835,12 +841,18 @@ proc_composev(struct privsep *ps, enum privsep_procid 
>>  	return (proc_composev_imsg(ps, id, -1, type, -1, -1, iov, iovcnt));
>>  }
>>  
>> -int
>> +void
>>  proc_forward_imsg(struct privsep *ps, struct imsg *imsg,
>> -    enum privsep_procid id, int n)
>> +    enum privsep_procid id)
>>  {
>> -	return (proc_compose_imsg(ps, id, n, imsg->hdr.type,
>> -	    imsg->hdr.peerid, -1, imsg->data, IMSG_DATA_SIZE(imsg)));
>> +	int	 m, n = -1;
>> +
>> +	proc_range(ps, id, &n, &m);
>> +	for (; n < m; n++) {
>> +		if (imsg_forward(&ps->ps_ievs[id][n].ibuf, imsg) == -1)
>> +			fatal("%s: imsg_forward", __func__);
>> +		imsg_event_add(&ps->ps_ievs[id][n]);
>> +	}
>>  }
>>  
>>  struct imsgbuf *
>> blob - e0ae3f6adb3a22c7b8b272311d994be93efed4fe
>> blob + fbe3a7c2f3487d829e32c1d539732e895bc5844b
>> --- usr.sbin/snmpd/proc.c
>> +++ usr.sbin/snmpd/proc.c
>> @@ -606,7 +606,7 @@ proc_dispatch(int fd, short event, void *arg)
>>  	struct imsgbuf		*ibuf;
>>  	struct imsg		 imsg;
>>  	ssize_t			 n;
>> -	int			 verbose;
>> +	int			 ver;
>>  	const char		*title;
>>  	struct privsep_fd	 pf;
>>  
>> @@ -614,9 +614,11 @@ proc_dispatch(int fd, short event, void *arg)
>>  	ibuf = &iev->ibuf;
>>  
>>  	if (event & EV_READ) {
>> -		if ((n = imsgbuf_read(ibuf)) == -1)
>> +		switch (imsgbuf_read(ibuf)) {
>> +		case -1:
>>  			fatal("%s: imsgbuf_read", __func__);
>> -		if (n == 0) {
>> +			break;
>> +		case 0:
>>  			/* this pipe is dead, so remove the event handler */
>>  			event_del(&iev->ev);
>>  			event_loopexit(NULL);
>> @@ -636,15 +638,16 @@ proc_dispatch(int fd, short event, void *arg)
>>  	}
>>  
>>  	for (;;) {
>> -		if ((n = imsg_get(ibuf, &imsg)) == -1)
>> -			fatal("%s: imsg_get", __func__);
>> +		if ((n = imsgbuf_get(ibuf, &imsg)) == -1)
>> +			fatal("%s: imsgbuf_get", __func__);
>>  		if (n == 0)
>>  			break;
>>  
>>  #if DEBUG > 1
>>  		log_debug("%s: %s %d got imsg %d peerid %d from %s %d",
>>  		    __func__, title, ps->ps_instance + 1,
>> -		    imsg.hdr.type, imsg.hdr.peerid, p->p_title, imsg.hdr.pid);
>> +		    imsg_get_type(&imsg), imsg_get_id(&imsg),
>> +		    p->p_title, imsg_get_pid(&imsg));
>>  #endif
>>  
>>  		/*
>> @@ -659,11 +662,12 @@ proc_dispatch(int fd, short event, void *arg)
>>  		/*
>>  		 * Generic message handling
>>  		 */
>> -		switch (imsg.hdr.type) {
>> +		switch (imsg_get_type(&imsg)) {
>>  		case IMSG_CTL_VERBOSE:
>> -			IMSG_SIZE_CHECK(&imsg, &verbose);
>> -			memcpy(&verbose, imsg.data, sizeof(verbose));
>> -			log_setverbose(verbose);
>> +			if (imsg_get_data(&imsg, &ver, sizeof(ver)) == -1)
>> +			       fatalx("%s: imsg_get_data", __func__);
> 
> spaces instead of tab.
> 
>> +
>> +			log_setverbose(ver);
>>  			break;
>>  		case IMSG_CTL_PROCFD:
>>  			if (p->p_id != PROC_PARENT || ps->ps_run == NULL) {
>> @@ -671,8 +675,10 @@ proc_dispatch(int fd, short event, void *arg)
>>  				    "IMSG_CTL_PROCFD from %s",
>>  				    __func__, p->p_title);
>>  			}
>> -			IMSG_SIZE_CHECK(&imsg, &pf);
>> -			memcpy(&pf, imsg.data, sizeof(pf));
>> +
>> +			if (imsg_get_data(&imsg, &pf, sizeof(pf)) == -1)
>> +				fatalx("%s: imsg_get_data", __func__);
>> +
>>  			proc_accept(ps, imsg_get_fd(&imsg), pf.pf_procid,
>>  			    pf.pf_instance);
>>  			break;
>> @@ -829,12 +835,18 @@ proc_composev(struct privsep *ps, enum privsep_procid 
>>  	return (proc_composev_imsg(ps, id, -1, type, -1, -1, iov, iovcnt));
>>  }
>>  
>> -int
>> +void
>>  proc_forward_imsg(struct privsep *ps, struct imsg *imsg,
>> -    enum privsep_procid id, int n)
>> +    enum privsep_procid id)
>>  {
>> -	return (proc_compose_imsg(ps, id, n, imsg->hdr.type,
>> -	    imsg->hdr.peerid, -1, imsg->data, IMSG_DATA_SIZE(imsg)));
>> +	int	 m, n = -1;
>> +
>> +	proc_range(ps, id, &n, &m);
>> +	for (; n < m; n++) {
>> +		if (imsg_forward(&ps->ps_ievs[id][n].ibuf, imsg) == -1)
>> +			fatal("%s: imsg_forward", __func__);
>> +		imsg_event_add(&ps->ps_ievs[id][n]);
>> +	}
>>  }
>>  
>>  struct imsgbuf *
>> blob - 1db290b765c0beff7e5b7680e6a13e29a648c4cd
>> blob + 1db6a3d3ad412fc3bca80571a6ad465430ea4cc6
>> --- usr.sbin/snmpd/snmpd.h
>> +++ usr.sbin/snmpd/snmpd.h
>> @@ -519,8 +519,8 @@ int	 proc_composev_imsg(struct privsep *, enum privsep
>>  	    u_int16_t, u_int32_t, int, const struct iovec *, int);
>>  int	 proc_composev(struct privsep *, enum privsep_procid,
>>  	    uint16_t, const struct iovec *, int);
>> -int	 proc_forward_imsg(struct privsep *, struct imsg *,
>> -	    enum privsep_procid, int);
>> +void	 proc_forward_imsg(struct privsep *, struct imsg *,
>> +	    enum privsep_procid);
>>  struct imsgbuf *
>>  	 proc_ibuf(struct privsep *, enum privsep_procid, int);
>>  struct imsgev *
>>
>