grub.cfg parser still doesn't abort on error

grub.cfg parser still doesn't abort on error

[Robert Millan]

Try the following grub.cfg:

  cat (memdisk)/non-existant-file
  echo this point should never be reached
  read

You'll see that the echo is reached, even if cat returned an error.

This becomes much worse when found in situations like:

  font (xxx)/some-file.pff
  terminal gfxterm

and the font wasn't loaded.

-- 
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: grub.cfg parser still doesn't abort on error

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> Try the following grub.cfg:
>
>   cat (memdisk)/non-existant-file
>   echo this point should never be reached
>   read
>
> You'll see that the echo is reached, even if cat returned an error.
>
> This becomes much worse when found in situations like:
>
>   font (xxx)/some-file.pff
>   terminal gfxterm
>
> and the font wasn't loaded.

Urgh!  Bean committed a patch that zero'ed the error.  Perhaps it was
wrong? ;-)

Can you try reverting it? :-)

--
Marco

Re: grub.cfg parser still doesn't abort on error

[Robert Millan]

On Sat, Feb 02, 2008 at 09:59:57PM +0100, Marco Gerards wrote:
> Robert Millan <rmh@aybabtu.com> writes:
> 
> > Try the following grub.cfg:
> >
> >   cat (memdisk)/non-existant-file
> >   echo this point should never be reached
> >   read
> >
> > You'll see that the echo is reached, even if cat returned an error.
> >
> > This becomes much worse when found in situations like:
> >
> >   font (xxx)/some-file.pff
> >   terminal gfxterm
> >
> > and the font wasn't loaded.
> 
> Urgh!  Bean committed a patch that zero'ed the error.  Perhaps it was
> wrong? ;-)
> 
> Can you try reverting it? :-)

Actually, it was wrong before that.

-- 
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: grub.cfg parser still doesn't abort on error

[Bean]

On Feb 3, 2008 5:07 AM, Robert Millan <rmh@aybabtu.com> wrote:
> On Sat, Feb 02, 2008 at 09:59:57PM +0100, Marco Gerards wrote:
> > Robert Millan <rmh@aybabtu.com> writes:
> >
> > > Try the following grub.cfg:
> > >
> > >   cat (memdisk)/non-existant-file
> > >   echo this point should never be reached
> > >   read
> > >
> > > You'll see that the echo is reached, even if cat returned an error.
> > >
> > > This becomes much worse when found in situations like:
> > >
> > >   font (xxx)/some-file.pff
> > >   terminal gfxterm
> > >
> > > and the font wasn't loaded.
> >
> > Urgh!  Bean committed a patch that zero'ed the error.  Perhaps it was
> > wrong? ;-)
> >
> > Can you try reverting it? :-)
>
> Actually, it was wrong before that.

i think you can use the if statement to test command result.

-- 
Bean

Re: grub.cfg parser still doesn't abort on error

[Robert Millan]

On Sun, Feb 03, 2008 at 11:05:26AM +0800, Bean wrote:
> > > > Try the following grub.cfg:
> > > >
> > > >   cat (memdisk)/non-existant-file
> > > >   echo this point should never be reached
> > > >   read
> > > >
> > > > You'll see that the echo is reached, even if cat returned an error.
> > > >
> > > > This becomes much worse when found in situations like:
> > > >
> > > >   font (xxx)/some-file.pff
> > > >   terminal gfxterm
> > > >
> > > > and the font wasn't loaded.
> > >
> > > Urgh!  Bean committed a patch that zero'ed the error.  Perhaps it was
> > > wrong? ;-)
> > >
> > > Can you try reverting it? :-)
> >
> > Actually, it was wrong before that.
> 
> i think you can use the if statement to test command result.

You mean it is intentional?  I think it's a bad plan to assume user will check
for error status of each command, specially in situations like font/gfxterm.

-- 
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: grub.cfg parser still doesn't abort on error

[Bean]

On Feb 3, 2008 6:55 PM, Robert Millan <rmh@aybabtu.com> wrote:
> On Sun, Feb 03, 2008 at 11:05:26AM +0800, Bean wrote:
> > > > > Try the following grub.cfg:
> > > > >
> > > > >   cat (memdisk)/non-existant-file
> > > > >   echo this point should never be reached
> > > > >   read
> > > > >
> > > > > You'll see that the echo is reached, even if cat returned an error.
> > > > >
> > > > > This becomes much worse when found in situations like:
> > > > >
> > > > >   font (xxx)/some-file.pff
> > > > >   terminal gfxterm
> > > > >
> > > > > and the font wasn't loaded.
> > > >
> > > > Urgh!  Bean committed a patch that zero'ed the error.  Perhaps it was
> > > > wrong? ;-)
> > > >
> > > > Can you try reverting it? :-)
> > >
> > > Actually, it was wrong before that.
> >
> > i think you can use the if statement to test command result.
>
> You mean it is intentional?  I think it's a bad plan to assume user will check
> for error status of each command, specially in situations like font/gfxterm.

In fact, the most common error is the font command, as sometimes it
can't find the font file from disk. I think you can change the
grub.cfg to something like this:

if font (xxx)/some-file.pff ; then
  # graphic mode setting
else
  # text mode setting
fi

i think this behavior is consistent with sh, an error code returned
from command should not cause the script to abort.

-- 
Bean

Re: grub.cfg parser still doesn't abort on error

[Yoshinori K. Okuji]

On Sunday 03 February 2008 11:55, Robert Millan wrote:
> You mean it is intentional?  I think it's a bad plan to assume user will
> check for error status of each command, specially in situations like
> font/gfxterm.

It is quite intentional. This behavior is consistent with Bash and GRUB 
Legacy. The concept is that the user still gets something, even if there is a 
small error. Since GRUB supports rewriting menu entries at runtime, this is 
generally a good thing.

Okuji

Re: grub.cfg parser still doesn't abort on error

[Robert Millan]

On Sun, Feb 03, 2008 at 09:05:38PM +0800, Bean wrote:
> 
> if font (xxx)/some-file.pff ; then
>   # graphic mode setting
> else
>   # text mode setting
> fi
> 
> i think this behavior is consistent with sh, an error code returned
> from command should not cause the script to abort.

Urg, part of update-grub was written with the idea that this ought to be
handled in "build" time.  I guess I'll have to overhaul some of it...

-- 
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: grub.cfg parser still doesn't abort on error

[Robert Millan]

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

On Sun, Feb 03, 2008 at 02:29:55PM +0100, Robert Millan wrote:
> On Sun, Feb 03, 2008 at 09:05:38PM +0800, Bean wrote:
> > 
> > if font (xxx)/some-file.pff ; then
> >   # graphic mode setting
> > else
> >   # text mode setting
> > fi
> > 
> > i think this behavior is consistent with sh, an error code returned
> > from command should not cause the script to abort.
> 
> Urg, part of update-grub was written with the idea that this ought to be
> handled in "build" time.  I guess I'll have to overhaul some of it...

Well, it wasn't such a big change after all.  The sanity checks done at grub.cfg
generation time are worth keeping, IMHO, since they off-load GRUB from
performing them (and it is more practical to add logic in bash when possible).

This should address the problem.

-- 
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: font_fallback.diff --]
[-- Type: text/x-diff, Size: 1027 bytes --]


	* util/grub.d/00_header.in: Issue scripting commands for GRUB to
	only setup gfxterm when `font' command has succeeded.

diff -x CVS -x '*~' -x '*.mk' -urp ../grub2/util/grub.d/00_header.in ./util/grub.d/00_header.in
--- ../grub2/util/grub.d/00_header.in	2008-01-12 16:11:56.000000000 +0100
+++ ./util/grub.d/00_header.in	2008-02-03 15:50:58.000000000 +0100
@@ -42,15 +42,14 @@ if [ "x${GRUB_DRIVE}" = "x" ] ; then : ;
   echo "set root=${GRUB_DRIVE}"
 fi
 
-if [ "x${GRUB_FONT_PATH}" = "x" ] ; then : ; else
-  echo "font ${GRUB_FONT_PATH}"
-fi
-
 case ${platform}:${GRUB_TERMINAL} in
   pc:gfxterm) cat << EOF
-set gfxmode=640x480
-insmod gfxterm
-insmod vbe
+if font ${GRUB_FONT_PATH} ; then
+  set gfxmode=640x480
+  insmod gfxterm
+  insmod vbe
+  terminal gfxterm
+fi
 EOF
   ;;
   *:serial)
@@ -59,7 +58,9 @@ EOF
       GRUB_SERIAL_COMMAND=serial
     fi
     echo "${GRUB_SERIAL_COMMAND}"
+    echo "terminal serial"
+  ;;
+  *:*)
+    echo "terminal ${GRUB_TERMINAL}"
   ;;
 esac
-
-echo "terminal ${GRUB_TERMINAL}"

Re: grub.cfg parser still doesn't abort on error

[Robert Millan]

On Sun, Feb 03, 2008 at 03:53:42PM +0100, Robert Millan wrote:
> On Sun, Feb 03, 2008 at 02:29:55PM +0100, Robert Millan wrote:
> > On Sun, Feb 03, 2008 at 09:05:38PM +0800, Bean wrote:
> > > 
> > > if font (xxx)/some-file.pff ; then
> > >   # graphic mode setting
> > > else
> > >   # text mode setting
> > > fi
> > > 
> > > i think this behavior is consistent with sh, an error code returned
> > > from command should not cause the script to abort.
> > 
> > Urg, part of update-grub was written with the idea that this ought to be
> > handled in "build" time.  I guess I'll have to overhaul some of it...
> 
> Well, it wasn't such a big change after all.  The sanity checks done at grub.cfg
> generation time are worth keeping, IMHO, since they off-load GRUB from
> performing them (and it is more practical to add logic in bash when possible).
> 
> This should address the problem.

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 /.)