[virt-tools-list] [virt-manager PATCH v2] domain: add support to rename domain with nvram vars file
Pavel Hrdina
phrdina at redhat.com
Tue Mar 7 17:22:33 UTC 2017
On Tue, Mar 07, 2017 at 11:20:50AM -0500, Cole Robinson wrote:
> On 03/07/2017 07:33 AM, Pavel Hrdina wrote:
> > Libvirt storage API doesn't support renaming storage volumes so
> > we need to copy the nvram file and remove the old one.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1368922
> >
> > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > ---
> >
> > new in v2:
> > - properly implement the rename process
> > - introduced new method has_nvram()
> > - some comments
> >
> > virtManager/details.py | 3 +-
> > virtManager/domain.py | 74 ++++++++++++++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 73 insertions(+), 4 deletions(-)
> >
> > diff --git a/virtManager/details.py b/virtManager/details.py
> > index d6fef967..0b2754d8 100644
> > --- a/virtManager/details.py
> > +++ b/virtManager/details.py
> > @@ -1975,7 +1975,8 @@ class vmmDetails(vmmGObjectUI):
> > # This needs to be last
> > if self.edited(EDIT_NAME):
> > # Renaming is pretty convoluted, so do it here synchronously
> > - self.vm.define_name(self.widget("overview-name").get_text())
> > + self.vm.rename_domain(self.widget("overview-name").get_text(),
> > + kwargs)
> >
> > if not kwargs and not hotplug_args:
> > # Saves some useless redefine attempts
> > diff --git a/virtManager/domain.py b/virtManager/domain.py
> > index d9e17dbb..f7d76af6 100644
> > --- a/virtManager/domain.py
> > +++ b/virtManager/domain.py
> > @@ -32,6 +32,7 @@ from virtinst import DomainSnapshot
> > from virtinst import Guest
> > from virtinst import util
> > from virtinst import VirtualController
> > +from virtinst import VirtualDisk
> >
> > from .libvirtobject import vmmLibvirtObject
> >
> > @@ -479,6 +480,10 @@ class vmmDomain(vmmLibvirtObject):
> > return "-"
> > return str(i)
> >
> > + def has_nvram(self):
> > + return bool(self.get_xmlobj().os.loader_ro is True and
> > + self.get_xmlobj().os.loader_type == "pflash")
> > +
> > ##################
> > # Support checks #
> > ##################
> > @@ -552,11 +557,63 @@ class vmmDomain(vmmLibvirtObject):
> > raise RuntimeError(_("Could not find specified device in the "
> > "inactive VM configuration: %s") % repr(origdev))
> >
> > + def _copy_nvram_file(self, new_name):
> > + """
> > + We need to do this copy magic because there is no Libvirt storage API
> > + to rename storage volume.
> > + """
> > + old_nvram = VirtualDisk(self.conn.get_backend())
> > + old_nvram.path = self.get_xmlobj().os.nvram
> > +
> > + nvram_dir = os.path.dirname(old_nvram.path)
> > + new_nvram_path = os.path.join(nvram_dir, "%s_VARS.fd" % new_name)
> > +
> > + new_nvram = VirtualDisk(self.conn.get_backend())
> > + new_nvram.path = new_nvram_path
> > +
> > + nvram_install = VirtualDisk.build_vol_install(
> > + self.conn.get_backend(), os.path.basename(new_nvram.path),
> > + new_nvram.get_parent_pool(), new_nvram.get_size(), False)
> > + nvram_install.input_vol = old_nvram.get_vol_object()
> > + nvram_install.sync_input_vol(only_format=True)
> > +
> > + new_nvram.set_vol_install(nvram_install)
> > + new_nvram.validate()
> > + new_nvram.setup()
> > +
> > + return new_nvram, old_nvram
> > +
> >
> > ##############################
> > # Persistent XML change APIs #
> > ##############################
> >
> > + def rename_domain(self, new_name, kwargs):
> > + if self.has_nvram():
> > + try:
> > + new_nvram, old_nvram = self._copy_nvram_file(new_name)
> > + except Exception as e:
> > + raise RuntimeError("Cannot rename nvram VARS: '%s'" % e)
> > +
>
> Rather than use the self.has_nvram() check to protect new_nvram access, can you do
>
> new_nvram = None
> old_nvram = None
> if self.has_nvram():
> ....
>
> Then later on just check 'if new_nvram', etc. I'm surprised pylint doesn't
> warn about that pattern actually, even though the code is correct here.
I had it exactly like that but then I changed it because this seemed like
cleaner solution. So I'll fix it.
>
> > + try:
> > + self.define_name(new_name)
> > + except Exception as e:
> > + if self.has_nvram():
> > + try:
> > + new_nvram.get_vol_object().delete(0)
> > + except Exception as e:
> > + logging.debug("rename failed and new nvram was not "
> > + "removed: '%s'", e)
> > + raise e
> > +
>
> Since you use 'e' twice, the intended error is overwritten if vol delete
> fails. So have the second except use e2 or something
>
> > + if self.has_nvram():
> > + kwargs["nvram"] = new_nvram.path
> > +
>
> define_overview is never actually called here.
It is called in config_overview_apply, the kwargs from that function is
passed as a parameter and rename_domain() only fills the new nvram value.
>
> Thanks,
> Cole
>
> > + try:
> > + old_nvram.get_vol_object().delete(0)
> > + except Exception as e:
> > + logging.debug("old nvram file was not removed: '%s'", e)
> > +
> > # Device Add/Remove
> > def add_device(self, devobj):
> > """
> > @@ -621,7 +678,8 @@ class vmmDomain(vmmLibvirtObject):
> > self._redefine_xmlobj(guest)
> >
> > def define_overview(self, machine=_SENTINEL, description=_SENTINEL,
> > - title=_SENTINEL, idmap_list=_SENTINEL, loader=_SENTINEL):
> > + title=_SENTINEL, idmap_list=_SENTINEL, loader=_SENTINEL,
> > + nvram=_SENTINEL):
> > guest = self._make_xmlobj_to_define()
> > if machine != _SENTINEL:
> > guest.os.machine = machine
> > @@ -644,6 +702,9 @@ class vmmDomain(vmmLibvirtObject):
> > guest.os.loader_type = "pflash"
> > guest.os.loader_ro = True
> >
> > + if nvram != _SENTINEL and guest.os.loader is not None:
> > + guest.os.nvram = nvram
> > +
>
> Is the guest.os.loader check here actually protecting something? The caller
> should protect this already I think
I guess that the check can be removed, it should not happen.
Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20170307/4a814be9/attachment.sig>
More information about the virt-tools-list
mailing list