[PATCH 0/4] Make hidden menu really hidden

[PATCH 0/4] Make hidden menu really hidden

[Hans de Goede]

Hi All,

Let me start with a quick self-intro:

I'm a FOSS enthusiast / developer working for Red Hat, my latest project
at Red Hat is chasing what has over the years become the magical unicorn
of desktop Linux distros: a smooth graphical boot where the machine goes
from the BIOS screen to the graphical login manager in one smooth flow,
without any (80x25) text mode messages or black-screens in between.

These 4 patches modify grub so that when timeout_style=hidden is used,
and the user does not press a key to show the menu the EFI display is never
switched to text mode and the vendor logo stays in place on the monitor.

The first patch adds a new "version" command, this is not really related
to the other 3 patches, but when I started working on grub I wanted to
verify I was running my own build and I was surprised there was no such
command, so I added one.

The second patch is also only somewhat related, if the menu-timeout is
short, one way to still make sure you get the grub menu is to start
pressing the ESC key before grub is loaded, but on some systems ESC is
the hotkey to enter the firmware setup, so this commit also adds support
for pressing F8 to ge the menu. F8 is used by the Windows boot menu, so
almost all x86 firmwares don't use this key for their own purposes. This
will also make it easier for users coming from Windows to get the menu
(if they know that F8 is used for the boot menu Windows).

The third patch makes changes which a lot of distros have been carrying
as distro patches since 2013 at least, these changes make grub be quiet
during boot, except for the menu. These changes were not universally liked
in the past, because they may make debugging boot problems harder in some
cases, so they have never been merged.

This version of these changes makes the quiet behavior configurable
through a ./configure option which defaults to the old verbose behavior,
which should hopefully make everyone happy. This patch is a mix of Fedora
and Ubuntu patches for this, picking the best of both.

The fourth patch modifies the EFI terminal code to not switch the EFI
display to textmode until the first text is output. Note that for this
patch to actually make a difference, no text must be output, so we need
the "quiet" patch to be enabled and timeout_style=hidden. If any error
(or other output happens) grub will immediately switch to text mode and
show the message to the user.

Regards,

Hans

[PATCH 1/4] Add new "version" command

[Hans de Goede]

Add a new "version" command which prints the grub PACKAGE_STRING +
build time and date. This is useful to check if the expected version is
running, for e.g. trouble-shooting purposes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 grub-core/kern/corecmd.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/grub-core/kern/corecmd.c b/grub-core/kern/corecmd.c
index d9412a316..43273b901 100644
--- a/grub-core/kern/corecmd.c
+++ b/grub-core/kern/corecmd.c
@@ -170,6 +170,15 @@ grub_core_cmd_ls (struct grub_command *cmd __attribute__ ((unused)),
   return grub_errno;
 }
 
+/* version */
+static grub_err_t
+grub_core_cmd_version (struct grub_command *cmd __attribute__ ((unused)),
+		       int argc, char *argv[])
+{
+  grub_printf ("%s, build %s %s\n", PACKAGE_STRING, __DATE__, __TIME__);
+  return 0;
+}
+
 void
 grub_register_core_commands (void)
 {
@@ -186,4 +195,6 @@ grub_register_core_commands (void)
 			 N_("[ARG]"), N_("List devices or files."));
   grub_register_command ("insmod", grub_core_cmd_insmod,
 			 N_("MODULE"), N_("Insert a module."));
+  grub_register_command ("version", grub_core_cmd_version, 0,
+			 N_("Print grub version and build time."));
 }
-- 
2.17.0.rc1

[PATCH 2/4] Accept Both ESC and F8 as user interrupt keys

[Hans de Goede]

On most Bay Trail, Cherry Trail and Apollo Lake devices the ESC key is
the hotkey to enter the BIOS/EFI setup screen.

This makes it hard for the user to show the grub-menu when it is hidden
and a short timeout is used, because pressing ESC too early leads to the
user entering the BIOS/EFI setup screen.

F8 is (almost?) always free (on X86/PC platforms) as Windows uses this for
its boot menu, so also accept F8 as interrupt/show-menu key. As an added
advantage this is also more discoverable / easier for users coming from
Windows.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 grub-core/commands/sleep.c | 8 ++++++--
 grub-core/normal/menu.c    | 7 ++++---
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/grub-core/commands/sleep.c b/grub-core/commands/sleep.c
index e77e7900f..9f16956e0 100644
--- a/grub-core/commands/sleep.c
+++ b/grub-core/commands/sleep.c
@@ -51,12 +51,16 @@ static int
 grub_interruptible_millisleep (grub_uint32_t ms)
 {
   grub_uint64_t start;
+  int key;
 
   start = grub_get_time_ms ();
 
-  while (grub_get_time_ms () - start < ms)
-    if (grub_getkey_noblock () == GRUB_TERM_ESC)
+  while (grub_get_time_ms () - start < ms) {
+    key = grub_getkey_noblock ();
+    /* ESC sometimes is the BIOS setup hotkey, also allow F8 as intr. */
+    if (key == GRUB_TERM_ESC || key == GRUB_TERM_KEY_F8)
       return 1;
+  }
 
   return 0;
 }
diff --git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
index e7a83c2d6..d813fade1 100644
--- a/grub-core/normal/menu.c
+++ b/grub-core/normal/menu.c
@@ -610,8 +610,8 @@ run_menu (grub_menu_t menu, int nested, int *auto_boot)
 	  print_countdown (pos, timeout);
 	}
 
-      /* Enter interruptible sleep until Escape or a menu hotkey is pressed,
-         or the timeout expires.  */
+      /* Sleep until a menu hotkey is pressed, we are interrupted by an ESC/F8
+         keypress, or the timeout expires. */
       saved_time = grub_get_time_ms ();
       while (1)
 	{
@@ -624,7 +624,8 @@ run_menu (grub_menu_t menu, int nested, int *auto_boot)
 	      if (entry >= 0)
 		break;
 	    }
-	  if (key == GRUB_TERM_ESC)
+	  /* ESC sometimes is the BIOS setup hotkey, also allow F8 as intr. */
+	  if (key == GRUB_TERM_ESC || key == GRUB_TERM_KEY_F8)
 	    {
 	      timeout = -1;
 	      break;
-- 
2.17.0.rc1

[PATCH 3/4] Optionally print less messages at boot

[Hans de Goede]

The patch optionally makes grub not show any text (be fully quiet) when
timeout_style=hidden is set and the user does not interrupt the boot.

Combined with a later patch in this series which makes grub not touch
the EFI console unless it actually has some text to print, this will keep
the vendor logo which EFI put on the display in place until the kernel
touches the display. Leading to a more smooth / seamless boot experience.

At least Fedora/RHEL/CentOS and Ubuntu have been carrying patches for this
for a long time now (since 2013). There have been several attempts to
upstream these patches in the past already, which have been rejected
because not everyone likes the quiet behavior.

This patch makes the quiet behavior optional and defaults to off, so
unless grub is compiled with the new --enable-quiet-boot configure option
this patch changes nothing.

This patch is a mix of the Fedora patches for this and:
https://git.launchpad.net/~ubuntu-core-dev/grub/+git/ubuntu/tree/debian/patches/maybe_quiet.patch
https://git.launchpad.net/~ubuntu-core-dev/grub/+git/ubuntu/tree/debian/patches/gettext_quiet.patch
Specifically the configure.ac changes for making this optional come from
the Ubuntu patches, as Fedora's patches where simply unconditionally
removing all the unwanted grub_printf calls.

Cc: Colin Watson <cjwatson@ubuntu.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 config.h.in                       |  2 ++
 configure.ac                      | 16 ++++++++++++++++
 grub-core/boot/i386/pc/boot.S     |  2 ++
 grub-core/boot/i386/pc/diskboot.S | 12 +++++++++---
 grub-core/gettext/gettext.c       |  7 +++++++
 grub-core/kern/main.c             |  2 ++
 grub-core/normal/menu.c           |  2 ++
 grub-core/normal/menu_entry.c     |  2 ++
 util/grub.d/10_linux.in           | 19 ++++++++++++++-----
 9 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/config.h.in b/config.h.in
index 9e8f9911b..d2c4ce8e5 100644
--- a/config.h.in
+++ b/config.h.in
@@ -12,6 +12,8 @@
 /* Define to 1 to enable disk cache statistics.  */
 #define DISK_CACHE_STATS @DISK_CACHE_STATS@
 #define BOOT_TIME_STATS @BOOT_TIME_STATS@
+/* Define to 1 to make GRUB quieter at boot time.  */
+#define QUIET_BOOT @QUIET_BOOT@
 
 /* We don't need those.  */
 #define MINILZO_CFG_SKIP_LZO_PTR 1
diff --git a/configure.ac b/configure.ac
index c7888e40f..a544080d7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1834,6 +1834,17 @@ fi
 AC_SUBST([LIBZFS])
 AC_SUBST([LIBNVPAIR])
 
+AC_ARG_ENABLE([quiet-boot],
+              [AS_HELP_STRING([--enable-quiet-boot],
+                              [emit fewer messages at boot time (default=no)])],
+              [], [enable_quiet_boot=no])
+if test x"$enable_quiet_boot" = xyes ; then
+  QUIET_BOOT=1
+else
+  QUIET_BOOT=0
+fi
+AC_SUBST([QUIET_BOOT])
+
 LIBS=""
 
 AC_SUBST([FONT_SOURCE])
@@ -2086,5 +2097,10 @@ echo "Without liblzma (no support for XZ-compressed mips images) ($liblzma_excus
 else
 echo "With liblzma from $LIBLZMA (support for XZ-compressed mips images)"
 fi
+if [ x"$enable_quiet_boot" = xyes ]; then
+echo With quiet boot: Yes
+else
+echo With quiet boot: No
+fi
 echo "*******************************************************"
 ]
diff --git a/grub-core/boot/i386/pc/boot.S b/grub-core/boot/i386/pc/boot.S
index 2bd0b2d28..a0b023589 100644
--- a/grub-core/boot/i386/pc/boot.S
+++ b/grub-core/boot/i386/pc/boot.S
@@ -249,8 +249,10 @@ real_start:
 	/* save drive reference first thing! */
 	pushw	%dx
 
+#if !QUIET_BOOT
 	/* print a notification message on the screen */
 	MSG(notification_string)
+#endif
 
 	/* set %si to the disk address packet */
 	movw	$disk_address_packet, %si
diff --git a/grub-core/boot/i386/pc/diskboot.S b/grub-core/boot/i386/pc/diskboot.S
index 1ee4cf5b2..9fb44acbb 100644
--- a/grub-core/boot/i386/pc/diskboot.S
+++ b/grub-core/boot/i386/pc/diskboot.S
@@ -23,7 +23,13 @@
  *  defines for the code go here
  */
 
+#if !QUIET_BOOT
 #define MSG(x)	movw $x, %si; call LOCAL(message)
+#else
+#define MSG(x)
+#endif
+
+#define ERR(x)	movw $x, %si; call LOCAL(message)
 
 	.file	"diskboot.S"
 
@@ -305,17 +311,17 @@ LOCAL(bootit):
  * BIOS Geometry translation error (past the end of the disk geometry!).
  */
 LOCAL(geometry_error):
-	MSG(geometry_error_string)
+	ERR(geometry_error_string)
 	jmp	LOCAL(general_error)
 
 /*
  * Read error on the disk.
  */
 LOCAL(read_error):
-	MSG(read_error_string)
+	ERR(read_error_string)
 
 LOCAL(general_error):
-	MSG(general_error_string)
+	ERR(general_error_string)
 
 /* go here when you need to stop the machine hard after an error condition */
 LOCAL(stop):	jmp	LOCAL(stop)
diff --git a/grub-core/gettext/gettext.c b/grub-core/gettext/gettext.c
index 4880cefe3..f0e7a24ed 100644
--- a/grub-core/gettext/gettext.c
+++ b/grub-core/gettext/gettext.c
@@ -427,6 +427,13 @@ grub_gettext_init_ext (struct grub_gettext_context *ctx,
   if (locale[0] == 'e' && locale[1] == 'n'
       && (locale[2] == '\0' || locale[2] == '_'))
     grub_errno = err = GRUB_ERR_NONE;
+
+#if QUIET_BOOT
+  /* If no translations are available, silently fall back to untranslated. */
+  if (err == GRUB_ERR_FILE_NOT_FOUND)
+    grub_errno = err = GRUB_ERR_NONE;
+#endif
+
   return err;
 }
 
diff --git a/grub-core/kern/main.c b/grub-core/kern/main.c
index 9cad0c448..1f7bf9a9a 100644
--- a/grub-core/kern/main.c
+++ b/grub-core/kern/main.c
@@ -269,10 +269,12 @@ grub_main (void)
 
   grub_boot_time ("After machine init.");
 
+#if !QUIET_BOOT
   /* Hello.  */
   grub_setcolorstate (GRUB_TERM_COLOR_HIGHLIGHT);
   grub_printf ("Welcome to GRUB!\n\n");
   grub_setcolorstate (GRUB_TERM_COLOR_STANDARD);
+#endif
 
   grub_load_config ();
 
diff --git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
index d813fade1..7675c77bf 100644
--- a/grub-core/normal/menu.c
+++ b/grub-core/normal/menu.c
@@ -342,7 +342,9 @@ grub_menu_execute_with_fallback (grub_menu_t menu,
 {
   int fallback_entry;
 
+#if !QUIET_BOOT
   callback->notify_booting (entry, callback_data);
+#endif
 
   grub_menu_execute_entry (entry, 1);
 
diff --git a/grub-core/normal/menu_entry.c b/grub-core/normal/menu_entry.c
index cdf3590a3..32b35d895 100644
--- a/grub-core/normal/menu_entry.c
+++ b/grub-core/normal/menu_entry.c
@@ -1167,9 +1167,11 @@ run (struct screen *screen)
   char *dummy[1] = { NULL };
 
   grub_cls ();
+#if !QUIET_BOOT
   grub_printf ("  ");
   grub_printf_ (N_("Booting a command list"));
   grub_printf ("\n\n");
+#endif
 
   errs_before = grub_err_printed_errors;
 
diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
index faedf74e1..a1046a21a 100644
--- a/util/grub.d/10_linux.in
+++ b/util/grub.d/10_linux.in
@@ -20,6 +20,7 @@ set -e
 prefix="@prefix@"
 exec_prefix="@exec_prefix@"
 datarootdir="@datarootdir@"
+quiet_boot="@QUIET_BOOT@"
 
 . "$pkgdatadir/grub-mkconfig_lib"
 
@@ -128,20 +129,28 @@ linux_entry ()
     fi
     printf '%s\n' "${prepare_boot_cache}" | sed "s/^/$submenu_indentation/"
   fi
-  message="$(gettext_printf "Loading Linux %s ..." ${version})"
-  sed "s/^/$submenu_indentation/" << EOF
+  if [ x"$quiet_boot" = x0 ]; then
+    message="$(gettext_printf "Loading Linux %s ..." ${version})"
+    sed "s/^/$submenu_indentation/" << EOF
 	echo	'$(echo "$message" | grub_quote)'
+EOF
+  fi
+  sed "s/^/$submenu_indentation/" << EOF
 	linux	${rel_dirname}/${basename} root=${linux_root_device_thisversion} ro ${args}
 EOF
   if test -n "${initrd}" ; then
-    # TRANSLATORS: ramdisk isn't identifier. Should be translated.
-    message="$(gettext_printf "Loading initial ramdisk ...")"
+    if [ x"$quiet_boot" = x0 ]; then
+      # TRANSLATORS: ramdisk isn't identifier. Should be translated.
+      message="$(gettext_printf "Loading initial ramdisk ...")"
+      sed "s/^/$submenu_indentation/" << EOF
+	echo	'$(echo "$message" | grub_quote)'
+EOF
+    fi
     initrd_path=
     for i in ${initrd}; do
       initrd_path="${initrd_path} ${rel_dirname}/${i}"
     done
     sed "s/^/$submenu_indentation/" << EOF
-	echo	'$(echo "$message" | grub_quote)'
 	initrd	$(echo $initrd_path)
 EOF
   fi
-- 
2.17.0.rc1

[PATCH 4/4] EFI: Do not set text-mode until we actually need it

[Hans de Goede]

If we're running with a hidden menu we may never need text mode, so do not
change the video-mode to text until we actually need it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 grub-core/term/efi/console.c | 72 +++++++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 25 deletions(-)

diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
index 02f64ea74..d9fd7cf48 100644
--- a/grub-core/term/efi/console.c
+++ b/grub-core/term/efi/console.c
@@ -24,6 +24,11 @@
 #include <grub/efi/api.h>
 #include <grub/efi/console.h>
 
+static grub_err_t grub_prepare_for_text_output(struct grub_term_output *term);
+
+static int text_mode_available = -1;
+static int text_colorstate = -1;
+
 static grub_uint32_t
 map_char (grub_uint32_t c)
 {
@@ -66,14 +71,14 @@ map_char (grub_uint32_t c)
 }
 
 static void
-grub_console_putchar (struct grub_term_output *term __attribute__ ((unused)),
+grub_console_putchar (struct grub_term_output *term,
 		      const struct grub_unicode_glyph *c)
 {
   grub_efi_char16_t str[2 + 30];
   grub_efi_simple_text_output_interface_t *o;
   unsigned i, j;
 
-  if (grub_efi_is_finished)
+  if (grub_prepare_for_text_output (term))
     return;
 
   o = grub_efi_system_table->con_out;
@@ -220,14 +225,15 @@ grub_console_getkey (struct grub_term_input *term)
 }
 
 static struct grub_term_coordinate
-grub_console_getwh (struct grub_term_output *term __attribute__ ((unused)))
+grub_console_getwh (struct grub_term_output *term)
 {
   grub_efi_simple_text_output_interface_t *o;
   grub_efi_uintn_t columns, rows;
 
   o = grub_efi_system_table->con_out;
-  if (grub_efi_is_finished || efi_call_4 (o->query_mode, o, o->mode->mode,
-					  &columns, &rows) != GRUB_EFI_SUCCESS)
+  if (grub_prepare_for_text_output (term) != GRUB_ERR_NONE ||
+      efi_call_4 (o->query_mode, o, o->mode->mode,
+		  &columns, &rows) != GRUB_EFI_SUCCESS)
     {
       /* Why does this fail?  */
       columns = 80;
@@ -238,11 +244,11 @@ grub_console_getwh (struct grub_term_output *term __attribute__ ((unused)))
 }
 
 static struct grub_term_coordinate
-grub_console_getxy (struct grub_term_output *term __attribute__ ((unused)))
+grub_console_getxy (struct grub_term_output *term)
 {
   grub_efi_simple_text_output_interface_t *o;
 
-  if (grub_efi_is_finished)
+  if (grub_efi_is_finished || text_mode_available != 1)
     return (struct grub_term_coordinate) { 0, 0 };
 
   o = grub_efi_system_table->con_out;
@@ -250,12 +256,12 @@ grub_console_getxy (struct grub_term_output *term __attribute__ ((unused)))
 }
 
 static void
-grub_console_gotoxy (struct grub_term_output *term __attribute__ ((unused)),
+grub_console_gotoxy (struct grub_term_output *term,
 		     struct grub_term_coordinate pos)
 {
   grub_efi_simple_text_output_interface_t *o;
 
-  if (grub_efi_is_finished)
+  if (grub_prepare_for_text_output (term))
     return;
 
   o = grub_efi_system_table->con_out;
@@ -268,7 +274,7 @@ grub_console_cls (struct grub_term_output *term __attribute__ ((unused)))
   grub_efi_simple_text_output_interface_t *o;
   grub_efi_int32_t orig_attr;
 
-  if (grub_efi_is_finished)
+  if (grub_efi_is_finished || text_mode_available != 1)
     return;
 
   o = grub_efi_system_table->con_out;
@@ -279,8 +285,7 @@ grub_console_cls (struct grub_term_output *term __attribute__ ((unused)))
 }
 
 static void
-grub_console_setcolorstate (struct grub_term_output *term
-			    __attribute__ ((unused)),
+grub_console_setcolorstate (struct grub_term_output *term,
 			    grub_term_color_state state)
 {
   grub_efi_simple_text_output_interface_t *o;
@@ -288,6 +293,12 @@ grub_console_setcolorstate (struct grub_term_output *term
   if (grub_efi_is_finished)
     return;
 
+  if (text_mode_available != 1) {
+    /* Avoid "color_normal" environment writes causing a switch to textmode */
+    text_colorstate = state;
+    return;
+  }
+
   o = grub_efi_system_table->con_out;
 
   switch (state) {
@@ -307,12 +318,12 @@ grub_console_setcolorstate (struct grub_term_output *term
 }
 
 static void
-grub_console_setcursor (struct grub_term_output *term __attribute__ ((unused)),
+grub_console_setcursor (struct grub_term_output *term,
 			int on)
 {
   grub_efi_simple_text_output_interface_t *o;
 
-  if (grub_efi_is_finished)
+  if (grub_efi_is_finished || text_mode_available != 1)
     return;
 
   o = grub_efi_system_table->con_out;
@@ -320,18 +331,38 @@ grub_console_setcursor (struct grub_term_output *term __attribute__ ((unused)),
 }
 
 static grub_err_t
-grub_efi_console_output_init (struct grub_term_output *term)
+grub_prepare_for_text_output(struct grub_term_output *term)
 {
-  grub_efi_set_text_mode (1);
+  if (grub_efi_is_finished)
+    return GRUB_ERR_BAD_DEVICE;
+
+  if (text_mode_available != -1)
+    return text_mode_available ? 0 : GRUB_ERR_BAD_DEVICE;
+
+  if (! grub_efi_set_text_mode (1))
+    {
+      /* This really should never happen */
+      grub_error (GRUB_ERR_BAD_DEVICE, "cannot set text mode");
+      text_mode_available = 0;
+      return GRUB_ERR_BAD_DEVICE;
+    }
+
   grub_console_setcursor (term, 1);
+  if (text_colorstate != -1)
+    grub_console_setcolorstate (term, text_colorstate);
+  text_mode_available = 1;
   return 0;
 }
 
 static grub_err_t
 grub_efi_console_output_fini (struct grub_term_output *term)
 {
+  if (text_mode_available != 1)
+    return 0;
+
   grub_console_setcursor (term, 0);
   grub_efi_set_text_mode (0);
+  text_mode_available = -1;
   return 0;
 }
 
@@ -345,7 +376,6 @@ static struct grub_term_input grub_console_term_input =
 static struct grub_term_output grub_console_term_output =
   {
     .name = "console",
-    .init = grub_efi_console_output_init,
     .fini = grub_efi_console_output_fini,
     .putchar = grub_console_putchar,
     .getwh = grub_console_getwh,
@@ -361,14 +391,6 @@ static struct grub_term_output grub_console_term_output =
 void
 grub_console_init (void)
 {
-  /* FIXME: it is necessary to consider the case where no console control
-     is present but the default is already in text mode.  */
-  if (! grub_efi_set_text_mode (1))
-    {
-      grub_error (GRUB_ERR_BAD_DEVICE, "cannot set text mode");
-      return;
-    }
-
   grub_term_register_output ("console", &grub_console_term_output);
   grub_term_register_input ("console", &grub_console_term_input);
 }
-- 
2.17.0.rc1

Re: [PATCH 2/4] Accept Both ESC and F8 as user interrupt keys

[Lennart Sorensen]

On Wed, Mar 28, 2018 at 04:50:26PM +0200, Hans de Goede wrote:
> On most Bay Trail, Cherry Trail and Apollo Lake devices the ESC key is
> the hotkey to enter the BIOS/EFI setup screen.
> 
> This makes it hard for the user to show the grub-menu when it is hidden
> and a short timeout is used, because pressing ESC too early leads to the
> user entering the BIOS/EFI setup screen.
> 
> F8 is (almost?) always free (on X86/PC platforms) as Windows uses this for
> its boot menu, so also accept F8 as interrupt/show-menu key. As an added
> advantage this is also more discoverable / easier for users coming from
> Windows.

I have seen F8 and F12 used as boot menu keys on many systems.  I would
not consider it usually free.  Maybe it has become less common after
windows started using it, but it isn't that unusual.  ASUS seems to be
a major user of F8 for boot menu.  Not exactly an uncommon system board.
Some Lenovo desktops also use F8 for the boot menu.

-- 
Len Sorensen

Re: [PATCH 0/4] Make hidden menu really hidden

[Daniel Kiper]

Hi Hans,

On Wed, Mar 28, 2018 at 04:50:24PM +0200, Hans de Goede wrote:
> Hi All,
>
> Let me start with a quick self-intro:
>
> I'm a FOSS enthusiast / developer working for Red Hat, my latest project
> at Red Hat is chasing what has over the years become the magical unicorn
> of desktop Linux distros: a smooth graphical boot where the machine goes
> from the BIOS screen to the graphical login manager in one smooth flow,
> without any (80x25) text mode messages or black-screens in between.
>
> These 4 patches modify grub so that when timeout_style=hidden is used,
> and the user does not press a key to show the menu the EFI display is never
> switched to text mode and the vendor logo stays in place on the monitor.
>
> The first patch adds a new "version" command, this is not really related
> to the other 3 patches, but when I started working on grub I wanted to
> verify I was running my own build and I was surprised there was no such
> command, so I added one.
>
> The second patch is also only somewhat related, if the menu-timeout is
> short, one way to still make sure you get the grub menu is to start
> pressing the ESC key before grub is loaded, but on some systems ESC is
> the hotkey to enter the firmware setup, so this commit also adds support
> for pressing F8 to ge the menu. F8 is used by the Windows boot menu, so
> almost all x86 firmwares don't use this key for their own purposes. This
> will also make it easier for users coming from Windows to get the menu
> (if they know that F8 is used for the boot menu Windows).
>
> The third patch makes changes which a lot of distros have been carrying
> as distro patches since 2013 at least, these changes make grub be quiet
> during boot, except for the menu. These changes were not universally liked
> in the past, because they may make debugging boot problems harder in some
> cases, so they have never been merged.
>
> This version of these changes makes the quiet behavior configurable
> through a ./configure option which defaults to the old verbose behavior,
> which should hopefully make everyone happy. This patch is a mix of Fedora
> and Ubuntu patches for this, picking the best of both.
>
> The fourth patch modifies the EFI terminal code to not switch the EFI
> display to textmode until the first text is output. Note that for this
> patch to actually make a difference, no text must be output, so we need
> the "quiet" patch to be enabled and timeout_style=hidden. If any error
> (or other output happens) grub will immediately switch to text mode and
> show the message to the user.

I will take a look at the patches in a week or so. Please be patient.

Daniel

Re: [PATCH 2/4] Accept Both ESC and F8 as user interrupt keys

[Hans de Goede]

Hi,

On 28-03-18 16:56, Lennart Sorensen wrote:
> On Wed, Mar 28, 2018 at 04:50:26PM +0200, Hans de Goede wrote:
>> On most Bay Trail, Cherry Trail and Apollo Lake devices the ESC key is
>> the hotkey to enter the BIOS/EFI setup screen.
>>
>> This makes it hard for the user to show the grub-menu when it is hidden
>> and a short timeout is used, because pressing ESC too early leads to the
>> user entering the BIOS/EFI setup screen.
>>
>> F8 is (almost?) always free (on X86/PC platforms) as Windows uses this for
>> its boot menu, so also accept F8 as interrupt/show-menu key. As an added
>> advantage this is also more discoverable / easier for users coming from
>> Windows.
> 
> I have seen F8 and F12 used as boot menu keys on many systems.  I would
> not consider it usually free.  Maybe it has become less common after
> windows started using it, but it isn't that unusual.  ASUS seems to be
> a major user of F8 for boot menu.  Not exactly an uncommon system board.
> Some Lenovo desktops also use F8 for the boot menu.

Hmm, well people will still be able to use ESC to get the grub boot
menu, rather then the firmware boot-menu on those.

The problem is that our current check for ESC only approach is problematic
because it conflicts with the enter firmware-setup key on almost all Bay Trail,
Cherry Trail and Apollo Lake devices. You need to try hard to find a device
in one of those 3 categories which does not use ESC for this. Note I'm
aware some devices exist, but using ESC for this is really really common
among these devices.

I'm open to other suggestions, but I think we really need to add another
key to avoid the pressing ESC at boot already has another meaning problem
and F8 seems like an ok choice.

Either way thank you for the input on this.

Regards,

Hans

Re: [PATCH 2/4] Accept Both ESC and F8 as user interrupt keys

[Lennart Sorensen]

On Wed, Mar 28, 2018 at 05:06:53PM +0200, Hans de Goede wrote:
> Hmm, well people will still be able to use ESC to get the grub boot
> menu, rather then the firmware boot-menu on those.
> 
> The problem is that our current check for ESC only approach is problematic
> because it conflicts with the enter firmware-setup key on almost all Bay Trail,
> Cherry Trail and Apollo Lake devices. You need to try hard to find a device
> in one of those 3 categories which does not use ESC for this. Note I'm
> aware some devices exist, but using ESC for this is really really common
> among these devices.
> 
> I'm open to other suggestions, but I think we really need to add another
> key to avoid the pressing ESC at boot already has another meaning problem
> and F8 seems like an ok choice.
> 
> Either way thank you for the input on this.

I don't disagree about having another key, I just disagree that F8 is
a good choice.

Looking at https://kb.wisc.edu/page.php?id=58779#here it appears no one
uses F4 for anything.  That would seem like a better choice than F8
at least.  F6 also seems to be free but F4 seems nicer on desktop
keyboards.

-- 
Len Sorensen

Re: [PATCH 2/4] Accept Both ESC and F8 as user interrupt keys

[Hans de Goede]

Hi,

On 28-03-18 17:11, Lennart Sorensen wrote:
> On Wed, Mar 28, 2018 at 05:06:53PM +0200, Hans de Goede wrote:
>> Hmm, well people will still be able to use ESC to get the grub boot
>> menu, rather then the firmware boot-menu on those.
>>
>> The problem is that our current check for ESC only approach is problematic
>> because it conflicts with the enter firmware-setup key on almost all Bay Trail,
>> Cherry Trail and Apollo Lake devices. You need to try hard to find a device
>> in one of those 3 categories which does not use ESC for this. Note I'm
>> aware some devices exist, but using ESC for this is really really common
>> among these devices.
>>
>> I'm open to other suggestions, but I think we really need to add another
>> key to avoid the pressing ESC at boot already has another meaning problem
>> and F8 seems like an ok choice.
>>
>> Either way thank you for the input on this.
> 
> I don't disagree about having another key, I just disagree that F8 is
> a good choice.
> 
> Looking at https://kb.wisc.edu/page.php?id=58779#here it appears no one
> uses F4 for anything.  That would seem like a better choice than F8
> at least.  F6 also seems to be free but F4 seems nicer on desktop
> keyboards.

Looking at that table I have to agree that F4 seems like the best second
key to use. Actually looking at that table ESC seems like a poor choice,
but I guess it makes sense for serial-terminals as well as from a general
UI pov (and we're stuck with it now anyways).

I'm going to wait for Daniel's review of the other patches before I do
a v2 of this patch-set. I will switch to F4 for v2.

Regards,

Hans

Re: [PATCH 1/4] Add new "version" command

[Olaf Hering]

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

On Wed, Mar 28, Hans de Goede wrote:

> +  grub_printf ("%s, build %s %s\n", PACKAGE_STRING, __DATE__, __TIME__);

NACK.

Debian sells something what they call "reproducible build". Check their
website why usage of __DATE__ and __TIME__ is a bad thing.
SUSE does not want to republish an otherwise unchanged grub2 package
just because there was a need to rebuild the package.

Olaf

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 1/4] Add new "version" command

[Thomas Schmitt]

Hi,

Hans de Goede wrote:
> > +  grub_printf ("%s, build %s %s\n", PACKAGE_STRING, __DATE__, __TIME__);

Olaf Hering wrote:
> Debian sells something what they call "reproducible build".

It's not actually a Debian thing, although some Debian Developers seem to
have initiated it and urge people to make binary production deterministic
and reproducible.
  https://reproducible-builds.org/


> Check their website why usage of __DATE__ and __TIME__ is a bad thing.

They offer a solution for the problem of timestamps by the definition
of an environment variable SOURCE_DATE_EPOCH, which shall override the
current time.
  https://reproducible-builds.org/docs/timestamps/

In the libraries underneath xorriso i based all timestamps and pseudo-random
data fields on this user-set timestamp, if it is present.
So if you set the variable SOURCE_DATE_EPOCH to the same value before runs
of grub-mkrescue, and if all GRUB tools underneath grub-mkrescue build
reproducible binaries and xorriso options, and if the other input files
are still the same (*), then the resulting ISO images of two runs will be
identical. (Needs xorriso-1.4.6 or younger.)

(*) Some attributes of input files do not matter in this case.
    A good tar copy of the input tree should be sufficiently similar.


Have a nice day :)

Thomas

Re: [PATCH 1/4] Add new "version" command

[Hans de Goede]

Hi,

On 29-03-18 09:53, Olaf Hering wrote:
> On Wed, Mar 28, Hans de Goede wrote:
> 
>> +  grub_printf ("%s, build %s %s\n", PACKAGE_STRING, __DATE__, __TIME__);
> 
> NACK.
> 
> Debian sells something what they call "reproducible build". Check their
> website why usage of __DATE__ and __TIME__ is a bad thing.
> SUSE does not want to republish an otherwise unchanged grub2 package
> just because there was a need to rebuild the package.

Good point, lets drop this patch then, as without the date
I don't think there is much value in it and I don't feel
like spending time on this to come up with another solution.

Regards,

Hans

Re: [PATCH 1/4] Add new "version" command

[Olaf Hering]

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

On Thu, Mar 29, Hans de Goede wrote:

> I don't think there is much value in it and I don't feel
> like spending time on this to come up with another solution.

Not sure what you are trying to fix. At least for me the version is
shown all the time at the top. And the buildtime does not add any value.
Unclear why gcc still supports these two tags these days.

Olaf

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 1/4] Add new "version" command

[Daniel Kiper]

On Wed, Mar 28, 2018 at 04:50:25PM +0200, Hans de Goede wrote:
> Add a new "version" command which prints the grub PACKAGE_STRING +
> build time and date. This is useful to check if the expected version is
> running, for e.g. trouble-shooting purposes.

If you really need this I would consider setting some set of variables.
Currently GRUB2 provides grub_cpu and grub_platform. So, why not set e.g.
grub_version, grub_patchlevel, grub_sublevel and grub_extraversion (I have
taken the names from Linux kernel but we can use anything else which make
sense). This way you can get what you need using "set" command and scripts
may use variables to identify GRUB2 version. Ahhh... What about grub_commit
if it is available too?

Daniel

Re: [PATCH 2/4] Accept Both ESC and F8 as user interrupt keys

[Daniel Kiper]

On Wed, Mar 28, 2018 at 05:58:16PM +0200, Hans de Goede wrote:
> Hi,
>
> On 28-03-18 17:11, Lennart Sorensen wrote:
> >On Wed, Mar 28, 2018 at 05:06:53PM +0200, Hans de Goede wrote:
> >>Hmm, well people will still be able to use ESC to get the grub boot
> >>menu, rather then the firmware boot-menu on those.
> >>
> >>The problem is that our current check for ESC only approach is problematic
> >>because it conflicts with the enter firmware-setup key on almost all Bay Trail,
> >>Cherry Trail and Apollo Lake devices. You need to try hard to find a device
> >>in one of those 3 categories which does not use ESC for this. Note I'm
> >>aware some devices exist, but using ESC for this is really really common
> >>among these devices.
> >>
> >>I'm open to other suggestions, but I think we really need to add another
> >>key to avoid the pressing ESC at boot already has another meaning problem
> >>and F8 seems like an ok choice.
> >>
> >>Either way thank you for the input on this.
> >
> >I don't disagree about having another key, I just disagree that F8 is
> >a good choice.
> >
> >Looking at https://kb.wisc.edu/page.php?id=58779#here it appears no one
> >uses F4 for anything.  That would seem like a better choice than F8
> >at least.  F6 also seems to be free but F4 seems nicer on desktop
> >keyboards.
>
> Looking at that table I have to agree that F4 seems like the best second
> key to use. Actually looking at that table ESC seems like a poor choice,
> but I guess it makes sense for serial-terminals as well as from a general
> UI pov (and we're stuck with it now anyways).
>
> I'm going to wait for Daniel's review of the other patches before I do
> a v2 of this patch-set. I will switch to F4 for v2.

Please leave ESC as is and add F4 key. Though I would provide an option
to change the defaults at runtime.

Daniel

Re: [PATCH 3/4] Optionally print less messages at boot

[Daniel Kiper]

On Wed, Mar 28, 2018 at 04:50:27PM +0200, Hans de Goede wrote:
> The patch optionally makes grub not show any text (be fully quiet) when
> timeout_style=hidden is set and the user does not interrupt the boot.
>
> Combined with a later patch in this series which makes grub not touch
> the EFI console unless it actually has some text to print, this will keep
> the vendor logo which EFI put on the display in place until the kernel
> touches the display. Leading to a more smooth / seamless boot experience.
>
> At least Fedora/RHEL/CentOS and Ubuntu have been carrying patches for this
> for a long time now (since 2013). There have been several attempts to
> upstream these patches in the past already, which have been rejected
> because not everyone likes the quiet behavior.

May I ask you to provide links to the relevant conversations?

> This patch makes the quiet behavior optional and defaults to off, so
> unless grub is compiled with the new --enable-quiet-boot configure option
> this patch changes nothing.

I am not sure why this should be build time option. I would see this as
a runtime option, e.g. boot_quiet shell variable or even timeout_style=quiet.
Hmmm... Latter is probably preferred.

Daniel

Re: [PATCH 4/4] EFI: Do not set text-mode until we actually need it

[Daniel Kiper]

On Wed, Mar 28, 2018 at 04:50:28PM +0200, Hans de Goede wrote:
> If we're running with a hidden menu we may never need text mode, so do not
> change the video-mode to text until we actually need it.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  grub-core/term/efi/console.c | 72 +++++++++++++++++++++++-------------
>  1 file changed, 47 insertions(+), 25 deletions(-)
>
> diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
> index 02f64ea74..d9fd7cf48 100644
> --- a/grub-core/term/efi/console.c
> +++ b/grub-core/term/efi/console.c
> @@ -24,6 +24,11 @@
>  #include <grub/efi/api.h>
>  #include <grub/efi/console.h>
>
> +static grub_err_t grub_prepare_for_text_output(struct grub_term_output *term);

Please drop this forward declaration and put the function definition before the callers.

> +static int text_mode_available = -1;

Could you use bool type here? If yes please define grub_bool_t as stdbool.h
does (grub/bool.h?) and replace its usage in grub-core/term/tparm.c as separate
patch. If not bool please use enum here.

> +static int text_colorstate = -1;

Ditto.

Daniel

Re: [PATCH 3/4] Optionally print less messages at boot

[Hans de Goede]

Hi,

On 05-04-18 14:12, Daniel Kiper wrote:
> On Wed, Mar 28, 2018 at 04:50:27PM +0200, Hans de Goede wrote:
>> The patch optionally makes grub not show any text (be fully quiet) when
>> timeout_style=hidden is set and the user does not interrupt the boot.
>>
>> Combined with a later patch in this series which makes grub not touch
>> the EFI console unless it actually has some text to print, this will keep
>> the vendor logo which EFI put on the display in place until the kernel
>> touches the display. Leading to a more smooth / seamless boot experience.
>>
>> At least Fedora/RHEL/CentOS and Ubuntu have been carrying patches for this
>> for a long time now (since 2013). There have been several attempts to
>> upstream these patches in the past already, which have been rejected
>> because not everyone likes the quiet behavior.
> 
> May I ask you to provide links to the relevant conversations?

Sure you may ask, but I cannot really help with that because I was not
involved in grub development back then.

I've done a quick search of the mailinglist archives, but
I'm afraid I could not find anything that way.

>> This patch makes the quiet behavior optional and defaults to off, so
>> unless grub is compiled with the new --enable-quiet-boot configure option
>> this patch changes nothing.
> 
> I am not sure why this should be build time option. I would see this as
> a runtime option, e.g. boot_quiet shell variable or even timeout_style=quiet.
> Hmmm... Latter is probably preferred.

The problem is that grub already prints various things before reading
its environment or config file.

What I've understood from the history of this patches, removing those
earlier prints (see e.g. the grub-core/boot/i386/pc/boot.S change in
this patch) was objected against because that provides info that at
least the first stage (for a classic PC BIOS boot setup) has successfully
loaded.

The menu code itself is already quiet when using timeout_style=hidden,
the problem is the messages printed before (and after) the menu code.

If you're happy with killing the messages shown before the config file
is loaded (as most distros already do), then the rest could be made runtime
configurable.

Regards,

Hans

Re: [PATCH 4/4] EFI: Do not set text-mode until we actually need it

[Hans de Goede]

Hi,

On 05-04-18 14:34, Daniel Kiper wrote:
> On Wed, Mar 28, 2018 at 04:50:28PM +0200, Hans de Goede wrote:
>> If we're running with a hidden menu we may never need text mode, so do not
>> change the video-mode to text until we actually need it.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   grub-core/term/efi/console.c | 72 +++++++++++++++++++++++-------------
>>   1 file changed, 47 insertions(+), 25 deletions(-)
>>
>> diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
>> index 02f64ea74..d9fd7cf48 100644
>> --- a/grub-core/term/efi/console.c
>> +++ b/grub-core/term/efi/console.c
>> @@ -24,6 +24,11 @@
>>   #include <grub/efi/api.h>
>>   #include <grub/efi/console.h>
>>
>> +static grub_err_t grub_prepare_for_text_output(struct grub_term_output *term);
> 
> Please drop this forward declaration and put the function definition before the callers.

I did not do that initially because that requires also moving
grub_console_setcolorstate() and grub_console_setcursor() to
higher in the file (or do a forward declaration for those).

I'll add a preparation patch to v2 of the series moving just
those 2 up to higher in the file.

>> +static int text_mode_available = -1;
> 
> Could you use bool type here? If yes please define grub_bool_t as stdbool.h
> does (grub/bool.h?) and replace its usage in grub-core/term/tparm.c as separate
> patch. If not bool please use enum here.

There are 3 states, so a bool is not going to work I will
switch to an enum for v2.

>> +static int text_colorstate = -1;

There already is a grub_term_color_state enum for this, which
defines values 0-2, I want to know if setcolorstate has been
called and text_colorstate contains a valid value, so I made
this an int and use -1 to mean not set / invalid.

I can see a number of solutions here:

1) Leave as is
2) Use:

static grub_term_color_state text_colorstate = -1;

Assuming the compiler will not warn about putting -1 in there.

3) Extend the grub_term_color_state like this:

typedef enum
   {
     /* Used for uninitialized grub_term_color_state variables */
     GRUB_TERM_COLOR_UNDEFINED = -1,
     /* The color used to display all text that does not use the
        user defined colors below.  */
     GRUB_TERM_COLOR_STANDARD = 0,
     /* The user defined colors for normal text.  */
     GRUB_TERM_COLOR_NORMAL,
     /* The user defined colors for highlighted text.  */
     GRUB_TERM_COLOR_HIGHLIGHT
   }
grub_term_color_state;

Which solution would you prefer?

Regards,

Hans

Re: [PATCH 3/4] Optionally print less messages at boot

[Daniel Kiper]

On Thu, Apr 05, 2018 at 07:53:23PM +0200, Hans de Goede wrote:
> Hi,
>
> On 05-04-18 14:12, Daniel Kiper wrote:
> >On Wed, Mar 28, 2018 at 04:50:27PM +0200, Hans de Goede wrote:
> >>The patch optionally makes grub not show any text (be fully quiet) when
> >>timeout_style=hidden is set and the user does not interrupt the boot.
> >>
> >>Combined with a later patch in this series which makes grub not touch
> >>the EFI console unless it actually has some text to print, this will keep
> >>the vendor logo which EFI put on the display in place until the kernel
> >>touches the display. Leading to a more smooth / seamless boot experience.
> >>
> >>At least Fedora/RHEL/CentOS and Ubuntu have been carrying patches for this
> >>for a long time now (since 2013). There have been several attempts to
> >>upstream these patches in the past already, which have been rejected
> >>because not everyone likes the quiet behavior.
> >
> >May I ask you to provide links to the relevant conversations?
>
> Sure you may ask, but I cannot really help with that because I was not
> involved in grub development back then.
>
> I've done a quick search of the mailinglist archives, but
> I'm afraid I could not find anything that way.

OK, let's leave it.

> >>This patch makes the quiet behavior optional and defaults to off, so
> >>unless grub is compiled with the new --enable-quiet-boot configure option
> >>this patch changes nothing.
> >
> >I am not sure why this should be build time option. I would see this as
> >a runtime option, e.g. boot_quiet shell variable or even timeout_style=quiet.
> >Hmmm... Latter is probably preferred.
>
> The problem is that grub already prints various things before reading
> its environment or config file.
>
> What I've understood from the history of this patches, removing those
> earlier prints (see e.g. the grub-core/boot/i386/pc/boot.S change in
> this patch) was objected against because that provides info that at
> least the first stage (for a classic PC BIOS boot setup) has successfully
> loaded.
>
> The menu code itself is already quiet when using timeout_style=hidden,
> the problem is the messages printed before (and after) the menu code.
>
> If you're happy with killing the messages shown before the config file
> is loaded (as most distros already do), then the rest could be made runtime
> configurable.

If they are printed deliberately I am not happy with killing them blindly.
However, I think that it can be done in other way. Could we add an option
to grub-mkimage (and other tools if it is needed) which disables printing
of these messages?

Daniel

Re: [PATCH 4/4] EFI: Do not set text-mode until we actually need it

[Daniel Kiper]

On Thu, Apr 05, 2018 at 08:05:47PM +0200, Hans de Goede wrote:
> Hi,
>
> On 05-04-18 14:34, Daniel Kiper wrote:
> >On Wed, Mar 28, 2018 at 04:50:28PM +0200, Hans de Goede wrote:
> >>If we're running with a hidden menu we may never need text mode, so do not
> >>change the video-mode to text until we actually need it.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>---
> >>  grub-core/term/efi/console.c | 72 +++++++++++++++++++++++-------------
> >>  1 file changed, 47 insertions(+), 25 deletions(-)
> >>
> >>diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
> >>index 02f64ea74..d9fd7cf48 100644
> >>--- a/grub-core/term/efi/console.c
> >>+++ b/grub-core/term/efi/console.c
> >>@@ -24,6 +24,11 @@
> >>  #include <grub/efi/api.h>
> >>  #include <grub/efi/console.h>
> >>
> >>+static grub_err_t grub_prepare_for_text_output(struct grub_term_output *term);
> >
> >Please drop this forward declaration and put the function definition before the callers.
>
> I did not do that initially because that requires also moving
> grub_console_setcolorstate() and grub_console_setcursor() to
> higher in the file (or do a forward declaration for those).
>
> I'll add a preparation patch to v2 of the series moving just
> those 2 up to higher in the file.

Great!

> >>+static int text_mode_available = -1;
> >
> >Could you use bool type here? If yes please define grub_bool_t as stdbool.h
> >does (grub/bool.h?) and replace its usage in grub-core/term/tparm.c as separate
> >patch. If not bool please use enum here.
>
> There are 3 states, so a bool is not going to work I will
> switch to an enum for v2.

Perfect!

> >>+static int text_colorstate = -1;
>
> There already is a grub_term_color_state enum for this, which
> defines values 0-2, I want to know if setcolorstate has been
> called and text_colorstate contains a valid value, so I made
> this an int and use -1 to mean not set / invalid.
>
> I can see a number of solutions here:
>
> 1) Leave as is
> 2) Use:
>
> static grub_term_color_state text_colorstate = -1;
>
> Assuming the compiler will not warn about putting -1 in there.
>
> 3) Extend the grub_term_color_state like this:
>
> typedef enum
>   {
>     /* Used for uninitialized grub_term_color_state variables */
>     GRUB_TERM_COLOR_UNDEFINED = -1,
>     /* The color used to display all text that does not use the
>        user defined colors below.  */
>     GRUB_TERM_COLOR_STANDARD = 0,
>     /* The user defined colors for normal text.  */
>     GRUB_TERM_COLOR_NORMAL,
>     /* The user defined colors for highlighted text.  */
>     GRUB_TERM_COLOR_HIGHLIGHT
>   }
> grub_term_color_state;
>
> Which solution would you prefer?

3rd, enum please.

Thanks,

Daniel