[PATCH] Warning if grub.cfg not found

[PATCH] Warning if grub.cfg not found

[Carles Pina i Estany]

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


Hi,

I send attached a patch that shows a warning when Grub it's falling back
to the command line if grub.cfg is not found (so user knows why the menu
is not showed).

I'm not 100% that this is the best way to handle it, mainly because I'm
not used about the Grub execution workflow. The changes are:

a) in normal/main.c, grub_normal_execute I've changed grub_cmdline_run
(nested) by grub_cmdlin_run (-1) (I don't see any bad consequence of it)

b) in normal/cmdline.c in grub_cmdline_run if I receive a nested == -1 I
show the warning

Any better way to do it? Or I will write the ChangeLog.

Thanks,

PS: warning_with_function_names.patch includes the function names that
I've changed.

-- 
Carles Pina i Estany		GPG id: 0x17756391
	http://pinux.info

[-- Attachment #2: warning.patch --]
[-- Type: text/x-diff, Size: 1258 bytes --]

Index: normal/cmdline.c
===================================================================
--- normal/cmdline.c	(revision 1826)
+++ normal/cmdline.c	(working copy)
@@ -137,12 +137,17 @@
 {
   grub_normal_init_page ();
   grub_setcursor (1);
+
+  if ( nested == -1 )
+     grub_printf ("\n\
+ WARNING: GNU GRUB couldn't open /boot/grub/grub.cfg\n\
+ Falling back to GNU GRUB Command Line\n\n");
   
   grub_printf ("\
  [ Minimal BASH-like line editing is supported. For the first word, TAB\n\
    lists possible command completions. Anywhere else TAB lists possible\n\
    device/file completions.%s ]\n\n",
-	       nested ? " ESC at any time exits." : "");
+	       nested !=- 1 ? " ESC at any time exits." : "");
   
   while (1)
     {
@@ -153,7 +158,7 @@
       cmdline[0] = '\0';
       
       if (! grub_cmdline_get ("grub> ", cmdline, sizeof (cmdline), 0, 1)
-	  && nested)
+	  && nested > 0)
 	return;
 
       if (! *cmdline)
Index: normal/main.c
===================================================================
--- normal/main.c	(revision 1826)
+++ normal/main.c	(working copy)
@@ -481,7 +481,7 @@
 	free_menu (menu);
     }
   else
-    grub_cmdline_run (nested);
+    grub_cmdline_run (-1);
 }
 
 /* Enter normal mode from rescue mode.  */

[-- Attachment #3: warning_with_function_names.patch --]
[-- Type: text/x-diff, Size: 1359 bytes --]

Index: normal/cmdline.c
===================================================================
--- normal/cmdline.c	(revision 1826)
+++ normal/cmdline.c	(working copy)
@@ -137,12 +137,17 @@ grub_cmdline_run (int nested)
 {
   grub_normal_init_page ();
   grub_setcursor (1);
+
+  if ( nested == -1 )
+     grub_printf ("\n\
+ WARNING: GNU GRUB couldn't open /boot/grub/grub.cfg\n\
+ Falling back to GNU GRUB Command Line\n\n");
   
   grub_printf ("\
  [ Minimal BASH-like line editing is supported. For the first word, TAB\n\
    lists possible command completions. Anywhere else TAB lists possible\n\
    device/file completions.%s ]\n\n",
-	       nested ? " ESC at any time exits." : "");
+	       nested !=- 1 ? " ESC at any time exits." : "");
   
   while (1)
     {
@@ -153,7 +158,7 @@ grub_cmdline_run (int nested)
       cmdline[0] = '\0';
       
       if (! grub_cmdline_get ("grub> ", cmdline, sizeof (cmdline), 0, 1)
-	  && nested)
+	  && nested > 0)
 	return;
 
       if (! *cmdline)
Index: normal/main.c
===================================================================
--- normal/main.c	(revision 1826)
+++ normal/main.c	(working copy)
@@ -481,7 +481,7 @@ grub_normal_execute (const char *config,
 	free_menu (menu);
     }
   else
-    grub_cmdline_run (nested);
+    grub_cmdline_run (-1);
 }
 
 /* Enter normal mode from rescue mode.  */

Re: [PATCH] Warning if grub.cfg not found

[Carles Pina i Estany]

Hi,

On Aug/23/2008, Carles Pina i Estany wrote:

> b) in normal/cmdline.c in grub_cmdline_run if I receive a nested == -1 I
> show the warning

maybe somebody prefers to macrofy this -1 to CFG_GRUB_NOT_FOUND ? Or -1
is clear enough?

Thanks,

-- 
Carles Pina i Estany		GPG id: 0x17756391
	http://pinux.info

Re: [PATCH] Warning if grub.cfg not found

[Robert Millan]

Hi

On Sat, Aug 23, 2008 at 04:43:14PM +0200, Carles Pina i Estany wrote:
> Index: normal/cmdline.c
> ===================================================================
> --- normal/cmdline.c	(revision 1826)
> +++ normal/cmdline.c	(working copy)
> @@ -137,12 +137,17 @@ grub_cmdline_run (int nested)
>  {
>    grub_normal_init_page ();
>    grub_setcursor (1);
> +
> +  if ( nested == -1 )

nested was intended to be a "boolean";  this changes its meaning, so the name
becomes confusing.  I think there's no need to reuse the variable in this part
of GRUB, and it'd be fine to add a new one IMO.  However ...

> +     grub_printf ("\n\
> + WARNING: GNU GRUB couldn't open /boot/grub/grub.cfg\n\
> + Falling back to GNU GRUB Command Line\n\n");

... this looks like something that belongs whereever the decision to fall
back is taken.  Then once the problem is handled there, you don't need to
tell the lower layer whether to print a message or not.

Also, I think there are two separate cases:

  - grub.cfg is there but can't be opened (we need to tell the user about
    _why_ via grub_print_error()).

  - grub.cfg is simply not there (perhaps the user intended that).

and the messages should be somewhat different for each one.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Warning if grub.cfg not found

[Carles Pina i Estany]

Hi,

On Aug/30/2008, Robert Millan wrote:
> 
> Hi
> 
> On Sat, Aug 23, 2008 at 04:43:14PM +0200, Carles Pina i Estany wrote:
> > Index: normal/cmdline.c
> > ===================================================================
> > --- normal/cmdline.c	(revision 1826)
> > +++ normal/cmdline.c	(working copy)
> > @@ -137,12 +137,17 @@ grub_cmdline_run (int nested)
> >  {
> >    grub_normal_init_page ();
> >    grub_setcursor (1);
> > +
> > +  if ( nested == -1 )
> 
> nested was intended to be a "boolean";  this changes its meaning, so the name
> becomes confusing.  I think there's no need to reuse the variable in this part
> of GRUB, and it'd be fine to add a new one IMO.  However ...

Ok, I will change in this way that you suggest. Thanks

> > +     grub_printf ("\n\
> > + WARNING: GNU GRUB couldn't open /boot/grub/grub.cfg\n\
> > + Falling back to GNU GRUB Command Line\n\n");
> 
> ... this looks like something that belongs whereever the decision to fall
> back is taken.  Then once the problem is handled there, you don't need to
> tell the lower layer whether to print a message or not.

I think that you mean that this message should be showed before it goes
to command line layer? But i think that the command line layer cleans
the screen, so anyway have to know something (or change and avoid
cleaning the screen).
Anyway, I will check it more deep next days.

> 
> Also, I think there are two separate cases:
> 
>   - grub.cfg is there but can't be opened (we need to tell the user about
>     _why_ via grub_print_error()).
>   - grub.cfg is simply not there (perhaps the user intended that).
> 
> and the messages should be somewhat different for each one.

Ok, I understand this part

I sent this patch on 23th August, you replied on 30th August and me
again on 5th September. I think that until mid of next week I will a bit
too busy to do it, but I haven't forgot :-)

Thanks for the suggestions,

-- 
Carles Pina i Estany		GPG id: 0x17756391
	http://pinux.info

Re: [PATCH] Warning if grub.cfg not found

[Carles Pina i Estany]

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



Hello,

I "recover" an old thread...

Long time ago I was thinking in a better warning when Grub2 couldn't
open grub.cfg file.

I send attached a new patch for it (previous one was a bit incorrect,
this one is shorter cleaner, showing the warning in a better place).

Question: What should Grub do when Grub cannot open find fs.lst and
command.lst?  I'm reading the code and in both cases it's doing nothing
(no warning, etc.).

In my understanding, if grub.cfg is missing probably it's a user error
and we should warn the user. If user doesn't want a grub.cfg (??) user
can have an empty grub.cfg.

I also send the ChangeLog, just in case it's correct and we don't want
anything else for fs.lst and command.lst:

-------
2008-09-27  Carles Pina i Estany  <carles@pina.cat>
	* normal/main.c (read_config_file): Warn the user when Grub
	cannot open grub.cfg file.
-------
	  
Thanks,

PS: if you want to test the patch, don't waste your time and remember
that grub-mkrescue is generating a grub.cfg file with the modules! (I
lost quite much time with it :-) )

-- 
Carles Pina i Estany		GPG id: 0x17756391
	http://pinux.info

[-- Attachment #2: warning_grub.cfg.patch --]
[-- Type: text/x-diff, Size: 534 bytes --]

Index: normal/main.c
===================================================================
--- normal/main.c	(revision 1877)
+++ normal/main.c	(working copy)
@@ -230,7 +230,13 @@
   /* Try to open the config file.  */
   file = grub_file_open (config);
   if (! file)
-    return 0;
+    {
+      grub_printf("Cannot open %s file\n",config);
+      grub_print_error ();
+      grub_printf("Falling back to the Grub console\n");
+      grub_wait_after_message ();
+      return 0;
+    }
 
   grub_env_set_data_slot ("menu", newmenu);
 

Re: [PATCH] Warning if grub.cfg not found

[Robert Millan]

On Sat, Sep 27, 2008 at 07:37:52PM +0200, Carles Pina i Estany wrote:
> --- normal/main.c	(revision 1877)
> +++ normal/main.c	(working copy)
> @@ -230,7 +230,13 @@
>    /* Try to open the config file.  */
>    file = grub_file_open (config);
>    if (! file)
> -    return 0;
> +    {

This seems to include the possibility that file cannot be read simply because
it's not there.  Perhaps user intended so, and this shouldn't be reported as
an error.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Warning if grub.cfg not found

[Carles Pina i Estany]

Hello,

On Sep/28/2008, Robert Millan wrote:
> On Sat, Sep 27, 2008 at 07:37:52PM +0200, Carles Pina i Estany wrote:
> > --- normal/main.c	(revision 1877)
> > +++ normal/main.c	(working copy)
> > @@ -230,7 +230,13 @@
> >    /* Try to open the config file.  */
> >    file = grub_file_open (config);
> >    if (! file)
> > -    return 0;
> > +    {
> 
> This seems to include the possibility that file cannot be read simply because
> it's not there.  Perhaps user intended so, and this shouldn't be reported as
> an error.

Do you mean that we could change:
+      grub_print_error ();

by "grub_print_warning"?

Or the whole point (warn the user if file is not there) is incorrect?

If user doesn't want a grub.cfg, he/she could "touch grub.cfg" and
that's all, no?

If some user is miss-configuring Grub2 (pointing to an invalid device,
partition, etc.) he will prefer to read "cannot find grub.cfg" than just
see a shell (it's quite confusing, or it was for me when it happened :-)
)

-- 
Carles Pina i Estany		GPG id: 0x17756391
	http://pinux.info

Re: [PATCH] Warning if grub.cfg not found

[Robert Millan]

On Sun, Sep 28, 2008 at 11:41:02PM +0200, Carles Pina i Estany wrote:
> 
> Hello,
> 
> On Sep/28/2008, Robert Millan wrote:
> > On Sat, Sep 27, 2008 at 07:37:52PM +0200, Carles Pina i Estany wrote:
> > > --- normal/main.c	(revision 1877)
> > > +++ normal/main.c	(working copy)
> > > @@ -230,7 +230,13 @@
> > >    /* Try to open the config file.  */
> > >    file = grub_file_open (config);
> > >    if (! file)
> > > -    return 0;
> > > +    {
> > 
> > This seems to include the possibility that file cannot be read simply because
> > it's not there.  Perhaps user intended so, and this shouldn't be reported as
> > an error.
> 
> Do you mean that we could change:
> +      grub_print_error ();
> 
> by "grub_print_warning"?
> 
> Or the whole point (warn the user if file is not there) is incorrect?
> 
> If user doesn't want a grub.cfg, he/she could "touch grub.cfg" and
> that's all, no?
> 
> If some user is miss-configuring Grub2 (pointing to an invalid device,
> partition, etc.) he will prefer to read "cannot find grub.cfg" than just
> see a shell (it's quite confusing, or it was for me when it happened :-)
> )

I think it's fine to tell the user about it, but without impliing that it
is an error.  E.g. something like "grub.cfg not found, falling back to xxx",
without waiting for user confirmation.

What does everyone else think about this?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."