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

[Daniel Kiper]

On Mon, Sep 05, 2022 at 05:49:47PM -0500, Glenn Washburn wrote:
> 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.

I concur.

Dimitri, may I ask you to send a patch with fix as you suggested above?

Daniel