On Mon, Feb 27, 2017 at 10:55:31AM +0100, Reyk Floeter wrote:
> The following diff is not really needed without just yet, but:
> - openening /dev/ptm in advance might allow better pledge in the future
> - customizing "openpty" will allow to do what we need next
> Since openpty(4) is libutil and not libc, it should be fine not using it.
>
> OK?
>
> Replace openpty(3) with local function that uses pre-opened /dev/ptm fd
>
> This allows more flexibility for upcoming changes and better pledge.
> We also didn't use half of the features of libutil's openpty function.
> Additionally, make sure that the ttys are closed correctly on shutdown.
>
ok gilles@
> diff --git usr.sbin/vmd/config.c usr.sbin/vmd/config.c
> index f35a3b3..a16c143 100644
> --- usr.sbin/vmd/config.c
> +++ usr.sbin/vmd/config.c
> @@ -126,10 +126,9 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
> uint32_t peerid)
> struct vmop_create_params *vmc = &vm->vm_params;
> struct vm_create_params *vcp = &vmc->vmc_params;
> unsigned int i;
> - int fd = -1, ttys_fd;
> + int fd = -1;
> int kernfd = -1, *diskfds = NULL, *tapfds = NULL;
> int saved_errno = 0;
> - char ptyname[VM_TTYNAME_MAX];
> char ifname[IF_NAMESIZE], *s;
> char path[PATH_MAX];
> unsigned int unit;
> @@ -241,12 +240,11 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
> uint32_t peerid)
>
> /* Open TTY */
> if (vm->vm_ttyname == NULL) {
> - if (openpty(&vm->vm_tty, &ttys_fd, ptyname, NULL, NULL) == -1 ||
> - (vm->vm_ttyname = strdup(ptyname)) == NULL) {
> - log_warn("%s: can't open tty %s", __func__, ptyname);
> + if (vm_opentty(vm) == -1) {
> + log_warn("%s: can't open tty %s", __func__,
> + vm->vm_ttyname == NULL ? "" : vm->vm_ttyname);
> goto fail;
> }
> - close(ttys_fd);
> }
> if ((fd = dup(vm->vm_tty)) == -1) {
> log_warn("%s: can't re-open tty %s", __func__, vm->vm_ttyname);
> diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
> index a01e40f..ed678ee 100644
> --- usr.sbin/vmd/vmd.c
> +++ usr.sbin/vmd/vmd.c
> @@ -20,6 +20,8 @@
> #include <sys/queue.h>
> #include <sys/wait.h>
> #include <sys/cdefs.h>
> +#include <sys/tty.h>
> +#include <sys/ioctl.h>
>
> #include <stdio.h>
> #include <stdlib.h>
> @@ -33,7 +35,6 @@
> #include <syslog.h>
> #include <unistd.h>
> #include <ctype.h>
> -#include <util.h>
>
> #include "proc.h"
> #include "vmd.h"
> @@ -484,6 +485,9 @@ vmd_configure(void)
> struct vmd_vm *vm;
> struct vmd_switch *vsw;
>
> + if ((env->vmd_ptmfd = open(PATH_PTMDEV, O_RDWR|O_CLOEXEC)) == -1)
> + fatal("open %s", PATH_PTMDEV);
> +
> /*
> * pledge in the parent process:
> * stdio - for malloc and basic I/O including events.
> @@ -605,6 +609,12 @@ vmd_reload(unsigned int reset, const char *filename)
> void
> vmd_shutdown(void)
> {
> + struct vmd_vm *vm, *vm_next;
> +
> + TAILQ_FOREACH_SAFE(vm, env->vmd_vms, vm_entry, vm_next) {
> + vm_remove(vm);
> + }
> +
> proc_kill(&env->vmd_ps);
> free(env);
>
> @@ -702,16 +712,8 @@ vm_stop(struct vmd_vm *vm, int keeptty)
> close(vm->vm_kernel);
> vm->vm_kernel = -1;
> }
> -
> - if (keeptty)
> - return;
> -
> - if (vm->vm_tty != -1) {
> - close(vm->vm_tty);
> - vm->vm_tty = -1;
> - }
> - free(vm->vm_ttyname);
> - vm->vm_ttyname = NULL;
> + if (!keeptty)
> + vm_closetty(vm);
> }
>
> void
> @@ -769,6 +771,7 @@ vm_register(struct privsep *ps, struct vmop_create_params
> *vmc,
>
> memcpy(&vm->vm_params, vmc, sizeof(vm->vm_params));
> vm->vm_pid = -1;
> + vm->vm_tty = -1;
>
> for (i = 0; i < vcp->vcp_ndisks; i++)
> vm->vm_disks[i] = -1;
> @@ -787,12 +790,45 @@ vm_register(struct privsep *ps, struct
> vmop_create_params *vmc,
>
> *ret_vm = vm;
> return (0);
> -fail:
> + fail:
> if (errno == 0)
> errno = EINVAL;
> return (-1);
> }
>
> +int
> +vm_opentty(struct vmd_vm *vm)
> +{
> + struct ptmget ptm;
> +
> + /*
> + * Open tty with pre-opened PTM fd
> + */
> + if ((ioctl(env->vmd_ptmfd, PTMGET, &ptm) == -1))
> + return (-1);
> +
> + vm->vm_tty = ptm.cfd;
> + close(ptm.sfd);
> + if ((vm->vm_ttyname = strdup(ptm.sn)) == NULL)
> + goto fail;
> +
> + return (0);
> + fail:
> + vm_closetty(vm);
> + return (-1);
> +}
> +
> +void
> +vm_closetty(struct vmd_vm *vm)
> +{
> + if (vm->vm_tty != -1) {
> + close(vm->vm_tty);
> + vm->vm_tty = -1;
> + }
> + free(vm->vm_ttyname);
> + vm->vm_ttyname = NULL;
> +}
> +
> void
> switch_remove(struct vmd_switch *vsw)
> {
> diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
> index e371112..26d345c 100644
> --- usr.sbin/vmd/vmd.h
> +++ usr.sbin/vmd/vmd.h
> @@ -185,6 +185,7 @@ struct vmd {
> struct switchlist *vmd_switches;
>
> int vmd_fd;
> + int vmd_ptmfd;
> };
>
> /* vmd.c */
> @@ -197,6 +198,8 @@ void vm_stop(struct vmd_vm *, int);
> void vm_remove(struct vmd_vm *);
> int vm_register(struct privsep *, struct vmop_create_params *,
> struct vmd_vm **, uint32_t);
> +int vm_opentty(struct vmd_vm *);
> +void vm_closetty(struct vmd_vm *);
> void switch_remove(struct vmd_switch *);
> struct vmd_switch *switch_getbyname(const char *);
> char *get_string(uint8_t *, size_t);
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
