From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: [3/7] relayd: convert control imsg forwarding to imsg_forward()
To: Rafael Sadowski <rafael@sizeofvoid.org>
Cc: tech@openbsd.org
Date: Mon, 8 Jun 2026 15:20:06 +0200

On Sun, Jun 07, 2026 at 09:01:24AM +0200, Rafael Sadowski wrote:
> This is a series of commits for an imsg APIv2 rework in relayd. These
> were committed and tested individually. They can be reviewed in their
> entirety here:
> 
> gothub: https://rsadowski.gothub.org/?action=summary&headref=imsg_v2&path=relayd.git
> PR: https://codeberg.org/rsadowski/relayd/pulls/1/files
> 
> commit cdac3e43ca7856c92948a16de7bd0e4ec3e12f26
> Author: Rafael Sadowski <rafael@sizeofvoid.org>
> Date:   Sat Jun 6 16:22:49 2026 +0200
> 
>     relayd: convert control imsg forwarding to imsg_forward()
>     
>     Rework control_imsg_forward() to forward the message unaltered via
>     imsg_forward() instead of rebuilding it with imsg_compose_event().
>     
>     read the type via imsg_get_type(), dropping the manual header-length
>     Switch to use read the payload with imsg_get_data() and checks and the
>     memcpy() that wrote the data back into the imsg before forwarding.
> 
> diff --git a/control.c b/control.c
> index 6c29f2b..fe829ce 100644
> --- a/control.c
> +++ b/control.c
> @@ -228,7 +228,7 @@ control_dispatch_imsg(int fd, short event, void *arg)
>  	struct imsg		 imsg;
>  	struct ctl_id		 id;
>  	int			 n;
> -	int			 verbose;
> +	int			 verb;
>  	struct relayd		*env = cs->cs_env;
>  	struct privsep		*ps = env->sc_ps;
>  
> @@ -238,7 +238,11 @@ control_dispatch_imsg(int fd, short event, void *arg)
>  	}
>  
>  	if (event & EV_READ) {
> -		if (imsgbuf_read(&c->iev.ibuf) != 1) {
> +		switch (imsgbuf_read(&c->iev.ibuf)) {
> +		case -1:
> +			fatal("%s: imsgbuf_read", __func__);
> +			break;
> +		case 0:
>  			control_close(fd, cs);
>  			return;
>  		}
> @@ -262,13 +266,13 @@ control_dispatch_imsg(int fd, short event, void *arg)
>  
>  		if (c->waiting) {
>  			log_debug("%s: unexpected imsg %d",
> -			    __func__, imsg.hdr.type);
> +			    __func__, imsg_get_type(&imsg));
>  			imsg_free(&imsg);
>  			control_close(fd, cs);
>  			return;
>  		}
>  
> -		switch (imsg.hdr.type) {
> +		switch (imsg_get_type(&imsg)) {
>  		case IMSG_CTL_SHOW_SUM:
>  			show(c);
>  			break;
> @@ -276,85 +280,79 @@ control_dispatch_imsg(int fd, short event, void *arg)
>  			show_sessions(c);
>  			break;
>  		case IMSG_CTL_RDR_DISABLE:
> -			if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof(id))
> -				fatalx("invalid imsg header len");
> -			memcpy(&id, imsg.data, sizeof(id));
> +			if (imsg_get_data(&imsg, &id, sizeof(id)) == -1)
> +				fatalx("%s: imsg_get_data", __func__);
> +
>  			if (disable_rdr(c, &id))
>  				imsg_compose_event(&c->iev, IMSG_CTL_FAIL,
>  				    0, ps->ps_instance + 1, -1, NULL, 0);
>  			else {
> -				memcpy(imsg.data, &id, sizeof(id));
> -				control_imsg_forward(ps, &imsg);
> +				control_imsg_forward(&imsg);
>  				imsg_compose_event(&c->iev, IMSG_CTL_OK,
>  				    0, ps->ps_instance + 1, -1, NULL, 0);
>  			}
>  			break;
>  		case IMSG_CTL_RDR_ENABLE:
> -			if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof(id))
> -				fatalx("invalid imsg header len");
> -			memcpy(&id, imsg.data, sizeof(id));
> +			if (imsg_get_data(&imsg, &id, sizeof(id)) == -1)
> +				fatalx("%s: imsg_get_data", __func__);
> +
>  			if (enable_rdr(c, &id))
>  				imsg_compose_event(&c->iev, IMSG_CTL_FAIL,
>  				    0, ps->ps_instance + 1, -1, NULL, 0);
>  			else {
> -				memcpy(imsg.data, &id, sizeof(id));
> -				control_imsg_forward(ps, &imsg);
> +				control_imsg_forward(&imsg);
>  				imsg_compose_event(&c->iev, IMSG_CTL_OK,
>  				    0, ps->ps_instance + 1, -1, NULL, 0);
>  			}
>  			break;
>  		case IMSG_CTL_TABLE_DISABLE:
> -			if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof(id))
> -				fatalx("invalid imsg header len");
> -			memcpy(&id, imsg.data, sizeof(id));
> +			if (imsg_get_data(&imsg, &id, sizeof(id)) == -1)
> +				fatalx("%s: imsg_get_data", __func__);
> +
>  			if (disable_table(c, &id))
>  				imsg_compose_event(&c->iev, IMSG_CTL_FAIL,
>  				    0, ps->ps_instance + 1, -1, NULL, 0);
>  			else {
> -				memcpy(imsg.data, &id, sizeof(id));
> -				control_imsg_forward(ps, &imsg);
> +				control_imsg_forward(&imsg);
>  				imsg_compose_event(&c->iev, IMSG_CTL_OK,
>  				    0, ps->ps_instance + 1, -1, NULL, 0);
>  			}
>  			break;
>  		case IMSG_CTL_TABLE_ENABLE:
> -			if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof(id))
> -				fatalx("invalid imsg header len");
> -			memcpy(&id, imsg.data, sizeof(id));
> +			if (imsg_get_data(&imsg, &id, sizeof(id)) == -1)
> +				fatalx("%s: imsg_get_data", __func__);
> +
>  			if (enable_table(c, &id))
>  				imsg_compose_event(&c->iev, IMSG_CTL_FAIL,
>  				    0, ps->ps_instance + 1, -1, NULL, 0);
>  			else {
> -				memcpy(imsg.data, &id, sizeof(id));
> -				control_imsg_forward(ps, &imsg);
> +				control_imsg_forward(&imsg);
>  				imsg_compose_event(&c->iev, IMSG_CTL_OK,
>  				    0, ps->ps_instance + 1, -1, NULL, 0);
>  			}
>  			break;
>  		case IMSG_CTL_HOST_DISABLE:
> -			if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof(id))
> -				fatalx("invalid imsg header len");
> -			memcpy(&id, imsg.data, sizeof(id));
> +			if (imsg_get_data(&imsg, &id, sizeof(id)) == -1)
> +				fatalx("%s: imsg_get_data", __func__);
> +
>  			if (disable_host(c, &id, NULL))
>  				imsg_compose_event(&c->iev, IMSG_CTL_FAIL,
>  				    0, ps->ps_instance + 1, -1, NULL, 0);
>  			else {
> -				memcpy(imsg.data, &id, sizeof(id));
> -				control_imsg_forward(ps, &imsg);
> +				control_imsg_forward(&imsg);
>  				imsg_compose_event(&c->iev, IMSG_CTL_OK,
>  				    0, ps->ps_instance + 1, -1, NULL, 0);
>  			}
>  			break;
>  		case IMSG_CTL_HOST_ENABLE:
> -			if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof(id))
> -				fatalx("invalid imsg header len");
> -			memcpy(&id, imsg.data, sizeof(id));
> +			if (imsg_get_data(&imsg, &id, sizeof(id)) == -1)
> +				fatalx("%s: imsg_get_data", __func__);
> +
>  			if (enable_host(c, &id, NULL))
>  				imsg_compose_event(&c->iev, IMSG_CTL_FAIL,
>  				    0, ps->ps_instance + 1, -1, NULL, 0);
>  			else {
> -				memcpy(imsg.data, &id, sizeof(id));
> -				control_imsg_forward(ps, &imsg);
> +				control_imsg_forward(&imsg);
>  				imsg_compose_event(&c->iev, IMSG_CTL_OK,
>  				    0, ps->ps_instance + 1, -1, NULL, 0);
>  			}
> @@ -381,21 +379,19 @@ control_dispatch_imsg(int fd, short event, void *arg)
>  			c->flags |= CTL_CONN_NOTIFY;
>  			break;
>  		case IMSG_CTL_VERBOSE:
> -			IMSG_SIZE_CHECK(&imsg, &verbose);
> -
> -			memcpy(&verbose, imsg.data, sizeof(verbose));
> +			if (imsg_get_data(&imsg, &verb, sizeof(verb)) == -1)
> +				fatalx("%s: imsg_get_data", __func__);
>  
>  			proc_forward_imsg(env->sc_ps, &imsg, PROC_PARENT);
>  			proc_forward_imsg(env->sc_ps, &imsg, PROC_HCE);
>  			proc_forward_imsg(env->sc_ps, &imsg, PROC_RELAY);
>  
> -			memcpy(imsg.data, &verbose, sizeof(verbose));
> -			control_imsg_forward(ps, &imsg);
> -			log_setverbose(verbose);
> +			control_imsg_forward(&imsg);
> +			log_setverbose(verb);
>  			break;
>  		default:
>  			log_debug("%s: error handling imsg %d",
> -			    __func__, imsg.hdr.type);
> +			    __func__, imsg_get_type(&imsg));
>  			break;
>  		}
>  		imsg_free(&imsg);
> @@ -405,13 +401,15 @@ control_dispatch_imsg(int fd, short event, void *arg)
>  }
>  
>  void
> -control_imsg_forward(struct privsep *ps, struct imsg *imsg)
> +control_imsg_forward(struct imsg *imsg)
>  {
> -	struct ctl_conn *c;
> +        struct ctl_conn *c;
>  
> -	TAILQ_FOREACH(c, &ctl_conns, entry)
> -		if (c->flags & CTL_CONN_NOTIFY)
> -			imsg_compose_event(&c->iev, imsg->hdr.type,
> -			    0, ps->ps_instance + 1, -1, imsg->data,
> -			    imsg->hdr.len - IMSG_HEADER_SIZE);
> +	TAILQ_FOREACH(c, &ctl_conns, entry) {
> +		if (c->flags & CTL_CONN_NOTIFY) {
> +			if (imsg_forward(&c->iev.ibuf, imsg) == -1)
> +				fatal("%s: imsg_forward", __func__);
> +			imsg_event_add(&c->iev);
> +		}
> +	}
>  }
> diff --git a/pfe.c b/pfe.c
> index 0f865b8..0856ae9 100644
> --- a/pfe.c
> +++ b/pfe.c
> @@ -124,7 +124,7 @@ pfe_dispatch_hce(int fd, struct privsep_proc *p, struct imsg *imsg)
>  	struct table		*table;
>  	struct ctl_status	 st;
>  
> -	control_imsg_forward(p->p_ps, imsg);
> +	control_imsg_forward(imsg);
>  
>  	switch (imsg_get_type(imsg)) {
>  	case IMSG_HOST_STATUS:
> @@ -737,7 +737,7 @@ pfe_sync(void)
>  			imsg.hdr.len = sizeof(id) + IMSG_HEADER_SIZE;
>  			imsg.data = &id;
>  			sync_table(env, rdr, active);
> -			control_imsg_forward(env->sc_ps, &imsg);
> +			control_imsg_forward(&imsg);
>  		}
>  
>  		if (rdr->conf.flags & F_DOWN) {
> @@ -750,7 +750,7 @@ pfe_sync(void)
>  				imsg.hdr.len = sizeof(id) + IMSG_HEADER_SIZE;
>  				imsg.data = &id;
>  				sync_ruleset(env, rdr, 0);
> -				control_imsg_forward(env->sc_ps, &imsg);
> +				control_imsg_forward(&imsg);
>  			}
>  		} else if (!(rdr->conf.flags & F_ACTIVE_RULESET)) {
>  			log_debug("%s: enabling ruleset", __func__);
> @@ -760,7 +760,7 @@ pfe_sync(void)
>  			imsg.hdr.len = sizeof(id) + IMSG_HEADER_SIZE;
>  			imsg.data = &id;
>  			sync_ruleset(env, rdr, 1);
> -			control_imsg_forward(env->sc_ps, &imsg);
> +			control_imsg_forward(&imsg);
>  		}
>  	}
>  
> diff --git a/relayd.h b/relayd.h
> index 16fe2bf..373255e 100644
> --- a/relayd.h
> +++ b/relayd.h
> @@ -1154,7 +1154,7 @@ int	 control_init(struct privsep *, struct control_sock *);
>  int	 control_listen(struct control_sock *);
>  void	 control_cleanup(struct control_sock *);
>  void	 control_dispatch_imsg(int, short, void *);
> -void	 control_imsg_forward(struct privsep *ps, struct imsg *);
> +void	 control_imsg_forward(struct imsg *);
>  struct ctl_conn	*
>  	 control_connbyfd(int);
>  
> 

OK claudio@
-- 
:wq Claudio