On Fri, 2014-11-28 at 13:51 -0800, Adam Williamson wrote:
This is a follow-on from e2d10a04. To reduce the change from the previous behaviour and minimize the chance of any strange corner-case breakage, this sorts the disk list, rather than filtering it.
It also now checks the format_type constraint as well as the mountpoint constraint. There are platforms (PPC) which use a volume as the stage1 target, but don't require it to be mounted; this handles those as well as platforms which require the target volume to be mounted to a specific location.
It's also slightly re-arranged to flow better with the existing check that filters out protected, hidden and unusable disks.
The overall behaviour change can be described as: on platforms which require the stage1 target to be a volume, use the first enumerated disk which contains a potentially valid volume as the 'boot disk' if the user does not explicitly specify one, rather than just using the first enumerated disk and failing if it does not contain a valid stage1 target volume (#1168118).
This also debug logs the result of the 'disks with potential stage1 targets' check.
I like this improvement with two comments: 1) using the word 'volume' and accordingly named variables makes me think these are (only) LVM or BTRFS volumes which is not true. 'partition' is also problematic, because LVM could, in some cases, be used, but it's more typical to use partitions for stage1 devices. Or what about 'device' or 'dev'? That fits best I'd say.
2) see one neat pick below:
pyanaconda/kickstart.py | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/pyanaconda/kickstart.py b/pyanaconda/kickstart.py index d44b349..4aa4fd8 100644 --- a/pyanaconda/kickstart.py +++ b/pyanaconda/kickstart.py @@ -379,22 +379,30 @@ class Bootloader(commands.bootloader.F21_Bootloader): if self.timeout is not None: storage.bootloader.timeout = self.timeout
# try to reduce the set of possible 'boot disks' to the disks containing
# a valid stage1 mount point, if there are any, because those disks
# should be searched for a stage1_valid device
disks = storage.disks
if platform.bootStage1ConstraintDict["mountpoints"]:
disks = [p.disk for p in storage.devices if getattr(p.format, "mountpoint", None) in platform.bootStage1ConstraintDict["mountpoints"]]
if not disks:
# but if not, just go ahead and use the set of all disks or
# else we'll error out later
disks = storage.disks
# Throw out drives specified that don't exist or cannot be used (iSCSI # device on an s390 machine)
disk_names = [d.name for d in disks
if not d.format.hidden and not d.protected and
(not blivet.arch.isS390() or not isinstance(d, blivet.devices.iScsiDiskDevice))]
disks = [d for d in storage.disks
if not d.format.hidden and not d.protected and
(not blivet.arch.isS390() or not isinstance(d, blivet.devices.iScsiDiskDevice))]
if platform.bootStage1ConstraintDict["format_types"]:
# sort the list of disks with those that contain possibly-valid
# stage1 partitions first, so we'll pick a disk with one as
# bootDrive if it has not been explicitly set: RHBZ #1168118
s1vols = [vol for vol in storage.devices if
getattr(vol.format, "type", None) in
platform.bootStage1ConstraintDict["format_types"]]
This, after having a better name, could be a generator instead of a list comprehension (using parentheses instead of square brackets).
In relation to this I'd like to mention that your Python skills became waaaay better over the recent months. Nice!