Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Glenn Washburn]

On Wed, 31 Aug 2022 16:14:42 +0100
Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote:

> On Tue, 30 Aug 2022 at 21:22, Robbie Harwood <rharwood@redhat.com> wrote:
> >
> > Philip Müller <philm@manjaro.org> writes:
> >
> > >> Hello Robbie, hello Daniel,
> > >>
> > >> with the commit 26031d3b101648352e4e427f04bf69d320088e77
> > >> 30_uefi-firmware will always call `fwsetup --is-supported' to check
> > >> if the system supports EFI or not. However most installed grub
> > >> versions on MBR don't support the '--is-supported' flag and crash the
> > >> menu creation routine.
> > >>
> > >> Arch Linux is tracking the issue here: 
> > >> https://bugs.archlinux.org/task/75701
> > >>
> > >> What would be the best approach with older installations of grub via
> > >> grub-install not having to reinstall grub to MBR as some users don't
> > >> even know on how to install grub properly as that job a graphical
> > >> installer does.
> > >
> > > Hi all,
> >
> > Adding grub-devel to CC.
> >
> > > I looked into the '30_uefi-firmware' changes a little more and also
> > > commented my findings at the Arch-Bugtracker:
> > > https://bugs.archlinux.org/task/75701#comment210684
> > >
> > > Before Menu changes we simply had this:
> > >
> > > ### BEGIN /etc/grub.d/30_uefi-firmware ###
> > > menuentry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
> > >    fwsetup
> > > }
> > > ### END /etc/grub.d/30_uefi-firmware ###
> > >
> > > After Menu changes we went to that:
> > >
> > > ### BEGIN /etc/grub.d/30_uefi-firmware ###
> > > fwsetup --is-supported
> > > if [ "$grub_platform" = "efi" -a "$?" = 0 ]; then
> > >    menuentry 'UEFI Firmware Settings' $menuentry_id_option 
> > > 'uefi-firmware' {
> > >      fwsetup
> > >    }
> > > fi
> 
> above code looks buggy to me. As far as I understand fwsetup only
> works on EFI grub_platform, and thus originally the menu entry and
> fwsetup call was scoped under efi platform. thus there should be two
> if conditions, not one:
> 
> if [ "$grub_platform" = "efi" ]; then
> fwsetup --is-supported
> if [ "$?" = 0 ]; then
> menuetry 'UEFI Firmware Settings' $menuentry_id_option 'uefi-firmware' {
> fwsetup
> }
> fi
> fi

I agree there's a bug, but I don't think its what is mentioned. The bug
is that $? is not set to 1 when a command does not exist. So in
i386-pc, where there is no fwsetup, executing "fwsetup --is-supported"
will fail because there is no command fwsetup. And $? should be set to
1 in this case, which would allow the existing code to work as expected.

Even if we fix the bug I mention, I support the above suggested change
because its easier to read.

> 
> This way the code continues to work correctly on bios platforms
> (skipped completely), and on EFI like platforms fwsetup menu entry is
> only shown if fwsetup --is-supported executes successfully.
> 
> the optimisation of having two conditionals evaluated together, whilst
> efficient, is wrong given cross grub platform compatibilities desires.
> 
> This fwsetup has never before been executed on bios platform, and we
> should not be introducing this.

And it never will be because its never built for the BIOS platform.
GRUB will error with "error: [16] can't find command `fwsetup'.".

Glenn

> 
> > > ### END /etc/grub.d/30_uefi-firmware ###
> > >
> > > So 0eb684e8bfb0a9d2d42017a354740be25947babe I get and
> > > 26031d3b101648352e4e427f04bf69d320088e77 tries to improve things,
> > > however doesn't count in what will happen if 'fwsetup' is not supporting
> > > the flag. It may either crash due to
> > > 1e79bbfbda24a08cb856ff30f8b1bec460779b91 or start UEFI firmware instead.
> >
> > Why doesn't grub on the MBR get updated when you install a new grub
> > package?  That seems like the real issue here - what am I missing?
> >
> > Regardless, if we can't count on fwsetup being updated, then I think we
> > need to go back to the original version of my patch which doesn't have
> > --is-supported.
> >
> > > Simply don't break user-space when older grub got installed to MBR.
> > > Somehow this idea needs some more thinking and a solution for that
> > > case too.
> >
> > Sure, what do you suggest?
> >
> > Be well,
> > --Robbie
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
>