Re: relayd: use imsg_get_type and imsg_get_type

On Tue Jul 28, 2026 at 09:37:30PM +0200, Theo Buehler wrote:
> On Sat, Jul 25, 2026 at 07:23:56PM +0200, Rafael Sadowski wrote:
> > OK?
> > 
> > commit 0b7c23a563ac96506f78c660a431a3e3d6f2d020
> > Author: Rafael Sadowski <[email protected]>
> > Date:   Sat Jul 25 19:20:21 2026 +0200
> > 
> >     relayd: use imsg_get_type and imsg_get_type
> 
> Feels like two commits (one for relayd.c and one for the other two)
> and the second one concerns imsg_get_data, not imsg_get_type.

I'll commit it in two different commits.

> 
> > 
> > diff --git a/hce.c b/hce.c
> > index 82f3003..05c1555 100644
> > --- a/hce.c
> > +++ b/hce.c
> > @@ -291,7 +291,8 @@ hce_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
> >  
> >  	switch (imsg_get_type(imsg)) {
> >  	case IMSG_HOST_DISABLE:
> > -		memcpy(&id, imsg->data, sizeof(id));
> > +		if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
> > +			return (-1);
> 
> It might be preferable to fatal explicitly rather than having readers go
> look at the reykian proc.c where it falls through to another fatalx()
> in proc_dispatch().

Yes, it's certainly better for debugging.

> 
> If that's intentional deduplication, I guess it's fine, it's just a bit
> confusing for the casual reader.
> 

That wasn't the intention. I can see how it might have been confusing to review.



diff --git a/hce.c b/hce.c
index 82f3003..2d63d26 100644
--- a/hce.c
+++ b/hce.c
@@ -291,7 +291,8 @@ hce_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
 
 	switch (imsg_get_type(imsg)) {
 	case IMSG_HOST_DISABLE:
-		memcpy(&id, imsg->data, sizeof(id));
+		if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		if ((host = host_find(env, id)) == NULL)
 			fatalx("%s: desynchronized", __func__);
 		host->flags |= F_DISABLE;
@@ -301,7 +302,8 @@ hce_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
 		host->he = HCE_NONE;
 		break;
 	case IMSG_HOST_ENABLE:
-		memcpy(&id, imsg->data, sizeof(id));
+		if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		if ((host = host_find(env, id)) == NULL)
 			fatalx("%s: desynchronized", __func__);
 		host->flags &= ~(F_DISABLE);
@@ -309,7 +311,8 @@ hce_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
 		host->he = HCE_NONE;
 		break;
 	case IMSG_TABLE_DISABLE:
-		memcpy(&id, imsg->data, sizeof(id));
+		if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		if ((table = table_find(env, id)) == NULL)
 			fatalx("%s: desynchronized", __func__);
 		table->conf.flags |= F_DISABLE;
@@ -317,7 +320,8 @@ hce_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
 			host->up = HOST_UNKNOWN;
 		break;
 	case IMSG_TABLE_ENABLE:
-		memcpy(&id, imsg->data, sizeof(id));
+		if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		if ((table = table_find(env, id)) == NULL)
 			fatalx("%s: desynchronized", __func__);
 		table->conf.flags &= ~(F_DISABLE);
@@ -345,7 +349,7 @@ hce_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
 	switch (imsg_get_type(imsg)) {
 	case IMSG_SCRIPT:
 		if (imsg_get_data(imsg, &scr, sizeof(scr)) == -1)
-			return (-1);
+			fatalx("%s: imsg_get_data", __func__);
 		script_done(env, &scr);
 		break;
 	case IMSG_CFG_TABLE:
diff --git a/relay.c b/relay.c
index 412ae74..70f66c3 100644
--- a/relay.c
+++ b/relay.c
@@ -1870,7 +1870,8 @@ relay_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
 
 	switch (imsg_get_type(imsg)) {
 	case IMSG_HOST_DISABLE:
-		memcpy(&id, imsg->data, sizeof(id));
+		if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		if ((host = host_find(env, id)) == NULL)
 			fatalx("%s: desynchronized", __func__);
 		if ((table = table_find(env, host->conf.tableid)) ==
@@ -1882,14 +1883,16 @@ relay_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
 		host->up = HOST_UNKNOWN;
 		break;
 	case IMSG_HOST_ENABLE:
-		memcpy(&id, imsg->data, sizeof(id));
+		if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		if ((host = host_find(env, id)) == NULL)
 			fatalx("%s: desynchronized", __func__);
 		host->flags &= ~(F_DISABLE);
 		host->up = HOST_UNKNOWN;
 		break;
 	case IMSG_TABLE_DISABLE:
-		memcpy(&id, imsg->data, sizeof(id));
+		if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		if ((table = table_find(env, id)) == NULL)
 			fatalx("%s: desynchronized", __func__);
 		table->conf.flags |= F_DISABLE;
@@ -1898,7 +1901,8 @@ relay_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
 			host->up = HOST_UNKNOWN;
 		break;
 	case IMSG_TABLE_ENABLE:
-		memcpy(&id, imsg->data, sizeof(id));
+		if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		if ((table = table_find(env, id)) == NULL)
 			fatalx("%s: desynchronized", __func__);
 		table->conf.flags &= ~(F_DISABLE);
@@ -1908,7 +1912,7 @@ relay_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
 		break;
 	case IMSG_HOST_STATUS:
 		if (imsg_get_data(imsg, &st, sizeof(st)) == -1)
-			return (-1);
+			fatalx("%s: imsg_get_data", __func__);
 		if ((host = host_find(env, st.id)) == NULL)
 			fatalx("%s: invalid host id", __func__);
 		if (host->flags & F_DISABLE)
@@ -1939,7 +1943,8 @@ relay_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
 		host->up = st.up;
 		break;
 	case IMSG_NATLOOK:
-		bcopy(imsg->data, &cnl, sizeof(cnl));
+		if (imsg_get_data(imsg, &cnl, sizeof(cnl)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		if ((con = session_find(env, cnl.id)) == NULL ||
 		    con->se_cnl == NULL) {
 			log_debug("%s: session %d: expired",
@@ -1954,7 +1959,7 @@ relay_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
 		break;
 	case IMSG_CTL_SESSION:
 		if (imsg_get_data(imsg, &cid, sizeof(cid)) == -1)
-			return (-1);
+			fatalx("%s: imsg_get_data", __func__);
 		TAILQ_FOREACH(rlay, env->sc_relays, rl_entry) {
 			SPLAY_FOREACH(con, session_tree,
 			    &rlay->rl_sessions) {
@@ -2001,7 +2006,8 @@ relay_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
 
 	switch (imsg_get_type(imsg)) {
 	case IMSG_BINDANY:
-		bcopy(imsg->data, &id, sizeof(id));
+		if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		if ((con = session_find(env, id)) == NULL) {
 			log_debug("%s: session %d: expired",
 			    __func__, id);
@@ -2058,7 +2064,7 @@ relay_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
 	case IMSG_TLSTICKET_REKEY:
 		if (imsg_get_data(imsg, &env->sc_ticket,
 		    sizeof(env->sc_ticket)) == -1)
-			return (-1);
+			fatalx("%s: imsg_get_data", __func__);
 		TAILQ_FOREACH(rlay, env->sc_relays, rl_entry) {
 			if (rlay->rl_conf.flags & F_TLS)
 				tls_config_add_ticket_key(rlay->rl_tls_cfg,
diff --git a/relayd.c b/relayd.c
index 7b8af3b..b174fc8 100644
--- a/relayd.c
+++ b/relayd.c
@@ -407,7 +407,7 @@ parent_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
 	char			*str = NULL;
 	size_t			 s;
 
-	switch (imsg->hdr.type) {
+	switch (imsg_get_type(imsg)) {
 	case IMSG_DEMOTE:
 		if (imsg_get_data(imsg, &demote, sizeof(demote)) == -1) {
 			log_warn("%s: imsg_get_data", __func__);
@@ -469,7 +469,7 @@ parent_dispatch_hce(int fd, struct privsep_proc *p, struct imsg *imsg)
 	struct relayd		*env = ps->ps_env;
 	struct ctl_script	 scr;
 
-	switch (imsg->hdr.type) {
+	switch (imsg_get_type(imsg)) {
 	case IMSG_SCRIPT:
 		if (imsg_get_data(imsg, &scr, sizeof(scr)) == -1) {
 			log_warn("%s: imsg_get_data", __func__);
@@ -498,7 +498,7 @@ parent_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
 	struct ctl_bindany	 bnd;
 	int			 s;
 
-	switch (imsg->hdr.type) {
+	switch (imsg_get_type(imsg)) {
 	case IMSG_BINDANY:
 		if (imsg_get_data(imsg, &bnd, sizeof(bnd)) == -1) {
 			log_warn("%s: imsg_get_data", __func__);
@@ -537,7 +537,7 @@ parent_dispatch_ca(int fd, struct privsep_proc *p, struct imsg *imsg)
 	struct privsep		*ps = p->p_ps;
 	struct relayd		*env = ps->ps_env;
 
-	switch (imsg->hdr.type) {
+	switch (imsg_get_type(imsg)) {
 	case IMSG_CFG_DONE:
 		parent_configure_done(env);
 		break;