From: hshoexer Subject: Re: disambiguate fd variable naming in vmd To: tech@openbsd.org Date: Wed, 15 Jul 2026 12:18:09 +0200 Hi, On Mon, Jul 13, 2026 at 09:45:32AM -0400, Dave Voutila wrote: > Boring but helpful diff leading up to my sharing a much larger one. The > global instance of struct vmd uses its vmd_fd member for multiple things > depending on the vmd process. In one case, it's a socket(2)...in > another, an open vmm(4). > > This redesigns struct vmd to have distinct members named based on usage > to make vmd slightly easier to reason about. > > Bonus tweaks: properly initializes the SEV, ptm, and socket fd's to -1 > and changes a logic check from the strange semantic of "> -1" to "!= > -1". (At no point should 0 be a valid fd value for the psp or vmm > devices.) > > ok? diff reads good, my tests didn't show any regressions ok hshoexer@ > > > diffstat refs/heads/master refs/heads/vmd-fd-cleanup-simple > M usr.sbin/vmd/priv.c | 12+ 12- > M usr.sbin/vmd/virtio.c | 1+ 1- > M usr.sbin/vmd/vm.c | 5+ 5- > M usr.sbin/vmd/vmd.c | 11+ 7- > M usr.sbin/vmd/vmd.h | 4+ 3- > M usr.sbin/vmd/vmm.c | 7+ 7- > > 6 files changed, 40 insertions(+), 35 deletions(-) > > diff refs/heads/master refs/heads/vmd-fd-cleanup-simple > commit - 9598e7ed943eac95d9532381b8c8e3bb46321fbd > commit + 291de405160c12f78e9103b08983b3dae43d372e > blob - 5c7338ba24c7d867fde84a2b1d1eddcdac03b5a7 > blob + d8739aa6010babc401b91f0d0277ff6ac7793657 > --- usr.sbin/vmd/priv.c > +++ usr.sbin/vmd/priv.c > @@ -63,11 +63,11 @@ priv_run(struct privsep *ps, struct privsep_proc *p, v > */ > > /* Open our own socket for generic interface ioctls */ > - if ((env->vmd_fd = socket(AF_INET, SOCK_DGRAM, 0)) == -1) > + if ((env->vmd_sock_fd = socket(AF_INET, SOCK_DGRAM, 0)) == -1) > fatal("socket"); > > /* But we need a different fd for IPv6 */ > - if ((env->vmd_fd6 = socket(AF_INET6, SOCK_DGRAM, 0)) == -1) > + if ((env->vmd_sock_fd6 = socket(AF_INET6, SOCK_DGRAM, 0)) == -1) > fatal("socket6"); > } > > @@ -124,13 +124,13 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, s > /* Set the interface description */ > strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name)); > ifr.ifr_data = (caddr_t)vfr.vfr_value; > - if (ioctl(env->vmd_fd, SIOCSIFDESCR, &ifr) == -1) > + if (ioctl(env->vmd_sock_fd, SIOCSIFDESCR, &ifr) == -1) > log_warn("SIOCSIFDESCR"); > break; > case IMSG_VMDOP_PRIV_IFRDOMAIN: > strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name)); > ifr.ifr_rdomainid = vfr.vfr_id; > - if (ioctl(env->vmd_fd, SIOCSIFRDOMAIN, &ifr) == -1) > + if (ioctl(env->vmd_sock_fd, SIOCSIFRDOMAIN, &ifr) == -1) > log_warn("SIOCSIFRDOMAIN"); > break; > case IMSG_VMDOP_PRIV_IFADD: > @@ -143,14 +143,14 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, s > sizeof(ifbr.ifbr_name)); > strlcpy(ifbr.ifbr_ifsname, vfr.vfr_value, > sizeof(ifbr.ifbr_ifsname)); > - if (ioctl(env->vmd_fd, SIOCBRDGADD, &ifbr) == -1 && > + if (ioctl(env->vmd_sock_fd, SIOCBRDGADD, &ifbr) == -1 && > errno != EEXIST) > log_warn("SIOCBRDGADD"); > break; > case IMSG_VMDOP_PRIV_IFEXISTS: > /* Determine if bridge exists */ > strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name)); > - if (ioctl(env->vmd_fd, SIOCGIFFLAGS, &ifr) == -1) > + if (ioctl(env->vmd_sock_fd, SIOCGIFFLAGS, &ifr) == -1) > fatalx("%s: bridge \"%s\" does not exist", > __func__, vfr.vfr_name); > break; > @@ -158,7 +158,7 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, s > case IMSG_VMDOP_PRIV_IFDOWN: > /* Set the interface status */ > strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name)); > - if (ioctl(env->vmd_fd, SIOCGIFFLAGS, &ifr) == -1) { > + if (ioctl(env->vmd_sock_fd, SIOCGIFFLAGS, &ifr) == -1) { > log_warn("SIOCGIFFLAGS"); > break; > } > @@ -166,7 +166,7 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, s > ifr.ifr_flags |= IFF_UP; > else > ifr.ifr_flags &= ~IFF_UP; > - if (ioctl(env->vmd_fd, SIOCSIFFLAGS, &ifr) == -1) > + if (ioctl(env->vmd_sock_fd, SIOCSIFFLAGS, &ifr) == -1) > log_warn("SIOCSIFFLAGS"); > break; > case IMSG_VMDOP_PRIV_IFGROUP: > @@ -179,7 +179,7 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, s > sizeof(ifgr.ifgr_group)) >= sizeof(ifgr.ifgr_group)) > fatalx("%s: group name too long", __func__); > > - if (ioctl(env->vmd_fd, SIOCAIFGROUP, &ifgr) == -1 && > + if (ioctl(env->vmd_sock_fd, SIOCAIFGROUP, &ifgr) == -1 && > errno != EEXIST) > log_warn("SIOCAIFGROUP"); > break; > @@ -198,7 +198,7 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, s > memcpy(&ifra.ifra_mask, &vfr.vfr_mask, > sizeof(ifra.ifra_mask)); > > - if (ioctl(env->vmd_fd, SIOCAIFADDR, &ifra) == -1) > + if (ioctl(env->vmd_sock_fd, SIOCAIFADDR, &ifra) == -1) > log_warn("SIOCAIFADDR"); > break; > case IMSG_VMDOP_PRIV_IFADDR6: > @@ -221,11 +221,11 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, s > in6_ifra.ifra_lifetime.ia6t_vltime = ND6_INFINITE_LIFETIME; > in6_ifra.ifra_lifetime.ia6t_pltime = ND6_INFINITE_LIFETIME; > > - if (ioctl(env->vmd_fd6, SIOCDIFADDR_IN6, &in6_ifra) == -1 && > + if (ioctl(env->vmd_sock_fd6, SIOCDIFADDR_IN6, &in6_ifra) == -1 && > errno != EADDRNOTAVAIL) > log_warn("SIOCDIFADDR_IN6"); > > - if (ioctl(env->vmd_fd6, SIOCAIFADDR_IN6, &in6_ifra) == -1) > + if (ioctl(env->vmd_sock_fd6, SIOCAIFADDR_IN6, &in6_ifra) == -1) > log_warn("SIOCAIFADDR_IN6"); > break; > case IMSG_VMDOP_PRIV_GET_ADDR: > blob - 75f783d8fa8aeadc181a952c091ee2d6d7de85d1 > blob + d10040759267eb24bfb991643fef02be88b0a5a8 > --- usr.sbin/vmd/virtio.c > +++ usr.sbin/vmd/virtio.c > @@ -1657,7 +1657,7 @@ virtio_dev_launch(struct vmd_vm *vm, struct virtio_dev > memset(num, 0, sizeof(num)); > snprintf(num, sizeof(num), "%d", sync_fds[1]); > memset(vmm_fd, 0, sizeof(vmm_fd)); > - snprintf(vmm_fd, sizeof(vmm_fd), "%d", env->vmd_fd); > + snprintf(vmm_fd, sizeof(vmm_fd), "%d", env->vmd_vmm_fd); > memset(vm_name, 0, sizeof(vm_name)); > snprintf(vm_name, sizeof(vm_name), "%s", > vm->vm_params.vmc_name); > blob - e2a7451dfa63ea8c8b512766e48acbcb2533e4be > blob + 66b6e2f705591cb9a43d9a82d563132ad1a57924 > --- usr.sbin/vmd/vm.c > +++ usr.sbin/vmd/vm.c > @@ -89,7 +89,7 @@ vm_main(int fd, int fd_vmm) > /* > * The vm process relies on global state. Set the fd for /dev/vmm. > */ > - env->vmd_fd = fd_vmm; > + env->vmd_vmm_fd = fd_vmm; > > /* > * We aren't root, so we can't chroot(2). Use unveil(2) instead. > @@ -507,7 +507,7 @@ vcpu_reset(uint32_t vmid, uint32_t vcpu_id, struct vcp > > log_debug("%s: resetting vcpu %d for vm %d", __func__, vcpu_id, vmid); > > - if (ioctl(env->vmd_fd, VMM_IOC_RESETCPU, &vrp) == -1) > + if (ioctl(env->vmd_vmm_fd, VMM_IOC_RESETCPU, &vrp) == -1) > return (errno); > > return (0); > @@ -557,7 +557,7 @@ vmm_create_vm(struct vmd_vm *vm) > vcp.vcp_sev = vmc->vmc_sev; > vcp.vcp_seves = vmc->vmc_seves; > > - if (ioctl(env->vmd_fd, VMM_IOC_CREATE, &vcp) == -1) > + if (ioctl(env->vmd_vmm_fd, VMM_IOC_CREATE, &vcp) == -1) > return (errno); > > vm->vm_vmmid = vcp.vcp_id; > @@ -894,7 +894,7 @@ vcpu_run_loop(void *arg) > /* Still more interrupts pending? */ > vrp->vrp_intr_pending = intr_pending(current_vm); > > - if (ioctl(env->vmd_fd, VMM_IOC_RUN, vrp) == -1) { > + if (ioctl(env->vmd_vmm_fd, VMM_IOC_RUN, vrp) == -1) { > /* If run ioctl failed, exit */ > ret = errno; > log_warn("%s: vm %d / vcpu %d run ioctl failed", > @@ -941,7 +941,7 @@ vcpu_intr(uint32_t vmm_id, uint32_t vcpu_id, uint8_t i > vip.vip_vcpu_id = vcpu_id; /* XXX always 0? */ > vip.vip_intr = intr; > > - if (ioctl(env->vmd_fd, VMM_IOC_INTR, &vip) == -1) > + if (ioctl(env->vmd_vmm_fd, VMM_IOC_INTR, &vip) == -1) > return (errno); > > return (0); > blob - 0a28c186bf2d0c9905123fe5d1f2701cbc287c77 > blob + dd4720fad2ac58c09a34d105ef3b3cd58bc49de7 > --- usr.sbin/vmd/vmd.c > +++ usr.sbin/vmd/vmd.c > @@ -549,9 +549,13 @@ main(int argc, char **argv) > > if ((env = calloc(1, sizeof(*env))) == NULL) > fatal("calloc: env"); > - env->vmd_fd = -1; > - env->vmd_fd6 = -1; > > + env->vmd_ptm_fd = -1; > + env->vmd_psp_fd = -1; > + env->vmd_sock_fd = -1; > + env->vmd_sock_fd6 = -1; > + env->vmd_vmm_fd = -1; > + > while ((ch = getopt(argc, argv, "D:P:V:X:df:i:j:nt:vp:")) != -1) { > switch (ch) { > case 'D': > @@ -680,8 +684,8 @@ main(int argc, char **argv) > > /* Open /dev/vmm early. */ > if (env->vmd_noaction == 0 && proc_id == PROC_PARENT) { > - env->vmd_fd = open(VMM_NODE, O_RDWR | O_CLOEXEC); > - if (env->vmd_fd == -1) > + env->vmd_vmm_fd = open(VMM_NODE, O_RDWR | O_CLOEXEC); > + if (env->vmd_vmm_fd == -1) > fatal("%s", VMM_NODE); > } > > @@ -785,7 +789,7 @@ vmd_configure(void) > " chown fattr flock", NULL) == -1) > fatal("pledge"); > > - if ((env->vmd_ptmfd = getptmfd()) == -1) > + if ((env->vmd_ptm_fd = getptmfd()) == -1) > fatal("getptmfd %s", PATH_PTMDEV); > > if (parse_config(env->vmd_conffile) == -1) { > @@ -801,7 +805,7 @@ vmd_configure(void) > > /* Send VMM device fd to vmm proc. */ > proc_compose_imsg(&env->vmd_ps, PROC_VMM, > - IMSG_VMDOP_RECEIVE_VMM_FD, -1, env->vmd_fd, NULL, 0); > + IMSG_VMDOP_RECEIVE_VMM_FD, -1, env->vmd_vmm_fd, NULL, 0); > > /* Send PSP device fd to vmm proc. */ > if (env->vmd_psp_fd != -1) { > @@ -1607,7 +1611,7 @@ vm_opentty(struct vmd_vm *vm) > /* > * Open tty with pre-opened PTM fd > */ > - if (fdopenpty(env->vmd_ptmfd, &vm->vm_tty, &tty_slave, vm->vm_ttyname, > + if (fdopenpty(env->vmd_ptm_fd, &vm->vm_tty, &tty_slave, vm->vm_ttyname, > NULL, NULL) == -1) { > log_warn("fdopenpty"); > return (-1); > blob - eced284a8b6f93751acd606272cb09deaa47df37 > blob + 2dadede5d6e20b59ed8141fe67c12368cfd75ebd > --- usr.sbin/vmd/vmd.h > +++ usr.sbin/vmd/vmd.h > @@ -408,10 +408,11 @@ struct vmd { > uint32_t vmd_nswitches; > struct switchlist *vmd_switches; > > - int vmd_fd; > - int vmd_fd6; > - int vmd_ptmfd; > + int vmd_ptm_fd; > int vmd_psp_fd; > + int vmd_sock_fd; > + int vmd_sock_fd6; > + int vmd_vmm_fd; > }; > > struct vm_dev_pipe { > blob - ebf5a5c5f6dbb41cb03f967119451be32b624396 > blob + 06bd649662ea0d9af4953b13d6fe6370c3cc31f9 > --- usr.sbin/vmd/vmm.c > +++ usr.sbin/vmd/vmm.c > @@ -271,15 +271,15 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p, st > imsg_get_fd(imsg), &var, sizeof(var)); > break; > case IMSG_VMDOP_RECEIVE_VMM_FD: > - if (env->vmd_fd > -1) > + if (env->vmd_vmm_fd != -1) > fatalx("already received vmm fd"); > - env->vmd_fd = imsg_get_fd(imsg); > + env->vmd_vmm_fd = imsg_get_fd(imsg); > > /* Get and terminate all running VMs */ > get_info_vm(ps, NULL, 1); > break; > case IMSG_VMDOP_RECEIVE_PSP_FD: > - if (env->vmd_psp_fd > -1) > + if (env->vmd_psp_fd != -1) > fatalx("already received psp fd"); > env->vmd_psp_fd = imsg_get_fd(imsg); > break; > @@ -530,7 +530,7 @@ vmm_dispatch_vm(int fd, short event, void *arg) > int > terminate_vm(struct vm_terminate_params *vtp) > { > - if (ioctl(env->vmd_fd, VMM_IOC_TERM, vtp) == -1) > + if (ioctl(env->vmd_vmm_fd, VMM_IOC_TERM, vtp) == -1) > return (errno); > > return (0); > @@ -734,7 +734,7 @@ vmm_start_vm(struct imsg *imsg, uint32_t *id, pid_t *p > memset(num, 0, sizeof(num)); > snprintf(num, sizeof(num), "%d", fds[1]); > memset(vmm_fd, 0, sizeof(vmm_fd)); > - snprintf(vmm_fd, sizeof(vmm_fd), "%d", env->vmd_fd); > + snprintf(vmm_fd, sizeof(vmm_fd), "%d", env->vmd_vmm_fd); > memset(psp_fd, 0, sizeof(psp_fd)); > snprintf(psp_fd, sizeof(psp_fd), "%d", env->vmd_psp_fd); > > @@ -817,7 +817,7 @@ get_info_vm(struct privsep *ps, struct imsg *imsg, int > memset(&vir, 0, sizeof(vir)); > > /* First ioctl to see how many bytes needed (vip.vip_size) */ > - if (ioctl(env->vmd_fd, VMM_IOC_INFO, &vip) == -1) > + if (ioctl(env->vmd_vmm_fd, VMM_IOC_INFO, &vip) == -1) > return (errno); > > if (vip.vip_info_ct != 0) > @@ -829,7 +829,7 @@ get_info_vm(struct privsep *ps, struct imsg *imsg, int > > /* Second ioctl to get the actual list */ > vip.vip_info = info; > - if (ioctl(env->vmd_fd, VMM_IOC_INFO, &vip) == -1) { > + if (ioctl(env->vmd_vmm_fd, VMM_IOC_INFO, &vip) == -1) { > ret = errno; > free(info); > return (ret); >