Re: [PATCH 08/10] templates: Output a menu entry for firmware setup on UEFI FastBoot systems

[Daniel Kiper]

On Fri, Mar 13, 2020 at 08:15:53PM +0100, Javier Martinez Canillas wrote:
> From: Steve Langasek <address@hidden>
>
> If fastboot is enabled in the BIOS then often it is not possible to
> enter the firmware setup menu, add a menu entry for this.
>
> hdegoede: Cherry picked the Ubuntu patch from:
> https://git.launchpad.net/~ubuntu-core-dev/grub/+git/ubuntu/tree/debian/patches/uefi_firmware_setup.patch
> Into the Fedora / RH grub version
>
> According to:
> https://git.launchpad.net/~ubuntu-core-dev/grub/+git/ubuntu/tree/debian/copyright
> The patch is licensed under GPL-3+
>
> Signed-off-by: Steve Langasek <address@hidden>
> [hdegoede: fix use with /sys/firmware/efi/efivars]

Please drop this line or move somewhere else...

...and in general this commit message formatting looks strange and makes
reading quite difficult. Same applies to some other patches in this
patchset...

> Signed-off-by: Hans de Goede <address@hidden>
> Signed-off-by: Javier Martinez Canillas <address@hidden>
>

Please drop this empty line...

> ---
>
>  Makefile.util.def               |  6 +++++
>  util/grub.d/30_uefi-firmware.in | 46 +++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
>  create mode 100644 util/grub.d/30_uefi-firmware.in
>
> diff --git a/Makefile.util.def b/Makefile.util.def
> index 1e0799a68f9..d9e2bd84de8 100644
> --- a/Makefile.util.def
> +++ b/Makefile.util.def
> @@ -515,6 +515,12 @@ script = {
>    installdir = grubconf;
>  };
>
> +script = {
> +  name = '30_uefi-firmware';
> +  common = util/grub.d/30_uefi-firmware.in;
> +  installdir = grubconf;
> +};
> +
>  script = {
>    name = '40_custom';
>    common = util/grub.d/40_custom.in;
> diff --git a/util/grub.d/30_uefi-firmware.in b/util/grub.d/30_uefi-firmware.in
> new file mode 100644
> index 00000000000..93ececffea7
> --- /dev/null
> +++ b/util/grub.d/30_uefi-firmware.in
> @@ -0,0 +1,46 @@
> +#! /bin/sh
> +set -e
> +
> +# grub-mkconfig helper script.
> +# Copyright (C) 2012  Free Software Foundation, Inc.
> +#
> +# GRUB is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# GRUB is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> +
> +prefix="@prefix@"
> +exec_prefix="@exec_prefix@"
> +datarootdir="@datarootdir@"
> +
> +export TEXTDOMAIN=@PACKAGE@
> +export TEXTDOMAINDIR="@localedir@"
> +
> +. "@datadir@/@PACKAGE@/grub-mkconfig_lib"

. "$pkgdatadir/grub-mkconfig_lib"

> +efi_vars_dir=/sys/firmware/efi/efivars

s/efi_vars_dir/EFI_VARS_DIR/g

> +EFI_GLOBAL_VARIABLE=8be4df61-93ca-11d2-aa0d-00e098032b8c
> +OsIndications="$efi_vars_dir/OsIndicationsSupported-$EFI_GLOBAL_VARIABLE"

s/OsIndications/OS_INDICATIONS/g

> +if [ -e "$OsIndications" ] && \
> +   [ "$(( $(printf 0x%x \'"$(cat $OsIndications | cut -b5)") & 1 ))" = 1 ]; 
> then

It seems to me that another \' is missing here...

> +  LABEL="System setup"
> +
> +  gettext_printf "Adding boot menu entry for EFI firmware configuration\n" 
> >&2

s/configuration/configuration .../

> +  onstr="$(gettext_printf "(on %s)" "${DEVICE}")"

This probably can be dropped...

> +  cat << EOF
> +menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
> +     fwsetup
> +}
> +EOF
> +fi

Daniel