On Mon, Sep 04, 2017 at 12:03:31AM -0700, Carlos Cardenas wrote: > * Fix logic handling stopping a VM. Prevents VMD from crashing. > * Add additional error code to notify the user that a vm cannot be > stopped when not running. > * Add additional log_debug statements. >
See below. Also this diff has tabs vs spaces problems like the previous one. If we fix the style issues and you can shed some light on the part I noted below, I think we can get both these diffs in. -ml > diff --git usr.sbin/vmctl/vmctl.c usr.sbin/vmctl/vmctl.c > index 64d82ca847d..5fb7fbfd74c 100644 > --- usr.sbin/vmctl/vmctl.c > +++ usr.sbin/vmctl/vmctl.c > @@ -439,12 +439,19 @@ terminate_vm_complete(struct imsg *imsg, int *ret) > vmr = (struct vmop_result *)imsg->data; > res = vmr->vmr_result; > if (res) { > - errno = res; > - if (res == ENOENT) > - warnx("vm not found"); > - else > - warn("terminate vm command failed"); > - *ret = EIO; > + switch (res) { > + case VMD_VM_STOP_INVALID: > + warnx("cannot stop vm that is not running"); > + *ret = EINVAL; > + break; > + case ENOENT: > + warnx("vm not found"); > + *ret = EIO; > + break; > + default: > + warn("terminate vm command failed"); > + *ret = EIO; > + } > } else { > warnx("sent request to terminate vm %d", vmr->vmr_id); > *ret = 0; > @@ -453,6 +460,7 @@ terminate_vm_complete(struct imsg *imsg, int *ret) > warnx("unexpected response received from vmd"); > *ret = EINVAL; > } > + errno = *ret; > > return (1); > } > diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h > index 22da6d58a7b..1240339db52 100644 > --- usr.sbin/vmd/vmd.h > +++ usr.sbin/vmd/vmd.h > @@ -54,6 +54,7 @@ > #define VMD_BIOS_MISSING 1001 > #define VMD_DISK_MISSING 1002 > #define VMD_DISK_INVALID 1003 > +#define VMD_VM_STOP_INVALID 1004 > > /* 100.64.0.0/10 from rfc6598 (IPv4 Prefix for Shared Address Space) */ > #define VMD_DHCP_PREFIX "100.64.0.0/10" > diff --git usr.sbin/vmd/vmm.c usr.sbin/vmd/vmm.c > index d3233147cff..66083a5f959 100644 > --- usr.sbin/vmd/vmm.c > +++ usr.sbin/vmd/vmm.c > @@ -169,10 +169,9 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p, > struct imsg *imsg) > else > res = 0; > } else { > - /* Terminate VMs that are unknown or shutting down */ > - vtp.vtp_vm_id = vm_vmid2id(vm->vm_vmid, vm); > - res = terminate_vm(&vtp); > - vm_remove(vm); > + log_debug("%s: cannot stop vm that is not running", > + __func__); > + res = VMD_VM_STOP_INVALID; Is this really what we want? (Was this the part that was crashing vmd?) This will break (I think) stopping a vm, then while it is shutting down during vmmci shutdown processing, stopping it again to force kill it. Is the problem that we are doing vm_remove unconditionally, regardless of the result of the previous calls? (Eg, what if vm_vmid2id or terminate_vm failed?) > } > cmd = IMSG_VMDOP_TERMINATE_VM_RESPONSE; > break; > @@ -346,6 +345,8 @@ vmm_sighdlr(int sig, short event, void *arg) > > vmid = vm->vm_params.vmc_params.vcp_id; > vtp.vtp_vm_id = vmid; > + log_debug("%s: attempting to terminate vm > %d", > + __func__, vm->vm_vmid); > if (terminate_vm(&vtp) == 0) { > memset(&vmr, 0, sizeof(vmr)); > vmr.vmr_result = ret; > @@ -505,7 +506,7 @@ vmm_dispatch_vm(int fd, short event, void *arg) > * supplied vm_terminate_params structure (vtp->vtp_vm_id) > * > * Parameters > - * vtp: vm_create_params struct containing the ID of the VM to terminate > + * vtp: vm_terminate_params struct containing the ID of the VM to terminate > * > * Return values: > * 0: success > @@ -515,6 +516,7 @@ vmm_dispatch_vm(int fd, short event, void *arg) > int > terminate_vm(struct vm_terminate_params *vtp) > { > + log_debug("%s: terminating vmid %d", __func__, vtp->vtp_vm_id); > if (ioctl(env->vmd_fd, VMM_IOC_TERM, vtp) < 0) > return (errno); > > -- > 2.14.1 >