[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