[f21-branch][PATCH] clean up stage1 volume check, check format as well as mountpoint

Vratislav Podzimek vpodzime at redhat.com
Mon Dec 1 08:12:12 UTC 2014


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!

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list