[PATCH] provide a more useful error message if user fails to create an ESP

Adam Williamson awilliam at redhat.com
Mon Jan 27 20:35:26 UTC 2014


On Mon, 2014-01-27 at 12:01 -0600, David Lehman wrote:
> On Sun, 2014-01-26 at 23:39 -0800, Adam Williamson wrote:
> > If you do a UEFI native installation, use custom partitioning, and fail to
> > mount an EFI system partition at /boot/efi, blivet's __init__.py attempt to
> > set a stage1 device will fail completely and return our old friend, the
> > completely oblique error message "you have not created a bootloader stage1
> > target device". With this patch, when that test fails, if the arch is UEFI,
> > an additional error message is appended: "For a UEFI installation, your layout
> > must include an EFI system partition mounted at /boot/efi". It's always hard
> > to consider all possible paths to an error condition, but I don't _think_ it
> > would ever be incorrect to display this message when the attempt to set a
> > stage1 device fails during a UEFI installation, and usually it will be a
> > useful cue for the user.
> 
> It would be better to do this (in blivet/platform.py or maybe
> pyanaconda/bootloader.py) for all platforms than to cram this in just
> for EFI -- especially cramming it straight into the sanityCheck. Ugh.

That was actually my initial approach, and it didn't work, I *think*
because of https://bugzilla.redhat.com/show_bug.cgi?id=1058422 . I'm
fairly sure that, because of the overall logic of how bootloader setup
actually works right now, you can't easily get blivet's sanityCheck() to
display to the user any errors that come from
bootloader.is_valid_stage1_device() . There just isn't a simple way to
do it, at least that I can see.

I'm currently absolutely *drowning* in the relevant codepaths trying to
figure out the Really Right Way to do this. So far I think this is more
or less what's going on...

setupBootLoader() in blivet __init__.py is usually what actually gets to
set bootloader.stage1_device (there's a few special cases in
bootloader.py which circumvent this, I think, but I don't think we have
to care about those right now). It calls
"self.bootloader.set_stage1_device(self.devices)".

set_stage1_device() in bootloader.py does the is_valid_stage1_device
check, and raises BootLoaderError if it fails (those errors show up in
the logs, but are never displayed to the user).

sanityCheck() in blivet __init__.py , whenever it gets run, checks
whether self.bootloader.stage1_device is set, and just appends a
hard-coded error if it isn't (that's where I've inserted the 'extra'
error message for the UEFI case). It doesn't actually "invoke" anything
(I may be using the wrong terminology, but I hope YSWIM) to do that
check - it just reads in whatever the value's set to at that point, and
if it's not set to anything, appends the generic error message and just
moves right on to stage2 checking.

If self.bootloader.stage1_device *is* set, sanityCheck() then runs
is_valid_stage1_device() on the stage1 device itself, and *in theory*
appends any errors from that particular invocation to the set of errors
that will ultimately be returned by sanityCheck() and hence displayed to
the user. But that code is, if I'm following everything correctly,
currently either completely or nearly completely a dead letter - because
we're usually using setupBootLoader() to actually set up the bootloader,
and it calls set_stage1_device(), and *that* does
is_valid_stage1_device(), you're never going to reach the point where,
when sanityCheck() runs, self.bootloader.stage1_device is *set* but
*invalid*. If it's not valid it won't have been set, and you'll never
reach the else clause.

I'm fairly sure this is affecting already-existing code, not just making
it harder to add a new error. A call to is_valid_stage1_device() can
append various errors to bootloader.errors, and I think these were
intended to be displayed to the user by sanityCheck() and in the past
when setupBootLoader() did not exist they actually *were* displayed by
the user, but if I'm right, then at present, they never will be - you
can only find them in the logs.

What I'm now massively struggling with is the correct way to get errors
from is_valid_stage1_device into the set of errors returned by blivet's
sanityCheck() , but not break anything else. My brain is slowly melting.
-- 
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net



More information about the anaconda-patches mailing list