[PATCH 1/2] Move sanityCheck code to anaconda's codebase

Anne Mulhern amulhern at redhat.com
Wed Jul 30 08:38:59 UTC 2014





----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Tuesday, July 29, 2014 2:04:00 PM
> Subject: [PATCH 1/2] Move sanityCheck code to anaconda's codebase
> 
> This is the right place it belongs to.
> 
> Signed-off-by: Vratislav Podzimek <vpodzime at redhat.com>
> ---
>  pyanaconda/kickstart.py             |   7 +-
>  pyanaconda/storage_utils.py         | 189
>  +++++++++++++++++++++++++++++++++++-
>  pyanaconda/ui/gui/spokes/custom.py  |   8 +-
>  pyanaconda/ui/helpers.py            |   5 +-
>  pyanaconda/ui/tui/spokes/storage.py |   6 +-
>  5 files changed, 201 insertions(+), 14 deletions(-)
> 
> diff --git a/pyanaconda/kickstart.py b/pyanaconda/kickstart.py
> index 09f2a88..fcbfa72 100644
> --- a/pyanaconda/kickstart.py
> +++ b/pyanaconda/kickstart.py
> @@ -26,6 +26,7 @@ from blivet.devicelibs import swap
>  from blivet.formats import getFormat
>  from blivet.partitioning import doPartitioning
>  from blivet.partitioning import growLVM
> +from blivet.errors import PartitioningError
>  from blivet.size import Size
>  from blivet import udev
>  from blivet.platform import platform
> @@ -263,7 +264,7 @@ class AutoPart(commands.autopart.F21_AutoPart):
>  
>      def execute(self, storage, ksdata, instClass):
>          from blivet.partitioning import doAutoPartition
> -        from blivet.partitioning import sanityCheck
> +        from pyanaconda.storage_utils import sanity_check
>  
>          if not self.autopart:
>              return
> @@ -291,7 +292,9 @@ class AutoPart(commands.autopart.F21_AutoPart):
>              storage.autoPartType = self.type
>  
>          doAutoPartition(storage, ksdata)
> -        sanityCheck(storage)
> +        errors = sanity_check(storage)
> +        if errors:
> +            raise PartitioningError("autopart failed:\n" +
> "\n".join(error.message for error in errors))
>  
>  class Bootloader(commands.bootloader.F21_Bootloader):
>      def __init__(self, *args, **kwargs):
> diff --git a/pyanaconda/storage_utils.py b/pyanaconda/storage_utils.py
> index e6e3f70..f49cec0 100644
> --- a/pyanaconda/storage_utils.py
> +++ b/pyanaconda/storage_utils.py
> @@ -25,7 +25,10 @@ import locale
>  
>  from contextlib import contextmanager
>  
> +from blivet import arch
> +from blivet import util
>  from blivet.size import Size
> +from blivet.platform import platform as _platform
>  from blivet.devicefactory import DEVICE_TYPE_LVM
>  from blivet.devicefactory import DEVICE_TYPE_LVM_THINP
>  from blivet.devicefactory import DEVICE_TYPE_BTRFS
> @@ -33,7 +36,10 @@ from blivet.devicefactory import DEVICE_TYPE_MD
>  from blivet.devicefactory import DEVICE_TYPE_PARTITION
>  from blivet.devicefactory import DEVICE_TYPE_DISK
>  
> -from pyanaconda.i18n import N_
> +from pyanaconda.i18n import _, N_
> +from pyanaconda import isys
> +from pyanaconda.constants import productName
> +
>  from pykickstart.constants import AUTOPART_TYPE_PLAIN, AUTOPART_TYPE_BTRFS
>  from pykickstart.constants import AUTOPART_TYPE_LVM, AUTOPART_TYPE_LVM_THINP
>  
> @@ -128,3 +134,184 @@ def ui_storage_logger():
>      storage_log.addFilter(storage_filter)
>      yield
>      storage_log.removeFilter(storage_filter)
> +
> +class SanityException(Exception):
> +    pass
> +
> +class SanityError(SanityException):
> +    pass
> +
> +class SanityWarning(SanityException):
> +    pass
> +
> +class LUKSDeviceWithoutKeyError(SanityError):
> +    pass
> +
> +def sanity_check(storage):
> +    """
> +    Run a series of tests to verify the storage configuration.
> +
> +    This function is called at the end of partitioning so that
> +    we can make sure you don't have anything silly (like no /,
> +    a really small /, etc).
> +
> +    :rtype: a list of SanityExceptions
> +    :return: a list of accumulated errors and warnings
> +
> +    """
> +
> +    exns = []
> +
> +    checkSizes = [('/usr', 250), ('/tmp', 50), ('/var', 384),
> +                  ('/home', 100), ('/boot', 75)]
> +    mustbeonlinuxfs = ['/', '/var', '/tmp', '/usr', '/home', '/usr/share',
> '/usr/lib']
> +    mustbeonroot = ['/bin','/dev','/sbin','/etc','/lib','/root', '/mnt',
> 'lost+found', '/proc']
> +
> +    filesystems = storage.mountpoints
> +    root = storage.fsset.rootDevice
> +    swaps = storage.fsset.swapDevices
> +
> +    if root:
> +        if root.size < 250:
> +            exns.append(
> +               SanityWarning(_("Your root partition is less than 250 "
> +                              "megabytes which is usually too small to "
> +                              "install %s.") % (productName,)))
> +    else:
> +        exns.append(
> +           SanityError(_("You have not defined a root partition (/), "
> +                        "which is required for installation of %s "
> +                        "to continue.") % (productName,)))
> +
> +    # Prevent users from installing on s390x with (a) no /boot volume, (b)
> the
> +    # root volume on LVM, and (c) the root volume not restricted to a single
> +    # PV
> +    # NOTE: There is not really a way for users to create a / volume
> +    # restricted to a single PV.  The backend support is there, but there
> are
> +    # no UI hook-ups to drive that functionality, but I do not personally
> +    # care.  --dcantrell
> +    if arch.isS390() and '/boot' not in storage.mountpoints and root:
> +        if root.type == 'lvmlv' and not root.singlePV:
> +            exns.append(
> +               SanityError(_("This platform requires /boot on a dedicated "
> +                            "partition or logical volume.  If you do not "
> +                            "want a /boot volume, you must place / on a "
> +                            "dedicated non-LVM partition.")))
> +
> +    # FIXME: put a check here for enough space on the filesystems. maybe?
> +
> +    for (mount, size) in checkSizes:
> +        if mount in filesystems and filesystems[mount].size < size:
> +            exns.append(
> +               SanityWarning(_("Your %(mount)s partition is less than "
> +                              "%(size)s megabytes which is lower than "
> +                              "recommended for a normal %(productName)s "
> +                              "install.")
> +                            % {'mount': mount, 'size': size,
> +                               'productName': productName}))
> +
> +    for (mount, device) in filesystems.items():
> +        problem = filesystems[mount].checkSize()
> +        if problem < 0:
> +            exns.append(
> +               SanityError(_("Your %(mount)s partition is too small for
> %(format)s formatting "
> +                            "(allowable size is %(minSize)s to
> %(maxSize)s)")
> +                          % {"mount": mount, "format": device.format.name,
> +                             "minSize": device.minSize, "maxSize":
> device.maxSize}))
> +        elif problem > 0:
> +            exns.append(
> +               SanityError(_("Your %(mount)s partition is too large for
> %(format)s formatting "
> +                            "(allowable size is %(minSize)s to
> %(maxSize)s)")
> +                          % {"mount":mount, "format": device.format.name,
> +                             "minSize": device.minSize, "maxSize":
> device.maxSize}))
> +
> +    if storage.bootloader and not storage.bootloader.skip_bootloader:
> +        stage1 = storage.bootloader.stage1_device
> +        if not stage1:
> +            exns.append(
> +               SanityError(_("No valid bootloader target device found. "
> +                            "See below for details.")))
> +            pe = _platform.stage1MissingError
> +            if pe:
> +                exns.append(SanityError(_(pe)))
> +        else:
> +            storage.bootloader.is_valid_stage1_device(stage1)
> +            exns.extend(SanityError(msg) for msg in
> storage.bootloader.errors)
> +            exns.extend(SanityWarning(msg) for msg in
> storage.bootloader.warnings)
> +
> +        stage2 = storage.bootloader.stage2_device
> +        if stage1 and not stage2:
> +            exns.append(SanityError(_("You have not created a bootable
> partition.")))
> +        else:
> +            storage.bootloader.is_valid_stage2_device(stage2)
> +            exns.extend(SanityError(msg) for msg in
> storage.bootloader.errors)
> +            exns.extend(SanityWarning(msg) for msg in
> storage.bootloader.warnings)
> +            if not storage.bootloader.check():
> +                exns.extend(SanityError(msg) for msg in
> storage.bootloader.errors)
> +
> +        #
> +        # check that GPT boot disk on BIOS system has a BIOS boot partition
> +        #
> +        if _platform.weight(fstype="biosboot") and \
> +           stage1 and stage1.isDisk and \
> +           getattr(stage1.format, "labelType", None) == "gpt":
> +            missing = True
> +            for part in [p for p in storage.partitions if p.disk == stage1]:
> +                if part.format.type == "biosboot":
> +                    missing = False
> +                    break
> +
> +            if missing:
> +                exns.append(
> +                   SanityError(_("Your BIOS-based system needs a special "
> +                                "partition to boot from a GPT disk label. "
> +                                "To continue, please create a 1MiB "
> +                                "'biosboot' type partition.")))
> +
> +    if not swaps:
> +        installed = util.total_memory()
> +        required = Size("%s KiB" % isys.EARLY_SWAP_RAM)
> +
> +        if installed < required:
> +            exns.append(
> +               SanityError(_("You have not specified a swap partition.  "
> +                            "%(requiredMem)s of memory is required to
> continue installation "
> +                            "without a swap partition, but you only have
> %(installedMem)s.")
> +                          % {"requiredMem": required,
> +                             "installedMem": installed}))
> +        else:
> +            exns.append(
> +               SanityWarning(_("You have not specified a swap partition.  "
> +                              "Although not strictly required in all cases,
> "
> +                              "it will significantly improve performance "
> +                              "for most installations.")))
> +    no_uuid = [s for s in swaps if s.format.exists and not s.format.uuid]
> +    if no_uuid:
> +        exns.append(
> +           SanityWarning(_("At least one of your swap devices does not have
> "
> +                          "a UUID, which is common in swap space created "
> +                          "using older versions of mkswap. These devices "
> +                          "will be referred to by device path in "
> +                          "/etc/fstab, which is not ideal since device "
> +                          "paths can change under a variety of "
> +                          "circumstances. ")))
> +
> +    for (mountpoint, dev) in filesystems.items():
> +        if mountpoint in mustbeonroot:
> +            exns.append(
> +               SanityError(_("This mount point is invalid.  The %s directory
> must "
> +                            "be on the / file system.") % mountpoint))
> +
> +        if mountpoint in mustbeonlinuxfs and (not dev.format.mountable or
> not dev.format.linuxNative):
> +            exns.append(
> +               SanityError(_("The mount point %s must be on a linux file
> system.") % mountpoint))
> +
> +    if storage.rootDevice and storage.rootDevice.format.exists:
> +        e = storage.mustFormat(storage.rootDevice)
> +        if e:
> +            exns.append(SanityError(e))
> +
> +    exns += storage._verifyLUKSDevicesHaveKey()
> +
> +    return exns
> +
> diff --git a/pyanaconda/ui/gui/spokes/custom.py
> b/pyanaconda/ui/gui/spokes/custom.py
> index 45ab6de..e0a78b8 100644
> --- a/pyanaconda/ui/gui/spokes/custom.py
> +++ b/pyanaconda/ui/gui/spokes/custom.py
> @@ -56,8 +56,6 @@ from blivet.partitioning import doAutoPartition
>  from blivet.errors import StorageError
>  from blivet.errors import NoDisksError
>  from blivet.errors import NotEnoughFreeSpaceError
> -from blivet.errors import SanityError
> -from blivet.errors import SanityWarning
>  from blivet.errors import LUKSDeviceWithoutKeyError
>  from blivet.devicelibs import raid
>  from blivet.devices import LUKSDevice
> @@ -66,6 +64,8 @@ from pyanaconda.storage_utils import ui_storage_logger,
> device_type_from_autopar
>  from pyanaconda.storage_utils import DEVICE_TEXT_PARTITION, DEVICE_TEXT_MAP,
>  DEVICE_TEXT_MD
>  from pyanaconda.storage_utils import PARTITION_ONLY_FORMAT_TYPES,
>  MOUNTPOINT_DESCRIPTIONS
>  from pyanaconda.storage_utils import NAMED_DEVICE_TYPES,
>  CONTAINER_DEVICE_TYPES
> +from pyanaconda.storage_utils import SanityError, SanityWarning
> +from pyanaconda import storage_utils
>  
>  from pyanaconda.ui.communication import hubQ
>  from pyanaconda.ui.gui.spokes import NormalSpoke
> @@ -2167,7 +2167,7 @@ class CustomPartitioningSpoke(NormalSpoke,
> StorageChecker):
>      def _do_autopart(self):
>          """Helper function for on_create_clicked.
>             Assumes a non-final context in which at least some errors
> -           discovered by sanityCheck are not considered fatal because they
> +           discovered by sanity_check are not considered fatal because they
>             will be dealt with later.
>  
>             Note: There are never any non-existent devices around when this
>             runs.
> @@ -2209,7 +2209,7 @@ class CustomPartitioningSpoke(NormalSpoke,
> StorageChecker):
>              self._storage_playground.doAutoPart = False
>              log.debug("finished automatic partitioning")
>  
> -        exns = self._storage_playground.sanityCheck()
> +        exns = storage_utils.sanity_check(self._storage_playground)
>          errors = [exn for exn in exns if isinstance(exn, SanityError) and
>          not isinstance(exn, LUKSDeviceWithoutKeyError)]
>          warnings = [exn for exn in exns if isinstance(exn, SanityWarning)]
>          for error in errors:
> diff --git a/pyanaconda/ui/helpers.py b/pyanaconda/ui/helpers.py
> index 15597e8..e688a99 100644
> --- a/pyanaconda/ui/helpers.py
> +++ b/pyanaconda/ui/helpers.py
> @@ -84,14 +84,13 @@ class StorageChecker(object):
>                                       target=self.checkStorage))
>  
>      def checkStorage(self):
> -        from blivet.errors import SanityError
> -        from blivet.errors import SanityWarning
> +        from pyanaconda.storage_utils import sanity_check, SanityError,
> SanityWarning
>  
>          threadMgr.wait(constants.THREAD_EXECUTE_STORAGE)
>  
>          hubQ.send_not_ready(self._mainSpokeClass)
>          hubQ.send_message(self._mainSpokeClass, _("Checking storage
>          configuration..."))
> -        exns = self.storage.sanityCheck()
> +        exns = sanity_check(self.storage)
>          errors = [exn.message for exn in exns if isinstance(exn,
>          SanityError)]
>          warnings = [exn.message for exn in exns if isinstance(exn,
>          SanityWarning)]
>          (StorageChecker.errors, StorageChecker.warnings) = (errors,
>          warnings)
> diff --git a/pyanaconda/ui/tui/spokes/storage.py
> b/pyanaconda/ui/tui/spokes/storage.py
> index a0e05d9..e86f0bb 100644
> --- a/pyanaconda/ui/tui/spokes/storage.py
> +++ b/pyanaconda/ui/tui/spokes/storage.py
> @@ -27,13 +27,11 @@ from pyanaconda.ui.categories.system import
> SystemCategory
>  from pyanaconda.ui.tui.spokes import NormalTUISpoke
>  from pyanaconda.ui.tui.simpleline import TextWidget, CheckboxWidget
>  from pyanaconda.ui.tui.tuiobject import YesNoDialog
> -from pyanaconda.storage_utils import AUTOPART_CHOICES
> +from pyanaconda.storage_utils import AUTOPART_CHOICES, sanity_check,
> SanityError, SanityWarning
>  
>  from blivet import storageInitialize, arch
>  from blivet.size import Size
>  from blivet.errors import StorageError, DasdFormatError
> -from blivet.errors import SanityError
> -from blivet.errors import SanityWarning
>  from blivet.devices import DASDDevice, FcoeDiskDevice, iScsiDiskDevice,
>  MultipathDevice, ZFCPDiskDevice
>  from blivet.devicelibs.dasd import format_dasd, make_unformatted_dasd_list
>  from pyanaconda.flags import flags
> @@ -393,7 +391,7 @@ class StorageSpoke(NormalTUISpoke):
>              self._ready = True
>          else:
>              print(_("Checking storage configuration..."))
> -            exns = self.storage.sanityCheck()
> +            exns = sanity_check(self.storage)
>              errors = [exn.message for exn in exns if isinstance(exn,
>              SanityError)]
>              warnings = [exn.message for exn in exns if isinstance(exn,
>              SanityWarning)]
>              (self.errors, self.warnings) = (errors, warnings)
> --
> 1.9.3
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

It looks like you're being inconsistant w/ LUKSDeviceWithoutKeyError,
since you import it from blivet but also define it in anaconda.

I think you're better off just moving it and blivet._verifyLUKSDevicesHaveKey()
method into anaconda, preserving its place in the class hierarchy.
That way, the bits of code that assign to various lists based on type of error
will still do the right thing.

If you think that _verifyLUKSDevicesHaveKey() is actually finding out something
that is blivet specific, you could change it to a public method that just
returns a list of formatted LUKS devices that have no way of obtaining a key,
and use that info to raise an exception in anaconda.

- mulhern




More information about the anaconda-patches mailing list