On Mon, Mar 06, 2017 at 06:48:34PM -0500, Cole Robinson wrote: > On 03/06/2017 03:43 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> > > --- > > virtManager/details.py | 3 ++- > > virtManager/domain.py | 47 ++++++++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 48 insertions(+), 2 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 336584e5..79303890 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 > > > > @@ -557,6 +558,42 @@ class vmmDomain(vmmLibvirtObject): > > # Persistent XML change APIs # > > ############################## > > > > + def rename_domain(self, new_name, kwargs): > > + old_name = self.get_name() > > + > > + self.define_name(new_name) > > + > > + # We need to do this copy magic because there is no Libvirt storage API > > + # to rename storage volume. > > + try: > > + 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() > > + except Exception as e: > > + self.define_name(old_name) > > + raise RuntimeError("Cannot rename nvram VARS: '%s'" % e) > > This breaks when the VM doesn't have any nvram. virt-manager --connect > test:///default and try renaming the 'test' VM. > > Please break all the nvram copying out into a separate function, for code clarity > > I think the ordering here should be: > * If the VM has NVRAM, try to copy it. If it fails, exit. > * Rename the VM. If it fails, try to delete the new NVRAM file, but don't > overwrite the error > * Delete the original NVRAM file. If it fails, raise the error but don't try > to rollback the rename > > > + > > + kwargs["nvram"] = new_nvram_path > > + 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 +658,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 +682,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 > > + > > if idmap_list != _SENTINEL: > > if idmap_list is not None: > > # pylint: disable=unpacking-non-sequence > > @@ -1434,6 +1475,10 @@ class vmmDomain(vmmLibvirtObject): > > if (self.get_xmlobj().os.loader_ro is True and > > self.get_xmlobj().os.loader_type == "pflash"): > > flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_NVRAM", 0) > > + else: > > + if (self.get_xmlobj().os.loader_ro is True and > > + self.get_xmlobj().os.loader_type == "pflash"): > > + flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_KEEP_NVRAM", 0) > > Can you break out the duplicate loader check into a variable has_nvram or similar > > Also can you add a comment here that the force=False path is used when > renaming the VM, and KEEP_NVRAM is required for that case. It wasn't obvious to me Thanks for review, I'll send v2. This patch was not finished and apparently I forgot to write it into the commit message for myself to not post it. Pavel
Attachment:
signature.asc
Description: Digital signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
