From: Claudio Jeker Subject: Re: [1/7] relayd: convert proc.c to new imsg API To: Rafael Sadowski Cc: tech@openbsd.org Date: Mon, 8 Jun 2026 15:11:14 +0200 On Sun, Jun 07, 2026 at 08:57:54AM +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 11d91fb058229c848c44618ab0716659f0578d8c > Author: Rafael Sadowski > Date: Sat Jun 6 08:43:41 2026 +0200 > > relayd: convert proc.c to new imsg API > > Replace IMSG_SIZE_CHECK() + memcpy() with imsg_get_data(), which does > the length check and copy in one call. Use the imsg accessors > (imsg_get_*) instead of touching imsg.hdr directly and > imsgbuf_get()/imsgbuf_read() instead of imsg_get(). > > Rewrite proc_forward_imsg() to use imsg_forward() per target imsgbuf > instead of re-composing via proc_compose_imsg(); arm the write event > with imsg_event_add() after each forward. > > proc_forward_imsg() never forwarded an fd, and imsg_forward() rewinds > the buffer internally, so multiple forwards per message keep working. > > Drop the now-unused n parameter (all callers passed -1). Minor style issues below. > diff --git a/control.c b/control.c > index 2793461..6c29f2b 100644 > --- a/control.c > +++ b/control.c > @@ -361,7 +361,7 @@ control_dispatch_imsg(int fd, short event, void *arg) > break; > case IMSG_CTL_SHUTDOWN: > case IMSG_CTL_RELOAD: > - proc_forward_imsg(env->sc_ps, &imsg, PROC_PARENT, -1); > + proc_forward_imsg(env->sc_ps, &imsg, PROC_PARENT); > break; > case IMSG_CTL_POLL: > proc_compose(env->sc_ps, PROC_HCE, > @@ -385,9 +385,9 @@ control_dispatch_imsg(int fd, short event, void *arg) > > memcpy(&verbose, imsg.data, sizeof(verbose)); > > - proc_forward_imsg(env->sc_ps, &imsg, PROC_PARENT, -1); > - proc_forward_imsg(env->sc_ps, &imsg, PROC_HCE, -1); > - proc_forward_imsg(env->sc_ps, &imsg, PROC_RELAY, -1); > + 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); > diff --git a/proc.c b/proc.c > index 361f05a..d38bebd 100644 > --- a/proc.c > +++ b/proc.c > @@ -599,7 +599,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; > > @@ -607,9 +607,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); > @@ -630,15 +632,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 > > /* > @@ -653,24 +656,26 @@ 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__); > + > + log_setverbose(ver); > break; > case IMSG_CTL_PROCFD: > - 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); > + pf.pf_instance); This indent is wrong here, previous version was correct. > break; > default: > fatalx("%s: %s %d got invalid 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)); > } > imsg_free(&imsg); > } > @@ -790,10 +795,18 @@ proc_composev(struct privsep *ps, enum privsep_procid id, > > int > 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) > + return (-1); > + imsg_event_add(&ps->ps_ievs[id][n]); > + } > + > + return (0); > } > > struct imsgbuf * > diff --git a/relayd.h b/relayd.h > index 5536b47..16fe2bf 100644 > --- a/relayd.h > +++ b/relayd.h > @@ -1424,7 +1424,7 @@ int proc_composev_imsg(struct privsep *, enum privsep_procid, 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); > + enum privsep_procid); > struct imsgbuf * > proc_ibuf(struct privsep *, enum privsep_procid, int); > struct imsgev * > Apart from that diff is OK claudio@ -- :wq Claudio