[libvirt] [PATCH 0/3] add option to keep nvram file on undefine

[Daniel P. Berrange]

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 :|