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