Grub EFI fallback - patches for review

[Cyril Brulebois]

Steve McIntyre <steve at einval.com> (2014-12-01):
> 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...?

Looking at the EFI/UEFI history, I hope nobody will think EFI is the
deprecated, pre-UEFI implementation. With that in mind, and with all EFI
occurrences we already have (including “mandatory” names), it looks to
my non-expert eye that EFI might be the name to standardize over. Maybe
Colin could confirm that.

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

:p

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

OK. I suspected that way after having looked a bit, but thought asking
wouldn't hurt.

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

Nah, matching the surrounding code looks like a good idea.

> <snip>
> 
> Thanks for the review!

You're very welcome.

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

Well, given a few bug reports and (ISTR) some feedback from
users/reporters, it looks like the way to go, so sure.

Mraw,
KiBi.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-grub-devel/attachments/20141201/94667e2e/attachment-0003.sig>