Re: disambiguate fd variable naming in vmd
hshoexer <[email protected]> Wed, 15 Jul 2026 12:18:09 +0200
| Newsgroups | gmane.os.openbsd.tech |
|---|---|
| Message-ID | <[email protected]> |
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);
>