Steve McIntyre <st...@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.
signature.asc
Description: Digital signature
