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@xxxxxxxxxx> > --- > > 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. > + 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. 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 Thanks, Cole _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
