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

Adam Williamson awilliam at redhat.com
Mon Dec 1 08:22:25 UTC 2014


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.

> 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 ;)

> 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' ;)
-- 
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | identi.ca: adamwfedora
http://www.happyassassin.net



More information about the anaconda-patches mailing list