[PATCH 3/5] Pass information about old and new device as a dictionary

Martin Kolman mkolman at redhat.com
Mon Apr 7 11:50:41 UTC 2014


On Fri, 2014-04-04 at 21:54 +0200, Vratislav Podzimek wrote:
> Instead of passing so many arguments, we can pass a dictionary which is more
> elegant.
> 
> Signed-off-by: Vratislav Podzimek <vpodzime at redhat.com>
> ---
>  pyanaconda/ui/gui/spokes/custom.py | 117 ++++++++++++++++++++-----------------
>  1 file changed, 64 insertions(+), 53 deletions(-)
> 
> diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py
> index 59dd6ca..44407fa 100644
> --- a/pyanaconda/ui/gui/spokes/custom.py
> +++ b/pyanaconda/ui/gui/spokes/custom.py
> @@ -577,10 +577,12 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          for selector in page.members:
>              updateSelectorFromDevice(selector, selector.device)
>  
> -    def _replace_device(self, *args, **kwargs):
> +    def _replace_device(self, **kwargs):
>          """ Create a replacement device and update the device selector. """
>          selector = kwargs.pop("selector", None)
> -        new_device = self._storage_playground.factoryDevice(*args, **kwargs)
> +        dev_type = kwargs.pop("device_type")
> +        size = kwargs.pop("size")
> +        new_device = self._storage_playground.factoryDevice(dev_type, size, **kwargs)
>  
>          self._devices = self._storage_playground.devices
>  
> @@ -638,24 +640,14 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          self._update_selectors()
>          self._updateSpaceDisplay()
>  
> -    def _try_replace_device(self, device_type, size, fs_type, disks, mountpoint,
> -                            label, raid_level, encrypted, name, container_name,
> -                            container_encrypted, container_raid_level, container_size,
> -                            device, selector, old_device_type, old_disks, old_fs_type,
> -                            old_mountpoint, old_label, old_raid_level, old_encrypted,
> -                            old_name, old_container_name, old_container_encrypted,
> -                            old_container_raid_level, old_container_size):
> +    def _try_replace_device(self, selector, removed_device, new_device_info,
> +                            old_device_info):
> +        if removed_device:
> +            # we don't want to pass the device if we removed it
> +            new_device_info["device"] = None
>          try:
> -            self._replace_device(device_type, size, fstype=fs_type,
> -                                 disks=disks, mountpoint=mountpoint,
> -                                 label=label, raid_level=raid_level,
> -                                 encrypted=encrypted, name=name,
> -                                 container_name=container_name,
> -                                 container_encrypted=container_encrypted,
> -                                 container_raid_level=container_raid_level,
> -                                 container_size=container_size,
> -                                 device=_device,
> -                                 selector=selector)
> +            self._replace_device(selector=selector, **new_device_info)
> +            return True
>          except StorageError as e:
>              log.error("factoryDevice failed: %s", e)
>              # the factory's error handling has replaced all of the
> @@ -666,37 +658,26 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>              self.set_warning(_(DEVICE_CONFIGURATION_ERROR_MSG))
>              self.window.show_all()
>  
> -            if _device is None:
> -                # in this case we have removed the old device so we now have
> -                # to re-create it
> -
> -                        # the disks need to be updated since we've replaced all
> +            if not removed_device:
> +                # nothing more to do
> +                return True
> +            else:
> +                # we have removed the old device so we now have to re-create it
> +                # the disks need to be updated since we've replaced all
>                  # of the devices with copies in the devicefactory error
>                  # handler
>                  old_disk_names = (d.name for d in old_disks)
>                  old_disks = [self._storage_playground.devicetree.getDeviceByName(n) for n in old_disk_names]
>                  try:
> -                    self._replace_device(old_device_type, device.size,
> -                                         disks=old_disks,
> -                                         fstype=old_fs_type,
> -                                         mountpoint=old_mountpoint,
> -                                         label=old_label,
> -                                         raid_level=old_raid_level,
> -                                         encrypted=old_encrypted,
> -                                         name=old_name,
> -                                         container_name=old_container_name,
> -                                         container_encrypted=old_container_encrypted,
> -                                         container_raid_level=old_container_raid_level,
> -                                         container_size=old_container_size,
> -                                         selector=selector)
> +                    self._replace_device(selector=selector, **old_device_info)
> +                    return True
>                  except StorageError as e:
>                      # failed to recover.
>                      self.refresh()  # this calls self.clear_errors
>                      self._error = e
>                      self.set_warning(_(UNRECOVERABLE_ERROR_MSG))
>                      self.window.show_all()
> -                    return
> -
> +                    return False
>  
>      def _save_right_side(self, selector):
>          """ Save settings from RHS and apply changes to the device.
> @@ -706,11 +687,16 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          if not self._initialized or not selector:
>              return
>  
> +        # dictionaries for many, many pieces of information about the device and
> +        # requested changes
> +        new_device_info = dict()
> +        old_device_info = dict()
Just curious - why not:

new_device_info = {}
old_device_info = {}

?
>          device = selector.device
>          if device not in self._devices:
>              # just-removed device
>              return
>  
> +        new_device_info["device"] = device
>          if self._partitionsNotebook.get_current_page() != NOTEBOOK_DETAILS_PAGE:
>              return
>  
> @@ -733,6 +719,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>              # name entry insensitive means we don't control the name
>              name = None
>  
> +        old_device_info["name"] = old_name
> +        new_device_info["name"] = name
>          log.debug("old name: %s", old_name)
>          log.debug("new name: %s", name)
>  
> @@ -741,6 +729,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          size = size_from_entry(self._sizeEntry)
>          changed_size = ((use_dev.resizable or not use_dev.exists) and
>                          size != old_size)
> +        old_device_info["size"] = old_size
> +        new_device_info["size"] = size
>          log.debug("old size: %s", old_size)
>          log.debug("new size: %s", size)
>  
> @@ -748,6 +738,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          device_type = self._get_current_device_type()
>          old_device_type = devicefactory.get_device_type(device)
>          changed_device_type = (old_device_type != device_type)
> +        old_device_info["device_type"] = old_device_type
> +        new_device_info["device_type"] = device_type
>          log.debug("old device type: %s", old_device_type)
>          log.debug("new device type: %s", device_type)
>  
> @@ -758,17 +750,21 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          # FS TYPE
>          old_fs_type = device.format.type
>          fs_type_index = self._fsCombo.get_active()
> -        fs_type = self._fsCombo.get_model()[fs_type_index][0]
> -        new_fs = getFormat(fs_type)
> -        new_fs_type = new_fs.type
> -        changed_fs_type = (old_fs_type != new_fs_type)
> +        fs_type_str = self._fsCombo.get_model()[fs_type_index][0]
> +        new_fs = getFormat(fs_type_str)
> +        fs_type = new_fs.type
> +        changed_fs_type = (old_fs_type != fs_type)
> +        old_device_info["fstype"] = old_fs_type
> +        new_device_info["fstype"] = fs_type # XXX: should hold fstype_str?
>          log.debug("old fs type: %s", old_fs_type)
> -        log.debug("new fs type: %s", new_fs_type)
> +        log.debug("new fs type: %s", fs_type)
>  
>          # ENCRYPTION
>          old_encrypted = isinstance(device, LUKSDevice)
>          encrypted = self._encryptCheckbox.get_active()
>          changed_encryption = (old_encrypted != encrypted)
> +        old_device_info["encrypted"] = old_encrypted
> +        new_device_info["encrypted"] = encrypted
>          log.debug("old encryption setting: %s", old_encrypted)
>          log.debug("new encryption setting: %s", encrypted)
>  
> @@ -776,6 +772,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          label = self._labelEntry.get_text()
>          old_label = getattr(device.format, "label", "")
>          changed_label = (label != old_label)
> +        old_device_info["label"] = old_label
> +        new_device_info["label"] = label
>          log.debug("old label: %s", old_label)
>          log.debug("new_label: %s", label)
>          if changed_label or changed_fs_type:
> @@ -793,6 +791,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>              mountpoint = self._mountPointEntry.get_text()
>  
>          old_mountpoint = getattr(device.format, "mountpoint", "") or ""
> +        old_device_info["mountpoint"] = old_mountpoint
> +        new_device_info["mountpoint"] = mountpoint
>          log.debug("old mountpoint: %s", old_mountpoint)
>          log.debug("new mountpoint: %s", mountpoint or "")
>          if mountpoint is not None and (reformat or
> @@ -822,6 +822,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>                                device_type in (DEVICE_TYPE_MD,
>                                                DEVICE_TYPE_BTRFS) and
>                                old_raid_level != raid_level)
> +        old_device_info["raid_level"] = old_raid_level
> +        new_device_info["raid_level"] = raid_level
>          log.debug("old raid level: %s", old_raid_level)
>          log.debug("new raid level: %s", raid_level)
>  
> @@ -877,10 +879,14 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>              if old_container and container_name != old_container.name:
>                  changed_container = True
>  
> +        old_device_info["container_name"] = old_container_name
> +        new_device_info["container_name"] = container_name
>          log.debug("old container: %s", old_container_name)
>          log.debug("new container: %s", container_name)
>  
>          container_encrypted = self._device_container_encrypted
> +        old_device_info["container_encrypted"] = old_container_encrypted
> +        new_device_info["container_encrypted"] = container_encrypted
>          log.debug("old container encrypted: %s", old_container_encrypted)
>          log.debug("new container encrypted: %s", container_encrypted)
>          changed_container_encrypted = (container_encrypted != old_container_encrypted)
> @@ -889,11 +895,15 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>          if container_raid_level == "single" and device_type != DEVICE_TYPE_BTRFS:
>              container_raid_level = None
>  
> +        old_device_info["container_raid_level"] = old_container_raid_level
> +        new_device_info["container_raid_level"] = container_raid_level
>          log.debug("old container raid level: %s", old_container_raid_level)
>          log.debug("new container raid level: %s", container_raid_level)
>          changed_container_raid_level = (old_container_raid_level != container_raid_level)
>  
>          container_size = self._device_container_size
> +        old_device_info["container_size"] = old_container_size
> +        new_device_info["container_size"] = container_size
>          log.debug("old container size request: %s", old_container_size)
>          log.debug("new container size request: %s", container_size)
>          changed_container_size = (old_container_size != container_size)
> @@ -908,6 +918,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>              log.debug("overriding disk set with container's")
>              disks = container.disks[:]
>          changed_disk_set = (set(old_disks) != set(disks))
> +        old_device_info["disks"] = old_disks
> +        new_device_info["disks"] = disks
>          log.debug("old disks: %s", [d.name for d in old_disks])
>          log.debug("new disks: %s", [d.name for d in disks])
>  
> @@ -937,18 +949,17 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
>                      # the removal failed. don't continue.
>                      log.error("device removal failed")
>                      return
> -                _device = None
> +
> +                removed_device = True
>              else:
> -                _device = device
> +                removed_device = False
>  
>              with ui_storage_logger():
> -                self._try_replace_device(device_type, size, fs_type, disks, mountpoint,
> -                                         label, raid_level, encrypted, name, container_name,
> -                                         container_encrypted, container_raid_level, container_size,
> -                                         device, selector, old_device_type, old_disks, old_fs_type,
> -                                         old_mountpoint, old_label, old_raid_level, old_encrypted,
> -                                         old_name, old_container_name, old_container_encrypted,
> -                                         old_container_raid_level, old_container_size)
> +                succ = self._try_replace_device(selector, removed_device,
> +                                                new_device_info, old_device_info)
> +                if not succ:
> +                    # failed, nothing more to be done
> +                    return
>  
>              self._update_device_in_selectors(device, selector.device)
>              self._devices = self._storage_playground.devices




More information about the anaconda-patches mailing list