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

Anne Mulhern amulhern at redhat.com
Mon Dec 1 14:51:54 UTC 2014





----- Original Message -----
> From: "Adam Williamson" <awilliam at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Friday, November 28, 2014 4:51:35 PM
> Subject: [f21-branch][PATCH] clean up stage1 volume check,	check format as well as mountpoint
> 
> 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.
> ---
>  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"]]
> +            if platform.bootStage1ConstraintDict["mountpoints"]:
> +                # further sort by volumes with valid mount points, if
> +                # the platform requires this
> +                s1disks = [vol.disk for vol in s1vols if
> +                           getattr(vol.format, "mountpoint", None) in
> +                           platform.bootStage1ConstraintDict["mountpoints"]]
> +            else:
> +                s1disks = [vol.disk for vol in s1vols]
> +            log.debug("Disks with potentially valid stage1 volumes: %s",
> +                      ' '.join([d.name for d in s1disks]))
> +            disks.sort(key=lambda x: x in s1disks, reverse=True)

AFAICT the whole chunk of code above, other than the assignment to disks is a no-op.
The code goes to a lot of work to find this sldisks value, and then sorts disks using
that value. But then, the sortedness of the disks matters not a whit, since the sorted
names are just passed to set() which is unsorted by definition.

Do you mean filter, instead of sort, or is there another patch still un-applied
which makes use of the sorted disks value in the execute() method?


> +        disk_names = [d.name for d in disks]
>          diskSet = set(disk_names)
>  
>          for drive in self.driveorder[:]:
> --
> 2.1.0
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

- mulhern








More information about the anaconda-patches mailing list