[PATCH] Use submenus for 10_linux

[PATCH] Use submenus for 10_linux

[Colin Watson]

What do people think of this?  It's a remarkably small change now that
Vladimir's implemented submenus, and I've had a number of requests for
it.

I wonder if it would be worth having some visual indication that a menu
entry is a submenu; but that probably ought to be done centrally.

I don't know whether this is 1.99 material or not, and would appreciate
comments.

2010-11-24  Colin Watson  <cjwatson@ubuntu.com>

	* util/grub.d/10_linux.in: Put second and subsequent menu entries in
	a submenu.

=== modified file 'util/grub.d/10_linux.in'
--- util/grub.d/10_linux.in	2010-11-01 11:49:40 +0000
+++ util/grub.d/10_linux.in	2010-11-24 17:15:27 +0000
@@ -114,6 +114,7 @@ list=`for i in /boot/vmlinuz-* /boot/vml
       done`
 prepare_boot_cache=
 
+in_submenu=false
 while [ "x$list" != "x" ] ; do
   linux=`version_find_latest $list`
   echo "Found linux image: $linux" >&2
@@ -159,4 +160,13 @@ while [ "x$list" != "x" ] ; do
   fi
 
   list=`echo $list | tr ' ' '\n' | grep -vx $linux | tr '\n' ' '`
+
+  if [ "$list" ] && ! $in_submenu; then
+    echo "submenu \"Previous Linux versions\" {"
+    in_submenu=:
+  fi
 done
+
+if $in_submenu; then
+  echo "}"
+fi

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Use submenus for 10_linux

[Jordan Uggla]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

On 11/24/2010 09:21 AM, Colin Watson wrote:
> What do people think of this?  It's a remarkably small change now that
> Vladimir's implemented submenus, and I've had a number of requests for
> it.

<snip>

> 	* util/grub.d/10_linux.in: Put second and subsequent menu entries in
> 	a submenu.

<snip>

> +    echo "submenu \"Previous Linux versions\" {"

Since most users don't need to know what kernel version is being used by 
default, I think we should simplify the title of the default menu entry 
(by removing the "with Linux ...") and have a submenu labeled "Advanced 
options for $OS". This submenu would include all kernel entries, 
including the first entry (which would also be booted if you just 
selected the main entry instead of going to the submenu). So the submenu 
would look exactly like the main menu currently does, except only 
listing kernel entries for one OS.

I've attached a proof of concept patch which does this.

-- 
Jordan Uggla (Jordan_U on irc.freenode.net)

[-- Attachment #2: submenu.patch --]
[-- Type: text/x-patch, Size: 2014 bytes --]

=== modified file 'util/grub.d/10_linux.in'
--- util/grub.d/10_linux.in	2010-11-01 11:49:40 +0000
+++ util/grub.d/10_linux.in	2010-11-25 01:03:00 +0000
@@ -55,15 +55,25 @@
 {
   os="$1"
   version="$2"
-  recovery="$3"
+  type="$3"
   args="$4"
-  if ${recovery} ; then
-    title="$(gettext_quoted "%s, with Linux %s (recovery mode)")"
+
+  case $type in
+  simple)
+    title="$(gettext_quoted "%s")" ;;
+  recovery)
+    title="$(gettext_quoted "%s, with Linux %s (recovery mode)")" ;;
+  *)
+    title="$(gettext_quoted "%s, with Linux %s")" ;;
+  esac
+
+  if [ x$type = xsimple ]; then
+    printf "menuentry '${title}' ${CLASS} {\n" "${os}"
   else
-    title="$(gettext_quoted "%s, with Linux %s")"
+    printf "menuentry '${title}' ${CLASS} {\n" "${os}" "${version}"
   fi
-  printf "menuentry '${title}' ${CLASS} {\n" "${os}" "${version}"
-  if ! ${recovery} ; then
+
+  if [ x$type != xrecovery ] ; then
       save_default_entry | sed -e "s/^/\t/"
   fi
 
@@ -114,6 +124,8 @@
       done`
 prepare_boot_cache=
 
+is_first_entry=true
+
 while [ "x$list" != "x" ] ; do
   linux=`version_find_latest $list`
   echo "Found linux image: $linux" >&2
@@ -151,12 +163,26 @@
     linux_root_device_thisversion=${GRUB_DEVICE}
   fi
 
-  linux_entry "${OS}" "${version}" false \
+  if [ x$is_first_entry = xtrue ]; then
+    linux_entry "${OS}" "${version}" simple \
+    "${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_LINUX_DEFAULT}"
+
+    cat << EOF
+submenu '$(gettext_quoted "Advanced options for ${OS}")' {
+EOF
+  fi
+
+  linux_entry "${OS}" "${version}" advanced \
       "${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_LINUX_DEFAULT}"
   if [ "x${GRUB_DISABLE_RECOVERY}" != "xtrue" ]; then
-    linux_entry "${OS}" "${version}" true \
+    linux_entry "${OS}" "${version}" recovery \
 	"single ${GRUB_CMDLINE_LINUX}"
   fi
 
   list=`echo $list | tr ' ' '\n' | grep -vx $linux | tr '\n' ' '`
+  is_first_entry=false
 done
+
+if [ x$is_first_entry != xtrue ]; then #At least 1 kernel found, submenu started
+  echo '}'
+fi

Re: [PATCH] Use submenus for 10_linux

[Žika]

I like the idea. But how are we to set default kernel, other than first, to boot 
automatically, now...? Old-school method doesn't work...
Regards...

Re: [PATCH] Use submenus for 10_linux

[Colin Watson]

On Tue, Nov 30, 2010 at 01:36:15PM +0000, Žika wrote:
> I like the idea. But how are we to set default kernel, other than
> first, to boot automatically, now...? Old-school method doesn't
> work...

The following patch should fix handling of default='title' for entries
within submenus.  I haven't bothered defining a way to select submenu
entries by number.

Review welcome!  I'd particularly like confirmation from Vladimir that I'm
using the new extractor interface correctly, as I don't see any other
significant use of it in trunk at the moment beyond the extract_* commands
which nothing seems to be using yet.

2010-12-05  Colin Watson  <cjwatson@ubuntu.com>

	Fix defaults within submenus.

	* grub-core/normal/main.c (new_menu): New function.  Handles
	propagating `default', `fallback', and `timeout' variables to
	submenu contexts.
	(grub_normal_new_menu_context): Likewise.
	(grub_normal_new_menu_extractor): Likewise.
	* include/grub/normal.h (grub_normal_new_menu_context): Add
	prototype.
	(grub_normal_new_menu_extractor): Likewise.
	* grub-core/normal/menu.c (grub_menu_execute_entry): Use
	grub_normal_new_menu_context.  Unset `default', `fallback', and
	`timeout' if we fall off the end of the menu, to allow escape from a
	failed boot.
	(get_entry_number): Recursively scan submenus.
	(run_menu): Automatically traverse into a submenu if it contains the
	default entry.
	(show_menu): Initialise auto_boot to 0.
	* grub-core/script/execute.c (grub_script_execute_cmdline): Silence
	error messages for commands not valid when extracting menu entries
	(otherwise, if nothing else, we always get errors about setparams).

=== modified file 'grub-core/normal/main.c'
--- grub-core/normal/main.c	2010-09-20 22:47:49 +0000
+++ grub-core/normal/main.c	2010-12-05 00:36:28 +0000
@@ -123,6 +123,62 @@ grub_file_getline (grub_file_t file)
   return cmdline;
 }
 
+/* Open a new nested menu, propagating environment variables to the new
+   context as necessary.  */
+static grub_menu_t
+new_menu (int extractor)
+{
+  const char *default_val, *fallback_val, *timeout_val;
+  grub_menu_t menu;
+
+  default_val = grub_env_get ("default");
+  fallback_val = grub_env_get ("fallback");
+  timeout_val = grub_env_get ("timeout");
+
+  if (extractor)
+    grub_env_extractor_open (0);
+  else
+    grub_env_context_open ();
+
+  menu = grub_zalloc (sizeof (*menu));
+  if (!menu)
+    {
+      if (extractor)
+	grub_env_extractor_close (0);
+      else
+	grub_env_context_close ();
+      return NULL;
+    }
+  grub_env_set_menu (menu);
+
+  if (default_val)
+    {
+      /* Only propagate string defaults.  */
+      (void) grub_strtoul (default_val, 0, 0);
+      if (grub_errno == GRUB_ERR_BAD_NUMBER)
+	grub_env_set ("default", default_val);
+      grub_errno = GRUB_ERR_NONE;
+    }
+  if (fallback_val)
+    grub_env_set ("fallback", fallback_val);
+  if (timeout_val)
+    grub_env_set ("timeout", timeout_val);
+
+  return menu;
+}
+
+grub_menu_t
+grub_normal_new_menu_context (void)
+{
+  return new_menu (0);
+}
+
+grub_menu_t
+grub_normal_new_menu_extractor (void)
+{
+  return new_menu (1);
+}
+
 void
 grub_normal_free_menu (grub_menu_t menu)
 {

=== modified file 'grub-core/normal/menu.c'
--- grub-core/normal/menu.c	2010-09-20 22:47:49 +0000
+++ grub-core/normal/menu.c	2010-12-05 00:43:40 +0000
@@ -175,11 +175,9 @@ grub_menu_execute_entry(grub_menu_entry_
 
   if (entry->submenu)
     {
-      grub_env_context_open ();
-      menu = grub_zalloc (sizeof (*menu));
+      menu = grub_normal_new_menu_context ();
       if (! menu)
 	return;
-      grub_env_set_menu (menu);
     }
 
   grub_env_set ("chosen", entry->title);
@@ -201,6 +199,11 @@ grub_menu_execute_entry(grub_menu_entry_
 	}
       grub_env_context_close ();
     }
+
+  /* If we reach here, previous first-time defaults no longer make sense.  */
+  grub_env_unset ("default");
+  grub_env_unset ("fallback");
+  grub_env_unset ("timeout");
 }
 
 /* Execute ENTRY from the menu MENU, falling back to entries specified
@@ -311,7 +314,7 @@ grub_menu_register_viewer (struct grub_m
 
 /* Get the entry number from the variable NAME.  */
 static int
-get_entry_number (grub_menu_t menu, const char *name)
+get_entry_number (grub_menu_t menu, const char *name, int *in_submenu)
 {
   char *val;
   int entry;
@@ -323,6 +326,7 @@ get_entry_number (grub_menu_t menu, cons
   grub_error_push ();
 
   entry = (int) grub_strtoul (val, 0, 0);
+  *in_submenu = 0;
 
   if (grub_errno == GRUB_ERR_BAD_NUMBER)
     {
@@ -330,20 +334,43 @@ get_entry_number (grub_menu_t menu, cons
       grub_menu_entry_t e = menu->entry_list;
       int i;
 
+      entry = -1;
       grub_errno = GRUB_ERR_NONE;
 
-      for (i = 0; e; i++)
+      for (i = 0; e && entry == -1; i++)
 	{
 	  if (grub_strcmp (e->title, val) == 0)
+	    entry = i;
+	  else if (e->submenu)
 	    {
-	      entry = i;
-	      break;
+	      /* To make this work with submenus, we need to extract entries
+		 from the submenu, then return the entry number of the
+		 submenu itself if one of its entries (recursively) matches;
+		 when the submenu is executed for real, it will then have to
+		 call this function again one level down.  This is not going
+		 to be very efficient with a deep menu stack, but it seems
+		 unlikely that many people will bother with more than a
+		 couple of levels.  */
+	      grub_menu_t submenu;
+
+	      submenu = grub_normal_new_menu_extractor ();
+	      if (submenu)
+		{
+		  grub_script_execute_sourcecode (e->sourcecode,
+						  e->argc, e->args);
+		  if (get_entry_number (submenu, name, in_submenu) != -1)
+		    {
+		      entry = i;
+		      *in_submenu = 1;
+		    }
+
+		  grub_normal_free_menu (submenu);
+		  grub_env_extractor_close (0);
+		}
 	    }
+
 	  e = e->next;
 	}
-
-      if (! e)
-	entry = -1;
     }
 
   if (grub_errno != GRUB_ERR_NONE)
@@ -369,16 +396,21 @@ static int
 run_menu (grub_menu_t menu, int nested, int *auto_boot)
 {
   grub_uint64_t saved_time;
-  int default_entry, current_entry;
+  int default_entry = -1, current_entry;
+  int in_submenu = 0;
   int timeout;
 
-  default_entry = get_entry_number (menu, "default");
+  default_entry = get_entry_number (menu, "default", &in_submenu);
 
   /* If DEFAULT_ENTRY is not within the menu entries, fall back to
      the first entry.  */
   if (default_entry < 0 || default_entry >= menu->size)
     default_entry = 0;
 
+  /* If a submenu was automatically selected, traverse into it.  */
+  if (in_submenu)
+    return default_entry;
+
   /* If timeout is 0, drawing is pointless (and ugly).  */
   if (grub_menu_get_timeout () == 0)
     {
@@ -596,7 +628,7 @@ show_menu (grub_menu_t menu, int nested)
     {
       int boot_entry;
       grub_menu_entry_t e;
-      int auto_boot;
+      int auto_boot = 0;
 
       boot_entry = run_menu (menu, nested, &auto_boot);
       if (boot_entry < 0)

=== modified file 'grub-core/script/execute.c'
--- grub-core/script/execute.c	2010-11-07 10:43:14 +0000
+++ grub-core/script/execute.c	2010-12-04 23:58:02 +0000
@@ -628,7 +628,7 @@ grub_script_execute_cmdline (struct grub
 
   if (invert)
     {
-      if (ret == GRUB_ERR_TEST_FAILURE)
+      if (ret == GRUB_ERR_TEST_FAILURE || ret == GRUB_ERR_EXTRACTOR)
 	grub_errno = ret = GRUB_ERR_NONE;
       else if (ret == GRUB_ERR_NONE)
 	ret = grub_error (GRUB_ERR_TEST_FAILURE, "false");
@@ -642,7 +642,7 @@ grub_script_execute_cmdline (struct grub
   /* Free arguments.  */
   grub_script_argv_free (&argv);
 
-  if (grub_errno == GRUB_ERR_TEST_FAILURE)
+  if (grub_errno == GRUB_ERR_TEST_FAILURE || grub_errno == GRUB_ERR_EXTRACTOR)
     grub_errno = GRUB_ERR_NONE;
 
   grub_print_error ();

=== modified file 'include/grub/normal.h'
--- include/grub/normal.h	2010-09-20 22:47:49 +0000
+++ include/grub/normal.h	2010-12-05 00:36:26 +0000
@@ -125,6 +125,8 @@ grub_normal_add_menu_entry (int argc, co
 grub_err_t
 grub_normal_set_password (const char *user, const char *password);
 
+grub_menu_t grub_normal_new_menu_context (void);
+grub_menu_t grub_normal_new_menu_extractor (void);
 void grub_normal_free_menu (grub_menu_t menu);
 
 void grub_normal_auth_init (void);

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Use submenus for 10_linux

[Jordan Uggla]

On Sat, Dec 4, 2010 at 4:54 PM, Colin Watson <cjwatson@ubuntu.com> wrote:

> +             /* To make this work with submenus, we need to extract entries
> +                from the submenu, then return the entry number of the
> +                submenu itself if one of its entries (recursively) matches;
> +                when the submenu is executed for real, it will then have to
> +                call this function again one level down.  This is not going
> +                to be very efficient with a deep menu stack, but it seems
> +                unlikely that many people will bother with more than a
> +                couple of levels.  *

In addition to the performance impact with static submenus, this will
be completely unworkable with dynamically generated submenus. (Yes, I
can think of use cases for a saved default with a dynamically
generated submenu)

I would prefer something like:
GRUB_DEFAULT="Advanced options for Ubuntu>Ubuntu, with Linux 2.6.36.1"

Where '>' (or some other character) designates the what follows,
"Ubuntu, with Linux 2.6.36.1", is an entry within the submenu created
by selecting what precedes it, "Advanced options for Ubuntu".

Separately this also highlights an advantage of using a simplified
primary menu title (without a kernel version), as in this case if a
user wanted the default selection to be Ubuntu with the latest
installed kernel they would simply specify:
GRUB_DEFAULT="Ubuntu"

-- 
Jordan Uggla (Jordan_U on irc.freenode.net)