Bug#767037: Grub EFI fallback - patches for review

[Steve McIntyre]

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:
>
>grub-installer-rescue-UEFI-removable.patch:
>
>> diff --git a/debian/grub-installer.templates b/debian/grub-installer.templates
>> index e439ad0..a6af2ec 100644
>> --- a/debian/grub-installer.templates
>> +++ b/debian/grub-installer.templates
>> @@ -209,6 +209,21 @@ Type: text
>>  # :sl1:
>>  _Description: Updating /etc/kernel-img.conf...
>>  
>> +Template: grub-installer/progress/step_force_efi
>
>Perhaps add _removable at the end of the name?

Yeah, fair point.

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

>
>> +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. :-/

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

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

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

-- 
Steve McIntyre, Cambridge, UK.                                steve at einval.com
< Aardvark> I dislike C++ to start with. C++11 just seems to be
            handing rope-creating factories for users to hang multiple
            instances of themselves.