diff mbox series

[v4] templates: introduce GRUB_TOP_LEVEL_* vars

Message ID 20221017103532.845293-1-liu.denton@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v4] templates: introduce GRUB_TOP_LEVEL_* vars | expand

Commit Message

Denton Liu Oct. 17, 2022, 10:35 a.m. UTC
A user may wish to use an image that is not sorted as the "latest"
version as the top-level entry. For example, in Arch Linux, if a user
has the LTS and regular kernels installed, `/boot/vmlinuz-linux-lts`
gets sorted as the "latest" compared to `/boot/vmlinuz-linux`. However,
a user may wish to use the regular kernel as the default with the LTS
only existing as a backup.

Introduce the GRUB_TOP_LEVEL, GRUB_TOP_LEVEL_XEN and
GRUB_TOP_LEVEL_OS_PROBER variables to allow users to specify the
top-level entry.

Create grub_move_to_front() as a helper function which moves entries to
the front of a list. This function does the heavy lifting of moving
the menu entry to the front in each script.

In 10_netbsd, since there isn't an explicit list variable, extract the
items that are being iterated through into a list so that we can
optionally apply grub_move_to_front() to the list before the loop.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---

Notes:
    The only file that was tested is 10_linux. I do not have access to any
    of the other images or systems so they remain untested.
    
    Changes since v3:
    
    * Fix if formatting nit
    
    * Rebase on top of latest 'master'
    
    Changes since v2:
    
    * Added more detail to GRUB_TOP_LEVEL docs
    
    * Moved GRUB_TOP_LEVEL_OS_PROBER to separate section in docs
    
    * Renamed grub_move_entry_to_front() to grub_move_to_front() and added
      code comment
    
    * Give 10_netbsd an intermediate list of images to interact with

Range-diff against v3:
1:  3684d8fe2 ! 1:  d3a693804 templates: introduce GRUB_TOP_LEVEL_* vars
    @@ Notes
         The only file that was tested is 10_linux. I do not have access to any
         of the other images or systems so they remain untested.
     
    +    Changes since v3:
    +
    +    * Fix if formatting nit
    +
    +    * Rebase on top of latest 'master'
    +
         Changes since v2:
     
         * Added more detail to GRUB_TOP_LEVEL docs
    @@ util/grub-mkconfig_lib.in: version_sort ()
     +    echo "$item"
     +  fi
     +  for i in "$@"; do
    -+    if [ "x$i" = "x$item" ]; then continue; fi
    ++    if [ "x$i" = "x$item" ]; then
    ++      continue
    ++    fi
     +    echo "$i"
     +  done
     +}

 docs/grub.texi              | 10 ++++++++++
 util/grub-mkconfig.in       |  3 +++
 util/grub-mkconfig_lib.in   | 26 ++++++++++++++++++++++++++
 util/grub.d/10_hurd.in      |  4 ++++
 util/grub.d/10_kfreebsd.in  |  4 ++++
 util/grub.d/10_linux.in     |  4 ++++
 util/grub.d/10_netbsd.in    |  8 +++++++-
 util/grub.d/20_linux_xen.in |  7 +++++++
 util/grub.d/30_os-prober.in |  4 ++++
 9 files changed, 69 insertions(+), 1 deletion(-)

Comments

Oskari Pirhonen Oct. 18, 2022, 6:35 a.m. UTC | #1
On Mon, Oct 17, 2022 at 03:35:32 -0700, Denton Liu wrote:
> A user may wish to use an image that is not sorted as the "latest"
> version as the top-level entry. For example, in Arch Linux, if a user
> has the LTS and regular kernels installed, `/boot/vmlinuz-linux-lts`
> gets sorted as the "latest" compared to `/boot/vmlinuz-linux`. However,
> a user may wish to use the regular kernel as the default with the LTS
> only existing as a backup.
> 
> Introduce the GRUB_TOP_LEVEL, GRUB_TOP_LEVEL_XEN and
> GRUB_TOP_LEVEL_OS_PROBER variables to allow users to specify the
> top-level entry.
> 
> Create grub_move_to_front() as a helper function which moves entries to
> the front of a list. This function does the heavy lifting of moving
> the menu entry to the front in each script.
> 
> In 10_netbsd, since there isn't an explicit list variable, extract the
> items that are being iterated through into a list so that we can
> optionally apply grub_move_to_front() to the list before the loop.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>

Reviewed-by: Oskari Pirhonen <xxc3ncoredxx@gmail.com>

I've tested it on Linux, but the other platforms and os-prober are still
untested.

- Oskari
Olaf Hering Oct. 18, 2022, 11:12 a.m. UTC | #2
Mon, 17 Oct 2022 03:35:32 -0700 Denton Liu <liu.denton@gmail.com>:

> A user may wish to use an image that is not sorted as the "latest"
> version as the top-level entry. 

Correct.

What is really required is some form of file pattern matching and to use --id= in "menuentry title" commands.

That way one can have the "latest" of /boot/${short_pattern} as default entry, in case there are multiple variants of files matching ${short_pattern}. The pattern gets enabled with "grub-set-default pattern".

Based on this, I have to NACK your patch.


Olaf
Denton Liu Oct. 18, 2022, 1:57 p.m. UTC | #3
Hi Olaf,

On Tue, Oct 18, 2022 at 01:12:35PM +0200, Olaf Hering wrote:
> Mon, 17 Oct 2022 03:35:32 -0700 Denton Liu <liu.denton@gmail.com>:
> 
> > A user may wish to use an image that is not sorted as the "latest"
> > version as the top-level entry. 
> 
> Correct.
> 
> What is really required is some form of file pattern matching and to use --id= in "menuentry title" commands.
> 
> That way one can have the "latest" of /boot/${short_pattern} as default entry, in case there are multiple variants of files matching ${short_pattern}. The pattern gets enabled with "grub-set-default pattern".

If I'm understanding correctly, what you're proposing is a mechanism for
setting the default entry. If I'm not mistaken, this seems like an
orthogonal discussion to me. My patch proposes a method of setting the
top-level menu entry while this method only sets the default entry,
which may be hidden behind a submenu.

In my case, I keep a LTS kernel as backup but primarily use the latest
kernel as my daily driver. I only ever boot into the LTS kernel in case
there are any breakages, which is quite rare. As such, while it is
possible for me to configure my default to point within a submenu to the
non-LTS kernel, it seems like a bad user experience for Grub to impose a
top-level entry on the user, even if that entry is almost never used.

Allowing users to configure the top-level entry to fit their needs would
certainly make for happier users.

Thanks,
Denton
Olaf Hering Oct. 18, 2022, 2:18 p.m. UTC | #4
Tue, 18 Oct 2022 06:57:36 -0700 Denton Liu <liu.denton@gmail.com>:

> If I'm understanding correctly, what you're proposing is a mechanism for
> setting the default entry. If I'm not mistaken, this seems like an
> orthogonal discussion to me. My patch proposes a method of setting the
> top-level menu entry while this method only sets the default entry,
> which may be hidden behind a submenu.

I think this can be done already today. At least YaST offers a way to select a specific item in a submenu and pass it to grub-set-default. This leads to an entry like this in grubenv:

saved_entry=Advanced options for SLE15SP4 (with Xen hypervisor)>Xen hypervisor, version 4.17.20220823T122205.399bcbf2-xen_unstable.150400.370>SLE15SP4, with Xen 4.17.20220823T122205.399bcbf2-xen_unstable.150400.370 and Linux 5.14.21-150400.24.21-default

This entry will be booted as long as both this specific Xen version and this specific kernel version is found. There is a slim chance a SUSE specific patch exists to enable this functionality.

Maybe the patch description lacks a specific example how the proposed change is supposed to be used in your environment.

Olaf
Denton Liu Oct. 19, 2022, 4:53 a.m. UTC | #5
Hi Olaf,

On Tue, Oct 18, 2022 at 04:18:21PM +0200, Olaf Hering wrote:
> I think this can be done already today. At least YaST offers a way to select a specific item in a submenu and pass it to grub-set-default. This leads to an entry like this in grubenv:

Right, we currently offer the ability to navigate to a default _submenu_
but I think that it's bad UI to relegate the most oft-used entry to a
submenu entry instead of the top-level entry. I would like to be able to
specify the top-level entry, that is the first entry in the first menu.

> Maybe the patch description lacks a specific example how the proposed change is supposed to be used in your environment.

My patch description says:

	Introduce the GRUB_TOP_LEVEL, GRUB_TOP_LEVEL_XEN and
	GRUB_TOP_LEVEL_OS_PROBER variables to allow users to specify the
	top-level entry.

and I'm not quite sure how to make it more clear other than, perhaps,
explaining what the top-level entry means.

-Denton
Michael Chang Oct. 19, 2022, 6:23 a.m. UTC | #6
On Tue, Oct 18, 2022 at 04:18:21PM +0200, Olaf Hering wrote:
> Tue, 18 Oct 2022 06:57:36 -0700 Denton Liu <liu.denton@gmail.com>:
> 
> > If I'm understanding correctly, what you're proposing is a mechanism for
> > setting the default entry. If I'm not mistaken, this seems like an
> > orthogonal discussion to me. My patch proposes a method of setting the
> > top-level menu entry while this method only sets the default entry,
> > which may be hidden behind a submenu.
> 
> I think this can be done already today. At least YaST offers a way to select a specific item in a submenu and pass it to grub-set-default. This leads to an entry like this in grubenv:
> 
> saved_entry=Advanced options for SLE15SP4 (with Xen hypervisor)>Xen hypervisor, version 4.17.20220823T122205.399bcbf2-xen_unstable.150400.370>SLE15SP4, with Xen 4.17.20220823T122205.399bcbf2-xen_unstable.150400.370 and Linux 5.14.21-150400.24.21-default
> 
> This entry will be booted as long as both this specific Xen version and this specific kernel version is found. There is a slim chance a SUSE specific patch exists to enable this functionality.

Just to clarify. There is no specific patch, this is common function in grub.

Thanks,
Michael

> 
> Maybe the patch description lacks a specific example how the proposed change is supposed to be used in your environment.
> 
> Olaf



> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
Olaf Hering Oct. 20, 2022, 3:13 p.m. UTC | #7
Tue, 18 Oct 2022 21:53:32 -0700 Denton Liu <liu.denton@gmail.com>:

> On Tue, Oct 18, 2022 at 04:18:21PM +0200, Olaf Hering wrote:
> > Maybe the patch description lacks a specific example how the proposed change is supposed to be used in your environment.  
> My patch description says:
> 	Introduce the GRUB_TOP_LEVEL, GRUB_TOP_LEVEL_XEN and
> 	GRUB_TOP_LEVEL_OS_PROBER variables to allow users to specify the
> 	top-level entry.
> and I'm not quite sure how to make it more clear other than, perhaps,
> explaining what the top-level entry means.

After reading the patch again, the newly added documentation states:
"This option should be a path to a kernel image."

I think it needs to be more specific: is it expecting an absolute path, or just the basename of the desired image?


Olaf
Denton Liu Oct. 22, 2022, 3:53 a.m. UTC | #8
Hi Olaf,

On Thu, Oct 20, 2022 at 05:13:06PM +0200, Olaf Hering wrote:
> After reading the patch again, the newly added documentation states:
> "This option should be a path to a kernel image."
> 
> I think it needs to be more specific: is it expecting an absolute path, or just the basename of the desired image?

Thanks for the feedback, it should be an absolute path. I can submit a
new version of the patch later. However, before I do so, are you still
a NAK on the general idea?

-Denton
Olaf Hering Oct. 22, 2022, 6:28 a.m. UTC | #9
Fri, 21 Oct 2022 20:53:36 -0700 Denton Liu <liu.denton@gmail.com>:

> Thanks for the feedback, it should be an absolute path. I can submit a
> new version of the patch later. However, before I do so, are you still
> a NAK on the general idea?

It is not clear to me how useful it will be in practice.
In the end I have no saying about what goes into grub.git.


Olaf
diff mbox series

Patch

diff --git a/docs/grub.texi b/docs/grub.texi
index 0dbbdc374..5d41219ac 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -1444,6 +1444,16 @@  for all respectively normal entries.
 The values of these options replace the values of @samp{GRUB_CMDLINE_LINUX}
 and @samp{GRUB_CMDLINE_LINUX_DEFAULT} for Linux and Xen menu entries.
 
+@item GRUB_TOP_LEVEL
+@item GRUB_TOP_LEVEL_XEN
+This option should be a path to a kernel image. If provided, the image
+specified will be made the top-level entry if it is found in the scan.
+
+@item GRUB_TOP_LEVEL_OS_PROBER
+This option should be a line of output from @command{os-prober}. As
+@samp{GRUB_TOP_LEVEL}, if provided, the image specified will be made the
+top-level entry if it is found in the scan.
+
 @item GRUB_EARLY_INITRD_LINUX_CUSTOM
 @itemx GRUB_EARLY_INITRD_LINUX_STOCK
 List of space-separated early initrd images to be loaded from @samp{/boot}.
diff --git a/util/grub-mkconfig.in b/util/grub-mkconfig.in
index 62335d027..32c480dae 100644
--- a/util/grub-mkconfig.in
+++ b/util/grub-mkconfig.in
@@ -233,6 +233,9 @@  export GRUB_DEFAULT \
   GRUB_CMDLINE_NETBSD \
   GRUB_CMDLINE_NETBSD_DEFAULT \
   GRUB_CMDLINE_GNUMACH \
+  GRUB_TOP_LEVEL \
+  GRUB_TOP_LEVEL_XEN \
+  GRUB_TOP_LEVEL_OS_PROBER \
   GRUB_EARLY_INITRD_LINUX_CUSTOM \
   GRUB_EARLY_INITRD_LINUX_STOCK \
   GRUB_TERMINAL_INPUT \
diff --git a/util/grub-mkconfig_lib.in b/util/grub-mkconfig_lib.in
index 634bc8a50..08953287c 100644
--- a/util/grub-mkconfig_lib.in
+++ b/util/grub-mkconfig_lib.in
@@ -218,6 +218,32 @@  version_sort ()
    esac
 }
 
+# Given an item as the first argument and a list as the subsequent arguments,
+# returns the list with the first argument moved to the front if it exists in
+# the list.
+grub_move_to_front ()
+{
+  item="$1"
+  shift
+
+  item_found=false
+  for i in "$@"; do
+    if [ "x$i" = "x$item" ]; then
+      item_found=true
+    fi
+  done
+
+  if [ "x$item_found" = xtrue ]; then
+    echo "$item"
+  fi
+  for i in "$@"; do
+    if [ "x$i" = "x$item" ]; then
+      continue
+    fi
+    echo "$i"
+  done
+}
+
 # One layer of quotation is eaten by "" and the second by sed; so this turns
 # ' into \'.
 grub_quote () {
diff --git a/util/grub.d/10_hurd.in b/util/grub.d/10_hurd.in
index a021d02c2..b317a4b14 100644
--- a/util/grub.d/10_hurd.in
+++ b/util/grub.d/10_hurd.in
@@ -229,6 +229,10 @@  submenu_indentation=""
 
 reverse_sorted_kernels=$(echo ${kernels} | tr ' ' '\n' | sed -e 's/\.old$/ 1/; / 1$/! s/$/ 2/' | version_sort -r | sed -e 's/ 1$/.old/; s/ 2$//')
 
+if [ "x$GRUB_TOP_LEVEL" != x ]; then
+  reverse_sorted_kernels=$(grub_move_to_front "$GRUB_TOP_LEVEL" ${reverse_sorted_kernels})
+fi
+
 is_top_level=true
 
 for kernel in ${reverse_sorted_kernels}; do
diff --git a/util/grub.d/10_kfreebsd.in b/util/grub.d/10_kfreebsd.in
index 0a67decaa..83e9636e8 100644
--- a/util/grub.d/10_kfreebsd.in
+++ b/util/grub.d/10_kfreebsd.in
@@ -164,6 +164,10 @@  submenu_indentation=""
 
 reverse_sorted_list=$(echo ${list} | tr ' ' '\n' | sed -e 's/\.old$/ 1/; / 1$/! s/$/ 2/' | version_sort -r | sed -e 's/ 1$/.old/; s/ 2$//')
 
+if [ "x$GRUB_TOP_LEVEL" != x ]; then
+  reverse_sorted_list=$(grub_move_to_front "$GRUB_TOP_LEVEL" ${reverse_sorted_list})
+fi
+
 is_top_level=true
 
 for kfreebsd in ${reverse_sorted_list}; do
diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
index c6a1ec935..7263f2983 100644
--- a/util/grub.d/10_linux.in
+++ b/util/grub.d/10_linux.in
@@ -202,6 +202,10 @@  submenu_indentation=""
 
 reverse_sorted_list=$(echo $list | tr ' ' '\n' | sed -e 's/\.old$/ 1/; / 1$/! s/$/ 2/' | version_sort -r | sed -e 's/ 1$/.old/; s/ 2$//')
 
+if [ "x$GRUB_TOP_LEVEL" != x ]; then
+  reverse_sorted_list=$(grub_move_to_front "$GRUB_TOP_LEVEL" ${reverse_sorted_list})
+fi
+
 is_top_level=true
 for linux in ${reverse_sorted_list}; do
   gettext_printf "Found linux image: %s\n" "$linux" >&2
diff --git a/util/grub.d/10_netbsd.in b/util/grub.d/10_netbsd.in
index dc0cd1b17..3154e9e15 100644
--- a/util/grub.d/10_netbsd.in
+++ b/util/grub.d/10_netbsd.in
@@ -146,8 +146,14 @@  pattern="^ELF[^,]*executable.*statically linked"
 # yet, so it's empty. In a submenu it will be equal to '\t' (one tab).
 submenu_indentation=""
 
+list="/netbsd $(ls -t /netbsd?* 2>/dev/null)"
+
+if [ "x$GRUB_TOP_LEVEL" != x ]; then
+  list=$(grub_move_to_front "$GRUB_TOP_LEVEL" ${list})
+fi
+
 is_top_level=true
-for k in /netbsd $(ls -t /netbsd?* 2>/dev/null) ; do
+for k in ${list}; do
   if ! grub_file_is_not_garbage "$k" ; then
     continue
   fi
diff --git a/util/grub.d/20_linux_xen.in b/util/grub.d/20_linux_xen.in
index 626aed40c..386bfb9be 100644
--- a/util/grub.d/20_linux_xen.in
+++ b/util/grub.d/20_linux_xen.in
@@ -245,6 +245,13 @@  submenu_indentation=""
 reverse_sorted_xen_list=$(echo ${xen_list} | tr ' ' '\n' | sed -e 's/\.old$/ 1/; / 1$/! s/$/ 2/' | version_sort -r | sed -e 's/ 1$/.old/; s/ 2$//')
 reverse_sorted_linux_list=$(echo ${linux_list} | tr ' ' '\n' | sed -e 's/\.old$/ 1/; / 1$/! s/$/ 2/' | version_sort -r | sed -e 's/ 1$/.old/; s/ 2$//')
 
+if [ "x$GRUB_TOP_LEVEL_XEN" != x ]; then
+  reverse_sorted_xen_list=$(grub_move_to_front "$GRUB_TOP_LEVEL_XEN" ${reverse_sorted_xen_list})
+fi
+if [ "x$GRUB_TOP_LEVEL" != x ]; then
+  reverse_sorted_linux_list=$(grub_move_to_front "$GRUB_TOP_LEVEL" ${reverse_sorted_linux_list})
+fi
+
 is_top_level=true
 
 for current_xen in ${reverse_sorted_xen_list}; do
diff --git a/util/grub.d/30_os-prober.in b/util/grub.d/30_os-prober.in
index daa603778..656301eaf 100644
--- a/util/grub.d/30_os-prober.in
+++ b/util/grub.d/30_os-prober.in
@@ -113,6 +113,10 @@  EOF
 
 used_osprober_linux_ids=
 
+if [ "x$GRUB_TOP_LEVEL_OS_PROBER" != x ]; then
+  OSPROBED=$(grub_move_to_front "$GRUB_TOP_LEVEL_OS_PROBER" ${OSPROBED})
+fi
+
 for OS in ${OSPROBED} ; do
   DEVICE="`echo ${OS} | cut -d ':' -f 1`"
   LONGNAME="`echo ${OS} | cut -d ':' -f 2 | tr '^' ' '`"