Bug#767037: Grub EFI fallback - patches for review

[Steve McIntyre]

On Mon, Dec 01, 2014 at 03:52:51PM +0100, Cyril Brulebois wrote:
>Steve McIntyre <steve at einval.com> (2014-12-01):
>> Control: severity 767037 serious
>> Control: tag 767037 +patch
>> 
>> [ Raising severity to serious as I've heard more and more reports of
>>   the problems here recently. ]
>> 
>> Hi folks,
>> 
>> i have two patches attached here, one for grub and one for
>> grub-installer. They implement the logic I described below and are
>> reasonably self-contained and non-intrusive. Please check them over
>> and give feedback, I'd like to get these in ASAP.
>
>Some comments inline + there seems to be little to no consistency as far
>as “{U,}EFI removable path” is concerned. Maybe make that uniform across
>the patches to reduce user bewilderment?

Hmmm, you're right. There's some existing inconsistencies already,
which don't help. In various places we already use "EFI" (e.g. in the
GRUB package names, EFI System Partition etc.) but in others it's
UEFI. Maybe we'd be better with just EFI everywhere...?

>Did you send the templates for review through debian-l10n-english?

Nope. They're currently entirely my own set of mistakes written in the
early hours... :-)

<snip>

>> +@@ -846,6 +876,7 @@ main (int argc, char *argv[])
>> +   char *relative_grubdir;
>> +   char **efidir_device_names = NULL;
>> +   grub_device_t efidir_grub_dev = NULL;
>> ++  char *base_efidir = NULL;
>> +   char *efidir_grub_devname;
>> +   int efidir_is_mac = 0;
>> +   int is_prep = 0;
>> +@@ -878,6 +909,9 @@ main (int argc, char *argv[])
>> +       bootloader_id = xstrdup ("grub");
>> +     }
>> + 
>> ++  if (removable && force_extra_removable)
>> ++    grub_util_error (_("Invalid to use both --removable and --force_extra_removable"));
>> ++
>> +   if (!grub_install_source_directory)
>> +     {
>> +       if (!target)
>> +@@ -1087,6 +1121,8 @@ main (int argc, char *argv[])
>> +       if (!efidir_is_mac && grub_strcmp (fs->name, "fat") != 0)
>> + 	grub_util_error (_("%s doesn't look like an EFI partition.\n"), efidir);
>> + 
>> ++      base_efidir = xstrdup(efidir);

>Needs a matching free()?

I wish it was that easy - the code in grub-install.c is a mix of
styles here. Some allocations are freed immediately, while some stay
around for the life of the program. This needs to be one of the
latter, I think.

<snip>

>> --- a/debian/templates.in
>> +++ b/debian/templates.in
>> @@ -12,6 +12,17 @@ _Description: Linux default command line:
>>   The following string will be used as Linux parameters for the default menu
>>   entry but not for the recovery mode.
>>  
>> +Template: grub2/force_efi_extra_removable
>> +Type: boolean
>> +Default: false
>> +_Description: Force extra installation to the EFI removable path?
>> + Some UEFI-based systems are buggy and do not handle new bootloaders correctly.
>> + If you force extra installation of GRUB to the EFI removable path, it should
>> + make sure that this system will boot Debian correctly despite such a problem.
>> + However, this may remove the ability to boot any other operating systems that
>> + also depend on this path. If so, uou will need to ensure that GRUB is
>
>you

ACK, good catch.

<snip>

>> diff --git a/rescue.d/81grub-efi-force-removable b/rescue.d/81grub-efi-force-removable
>> new file mode 100644
>> index 0000000..b35b724
>> --- /dev/null
>> +++ b/rescue.d/81grub-efi-force-removable
>> @@ -0,0 +1,91 @@
>> +#! /bin/sh -e
>> +
>> +. /usr/share/debconf/confmodule
>> +
>> +. /usr/share/grub-installer/functions.sh
>> +
>> +log () {
>> +	logger -t grub-installer "grub-efi-force-removable $@"
>> +}
>> +
>> +error () {
>> +	log "error: $@"
>> +}
>> +
>> +die () {
>> +	local template="$1"
>> +	shift
>> +
>> +	error "$@"
>> +	db_input critical "$template" || [ $? -eq 30 ]
>> +	db_go || true
>> +	exit 1
>> +}
>> +
>> +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
>> +	fi
>> +}
>> +
>> +db_progress START 0 3 grub-installer/progress/title
>> +db_progress INFO grub-installer/progress/step_force_efi
>> +
>> +# Should we also install grub-efi to the removable media path?
>> +# Ask the user
>> +log "Prompting user about removable media path"
>> +db_input high grub-installer/force-efi-extra-removable
>> +if ! db_go; then
>> +	# back up to menu
>> +	db_progress STOP
>> +	exit 10
>> +fi
>> +db_get grub-installer/force-efi-extra-removable
>> +if [ "$RET" != true ]; then
>> +	db_progress STOP
>> +	exit 0
>> +fi
>> +
>> +db_progress STEP 1
>> +db_progress INFO grub-installer/progress/step_mount_filesystems
>> +
>> +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
>
>in-target?

Hmmm, maybe. I saw a lot of calls to chroot directly in the
grub-installer code, and almost none using in-target. There aren't any
other uses of in-target in the rescue scripts either. I was just
following existing convention, but I'm happy to switch if it makes
sense.

<snip>

Thanks for the review!

Are you happy with the general approach I'm using?

-- 
Steve McIntyre, Cambridge, UK.                                steve at einval.com
"Further comment on how I feel about IBM will appear once I've worked out
 whether they're being malicious or incompetent. Capital letters are forecast."
 Matthew Garrett, http://www.livejournal.com/users/mjg59/30675.html