On Sat, Oct 10, 2026 at 04:33:32PM -0300, Daniel Almeida wrote:
> Hi Deb,
>
> I find it unidiomatic to have containers destroy an item if addition fails. A
> more common pattern is to return the item in an error. Something like:
>
> pub(crate) struct AddError<'drm> {
> pub(crate) error: Error,
> pub(crate) vm: Arc<Vm<'drm>>,
> }
>
> This would not convert automatically to Error, so callers could not simply "?"
> it, they have to handle it:
>
> pool.add(vm).map_err(|e| {
> e.vm.kill();
> e.error
> })?;
Interesting, I see what you mean because it would give the caller control
over the vm and managing its tear down. And it would be nice to separate
the creation of the vm from inserting it into the pool. I implemented this
myself and that's fine if you want to go with adding a custom VmAddError
or something like that.
But, would you also consider going the other way and actually creating
the vm inside VmPool::add()?
One advantage of this approach is that we don't have to create the vm at
all if there are no ids available so we can just return ENOSPC.
And, since we have to kill a prev_vm if store unexpectedly returns a vm,
then this would centralize vm creation, registration, and failure cleanup
all in a single place. It would also be consistent with planned revisions
to VmPool::remove() which is going to take responsibility for tearing
down the vm.
>
> Don't worry, I plan to remove the need to manually call kill() soon, so this
> subject will become outdated in the near future.
>
> Paths where the Vm is already in the pool can call kill() though, this
> includes
> remove().
>
> -- Daniel
