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

Vratislav Podzimek vpodzime at redhat.com
Mon Dec 1 09:18:09 UTC 2014


On Mon, 2014-12-01 at 00:22 -0800, Adam Williamson wrote:
> On Mon, 2014-12-01 at 09:12 +0100, Vratislav Podzimek wrote:
> >  
> > > 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.
> 
> I thought 'volume' was more or less the standard term anaconda uses in
> this case - you know, when the first word that comes into your head is
> 'partition', but it's not actually quite correct =). So that's why I
> used it.
Standard term anaconda uses for something? That's hard to believe. ;) I
still think 'dev' would be best.

> 
> > 2) see one neat pick below:
> 
> > > +        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).
> 
> What would be the advantage of that, in this case? The list isn't likely
> to be particularly long, or anything. I don't mind doing it, I'd just
> like to learn why ;)
The only difference is that there won't be a temporary list created
(allocated, initialized,...) in the process. The expression is only used
to pre-filter some list for some other expression. Such "pre-filters"
are better done as generators instead of lists. It would make absolutely
no real difference in this case, however, that's why I called it a
"neat-pick".

> 
> > In relation to this I'd like to mention that your Python skills became
> > waaaay better over the recent months. Nice!
> 
> Thanks! It's not hard when the starting point is 'non-existent' ;)
That's hard to argue about. :)

-- 
Vratislav Podzimek

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



More information about the anaconda-patches mailing list