Jasper Lievisse Adriaanse <j...@jasper.la> writes:
> Hi,
>
> It seems there is an inconsistency when it comes to terminating a VM by
> id or name (4/web point to the same VM here):
>
> before:
> % vmctl stop 4
> stopping vm: requested to shutdown vm 4
> % vmctl stop web
> stopping vm web: failed: Invalid argument
>
> Here's a diff which moves the checks out of the block which is only
> entered when we pass it a name:
>
> after:
> % vmctl stop 4
> stopping vm: failed: Invalid argument
> % vmctl stop web
> stopping vm web: failed: Invalid argument
>
> If EINVAL is actually correct in this case I think is open for
> discussion.
> ENOTSUP might not be a bad candidate actually if the VM isn't running.
Maybe. Careful pulling that thread :)
>
> OK?
This looks good to me. ok dv@
>
> Index: vmd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
> retrieving revision 1.150
> diff -u -p -r1.150 vmd.c
> --- vmd.c 18 Jun 2023 11:45:11 -0000 1.150
> +++ vmd.c 2 Jul 2023 12:30:33 -0000
> @@ -159,20 +159,22 @@ vmd_dispatch_control(int fd, struct priv
> if ((vm = vm_getbyname(vid.vid_name)) == NULL) {
> res = ENOENT;
> break;
> - } else if ((vm->vm_state & VM_STATE_SHUTDOWN) &&
> - (flags & VMOP_FORCE) == 0) {
> - res = EALREADY;
> - break;
> - } else if (!(vm->vm_state & VM_STATE_RUNNING)) {
> - res = EINVAL;
> - break;
> }
> id = vm->vm_vmid;
> } else if ((vm = vm_getbyvmid(id)) == NULL) {
> res = ENOENT;
> break;
> }
> - if (vm_checkperm(vm, &vm->vm_params.vmc_owner, vid.vid_uid)) {
> +
> + /* Validate curent state of vm */
> + if ((vm->vm_state & VM_STATE_SHUTDOWN) &&
> + (flags & VMOP_FORCE) == 0) {
> + res = EALREADY;
> + break;
> + } else if (!(vm->vm_state & VM_STATE_RUNNING)) {
> + res = EINVAL;
> + break;
> + } else if (vm_checkperm(vm, &vm->vm_params.vmc_owner,
> vid.vid_uid)) {
> res = EPERM;
> break;
> }
