On Wed, 31 Aug 2022 16:14:42 +0100 Dimitri John Ledkov <dimitri.led...@canonical.com> wrote:
> On Tue, 30 Aug 2022 at 21:22, Robbie Harwood <rharw...@redhat.com> wrote: > > > > Philip Müller <ph...@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 > > > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
