This is a setup in blivet for an anaconda bug fix. The really significant changes are: 1) sanityCheck now returns a list of Errors instead of strings 2) sanityCheck is no longer called from doAutoPartition
mulhern (4): Remove unused exception class (#1060255) Separate sanityCheck-ing from doAutoPartition (#1060255) Introduce two new SanityErrors (#1060255) Change messages to SanityError objects (#1060255)
blivet/__init__.py | 108 +++++++++++++++++++++---------------------------- blivet/errors.py | 10 +++-- blivet/partitioning.py | 8 +++- 3 files changed, 60 insertions(+), 66 deletions(-)
Related: rhbz#1060255
It is not raised anywhere and seems not to really belong.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/errors.py | 3 --- 1 file changed, 3 deletions(-)
diff --git a/blivet/errors.py b/blivet/errors.py index c0449c9..bc538c9 100644 --- a/blivet/errors.py +++ b/blivet/errors.py @@ -28,9 +28,6 @@ class StorageError(Exception): class NoDisksError(StorageError): pass
-class ErrorRecoveryFailure(StorageError): - pass - # Device class DeviceError(StorageError): pass
Related: rhbz#1060255
Moves the sanityCheck call into a separate method. The separate method doesn't do much but allows to throw a PartitioningError in some cases where that is expected.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/partitioning.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/blivet/partitioning.py b/blivet/partitioning.py index abaf1b3..263ad2f 100644 --- a/blivet/partitioning.py +++ b/blivet/partitioning.py @@ -285,6 +285,8 @@ def _scheduleVolumes(storage, devs): storage.createDevice(dev)
def doAutoPartition(storage, data): + """Calculates a partition scheme. + """ log.debug("doAutoPart: %s" % storage.doAutoPart) log.debug("encryptedAutoPart: %s" % storage.encryptedAutoPart) log.debug("autoPartType: %s" % storage.autoPartType) @@ -325,7 +327,11 @@ def doAutoPartition(storage, data):
storage.setUpBootLoader()
- # now do a full check of the requests +def sanityCheck(storage): + """Do a sanity check in a partitioning context. + + Raises a partitioning error on fatal errors. + """ (errors, warnings) = storage.sanityCheck() for error in errors: log.error(error)
Related: rhbz#1060255
SanityError should be something generated by blivet.sanityCheck. LUKSDeviceWithoutKeyError is the only distinguished subclass. It's special, because in some situations where sanityCheck is called, a missing key should not be considered fatal, but it's the caller's job to figure out if that is the situation.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/errors.py | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/blivet/errors.py b/blivet/errors.py index bc538c9..b8d0ea0 100644 --- a/blivet/errors.py +++ b/blivet/errors.py @@ -192,3 +192,10 @@ class NoDisksError(StorageError): # factories class DeviceFactoryError(StorageError): pass + +#sanity +class SanityError(StorageError): + pass + +class LUKSDeviceWithoutKeyError(SanityError): + pass
Related: rhbz#1060255
Change string messages returned in sanityCheck tuple and in _verifyLUKSDevicesHaveKey to Error objects. The major purpose of this is to be able to distinguish and filter them.
Also, got rid of that large chunk of commented out code, if we want it, we know where to find it.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/__init__.py | 108 +++++++++++++++++++++++------------------------------ 1 file changed, 46 insertions(+), 62 deletions(-)
diff --git a/blivet/__init__.py b/blivet/__init__.py index ced62ef..777cc84 100644 --- a/blivet/__init__.py +++ b/blivet/__init__.py @@ -1462,7 +1462,7 @@ class Blivet(object): d.format.type == "luks" and \ not d.format.exists and \ not d.format.hasKey): - yield _("LUKS device %s has no encryption key") % (dev.name,) + yield LUKSDeviceWithoutKeyError(_("LUKS device %s has no encryption key") % (dev.name,))
def sanityCheck(self): """ Run a series of tests to verify the storage configuration. @@ -1471,6 +1471,9 @@ class Blivet(object): we can make sure you don't have anything silly (like no /, a really small /, etc). Returns (errors, warnings) where each is a list of strings. + + :rtype: a pair of lists of SanityErrors + :return: a pair of accumulated errors and warnings """ warnings = [] errors = [] @@ -1493,13 +1496,15 @@ class Blivet(object):
if root: if root.size < 250: - warnings.append(_("Your root partition is less than 250 " + warnings.append( + SanityError(_("Your root partition is less than 250 " "megabytes which is usually too small to " - "install %s.") % (productName,)) + "install %s.") % (productName,))) else: - errors.append(_("You have not defined a root partition (/), " + errors.append( + SanityError(_("You have not defined a root partition (/), " "which is required for installation of %s " - "to continue.") % (productName,)) + "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 @@ -1510,10 +1515,11 @@ class Blivet(object): # care. --dcantrell if arch.isS390() and not self.mountpoints.has_key('/boot') and root: if root.type == 'lvmlv' and not root.singlePV: - errors.append(_("This platform requires /boot on a dedicated " + errors.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.")) + "dedicated non-LVM partition.")))
# FIXME: put a check here for enough space on the filesystems. maybe?
@@ -1529,66 +1535,38 @@ class Blivet(object): for (mount, device) in filesystems.items(): problem = filesystems[mount].checkSize() if problem < 0: - errors.append(_("Your %(mount)s partition is too small for %(format)s formatting " + errors.append( + SanityError(_("Your %(mount)s partition is too small for %(format)s formatting " "(allowable size is %(minSize)d MB to %(maxSize)d MB)") % {"mount": mount, "format": device.format.name, - "minSize": device.minSize, "maxSize": device.maxSize}) + "minSize": device.minSize, "maxSize": device.maxSize})) elif problem > 0: - errors.append(_("Your %(mount)s partition is too large for %(format)s formatting " + errors.append( + SanityError(_("Your %(mount)s partition is too large for %(format)s formatting " "(allowable size is %(minSize)d MB to %(maxSize)d MB)") % {"mount":mount, "format": device.format.name, - "minSize": device.minSize, "maxSize": device.maxSize}) - - # FIXME: this does not work, but probably should - """ - usb_disks = [] - firewire_disks = [] - #for disk in self.disks: - if isys.driveUsesModule(disk.name, ["usb-storage", "ub"]): - usb_disks.append(disk) - elif isys.driveUsesModule(disk.name, ["sbp2", "firewire-sbp2"]): - firewire_disks.append(disk) - - uses_usb = False - uses_firewire = False - for device in filesystems.values(): - for disk in usb_disks: - if device.dependsOn(disk): - uses_usb = True - break - - for disk in firewire_disks: - if device.dependsOn(disk): - uses_firewire = True - break - - if uses_usb: - warnings.append(_("Installing on a USB device. This may " - "or may not produce a working system.")) - if uses_firewire: - warnings.append(_("Installing on a FireWire device. This may " - "or may not produce a working system.")) - """ + "minSize": device.minSize, "maxSize": device.maxSize}))
if self.bootloader and not self.bootloader.skip_bootloader: stage1 = self.bootloader.stage1_device if not stage1: - errors.append(_("you have not created a bootloader stage1 " - "target device")) + errors.append( + SanityError(_("you have not created a bootloader stage1 " + "target device"))) else: self.bootloader.is_valid_stage1_device(stage1) - errors.extend(self.bootloader.errors) - warnings.extend(self.bootloader.warnings) + errors.extend(SanityError(msg) for msg in self.bootloader.errors) + warnings.extend(SanityError(msg) for msg in self.bootloader.warnings)
stage2 = self.bootloader.stage2_device if not stage2: - errors.append(_("You have not created a bootable partition.")) + errors.append(SanityError(_("You have not created a bootable partition."))) else: self.bootloader.is_valid_stage2_device(stage2) - errors.extend(self.bootloader.errors) - warnings.extend(self.bootloader.warnings) + errors.extend(SanityError(msg) for msg in self.bootloader.errors) + warnings.extend(SanityError(msg) for msg in self.bootloader.warnings) if not self.bootloader.check(): - errors.extend(self.bootloader.errors) + errors.extend(SanityError(msg) for msg in self.bootloader.errors)
# # check that GPT boot disk on BIOS system has a BIOS boot partition @@ -1603,48 +1581,54 @@ class Blivet(object): break
if missing: - errors.append(_("Your BIOS-based system needs a special " + errors.append( + SanityError(_("Your BIOS-based system needs a special " "partition to boot from a GPT disk label. " "To continue, please create a 1MB " - "'biosboot' type partition.")) + "'biosboot' type partition.")))
if not swaps: installed = Size(spec="%s kb" % util.total_memory()) required = Size(spec="%s kb" % isys.EARLY_SWAP_RAM)
if installed < required: - errors.append(_("You have not specified a swap partition. " + errors.append( + SanityError(_("You have not specified a swap partition. " "%(requiredMem)s MB of memory is required to continue installation " "without a swap partition, but you only have %(installedMem)s MB.") % {"requiredMem": int(required.convertTo(spec="MB")), - "installedMem": int(installed.convertTo(spec="MB"))}) + "installedMem": int(installed.convertTo(spec="MB"))})) else: - warnings.append(_("You have not specified a swap partition. " + warnings.append( + SanityError(_("You have not specified a swap partition. " "Although not strictly required in all cases, " "it will significantly improve performance " - "for most installations.")) + "for most installations."))) no_uuid = [s for s in swaps if s.format.exists and not s.format.uuid] if no_uuid: - warnings.append(_("At least one of your swap devices does not have " + warnings.append( + SanityError(_("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. ")) + "circumstances. ")))
for (mountpoint, dev) in filesystems.items(): if mountpoint in mustbeonroot: - errors.append(_("This mount point is invalid. The %s directory must " - "be on the / file system.") % mountpoint) + errors.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): - errors.append(_("The mount point %s must be on a linux file system.") % mountpoint) + errors.append( + SanityError(_("The mount point %s must be on a linux file system.") % mountpoint))
if self.rootDevice and self.rootDevice.format.exists: e = self.mustFormat(self.rootDevice) if e: - errors.append(e) + errors.append(SanityError(e))
errors += self._verifyLUKSDevicesHaveKey()
Related: rhbz#1060255
Change string messages returned in sanityCheck tuple and in _verifyLUKSDevicesHaveKey to Error objects. The major purpose of this is to be able to distinguish and filter them.
Also, got rid of that large chunk of commented out code, if we want it, we know where to find it.
If you're going to make the messages into objects, you could also give the objects a type to differentiate warnings from errors. Or perhaps better, define a base warning and a base error exception that inherit from your base exception. Then sanityCheck doesn't have to return a tuple (which I've always disliked) and instead just returns a single list.
- Chris
----- Original Message -----
From: "Chris Lumens" clumens@redhat.com To: anaconda-patches@lists.fedorahosted.org Sent: Friday, February 7, 2014 2:28:17 PM Subject: Re: [blivet:rhel7/master 4/4] Change messages to SanityError objects (#1060255)
Related: rhbz#1060255
Change string messages returned in sanityCheck tuple and in _verifyLUKSDevicesHaveKey to Error objects. The major purpose of this is to be able to distinguish and filter them.
Also, got rid of that large chunk of commented out code, if we want it, we know where to find it.
If you're going to make the messages into objects, you could also give the objects a type to differentiate warnings from errors. Or perhaps better, define a base warning and a base error exception that inherit from your base exception. Then sanityCheck doesn't have to return a tuple (which I've always disliked) and instead just returns a single list.
- Chris
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
I think the main problem that I saw with that is that if sanityCheck changes its mind about what is an error and what is warning you have to shuffle the class hierarchy around.
I'll reconsider it, though, I don't particularly like that pair either.
- mulhern
I think the main problem that I saw with that is that if sanityCheck changes its mind about what is an error and what is warning you have to shuffle the class hierarchy around.
If your class hierarchy isn't especially deep, I think it will be okay. I was imagining something like this:
StorageError SanityException SanityError LUKSDeviceWithoutKeyError ... more stuff here ... SanityWarning ... more stuff here ...
- Chris
anaconda-patches@lists.fedorahosted.org