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
>
