Re: [PATCH v2] templates: introduce GRUB_TOP_LEVEL_* vars

[Oskari Pirhonen]

On Fri, Sep 30, 2022 at 04:11:11 -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.


> 
> 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.
> 

Blurbs like this should be placed below the "---" line so they don't
show up in commit messages. A brief changelog summarizing the
differences between patch versions is also nice (and belongs under the
"---" as well) so readers don't have to hunt for the previous versions
and analyze them to get an idea of the evolution of the patch. Bullet
points is generally enough for this.

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

(Example changelog)
v3:
- changes between v2 and v3
- ...

v2:
- use a more generic GRUB_TOP_LEVEL
- ...

>  docs/grub.texi              |  6 ++++++
>  util/grub-mkconfig.in       |  3 +++
>  util/grub-mkconfig_lib.in   | 21 +++++++++++++++++++++
>  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    | 10 +++++++++-
>  util/grub.d/20_linux_xen.in |  7 +++++++
>  util/grub.d/30_os-prober.in |  4 ++++
>  9 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 107f66ebc..16a445178 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -1444,6 +1444,12 @@ 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
> +@item GRUB_TOP_LEVEL_OS_PROBER
> +If this option is provided, the given image file will be made the top-level
> +entry if the image file is found in the scan.

I think explicitly mentioning that it's a _kernel_ image would be good.
That way no one can complain when their cute_cat.png wasn't made into a
top level entry. Also worth noting that you need to specify the path to
the kernel, not just the file name.

> +
>  @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..5be49a07c 100644
> --- a/util/grub-mkconfig_lib.in
> +++ b/util/grub-mkconfig_lib.in
> @@ -218,6 +218,27 @@ version_sort ()
>     esac
>  }
>  
> +grub_move_entry_to_front ()

I think a better name would be just `grub_move_to_front`. It makes it
more clear that this function can be used for more than just menu
entries in case someone wants to use it for something else in the
future. A short comment describing the arguments would be helpful for
that too.

Mention of this function is completely missing from the commit message.

> +{
> +  entry="$1"
> +  shift
> +
> +  entry_found=false
> +  for i in "$@"; do
> +    if [ "x$i" = "x$entry" ]; then
> +      entry_found=true
> +    fi
> +  done
> +
> +  if [ "x$entry_found" = xtrue ]; then
> +    echo "$entry"
> +  fi
> +  for i in "$@"; do
> +    if [ "x$i" = "x$entry" ]; 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 4294bbe4c..0ae5a3927 100644
> --- a/util/grub.d/10_hurd.in
> +++ b/util/grub.d/10_hurd.in
> @@ -205,6 +205,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_entry_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..506b2df66 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_entry_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..a479c16af 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_entry_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..4eae58e82 100644
> --- a/util/grub.d/10_netbsd.in
> +++ b/util/grub.d/10_netbsd.in
> @@ -147,10 +147,18 @@ pattern="^ELF[^,]*executable.*statically linked"
>  submenu_indentation=""
>  
>  is_top_level=true
> -for k in /netbsd $(ls -t /netbsd?* 2>/dev/null) ; do
> +grub_top_level_found=false
> +for k in "$GRUB_TOP_LEVEL" /netbsd $(ls -t /netbsd?* 2>/dev/null) ; do
>    if ! grub_file_is_not_garbage "$k" ; then
>      continue
>    fi
> +  if [ "x$GRUB_TOP_LEVEL" = "x$k" ] ; then
> +    if [ "x$grub_top_level_found" = xfalse ] ; then
> +      grub_top_level_found=true
> +    else
> +      continue
> +    fi
> +  fi
>    if ! (zcat -f "$k" | file -bL - | grep -q "${pattern}") 2>/dev/null ; then
>      continue
>    fi
> diff --git a/util/grub.d/20_linux_xen.in b/util/grub.d/20_linux_xen.in
> index 626aed40c..c32bb979f 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_entry_to_front "$GRUB_TOP_LEVEL_XEN" 
> ${reverse_sorted_xen_list})
> +fi
> +if [ "x$GRUB_TOP_LEVEL_LINUX" != x ]; then
> +  reverse_sorted_linux_list=$(grub_move_entry_to_front 
> "$GRUB_TOP_LEVEL_LINUX" ${reverse_sorted_linux_list})
> +fi

Here you've still got GRUB_TOP_LEVEL_LINUX but it's not documented
above. Is it meant to have its own, or did you just forget to replace it
with GRUB_TOP_LEVEL when going through them?

> +
>  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..9b2a96bd0 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_entry_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 '^' ' '`"
> -- 
> 2.37.3
> 

- Oskari

signature.asc
Description: PGP signature