[virt-tools-list] [virt-manager PATCH v2] domain: add support to rename domain with nvram vars file

[Cole Robinson]

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.

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