Bug#767037: Grub EFI fallback - patches for review

[Ian Campbell]

On Wed, 2014-12-03 at 02:10 +0000, Steve McIntyre wrote:
> On Tue, Dec 02, 2014 at 08:51:24AM +0000, Ian Campbell wrote:
> >On Mon, 2014-12-01 at 13:57 +0000, Steve McIntyre wrote:
> >I didn't review the text since that seems to have been done already.
> >
> >> diff --git a/rescue.d/81grub-efi-force-removable b/rescue.d/81grub-efi-force-removable
> >
> >I don't know much about rescue mode, is this offering an automatic fixup
> >for this issue? Does it appear in a menu to be selected rather than
> >asking everyone booting rescue on an EFI system? 
> 
> The .tst file is run as a test:
> 
> [ -f /target/boot/grub/grub.cfg ] && ( grep -q /boot/efi /target/etc/fstab )
> 
> So, the target system must have grub installed and a mention of
> /boot/efi in /etc/fstab. Assuming that it does, an extra rescue option
> of "Force GRUB installation to the EFI removable media path" will show
> up as an option for the user. If the user selects it, the help text in
> grub-installer/force-efi-extra-removable is shown, then they can
> select to set the option.
> 

Neat, thanks for explaining.
> >
> >> +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.

> >Also you umount explicitly on the exit path, but don't cancel this trap,
> >so I guess you'll see some noise from umount the second time.
> 
> True; I've not seen any such errors, as it seems they're hidden at
> that point.
> 
> >I know we've established that in-target isn't widely used in this
> >particular bit of code -- but it does take care of all this sort of
> >thing automatically and (presumably!) correctly, as well as being only a
> >single place to fix if it is wrong (e.g. in-target handles BSD
> >explicitly too).
> 
> Right. I've absolutely *no* idea how well any of the existing EFI
> stuff will work with BSD...!

Me neither.

> >> +log "Mounting filesystems"
> >> +# If we're installing grub-efi, it wants /sys mounted in the
> >> +# target. Maybe /proc too?
> >> +mountvirtfs proc /target/proc
> >> +mountvirtfs sysfs /target/sys
> >> +chroot /target mount /boot/efi || true
> >> +
> >> +db_progress STEP 1
> >> +db_progress INFO grub-installer/progress/step_install_loader
> >> +# Do the installation now
> >> +log "Running grub-install"
> >> +if ! chroot /target grub-install --force-extra-removable; 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.

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

Ian.