Problems with grub-reboot/savedefault/default=saved

* Problems with grub-reboot/savedefault/default=saved
@ 2009-12-16 22:28 Jordan Uggla
  2009-12-20  8:37 ` Jordan Uggla
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Jordan Uggla @ 2009-12-16 22:28 UTC (permalink / raw)
  To: grub-devel

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

There are multiple problems with grub-reboot/savedefault/default=saved.

1: grub-reboot doesn't restore the default after rebooting, making it 
effectively equivalent to grub-set-default. This is because the 
savedefault functionality currently saves the entry you boot from as the 
new default even when grub-reboot is used. Because of problem #2 there is 
no way ( outside manually editing the grub.cfg ) to use grub-reboot without 
savedefault functionality also enabled ( and thus no configuration where grub-reboot isn't broken ). I've added a patch for this to 
the bug report @ http://savannah.gnu.org/bugs/?28161 and as the first patch 
attached to this email.

2: Setting GRUB_DEFAULT=saved in /etc/default/grub also enables savedefault 
functionality. There are many people who would want to use grub-reboot and 
grub-set-default without savedefault. The second patch adds a separate 
variable, GRUB_SAVEDEFAULT, for enabling savedefault.

3: Savedefault functionality currently adds two lines ( one setting a 
variable, the other saving it ) to every menu entry and with my patch for 
fixing grub-reboot, an if statement as well. This clutters the menu entries 
and makes it hard to write and maintain custom menu entries with 
savedefault. The third patch moves this to a function "savedefault" defined 
in 00_header so entries just have to have "savedefault", like in grub 
legacy. And if the implementation of savedefault needs to change, custom 
menu entries will still work unmodified.

4: Even with the first grub-reboot fix the default is still not restored 
when grubenv is not writable ( /boot on lvm for example ). Since 
grub-reboot can't work without a writable grubenv it's at least safer to 
boot into the "real" default instead of the "temporary" default. The 
fourth patch sets default=$prev_saved_default if save_env fails. 
Grub-reboot ( the utility ) should also complain when grubenv isn't 
writable by grub. What is the best way to determine that grubenv likely is 
or isn't writable by grub?

5: Since grub2 uses menu entry titles for savedefault instead of numbers, 
the default kernel booted won't be changed when the user installs a new 
kernel. Using titles instead of numbers is good, but an unfortunate 
consequence that users may not realize ( and was not true with savedefault 
in grub-legacy ) is that if users set GRUB_SAVEDEFAULT=true they will never 
actually boot any updated kernels unless they select them manually at the 
grub menu. This could lead to not getting security updates for the kernel, 
and with major upgrades could cause serious problems when the old kernel 
doesn't work with newer userland components like Xorg. I can't think of a 
good way to solve this problem.

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

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

=== modified file 'util/grub-mkconfig_lib.in'

--- old/util/grub-mkconfig_lib.in	2009-12-09 21:03:26 +0000
+++ new/util/grub-mkconfig_lib.in	2009-12-16 20:12:30 +0000
@@ -97,8 +97,10 @@
 save_default_entry ()
 {
   if [ "x${GRUB_DEFAULT}" = "xsaved" ] ; then
-    echo 'saved_entry=${chosen}'
-    echo 'save_env saved_entry'
+    echo 'if [ ${boot_once} != true ]; then'
+    echo '	saved_entry=${chosen}'
+    echo '	save_env saved_entry'
+    echo 'fi'
   fi
 }
 

=== modified file 'util/grub.d/00_header.in'
--- old/util/grub.d/00_header.in	2009-11-24 00:22:41 +0000
+++ new/util/grub.d/00_header.in	2009-12-16 20:12:30 +0000
@@ -46,6 +46,7 @@
   save_env saved_entry
   prev_saved_entry=
   save_env prev_saved_entry
+  boot_once=true
 fi
 EOF
 


[-- Attachment #3: fix2.patch --]
[-- Type: text/x-patch, Size: 818 bytes --]

=== modified file 'util/grub-mkconfig.in'
--- old/util/grub-mkconfig.in	2009-12-12 00:43:32 +0000
+++ new/util/grub-mkconfig.in	2009-12-16 20:22:36 +0000
@@ -220,7 +220,8 @@
   GRUB_DISABLE_LINUX_UUID \
   GRUB_DISABLE_LINUX_RECOVERY \
   GRUB_GFXMODE \
-  GRUB_DISABLE_OS_PROBER
+  GRUB_DISABLE_OS_PROBER \
+  GRUB_SAVEDEFAULT
 
 if test "x${grub_cfg}" != "x"; then
   rm -f ${grub_cfg}.new

=== modified file 'util/grub-mkconfig_lib.in'
--- old/util/grub-mkconfig_lib.in	2009-12-16 20:12:30 +0000
+++ new/util/grub-mkconfig_lib.in	2009-12-16 20:22:36 +0000
@@ -96,7 +96,7 @@
 
 save_default_entry ()
 {
-  if [ "x${GRUB_DEFAULT}" = "xsaved" ] ; then
+  if [ "x${GRUB_SAVEDEFAULT}" = "xtrue" ] ; then
     echo 'if [ ${boot_once} != true ]; then'
     echo '	saved_entry=${chosen}'
     echo '	save_env saved_entry'


[-- Attachment #4: fix3.patch --]
[-- Type: text/x-patch, Size: 853 bytes --]

=== modified file 'util/grub-mkconfig_lib.in'
--- old/util/grub-mkconfig_lib.in	2009-12-16 20:22:36 +0000
+++ new/util/grub-mkconfig_lib.in	2009-12-16 20:25:37 +0000
@@ -97,10 +97,7 @@
 save_default_entry ()
 {
   if [ "x${GRUB_SAVEDEFAULT}" = "xtrue" ] ; then
-    echo 'if [ ${boot_once} != true ]; then'
-    echo '	saved_entry=${chosen}'
-    echo '	save_env saved_entry'
-    echo 'fi'
+    echo 'savedefault'
   fi
 }
 

=== modified file 'util/grub.d/00_header.in'
--- old/util/grub.d/00_header.in	2009-12-16 20:12:30 +0000
+++ new/util/grub.d/00_header.in	2009-12-16 20:25:37 +0000
@@ -48,6 +48,14 @@
   save_env prev_saved_entry
   boot_once=true
 fi
+
+function savedefault {
+  if [ \${boot_once} != true ]; then
+    saved_entry=\${chosen}
+    save_env saved_entry
+  fi
+}
+
 EOF
 
 case ${GRUB_TERMINAL_INPUT}:${GRUB_TERMINAL_OUTPUT} in


[-- Attachment #5: fix4.patch --]
[-- Type: text/x-patch, Size: 561 bytes --]

=== modified file 'util/grub.d/00_header.in'
--- old/util/grub.d/00_header.in	2009-12-16 20:25:37 +0000
+++ new/util/grub.d/00_header.in	2009-12-16 20:41:14 +0000
@@ -43,10 +43,13 @@
 set default=${GRUB_DEFAULT}
 if [ \${prev_saved_entry} ]; then
   saved_entry=\${prev_saved_entry}
-  save_env saved_entry
-  prev_saved_entry=
-  save_env prev_saved_entry
-  boot_once=true
+  if save_env saved_entry; then
+    prev_saved_entry=
+    save_env prev_saved_entry
+    boot_once=true
+  else
+    default=\${prev_saved_entry}
+  fi
 fi
 
 function savedefault {


^ permalink raw reply	[flat|nested] 11+ messages in thread