On Mon, Sep 12, 2016 at 07:01:38PM +0200, Andrea Bolognani wrote: > 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.
No objection from me. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
