From: Mike Larkin Subject: Re: vmd: remove PROC_CONTROL bits from proc.[ch] To: Martijn van Duren Cc: tech@openbsd.org, Dave Voutila Date: Sun, 16 Nov 2025 18:25:24 -0800 On Fri, Nov 14, 2025 at 10:07:46AM +0100, Martijn van Duren wrote: > Hello all, > > A long time ago I removed the control bits from snmpd's proc.c, because > snmpd lost any use for snmpctl. Revisiting proc.c made me realize that > control setup functionality doesn't need to be in proc.c, and only > forces us into creating the process, regardless of being needed. > > Since I have the impression that vmd is currently the most actively > maintained proc.c consumer I started here, but it should probably go > just as well for the other consumers, which I'll visit if people agree > with me here. > > Diff below removes all the control bits from proc.[ch] and places them > in their equivalent places inside control.c and struct vmd. Since vmd > doesn't allow for multiple control sockets I've only took over the > main, and dropped the TAILQ bits. > > Thoughts? OK? > dv@ should decide here. not sure I grok this but at first read it seems like shuffling deck chairs? what benefit does this gain? > martijn@ > > diff /usr/src > path + /usr/src > commit - 0377483fd63d89703211981e944e9e01c48a851d > blob - bf58b437b86d280d317198e9c0ad7bf3715d70e3 > file + usr.sbin/vmd/control.c > --- usr.sbin/vmd/control.c > +++ usr.sbin/vmd/control.c > @@ -59,12 +59,18 @@ static struct privsep_proc procs[] = { > void > control(struct privsep *ps, struct privsep_proc *p) > { > + extern struct vmd *env; > + > + if (control_init(ps, &env->vmd_csock) == -1) > + fatalx("%s: control_init", __func__); > proc_run(ps, p, procs, nitems(procs), control_run, NULL); > } > > void > control_run(struct privsep *ps, struct privsep_proc *p, void *arg) > { > + extern struct vmd *env; > + > /* > * pledge in the control process: > * stdio - for malloc and basic I/O including events. > @@ -75,6 +81,8 @@ control_run(struct privsep *ps, struct privsep_proc *p > if (pledge("stdio unix recvfd sendfd", NULL) == -1) > fatal("pledge"); > > + if (control_listen(&env->vmd_csock) == -1) > + fatalx("%s: control_listen", __func__); > /* Signal to the parent that we're done initializing. */ > proc_compose(ps, PROC_PARENT, IMSG_VMDOP_DONE, NULL, 0); > } > commit - 0377483fd63d89703211981e944e9e01c48a851d > blob - 967583ea5fa26da4f30fe4e6b6c7e9377782621a > file + usr.sbin/vmd/parse.y > --- usr.sbin/vmd/parse.y > +++ usr.sbin/vmd/parse.y > @@ -212,8 +212,8 @@ main : LOCAL INET6 { > free($3); > } > | SOCKET OWNER owner_id { > - env->vmd_ps.ps_csock.cs_uid = $3.uid; > - env->vmd_ps.ps_csock.cs_gid = $3.gid == -1 ? 0 : $3.gid; > + env->vmd_csock.cs_uid = $3.uid; > + env->vmd_csock.cs_gid = $3.gid == -1 ? 0 : $3.gid; > } > | AGENTX { > env->vmd_cfg.cfg_agentx.ax_enabled = 1; > commit - 0377483fd63d89703211981e944e9e01c48a851d > blob - 6bd8d973890fe4bb0d0bf805173538df98ef4515 > file + usr.sbin/vmd/proc.c > --- usr.sbin/vmd/proc.c > +++ usr.sbin/vmd/proc.c > @@ -362,18 +362,9 @@ proc_run(struct privsep *ps, struct privsep_proc *p, > { > struct passwd *pw; > const char *root; > - struct control_sock *rcs; > > log_procinit("%s", p->p_title); > > - if (p->p_id == PROC_CONTROL) { > - if (control_init(ps, &ps->ps_csock) == -1) > - fatalx("%s: control_init", __func__); > - TAILQ_FOREACH(rcs, &ps->ps_rcsocks, cs_entry) > - if (control_init(ps, rcs) == -1) > - fatalx("%s: control_init", __func__); > - } > - > /* Use non-standard user */ > if (p->p_pw != NULL) > pw = p->p_pw; > @@ -417,13 +408,6 @@ proc_run(struct privsep *ps, struct privsep_proc *p, > > proc_setup(ps, procs, nproc); > proc_accept(ps, PROC_PARENT_SOCK_FILENO, PROC_PARENT); > - if (p->p_id == PROC_CONTROL) { > - if (control_listen(&ps->ps_csock) == -1) > - fatalx("%s: control_listen", __func__); > - TAILQ_FOREACH(rcs, &ps->ps_rcsocks, cs_entry) > - if (control_listen(rcs) == -1) > - fatalx("%s: control_listen", __func__); > - } > > DPRINTF("%s: %s, pid %d", __func__, p->p_title, getpid()); > > commit - 0377483fd63d89703211981e944e9e01c48a851d > blob - 581739abf59d3d1bf0d9e026817c955fc17e6fe8 > file + usr.sbin/vmd/proc.h > --- usr.sbin/vmd/proc.h > +++ usr.sbin/vmd/proc.h > @@ -43,28 +43,6 @@ struct imsgev { > short events; > }; > > -/* control socket */ > -struct control_sock { > - const char *cs_name; > - struct event cs_ev; > - struct event cs_evt; > - int cs_fd; > - int cs_restricted; > - void *cs_env; > - uid_t cs_uid; > - gid_t cs_gid; > - > - TAILQ_ENTRY(control_sock) cs_entry; > -}; > -TAILQ_HEAD(control_socks, control_sock); > - > -struct ctl_conn { > - TAILQ_ENTRY(ctl_conn) entry; > - struct imsgev iev; > - struct sockpeercred peercred; > -}; > -TAILQ_HEAD(ctl_connlist, ctl_conn); > - > /* privsep */ > enum privsep_procid { > PROC_PARENT = 0, > @@ -91,9 +69,6 @@ struct privsep { > struct passwd *ps_pw; > int ps_noaction; > > - struct control_sock ps_csock; > - struct control_socks ps_rcsocks; > - > /* Event and signal handlers */ > struct event ps_evsigint; > struct event ps_evsigterm; > @@ -158,12 +133,6 @@ enum privsep_procid > proc_getid(struct privsep_proc *, unsigned int, const char *); > int proc_flush_imsg(struct privsep *, enum privsep_procid); > > -/* control.c */ > -void control(struct privsep *, struct privsep_proc *); > -int control_init(struct privsep *, struct control_sock *); > -int control_reset(struct control_sock *); > -int control_listen(struct control_sock *); > - > /* log.c */ > void log_init(int, int); > void log_procinit(const char *, ...); > commit - 0377483fd63d89703211981e944e9e01c48a851d > blob - 6132f15735b54204e3743df3bd16827a62bc74b6 > file + usr.sbin/vmd/vmd.c > --- usr.sbin/vmd/vmd.c > +++ usr.sbin/vmd/vmd.c > @@ -99,7 +99,6 @@ vmd_dispatch_control(int fd, struct privsep_proc *p, s > struct vmd_vm *vm = NULL; > char *str = NULL; > uint32_t peer_id, type, vm_id = 0; > - struct control_sock *rcs; > > peer_id = imsg_get_id(imsg); > type = imsg_get_type(imsg); > @@ -248,9 +247,7 @@ vmd_dispatch_control(int fd, struct privsep_proc *p, s > &vid, sizeof(vid)); > break; > case IMSG_VMDOP_DONE: > - control_reset(&ps->ps_csock); > - TAILQ_FOREACH(rcs, &ps->ps_rcsocks, cs_entry) > - control_reset(rcs); > + control_reset(&env->vmd_csock); > cmd = 0; > break; > default: > @@ -689,8 +686,7 @@ main(int argc, char **argv) > } > > /* Configure the control socket */ > - ps->ps_csock.cs_name = SOCKET_NAME; > - TAILQ_INIT(&ps->ps_rcsocks); > + env->vmd_csock.cs_name = SOCKET_NAME; > > /* Configuration will be parsed after forking the children */ > env->vmd_conffile = conffile; > commit - 0377483fd63d89703211981e944e9e01c48a851d > blob - c1a336bb580b1c254b0faa5d4d069c90109fd313 > file + usr.sbin/vmd/vmd.h > --- usr.sbin/vmd/vmd.h > +++ usr.sbin/vmd/vmd.h > @@ -353,6 +353,24 @@ struct vmd_config { > struct vmd_agentx cfg_agentx; > }; > > +struct control_sock { > + const char *cs_name; > + struct event cs_ev; > + struct event cs_evt; > + int cs_fd; > + int cs_restricted; > + void *cs_env; > + uid_t cs_uid; > + gid_t cs_gid; > +}; > + > +struct ctl_conn { > + TAILQ_ENTRY(ctl_conn) entry; > + struct imsgev iev; > + struct sockpeercred peercred; > +}; > +TAILQ_HEAD(ctl_connlist, ctl_conn); > + > struct vmd { > struct privsep vmd_ps; > const char *vmd_conffile; > @@ -361,6 +379,8 @@ struct vmd { > /* global configuration that is sent to the children */ > struct vmd_config vmd_cfg; > > + struct control_sock vmd_csock; > + > int vmd_debug; > int vmd_verbose; > int vmd_noaction; > @@ -422,6 +442,12 @@ struct packet_ctx { > struct sockaddr_storage pc_dst; > }; > > +/* control.c */ > +void control(struct privsep *, struct privsep_proc *); > +int control_init(struct privsep *, struct control_sock *); > +int control_reset(struct control_sock *); > +int control_listen(struct control_sock *); > + > /* packet.c */ > ssize_t assemble_hw_header(unsigned char *, size_t, size_t, > struct packet_ctx *, unsigned int); >