From: Dave Voutila Subject: disambiguate fd variable naming in vmd To: tech@openbsd.org Date: Mon, 13 Jul 2026 09:45:32 -0400 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? 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);