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

Anne Mulhern amulhern at redhat.com
Mon Dec 1 15:48:34 UTC 2014





----- Original Message -----
> From: "Anne Mulhern" <amulhern at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Monday, December 1, 2014 9:51:54 AM
> Subject: Re: [f21-branch][PATCH] clean up stage1 volume check,	check format	as well as mountpoint
> 
> 
> 
> 
> 
> ----- 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?
> 

I do see that disk_names[0] is accessed later on in the execute
method, so the sorting does make a difference. OTOH, the line of code
that gets the value from disk_names by position was committed early
in 2012, and was not influenced by the previous version of this patch.

Under the assumption that changing what is disk_names[0], i.e., which disk is
assigned to self.bootDrive is a main purpose
of this updated patch, I've got a few more comments, coming up.

> 
> > +        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
> 
> 
> 
> 
> 
> 
> _______________________________________________
> 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