Bug#767037: Grub EFI fallback - patches for review

[Steve McIntyre]

On Wed, Dec 03, 2014 at 09:44:08AM +0000, Ian Campbell wrote:
>On Wed, 2014-12-03 at 02:10 +0000, Steve McIntyre wrote:
>> >
>> >> +mountvirtfs () {
>> >> +       fstype="$1"
>> >> +       path="$2"
>> >> +       if grep -q "[[:space:]]$fstype\$" /proc/filesystems && \
>> >> +          ! grep -q "^[^ ]\+ \+$path " /proc/mounts; then
>> >> +               mkdir -p "$path" || \
>> >> +                       die grub-installer/mounterr "Error creating $path"
>> >> +               mount -t "$fstype" "$fstype" "$path" || \
>> >> +                       die grub-installer/mounterr "Error mounting $path"
>> >> +               trap "umount $path" HUP INT QUIT KILL PIPE TERM EXIT
>> >
>> >trap doesn't stack, does it? You call mountvirtfs twice, so only the
>> >second umount will actually happen on error.
>> 
>> True. This (buggy) code is already in use elsewhere in grub-installer,
>> I've just borrowed it. :-/
>
>Hrm, yes.

A more generic fix would be to add to a list of filesystems that need
unmounting, and trap to a new shell function that unmounts that
list. Not too hard, I think - I'll see if I can do that and get it
tested today.

Frankly, I'd also like to move mountvirtfs and that new function over
to a more central d-i scripts location and cut down on the duplicated
code. That's definitely something for post-jessie, as it's going to
potentially cut across a lot of the d-i packages.

>> Right. I've absolutely *no* idea how well any of the existing EFI
>> stuff will work with BSD...!
>
>Me neither.

Again, I'm tempted to leave that alone for now then.

>> >The main invocation would invoke this with a --target="foo-efi". Not
>> >sure if this matters or not.
>> 
>> Nope, the code in grub-install already picks up on the right platform
>> by default. I could add this too, but I'm not convinved it's necessary.
>
>Lets leave it then.

Agreed.

>> >In order to avoid having to repeat all the logic twice perhaps you could
>> >arrange to do the debconf-set-selections thing first and then run
>> >dpkg-reconfigure or something in the target to force the main postinst
>> >to rerun and reinstall?
>> 
>> Maybe, yeah. I'll take a look.
>> 
>> >> +       db_input critical grub-installer/grub-install-failed || true
>> >> +       db_go || true
>> >> +       db_progress STOP
>> >> +       exit 1
>> >
>> >You don't umount /target/boot/efi on this path.
>> 
>> Correct, and that's definitely wanted.
>
>The unmount is wanted or the leaving of /boot/efi mounted is? (I could
>see an argument either way actually).

I need to make sure that /target/boot/efi is unmounted; otherwise
exiting and re-entering the rescue menu fails.

Updated patch coming soon...

-- 
Steve McIntyre, Cambridge, UK.                                steve at einval.com
Support the Campaign for Audiovisual Free Expression: http://www.eff.org/cafe/