Tried it out here with my gimpy little test setup and your suggested repro
case.
Seems to be more sane to me in this case, and looks like the right thing to do,
So ok beck@ for what that’s worth.
> On Jan 21, 2023, at 8:08 AM, Dave Voutila wrote:
>
>
> *bump*... Anyone able to test or review? Other than bikeshedding some
> function naming, this isn't a dramatic change.
>
> Dave Voutila writes:
>
>> Dave Voutila writes:
>>
>>> It turns out not only does vmd have numerous error paths for handling
>>> when something is amiss with a guest, most of the paths don't check if
>>> it's a known vm defined in vm.conf.
>>>
>>> As a result, vmd often removes the vm from the SLIST of vm's meaning
>>> one can't easily attempt to start it again or see it in vmctl's status
>>> output.
>>>
>>> A simple reproduction:
>>>
>>> 1. define a vm with memory > 4gb in vm.conf
>>> 2. run vmd in the foreground (doas vmd -d) so it's not started by rc.d
>>> 3. try to start with `vmctl start -c ${vm_name}`, you should trigger
>>> an ENOMEM and get the "Cannot allocate memory" message from vmctl.
>>> 4. try to start the same vm again...now you get EPERM!
>>> 5. the vm is no longer visible in the output from `vmctl status` :(
>>>
>>> The problem is most of the error paths call vm_remove, which not only
>>> tears down the vm via vm_stop, but also removes it from the vm list and
>>> frees it. Only clean stops or restarts seem to perform this check
>>> currently.
>>>
>>> Below diff refactors into checking if the vm is defined in the global
>>> config before deciding to call vm_stop or vm_remove.
>>
>> Slight tweak... __func__->caller to actually pass the correct name to
>> vm_{stop,remove}() from vm_terminate()
>>
>>
>> diff refs/heads/master refs/heads/vmd-accounting
>> commit - d4e23fe7544b01187ebf3ac8ae32e955445ee666
>> commit + 46503195403bfab50cd34bd8682f35a17d54d03d
>> blob - 6bffb2519a31464836aa573dbccb7aa14ea97722
>> blob + f30dc14de1ff9d5cf121cbc08b6db183a06d0c07
>> --- usr.sbin/vmd/vmd.c
>> +++ usr.sbin/vmd/vmd.c
>> @@ -67,6 +67,8 @@ struct vmd *env;
>> int vm_claimid(const char *, int, uint32_t *);
>> void start_vm_batch(int, short, void*);
>>
>> +static inline void vm_terminate(struct vmd_vm *, const char *);
>> +
>> struct vmd *env;
>>
>> static struct privsep_proc procs[] = {
>> @@ -395,14 +397,14 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>> errno = vmr.vmr_result;
>> log_warn("%s: failed to forward vm result",
>>vcp->vcp_name);
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> return (-1);
>> }
>> }
>>
>> if (vmr.vmr_result) {
>> log_warnx("%s: failed to start vm", vcp->vcp_name);
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> errno = vmr.vmr_result;
>> break;
>> }
>> @@ -410,7 +412,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>> /* Now configure all the interfaces */
>> if (vm_priv_ifconfig(ps, vm) == -1) {
>> log_warn("%s: failed to configure vm", vcp->vcp_name);
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> break;
>> }
>>
>> @@ -441,10 +443,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>> log_info("%s: sent vm %d successfully.",
>>vm->vm_params.vmc_params.vcp_name,
>>vm->vm_vmid);
>> - if (vm->vm_from_config)
>> - vm_stop(vm, 0, __func__);
>> - else
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> }
>>
>> /* Send a response if a control client is waiting for it */
>> @@ -470,10 +469,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>> }
>> if (vmr.vmr_result != EAGAIN ||
>>vm->vm_params.vmc_bootdevice) {
>> - if (vm->vm_from_config)
>> - vm_stop(vm, 0, __func__);
>> - else
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> } else {
>> /* Stop VM instance but keep the tty open */
>> vm_stop(vm, 1, __func__);
>> @@ -509,7 +505,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>>imsg->hdr.peerid, -1, , sizeof(vir)) == -1) {
>> log_debug("%s: GET_INFO_VM failed for vm %d, removing",
>>__func__, vm->vm_vmid);
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> return (-1);
>> }
>> break;
>> @@ -545,7 +541,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>>sizeof(vir)) == -1) {
>> log_debug("%s: GET_INFO_VM_END failed",
>>__func__);
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> return (-1);
>> }
>> }
>>
>> @@ -1943,3 +1943,14 @@ getmonotime(struct timeval *tv)
>>
>> TIMESPEC_TO_TIMEVAL(tv, );
>> }
>> +
>> +static inline void
>> +vm_terminate(struct vmd_vm *vm, const char *caller)
>> +{
>> + if (vm->vm_from_config)
>> + vm_stop(vm, 0, caller);
>> + else {
>> + /* vm_remove calls vm_stop */
>> + vm_remove(vm, caller);
>> + }
>> +}
>
