On 27.05.2016 10:05, Nikolay Shirokovskiy wrote:
There is already a patch [1] on this topic with a different
approach - keep
nvram file by default. There is also some discussion there. To sum up keeping
nvram on undefine could be useful in some usecases so there should be an option
to do it. On the other hand there is a danger of leaving domain assets after
its undefine and unsing them unintentionally on defining domain with the same
name.
AFAIU keeping nvram by default was motivated by domain disks behaviour.
I think there is a difference as libvirt never create disks for domain as
opposed to nvram and managed save and without disks domain will not start so
user is quite aware of disks files. On the other hand one can start using nvram
file solely putting <nvram> in config and managed save is created on daemon
shutdown. So user is much less aware of nvram and managed save existence. Thus
one can easily mess up by unaware define $name/using/undefine/define $name again
usecase. Thus I vote for keeping said assets only if it is specified explicitly
so user knows what he is doing.
Adding option to undefine is best solution I come up with. The other options
are add checks on define or start and both are impossible. Such a check should
be done without any extra flags for it to be useful but this way we break
existing users.
As this a proof of concept this series does not add extra flag for managed save.
[1]
https://www.redhat.com/archives/libvir-list/2015-February/msg00915.html
Nikolay Shirokovskiy (3):
api: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM flag
qemu: add VIR_DOMAIN_UNDEFINE_KEEP_NVRAM support
virsh: add --keep-nvram option to undefine command
include/libvirt/libvirt-domain.h | 1 +
src/qemu/qemu_driver.c | 26 +++++++++++++++++---------
tools/virsh-domain.c | 8 ++++++++
tools/virsh.pod | 6 +++---
4 files changed, 29 insertions(+), 12 deletions(-)
The patches look good to me (although the commit messages could be a
little more verbose, e.g. explain why we need to have X and !X flags at
the same time). I've went trough the discussion you linked and even
though I usually agree with Dan, this time I think we must not change
the behaviour of undefine and thus need a special flag for keeping NVRAM.
ACK, but we might want to give him (or anybody else) chance to chime in,
so please postpone the push for a while. Thanks!
Michal