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(-)

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(-)

On Mon, 2016-09-12 at 10:20 +0200, Michal Privoznik wrote: > 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! I thought I would disagree with this approach, but after reading both the original thread and the current one again I've actually changed my mind :) It's very unfortunate that we have to patch stuff up this way, long after the fact, and it's even more unfortunate that the existing flag and virsh command line option have not been named something like REMOVE_NVRAM. Oh well. At least this solution, while a bit unconvenient for the user, will avoid any change in behavior. Still, I'd like to have Dan's opinion before merging this, since he did not like this approach a year or so ago.