[PATCH] tell the user why they are in rescue mode

[PATCH] tell the user why they are in rescue mode

[Sam Morris]

[-- Attachment #1.1: Type: text/plain, Size: 315 bytes --]

Here's a patch that explains to the user why they are being sent to
rescue mode in cast normal mode fails.

I've also corrected a grammatical error in the 'entering rescue mode'
message.

-- 
Sam Morris
http://robots.org.uk/

PGP key id 1024D/5EA01078
3412 EA18 1277 354B 991B  C869 B219 7FDB 5EA0 1078

[-- Attachment #1.2: verbose_error.patch --]
[-- Type: text/x-patch, Size: 1284 bytes --]

diff -ru grub2-1.96+20080203+orig/kern/main.c grub2-1.96+20080203+sam/kern/main.c
--- grub2-1.96+20080203+orig/kern/main.c	2008-01-05 12:04:35.000000000 +0000
+++ grub2-1.96+20080203+sam/kern/main.c	2008-02-07 08:41:01.000000000 +0000
@@ -102,8 +102,13 @@
   /* Load the module.  */
   grub_dl_load ("normal");
   
-  /* Ignore any error, because we have the rescue mode anyway.  */
-  grub_errno = GRUB_ERR_NONE;
+  if (grub_errno != GRUB_ERR_NONE)
+    {
+      grub_printf ("Unable to enter 'normal' mode (error %d: %s)\n", grub_errno, grub_errmsg);
+
+      /* We're about to continue into rescue mode, so clear the error. */
+      grub_errno = GRUB_ERR_NONE;
+    }
 }
 
 /* The main routine.  */
diff -ru grub2-1.96+20080203+orig/kern/rescue.c grub2-1.96+20080203+sam/kern/rescue.c
--- grub2-1.96+20080203+orig/kern/rescue.c	2008-01-30 14:42:09.000000000 +0000
+++ grub2-1.96+20080203+sam/kern/rescue.c	2008-02-07 08:38:46.000000000 +0000
@@ -618,7 +618,7 @@
   /* First of all, attempt to execute the normal mode.  */
   attempt_normal_mode ();
 
-  grub_printf ("Entering into rescue mode...\n");
+  grub_printf ("Entering rescue mode...\n");
   
   grub_rescue_register_command ("boot", grub_rescue_cmd_boot,
 				"boot an operating system");

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] tell the user why they are in rescue mode

[Robert Millan]

Hi Sam,

On Thu, Feb 07, 2008 at 06:34:26PM +0000, Sam Morris wrote:
> diff -ru grub2-1.96+20080203+orig/kern/main.c grub2-1.96+20080203+sam/kern/main.c
> --- grub2-1.96+20080203+orig/kern/main.c	2008-01-05 12:04:35.000000000 +0000
> +++ grub2-1.96+20080203+sam/kern/main.c	2008-02-07 08:41:01.000000000 +0000
> @@ -102,8 +102,13 @@
>    /* Load the module.  */
>    grub_dl_load ("normal");
>    
> -  /* Ignore any error, because we have the rescue mode anyway.  */
> -  grub_errno = GRUB_ERR_NONE;
> +  if (grub_errno != GRUB_ERR_NONE)
> +    {
> +      grub_printf ("Unable to enter 'normal' mode (error %d: %s)\n", grub_errno, grub_errmsg);
> +
> +      /* We're about to continue into rescue mode, so clear the error. */
> +      grub_errno = GRUB_ERR_NONE;
> +    }

I just checked, and it seems we already have a function for this:
grub_print_error().  If you just invoke this function, do you get the
desired result?

Also, I wonder why the existing grub_print_error() call in the rescue loop
doesn't handle this already (in kern/rescue.c).

Perhaps all you need to do is remove these two lines?

diff -ur grub2/kern/main.c tmp/kern/main.c
--- grub2/kern/main.c   2008-01-05 13:04:35.000000000 +0100
+++ tmp/kern/main.c     2008-02-08 02:09:03.000000000 +0100
@@ -101,9 +101,6 @@
 {
   /* Load the module.  */
   grub_dl_load ("normal");
-
-  /* Ignore any error, because we have the rescue mode anyway.  */
-  grub_errno = GRUB_ERR_NONE;
 }

 /* The main routine.  */

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] tell the user why they are in rescue mode

[Sam Morris]

On Fri, 08 Feb 2008 02:10:29 +0100, Robert Millan wrote:

> Hi Sam,
> 
> On Thu, Feb 07, 2008 at 06:34:26PM +0000, Sam Morris wrote:
>> diff -ru grub2-1.96+20080203+orig/kern/main.c
>> grub2-1.96+20080203+sam/kern/main.c ---
>> grub2-1.96+20080203+orig/kern/main.c	2008-01-05 12:04:35.000000000
>> +0000 +++ grub2-1.96+20080203+sam/kern/main.c	2008-02-07
>> 08:41:01.000000000 +0000 @@ -102,8 +102,13 @@
>>    /* Load the module.  */
>>    grub_dl_load ("normal");
>>    
>> -  /* Ignore any error, because we have the rescue mode anyway.  */ - 
>> grub_errno = GRUB_ERR_NONE;
>> +  if (grub_errno != GRUB_ERR_NONE)
>> +    {
>> +      grub_printf ("Unable to enter 'normal' mode (error %d: %s)\n",
>> grub_errno, grub_errmsg); +
>> +      /* We're about to continue into rescue mode, so clear the error.
>> */ +      grub_errno = GRUB_ERR_NONE;
>> +    }
> 
> I just checked, and it seems we already have a function for this:
> grub_print_error().  If you just invoke this function, do you get the
> desired result?

Yes, it works fine.

> Also, I wonder why the existing grub_print_error() call in the rescue
> loop doesn't handle this already (in kern/rescue.c).
> 
> Perhaps all you need to do is remove these two lines?
> 
> diff -ur grub2/kern/main.c tmp/kern/main.c --- grub2/kern/main.c  
> 2008-01-05 13:04:35.000000000 +0100 +++ tmp/kern/main.c     2008-02-08
> 02:09:03.000000000 +0100 @@ -101,9 +101,6 @@
>  {
>    /* Load the module.  */
>    grub_dl_load ("normal");
> -
> -  /* Ignore any error, because we have the rescue mode anyway.  */ - 
> grub_errno = GRUB_ERR_NONE;
>  }
> 
>  /* The main routine.  */

While that works, the output looks like this:

 Entering rescue mode...
 error: out of disk
 > _

Which does not present the information in the clearest possible way: it 
looks like the error happened in the rescue mode code, instead of the 
normal mode code.

Also, the error message could be lost if any of the code between the 
start of grub_enter_rescue_mode and its call to grub_print_error itself 
triggers an error.

For these reasons I think it's better to call grub_print_error from 
within grub_load_normal.

BTW, does anyone know the first thing grub_enter_rescue_mode does is call 
attempt_normal_mode?

-- 
Sam Morris
http://robots.org.uk/
 
PGP key id 1024D/5EA01078
3412 EA18 1277 354B 991B  C869 B219 7FDB 5EA0 1078

Re: [PATCH] tell the user why they are in rescue mode

[Robert Millan]

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

On Fri, Feb 08, 2008 at 06:44:26PM +0000, Sam Morris wrote:
> 
> While that works, the output looks like this:
> 
>  Entering rescue mode...
>  error: out of disk
>  > _
> 
> Which does not present the information in the clearest possible way: it 
> looks like the error happened in the rescue mode code, instead of the 
> normal mode code.
> 
> Also, the error message could be lost if any of the code between the 
> start of grub_enter_rescue_mode and its call to grub_print_error itself 
> triggers an error.
> 
> For these reasons I think it's better to call grub_print_error from 
> within grub_load_normal.

Agreed.  See attached patch.  Does this work as expected ?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

[-- Attachment #2: rescue_mode.diff --]
[-- Type: text/x-diff, Size: 824 bytes --]

2008-02-08  Robert Millan  <rmh@aybabtu.com>

	* kern/main.c (grub_load_normal_mode): Do not reset `grub_errno'.  Call
	grub_print_error() instead.  This will let user know why we're entering
	rescue mode.
	Based on suggestions from Sam Morris.

diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp -N ../grub2/kern/main.c ./kern/main.c
--- ../grub2/kern/main.c	2008-01-05 13:04:35.000000000 +0100
+++ ./kern/main.c	2008-02-08 20:11:52.000000000 +0100
@@ -102,8 +102,8 @@ grub_load_normal_mode (void)
   /* Load the module.  */
   grub_dl_load ("normal");
   
-  /* Ignore any error, because we have the rescue mode anyway.  */
-  grub_errno = GRUB_ERR_NONE;
+  /* Something went wrong.  Print errors here to let user know why we're entering rescue mode.  */
+  grub_print_error ();
 }
 
 /* The main routine.  */

Re: [PATCH] tell the user why they are in rescue mode

[Sam Morris]

On Fri, 08 Feb 2008 20:15:39 +0100, Robert Millan wrote:

> On Fri, Feb 08, 2008 at 06:44:26PM +0000, Sam Morris wrote:
>> 
>> While that works, the output looks like this:
>> 
>>  Entering rescue mode...
>>  error: out of disk
>>  > _
>> 
>> Which does not present the information in the clearest possible way: it
>> looks like the error happened in the rescue mode code, instead of the
>> normal mode code.
>> 
>> Also, the error message could be lost if any of the code between the
>> start of grub_enter_rescue_mode and its call to grub_print_error itself
>> triggers an error.
>> 
>> For these reasons I think it's better to call grub_print_error from
>> within grub_load_normal.
> 
> Agreed.  See attached patch.  Does this work as expected ?

Yes, it works fine.

-- 
Sam Morris
http://robots.org.uk/
 
PGP key id 1024D/5EA01078
3412 EA18 1277 354B 991B  C869 B219 7FDB 5EA0 1078

Re: [PATCH] tell the user why they are in rescue mode

[Robert Millan]

On Sat, Feb 09, 2008 at 03:42:24PM +0000, Sam Morris wrote:
> > 
> > Agreed.  See attached patch.  Does this work as expected ?
> 
> Yes, it works fine.

Ok, committed.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)