From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: [2/7] relayd: use imsg_get_data() and imsg_get_type()
To: Rafael Sadowski <rafael@sizeofvoid.org>
Cc: tech@openbsd.org
Date: Mon, 8 Jun 2026 15:14:59 +0200

On Sun, Jun 07, 2026 at 09:00:15AM +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 07fa9bca99ee3449460e7fa3e7207622056e62a9
> Author: Rafael Sadowski <rafael@sizeofvoid.org>
> Date:   Sat Jun 6 09:31:27 2026 +0200
> 
>     relayd: use imsg_get_data() and imsg_get_type()
>     
>     Replace IMSG_SIZE_CHECK() + memcpy()/bcopy() with imsg_get_data(),
>     which does the length check and copy in one call, and read the message
>     type via imsg_get_type() instead of imsg->hdr.type.
> 
> diff --git a/ca.c b/ca.c
> index c4f527f..9d4fc65 100644
> --- a/ca.c
> +++ b/ca.c
> @@ -193,7 +193,7 @@ ca_launch(void)
>  int
>  ca_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
>  {
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	case IMSG_CFG_RELAY:
>  		config_getrelay(env, imsg);
>  		break;
> @@ -226,7 +226,7 @@ ca_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  	struct iovec		 iov[2];
>  	int			 c = 0;
>  
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	case IMSG_CA_PRIVENC:
>  	case IMSG_CA_PRIVDEC:
>  		IMSG_SIZE_CHECK(imsg, (&cko));
> @@ -244,7 +244,7 @@ ca_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  			iov[c].iov_base = &cko;
>  			iov[c++].iov_len = sizeof(cko);
>  			if (proc_composev_imsg(env->sc_ps, PROC_RELAY,
> -			    cko.cko_proc, imsg->hdr.type, -1, -1, iov,
> +			    cko.cko_proc, imsg_get_type(imsg), -1, -1, iov,
>  			     c) == -1)
>  				log_warn("%s: proc_composev_imsg", __func__);
>  			break;
> @@ -260,7 +260,7 @@ ca_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  		if ((to = calloc(1, cko.cko_tlen)) == NULL)
>  			fatalx("%s: calloc", __func__);
>  
> -		switch (imsg->hdr.type) {
> +		switch (imsg_get_type(imsg)) {
>  		case IMSG_CA_PRIVENC:
>  			cko.cko_tlen = RSA_private_encrypt(cko.cko_flen,
>  			    from, to, rsa, cko.cko_padding);
> @@ -285,7 +285,7 @@ ca_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  		}
>  
>  		if (proc_composev_imsg(env->sc_ps, PROC_RELAY, cko.cko_proc,
> -		    imsg->hdr.type, -1, -1, iov, c) == -1)
> +		    imsg_get_type(imsg), -1, -1, iov, c) == -1)
>  			log_warn("%s: proc_composev_imsg", __func__);
>  
>  		free(to);

Have you considered to just store the imsg_get_type(imsg) once into a type
var and then use that one in the rest of ca_dispatch_relay?

> diff --git a/hce.c b/hce.c
> index 6598fea..4b82687 100644
> --- a/hce.c
> +++ b/hce.c
> @@ -289,7 +289,7 @@ hce_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
>  	struct host		*host;
>  	struct table		*table;
>  
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	case IMSG_HOST_DISABLE:
>  		memcpy(&id, imsg->data, sizeof(id));
>  		if ((host = host_find(env, id)) == NULL)
> @@ -342,10 +342,10 @@ hce_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
>  {
>  	struct ctl_script	 scr;
>  
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	case IMSG_SCRIPT:
> -		IMSG_SIZE_CHECK(imsg, &scr);
> -		bcopy(imsg->data, &scr, sizeof(scr));
> +		if (imsg_get_data(imsg, &scr, sizeof(scr)) == -1)
> +			return (-1);
>  		script_done(env, &scr);
>  		break;
>  	case IMSG_CFG_TABLE:
> @@ -373,7 +373,7 @@ hce_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
>  int
>  hce_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  {
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	default:
>  		break;
>  	}
> diff --git a/pfe.c b/pfe.c
> index 3d4b363..0f865b8 100644
> --- a/pfe.c
> +++ b/pfe.c
> @@ -126,10 +126,10 @@ pfe_dispatch_hce(int fd, struct privsep_proc *p, struct imsg *imsg)
>  
>  	control_imsg_forward(p->p_ps, imsg);
>  
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	case IMSG_HOST_STATUS:
> -		IMSG_SIZE_CHECK(imsg, &st);
> -		memcpy(&st, imsg->data, sizeof(st));
> +		if (imsg_get_data(imsg, &st, sizeof(st)) == -1)
> +			return (-1);
>  		if ((host = host_find(env, st.id)) == NULL)
>  			fatalx("%s: invalid host id", __func__);
>  		host->he = st.he;
> @@ -199,7 +199,7 @@ pfe_dispatch_hce(int fd, struct privsep_proc *p, struct imsg *imsg)
>  int
>  pfe_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
>  {
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	case IMSG_CFG_TABLE:
>  		config_gettable(env, imsg);
>  		break;
> @@ -260,10 +260,10 @@ pfe_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  	int			 cid;
>  	objid_t			 sid;
>  
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	case IMSG_NATLOOK:
> -		IMSG_SIZE_CHECK(imsg, &cnl);
> -		bcopy(imsg->data, &cnl, sizeof(cnl));
> +		if (imsg_get_data(imsg, &cnl, sizeof(cnl)) == -1)
> +			return (-1);
>  		if (cnl.proc > env->sc_conf.prefork_relay)
>  			fatalx("%s: invalid relay proc", __func__);
>  		if (natlook(env, &cnl) != 0)
> @@ -272,8 +272,8 @@ pfe_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  		    IMSG_NATLOOK, -1, -1, &cnl, sizeof(cnl));
>  		break;
>  	case IMSG_STATISTICS:
> -		IMSG_SIZE_CHECK(imsg, &crs);
> -		bcopy(imsg->data, &crs, sizeof(crs));
> +		if (imsg_get_data(imsg, &crs, sizeof(crs)) == -1)
> +			return (-1);
>  		if (crs.proc > env->sc_conf.prefork_relay)
>  			fatalx("%s: invalid relay proc", __func__);
>  		if ((rlay = relay_find(env, crs.id)) == NULL)
> @@ -283,8 +283,8 @@ pfe_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  		    env->sc_conf.statinterval.tv_sec;
>  		break;
>  	case IMSG_CTL_SESSION:
> -		IMSG_SIZE_CHECK(imsg, &con);
> -		memcpy(&con, imsg->data, sizeof(con));
> +		if (imsg_get_data(imsg, &con, sizeof(con)) == -1)
> +			return (-1);
>  		if ((c = control_connbyfd(con.se_cid)) == NULL) {
>  			log_debug("%s: control connection %d not found",
>  			    __func__, con.se_cid);
> @@ -295,8 +295,8 @@ pfe_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  		    &con, sizeof(con));
>  		break;
>  	case IMSG_CTL_END:
> -		IMSG_SIZE_CHECK(imsg, &cid);
> -		memcpy(&cid, imsg->data, sizeof(cid));
> +		if (imsg_get_data(imsg, &cid, sizeof(cid)) == -1)
> +			return (-1);
>  		if ((c = control_connbyfd(cid)) == NULL) {
>  			log_debug("%s: control connection %d not found",
>  			    __func__, cid);
> @@ -312,10 +312,10 @@ pfe_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  		}
>  		break;
>  	case IMSG_SESS_PUBLISH:
> -		IMSG_SIZE_CHECK(imsg, s);
>  		if ((s = calloc(1, sizeof(*s))) == NULL)
>  			return (0);		/* XXX */
> -		memcpy(s, imsg->data, sizeof(*s));
> +		if (imsg_get_data(imsg, s, sizeof(*s)) == -1)
> +			return (-1);
>  		TAILQ_FOREACH(t, &env->sc_sessions, se_entry) {
>  			/* duplicate registration */
>  			if (t->se_id == s->se_id) {
> @@ -331,8 +331,8 @@ pfe_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  			TAILQ_INSERT_TAIL(&env->sc_sessions, s, se_entry);
>  		break;
>  	case IMSG_SESS_UNPUBLISH:
> -		IMSG_SIZE_CHECK(imsg, &sid);
> -		memcpy(&sid, imsg->data, sizeof(sid));
> +		if (imsg_get_data(imsg, &sid, sizeof(sid)) == -1)
> +			return (-1);
>  		TAILQ_FOREACH(s, &env->sc_sessions, se_entry)
>  			if (s->se_id == sid)
>  				break;
> diff --git a/relay.c b/relay.c
> index a035f99..2d62607 100644
> --- a/relay.c
> +++ b/relay.c
> @@ -1868,7 +1868,7 @@ relay_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
>  	objid_t			 id;
>  	int			 cid;
>  
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	case IMSG_HOST_DISABLE:
>  		memcpy(&id, imsg->data, sizeof(id));
>  		if ((host = host_find(env, id)) == NULL)
> @@ -1907,8 +1907,8 @@ relay_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
>  			host->up = HOST_UNKNOWN;
>  		break;
>  	case IMSG_HOST_STATUS:
> -		IMSG_SIZE_CHECK(imsg, &st);
> -		memcpy(&st, imsg->data, sizeof(st));
> +		if (imsg_get_data(imsg, &st, sizeof(st)) == -1)
> +			return (-1);
>  		if ((host = host_find(env, st.id)) == NULL)
>  			fatalx("%s: invalid host id", __func__);
>  		if (host->flags & F_DISABLE)
> @@ -1953,8 +1953,8 @@ relay_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
>  		evtimer_add(&con->se_ev, &tv);
>  		break;
>  	case IMSG_CTL_SESSION:
> -		IMSG_SIZE_CHECK(imsg, &cid);
> -		memcpy(&cid, imsg->data, sizeof(cid));
> +		if (imsg_get_data(imsg, &cid, sizeof(cid)) == -1)
> +			return (-1);
>  		TAILQ_FOREACH(rlay, env->sc_relays, rl_entry) {
>  			SPLAY_FOREACH(con, session_tree,
>  			    &rlay->rl_sessions) {
> @@ -1977,11 +1977,11 @@ relay_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
>  int
>  relay_dispatch_ca(int fd, struct privsep_proc *p, struct imsg *imsg)
>  {
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	case IMSG_CA_PRIVENC:
>  	case IMSG_CA_PRIVDEC:
>  		log_warnx("%s: priv%s result after timeout", __func__,
> -		    imsg->hdr.type == IMSG_CA_PRIVENC ? "enc" : "dec");
> +		    imsg_get_type(imsg) == IMSG_CA_PRIVENC ? "enc" : "dec");
>  		return (0);
>  	}
>  
> @@ -1991,13 +1991,12 @@ relay_dispatch_ca(int fd, struct privsep_proc *p, struct imsg *imsg)
>  int
>  relay_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
>  {
> -	struct relay_ticket_key	 ticket;
>  	struct relay		*rlay;
>  	struct rsession		*con;
>  	struct timeval		 tv;
>  	objid_t			 id;
>  
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	case IMSG_BINDANY:
>  		bcopy(imsg->data, &id, sizeof(id));
>  		if ((con = session_find(env, id)) == NULL) {
> @@ -2045,8 +2044,9 @@ relay_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
>  		config_getreset(env, imsg);
>  		break;
>  	case IMSG_TLSTICKET_REKEY:
> -		IMSG_SIZE_CHECK(imsg, (&ticket));
> -		memcpy(&env->sc_ticket, imsg->data, sizeof(env->sc_ticket));
> +		if (imsg_get_data(imsg, &env->sc_ticket,
> +		    sizeof(env->sc_ticket)) == -1)
> +			return (-1);
>  		TAILQ_FOREACH(rlay, env->sc_relays, rl_entry) {
>  			if (rlay->rl_conf.flags & F_TLS)
>  				tls_config_add_ticket_key(rlay->rl_tls_cfg,
> @@ -2065,7 +2065,7 @@ relay_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
>  int
>  relay_dispatch_hce(int fd, struct privsep_proc *p, struct imsg *imsg)
>  {
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	default:
>  		break;
>  	}
> 

Lovely, OK claudio (apart from the little nit above).

-- 
:wq Claudio