problem in usage of grub_errno...

problem in usage of grub_errno...

[Vesa Jääskeläinen]

Some folks might have overlooked my earlier post as I wrote it to video
subsystem thread. As this problem needs some kind of resolution and it
affects larger area of code than just video subsystem it needs to be
discussed first.

Problem is this:
1. error occures and grub_errno is being set to something else than
GRUB_ERR_NONE (0).
2. now some operation needs to read from disk, but it will fail as
gurb_errno was set.

Real world example:
Let's assume that there is a file not found exception. There is graphics
mode activated and not all fonts are cached in memory (as is currently
the case). Now as file not found exception sets grub_errno to
GRUB_ERR_FILE_NOT_FOUND and most likely sets some string to grub_errmsg.
 All is good so far. But when the actual rendering happens, and font
manager tries to read font data from disk it fails, because grub_errno
is set. In many place there is code like this "if (grub_errno) return
grub_errno;" in file system code and in disk drivers. Now if grub_errno
is set else where this code will fail, even if there wasn't really i/o
error.

There could be other places where before printing out the error message
could come another error message and it would replace older error
message and it not get users attention. This could at best hide the real
problem from user and make it harder for user fixing the issue.

This problem _must_ be solved somehow. Either implementing more advanced
error reporting system and/or file system & disk code must be corrected.
I can try to modify those myself, but I think it would be wise if
someone how knows fs/disk code better would fix those.

Re: problem in usage of grub_errno...

[Marco Gerards]

Vesa Jääskeläinen <chaac@nic.fi> writes:

> Some folks might have overlooked my earlier post as I wrote it to video
> subsystem thread. As this problem needs some kind of resolution and it
> affects larger area of code than just video subsystem it needs to be
> discussed first.
>
> Problem is this:
> 1. error occures and grub_errno is being set to something else than
> GRUB_ERR_NONE (0).
> 2. now some operation needs to read from disk, but it will fail as
> gurb_errno was set.
>
> Real world example:
> Let's assume that there is a file not found exception. There is graphics
> mode activated and not all fonts are cached in memory (as is currently
> the case). Now as file not found exception sets grub_errno to
> GRUB_ERR_FILE_NOT_FOUND and most likely sets some string to grub_errmsg.
>  All is good so far. But when the actual rendering happens, and font
> manager tries to read font data from disk it fails, because grub_errno
> is set. In many place there is code like this "if (grub_errno) return
> grub_errno;" in file system code and in disk drivers. Now if grub_errno
> is set else where this code will fail, even if there wasn't really i/o
> error.

You should in general test all functions if they return an error.  If
you think an error should be ignored, set grub_errno to 0.  So if the
file was not found you should either clear the error and ignore it, or
return the error so the error can be reported to the user.

But I don't like it that the console can get critical errors.  When
can and does this happen?  What are the worst case scenarious?

--
Marco

Re: problem in usage of grub_errno...

[Vesa Jääskeläinen]

Marco Gerards wrote:
> Vesa Jääskeläinen <chaac@nic.fi> writes:
> 
>> Some folks might have overlooked my earlier post as I wrote it to video
>> subsystem thread. As this problem needs some kind of resolution and it
>> affects larger area of code than just video subsystem it needs to be
>> discussed first.
>>
>> Problem is this:
>> 1. error occures and grub_errno is being set to something else than
>> GRUB_ERR_NONE (0).
>> 2. now some operation needs to read from disk, but it will fail as
>> gurb_errno was set.
>>
>> Real world example:
>> Let's assume that there is a file not found exception. There is graphics
>> mode activated and not all fonts are cached in memory (as is currently
>> the case). Now as file not found exception sets grub_errno to
>> GRUB_ERR_FILE_NOT_FOUND and most likely sets some string to grub_errmsg.
>>  All is good so far. But when the actual rendering happens, and font
>> manager tries to read font data from disk it fails, because grub_errno
>> is set. In many place there is code like this "if (grub_errno) return
>> grub_errno;" in file system code and in disk drivers. Now if grub_errno
>> is set else where this code will fail, even if there wasn't really i/o
>> error.
> 
> You should in general test all functions if they return an error.  If
> you think an error should be ignored, set grub_errno to 0.  So if the
> file was not found you should either clear the error and ignore it, or
> return the error so the error can be reported to the user.

I have no problem with checking if there is a error. But let's see what
could be problems of checking error with little different styles..

1) In disk/i386/pc/biosdisk.c (grub_biosdisk_get_drive):
--
  drive = grub_strtoul (name + 2, 0, 10);
  if (grub_errno != GRUB_ERR_NONE)
    goto fail;
--

Now if we analyze this code here. grub_strtoul only sets grub_errno when
there is actual error. Good so far. But now the code in biosdisk.c
assumes that grub_errno is being set every time. This code block will
cause function to report error even when there wasn't one (in case
grub_errno was set beforehand).

Let's continue to place where this code is used, disk/i386/pc/biosdisk.c
(grub_biosdisk_open):
--
grub_biosdisk_open (const char *name, grub_disk_t disk)
{
  unsigned long total_sectors = 0;
  int drive;
  struct grub_biosdisk_data *data;

  drive = grub_biosdisk_get_drive (name);
  if (drive < 0)
    return grub_errno;
--

Let's assume that grub_errno is set before we call this function. Now
grub_biosdisk_get_drive fails with an error and replaces grub_errno with
it's own error message (hides previous error). So function failed even
when there wasn't real error.

There is similar problem in grub_fs_blocklist_open with grub_strtoul.

2) If code is structured like this in fs/fat.c (grub_fat_read_data):
--
      disk->read_hook = read_hook;
      grub_disk_read (disk, sector, offset, size, buf);
      disk->read_hook = 0;
      if (grub_errno)
        return -1;
--
Idea here is most likely to zero out read_hook and after it returns
actual error, but let's see what happens here in reality. Code
constructed like this assumes that grub_errno is being set every time on
grub_disk_read().

Taking a quick look of grub_disk_read function, it seems to behave
nicely. On successful read it returns grub_errno (at bottom of
function). Ok... only if grub_errno is being set to GRUB_ERR_NONE
somewhere here. Now let's go deeper and go to disk->dev->read() ==
grub_biosdisk_read(). Code seems to be ok here... but lets go deeper.
Now we are at grub_biosdsk_rw(). First thing I noticed that this
function doesn't set grub_errno on successful read. Now we return back
to grub_disk_read, it return OLD grub_errno what was set before calling
function to read data from FAT partition.

And this is not only problem of FAT fs driver, if we look at ext2 driver
quickly, in grub_ext2_read_inode there is:
--
  grub_ext2_blockgroup (data, ino / grub_le_to_cpu32
(sblock->inodes_per_group)
                        &blkgrp);
  if (grub_errno)
    return grub_errno;
--
If we go deeper here, we can see that again there is the same problem.

Now to shorten this email, I will stop here ;)

My conclusion:
Changing coding guide of using grub_errno we could probably get this
issue "working". But then there is still problem with lost error
messages if subsequent code fails with real error.

If we just zero out grub_errno in code before printing out error(s).
Then the actual error message will get lost. So it is important not to
zero out spuriously grub_errno.

Good way of checking for errors could be like:
--
grub_errno_t rc;

rc = function_that_could_fail ();
/* do some stuff here.  */
if (rc)
  return rc;
--

> But I don't like it that the console can get critical errors.  When
> can and does this happen?  What are the worst case scenarious?

One worst case scenarios I could see quickly:

1. Now we are in graphics mode, with loaded font.
2. User try to load kernel from file that is not found.
3. Font manager try to load font from disk, but as error was set fs/disk
driver reports errornous error code to font manager and it assumes that
font is corrupted and delists font.
4. All text after this would be drawn as blocks (no glyph found).
5. Result is that user doesn't see that file was not found and is puzzle
what is going on.

Re: problem in usage of grub_errno...

[Yoshinori K. Okuji]

On Saturday 10 December 2005 11:05 am, Vesa Jääskeläinen wrote:
> Changing coding guide of using grub_errno we could probably get this
> issue "working". But then there is still problem with lost error
> messages if subsequent code fails with real error.
>
> If we just zero out grub_errno in code before printing out error(s).
> Then the actual error message will get lost. So it is important not to
> zero out spuriously grub_errno.
>
> Good way of checking for errors could be like:
> --
> grub_errno_t rc;
>
> rc = function_that_could_fail ();
> /* do some stuff here.  */
> if (rc)
>   return rc;

Please do not change it in this way. The error subsystem of GRUB is based on 
the idea of exceptions, that is to say, passing an error to higher levels 
until it is caught explicitly. This is similar to C++, Java, Python or Ruby. 
Unfortunately, C does not support exceptions directly, so we must emulate it. 
So, what we must do is not to ignore an error where it happens.

I use Python as an example. Suppose this code:

def foo():
  bar()

def bar()
  baz()
  print 'bar'

def baz()
  raise RuntimeError, 'baz'

foo()

When executing this, Python does not print 'bar', because the exception raised 
by baz is not caught by bar, and the control is immediately transferred to 
foo.

In C, to emulate this behavior, we must do like this:

void
foo (void)
{
  bar ();
}

void
bar (void)
{
  baz ();
  if (grub_errno != GRUB_ERR_NONE)
    return;
  grub_printf ("bar\n");
}

void
baz (void)
{
  grub_error (GRUB_ERR_SOMETHING, "baz");
}

In other words, if you ignore an error set by calling a function and pass the 
control to another, it violates the semantics. So, strictly speaking, you 
must choose either of these when an error is set:

- deciding to ignore the error explicitly by setting GRUB_ERRNO to 
GRUB_ERR_NONE

- handling the error by returning to the higher level or dealing with the 
error  locally (e.g. printing the error)

We (at least I) sometimes violate this rule by intention when knowing that it 
is safe to ignore an error, simply because it is too heavy to deal with 
errors all the time.

In your situation, things are a bit complicated. In my opinion, all you should 
do is to save error information in a temporary place and reset it afterwards. 
For example, you can define a function like this:

void
grub_error_push (void)
{
  /* Push the current error in a stack and clear GRUB_ERRNO.  */
  ...
}

void
grub_error_pop (void)
{
  /* Pop the previous error and set GRUB_ERRNO.  */
  ...
}

I don't know if this should be stack-based or flat. For example, you can 
allocate space in a function and pass the pointer to a function which saves 
current error information. In this case, functions should be named 
grub_error_save and grub_error_restore.

The difficulty is that it is not very safe to use a heap to allocate space to 
store error information, because such a memory allocation can generate 
another error (due to memory shortage). The possibility is low, but we must 
still consider it.

One way is to allocate such space in advance. For example, if we use the 
stack-based approach (personally I prefer this for consistency), instead of 
allocating space dynamically, we can allocate it statically. That is, at 
linking time or at initialization time. This way limits the maximum number of 
slots arbitrarily, but it should be enough for the reality (e.g. 10).

What do you think?

Okuji

Re: problem in usage of grub_errno...

[Vesa Jääskeläinen]

Yoshinori K. Okuji wrote:
> On Saturday 10 December 2005 11:05 am, Vesa Jääskeläinen wrote:
>> Changing coding guide of using grub_errno we could probably get this
>> issue "working". But then there is still problem with lost error
>> messages if subsequent code fails with real error.
>>
>> If we just zero out grub_errno in code before printing out error(s).
>> Then the actual error message will get lost. So it is important not to
>> zero out spuriously grub_errno.
>>
>> Good way of checking for errors could be like:
>> --
>> grub_errno_t rc;
>>
>> rc = function_that_could_fail ();
>> /* do some stuff here.  */
>> if (rc)
>>   return rc;
> 
> Please do not change it in this way. The error subsystem of GRUB is based on 
> the idea of exceptions, that is to say, passing an error to higher levels 
> until it is caught explicitly. This is similar to C++, Java, Python or Ruby. 
> Unfortunately, C does not support exceptions directly, so we must emulate it. 
> So, what we must do is not to ignore an error where it happens.

[snip]

> In other words, if you ignore an error set by calling a function and pass the 
> control to another, it violates the semantics. So, strictly speaking, you 
> must choose either of these when an error is set:
> 
> - deciding to ignore the error explicitly by setting GRUB_ERRNO to 
> GRUB_ERR_NONE
> 
> - handling the error by returning to the higher level or dealing with the 
> error  locally (e.g. printing the error)
> 
> We (at least I) sometimes violate this rule by intention when knowing that it 
> is safe to ignore an error, simply because it is too heavy to deal with 
> errors all the time.

Perhaps we could put this information somewhere on wiki? :)

It could help a bit new developers and would also give some guidelines
how error handling should be implemented.

> In your situation, things are a bit complicated. In my opinion, all you should 
> do is to save error information in a temporary place and reset it afterwards. 
> For example, you can define a function like this:
> 
> void
> grub_error_push (void)
> {
>   /* Push the current error in a stack and clear GRUB_ERRNO.  */
>   ...
> }
> 
> void
> grub_error_pop (void)
> {
>   /* Pop the previous error and set GRUB_ERRNO.  */
>   ...
> }
> 
> I don't know if this should be stack-based or flat. For example, you can 
> allocate space in a function and pass the pointer to a function which saves 
> current error information. In this case, functions should be named 
> grub_error_save and grub_error_restore.
> 
> The difficulty is that it is not very safe to use a heap to allocate space to 
> store error information, because such a memory allocation can generate 
> another error (due to memory shortage). The possibility is low, but we must 
> still consider it.
> 
> One way is to allocate such space in advance. For example, if we use the 
> stack-based approach (personally I prefer this for consistency), instead of 
> allocating space dynamically, we can allocate it statically. That is, at 
> linking time or at initialization time. This way limits the maximum number of 
> slots arbitrarily, but it should be enough for the reality (e.g. 10).
> 
> What do you think?

Sounds good.

Enhancing the error handling could solve this problem. Stack based
approach here sounds more reasonable.

I think we should also modify grub_print_error () in way that it would
print out all messages from stack too. Perhaps add some kind of boolean
information to pop function to tell whether there are more entries in stack.

Something like this:
--
do {
  if (grub_errno != GRUB_ERR_NONE)
    grub_printf (...)
} while (grub_error_pop ());
--

In case of stack is empty, grub_error_pop would set grub_errno to
GRUB_ERR_NONE and return zero. If there is still entries left grub_errno
and error string would be copied from stack and non-zero value would be
returned.

If the stack is about to be filled full, it could show some kind of
assert message to user and keep stack at it's current state. One option
could be that if there is room for one error message when push comes it
would put assert message there and then refuse to push more messages. So
new error messages would override latests ones. Most likely the first
error message is the most important.

Only thing here is what is preferred order to display error messages. It
would be more traditional to display first error message first and then
error message that came after that. Stack model for using it sounds
reasonable, but for printing it, reverse order is more reasonable.

Now if we think about the font manager issue.

First it should search from it's cache for fonts and if it could not
find it then do the following:

1. grub_error_push ()
2. read glyph from disk
3. if there is error, ->leave & return dummy glyph (but do not release font)
4. if all went correctly, grub_error_pop ()

This would leave last error message to stack and then newest one from
read glyph message is also shown to user. Assuming that there is
understandable glyphs :).

This would require that either fonts are validated at load time or then
assumption that fonts will contain correct information. If there is only
couple of displayable fonts corrupted then at least some of the message
would be readable by the user.

How this sounds to you ?

Re: problem in usage of grub_errno...

[Vesa Jääskeläinen]

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

I made a test implementation of the proposal. I also made some tests
with video subsystem and it eliminated problem I was having.

In case there is a similar case that I had, so it is necessary to reset
error state but not to discard the actual error message here is some
idea how it should be used:

---
/* Save possible old error message.  */
grub_error_push ();

/* Do your stuff here.  */
call_possibly_failing_function ();

if (grub_errno != GRUB_ERR_NONE)
  {
    /* Inform rest of the code that there is error (grub_errno
       is set). */
    return;
  }

/* Restore old error state by popping previous item from stack. */
grub_error_pop ();
---

I made a change to proposed error stack overflow indication mechanism as
there was possibility that assert message could get lost. Instead now it
marks an assert flag that overflow has happened. When error stack is
full and push is attempted, error message will be discarded and
grub_errno will be set to GRUB_ERR_NONE in order to other parts of the
system to work as excepted.

When prompt is about to be drawn again, I modified it to print whole
error stack in reverse order. I concluded that closest message to prompt
will be easiest to pick up. But the order can be changed if it is a problem.

I will wait couple of days for comments, if patch is not "killed" I will
commit it to CVS.

Thanks,
Vesa Jääskeläinen

[-- Attachment #2: error-stack.diff --]
[-- Type: text/plain, Size: 5160 bytes --]

Index: ChangeLog
===================================================================
RCS file: /sources/grub/grub2/ChangeLog,v
retrieving revision 1.210
diff -u -r1.210 ChangeLog
--- ChangeLog	10 Dec 2005 05:24:58 -0000	1.210
+++ ChangeLog	17 Dec 2005 17:32:13 -0000
@@ -1,3 +1,18 @@
+2005-12-17  Vesa Jaaskelainen  <chaac@nic.fi>
+
+	* kern/err.c (grub_error_push): Added new function to support error
+	stacks.
+	(grub_error_pop): Likewise.
+	(grub_error_stack_items): New local variable to support error stacks.
+	(grub_error_stack_pos): Likewise.
+	(grub_error_stack_assert): Likewise.
+	(GRUB_ERROR_STACK_SIZE): Added new define to configure maximum error
+	stack depth.
+	(grub_print_error): Added support to print errors from error stack.
+
+	* include/grub/err.h (grub_error_push): Added function prototype.
+	(grub_error_pop): Likewise.
+
 2005-12-09  Hollis Blanchard  <hollis@penguinppc.org>
 
 	* configure.ac: Accept `powerpc64' as host_cpu.
Index: include/grub/err.h
===================================================================
RCS file: /sources/grub/grub2/include/grub/err.h,v
retrieving revision 1.10
diff -u -r1.10 err.h
--- include/grub/err.h	6 Nov 2005 22:19:59 -0000	1.10
+++ include/grub/err.h	17 Dec 2005 17:32:14 -0000
@@ -1,7 +1,7 @@
 /* err.h - error numbers and prototypes */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2004  Free Software Foundation, Inc.
+ *  Copyright (C) 2002-2005  Free Software Foundation, Inc.
  *
  *  GRUB is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -60,6 +60,8 @@
 
 grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *fmt, ...);
 void EXPORT_FUNC(grub_fatal) (const char *fmt, ...) __attribute__ ((noreturn));
+void EXPORT_FUNC(grub_error_push) (void);
+int EXPORT_FUNC(grub_error_pop) (void);
 void EXPORT_FUNC(grub_print_error) (void);
 
 #endif /* ! GRUB_ERR_HEADER */
Index: kern/err.c
===================================================================
RCS file: /sources/grub/grub2/kern/err.c,v
retrieving revision 1.3
diff -u -r1.3 err.c
--- kern/err.c	4 Apr 2004 13:46:01 -0000	1.3
+++ kern/err.c	17 Dec 2005 17:32:14 -0000
@@ -1,7 +1,7 @@
 /* err.c - error handling routines */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2005  Free Software Foundation, Inc.
  *
  *  GRUB is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -22,11 +22,21 @@
 #include <grub/misc.h>
 #include <stdarg.h>
 
-#define GRUB_MAX_ERRMSG	256
+#define GRUB_MAX_ERRMSG		256
+#define GRUB_ERROR_STACK_SIZE	10
 
 grub_err_t grub_errno;
 char grub_errmsg[GRUB_MAX_ERRMSG];
 
+static struct
+{
+  grub_err_t errno;
+  char errmsg[GRUB_MAX_ERRMSG];
+} grub_error_stack_items[GRUB_ERROR_STACK_SIZE];
+
+static int grub_error_stack_pos;
+static int grub_error_stack_assert;
+
 grub_err_t
 grub_error (grub_err_t n, const char *fmt, ...)
 {
@@ -54,8 +64,72 @@
 }
 
 void
+grub_error_push (void)
+{
+  /* Only add items to stack, if there is enough room.  */
+  if (grub_error_stack_pos < GRUB_ERROR_STACK_SIZE)
+    {
+      /* Copy active error message to stack.  */
+      grub_error_stack_items[grub_error_stack_pos].errno = grub_errno;
+      grub_memcpy (grub_error_stack_items[grub_error_stack_pos].errmsg,
+                   grub_errmsg,
+                   sizeof (grub_errmsg));
+        
+      /* Advance to next error stack position.  */
+      grub_error_stack_pos++;
+    }
+  else
+    {
+      /* There is no room for new error message. Discard new error message
+         and mark error stack assertion flag.  */
+      grub_error_stack_assert = 1;
+    }
+
+  /* Allow further operation of other components by resetting
+     active errno to GRUB_ERR_NONE.  */
+  grub_errno = GRUB_ERR_NONE;
+}
+
+int
+grub_error_pop (void)
+{
+  if (grub_error_stack_pos > 0)
+    {
+      /* Pop error message from error stack to current active error.  */
+      grub_error_stack_pos--;
+      
+      grub_errno = grub_error_stack_items[grub_error_stack_pos].errno;
+      grub_memcpy (grub_errmsg,
+                   grub_error_stack_items[grub_error_stack_pos].errmsg,
+                   sizeof (grub_errmsg));
+                  
+      return 1;
+    }
+  else
+    {
+      /* There is no more items on error stack, reset to no error state.  */
+      grub_errno = GRUB_ERR_NONE;
+      
+      return 0;
+    }
+}
+
+void
 grub_print_error (void)
 {
-  if (grub_errno != GRUB_ERR_NONE)
-    grub_printf ("error: %s\n", grub_errmsg);
+  /* Print error messages in reverse order. First print active error message
+     and then empty error stack.  */
+  do
+    {
+      if (grub_errno != GRUB_ERR_NONE)
+        grub_printf ("error: %s\n", grub_errmsg);
+    } 
+  while (grub_error_pop ());
+  
+  /* If there was an assert while using error stack, report about it.  */
+  if (grub_error_stack_assert)
+    {
+      grub_printf ("assert: error stack overflow detected!\n");
+      grub_error_stack_assert = 0;
+    }
 }

Re: problem in usage of grub_errno...

[Tomáš Ebenlendr]

I think there is a 'bug' in the example. Or maybe I'm missing something.

On 17 Prosinec 2005, 18:40, Vesa Jääskeläinen napsal(a):
....
> ---
> /* Save possible old error message.  */
> grub_error_push ();
>
> /* Do your stuff here.  */
> call_possibly_failing_function ();
>
> if (grub_errno != GRUB_ERR_NONE)
>   {
>     /* Inform rest of the code that there is error (grub_errno
>        is set). */
>     return;
>   }
>
> /* Restore old error state by popping previous item from stack. */
> grub_error_pop ();
> ---
....

When grub_errno != GRUB_ERR_NONE (branching into the 'if'), then
there is no grub_error_pop() in the example.
If the whole thing will be called in a loop, then unwanted
stack overflow can simply occur.
There are two options how to solve this:
 * pop the stack (and choose which error should stay in grub_errno,
   handle (report) the other one)
 * write with BIG LETTERS to documentation of the function, that it
   may push and not pop grub_errno when it fails. And check all calls
   of this function, that this is handled properly (in code and/or in
   documentation).

-- 
                                            Tomas 'Ebi' Ebenlendr
                                            http://get.to/ebik

Re: problem in usage of grub_errno...

[Vesa Jääskeläinen]

Tomáš Ebenlendr wrote:
> I think there is a 'bug' in the example. Or maybe I'm missing something.

Nope, it was intentional. :)

> On 17 Prosinec 2005, 18:40, Vesa Jääskeläinen napsal(a):
> ....
>> ---
>> /* Save possible old error message.  */
>> grub_error_push ();
>>
>> /* Do your stuff here.  */
>> call_possibly_failing_function ();
>>
>> if (grub_errno != GRUB_ERR_NONE)
>>   {
>>     /* Inform rest of the code that there is error (grub_errno
>>        is set). */
>>     return;
>>   }
>>
>> /* Restore old error state by popping previous item from stack. */
>> grub_error_pop ();
>> ---
> ....
> 
> When grub_errno != GRUB_ERR_NONE (branching into the 'if'), then
> there is no grub_error_pop() in the example.
> If the whole thing will be called in a loop, then unwanted
> stack overflow can simply occur.

Error stack is statically allocated and it is protected so that it
cannot overflow. Error stack is defined as being static to file
kern/err.c so there is no direct access possible to error stack.

Error stack is emptied in grub_print_error function. (And this is called
before prompt is printed to screen)

Idea of the error stack is to record all error messages before they are
printed.

Or did I miss your idea with the "unwanted stack overflow" ?

Thanks,
Vesa Jääskeläinen

Re: problem in usage of grub_errno...

[Joel Buckley]

There is a second bug though...

A possible stack memory-leak is possible.  Replace the
first line with the following to avoid the memory leak:

    if (grub_errno != GRUB_ERR_NONE) {
       grub_error_push();
    }

Otherwise, a useless push is done.

leoJ.


Vesa Jääskeläinen wrote:

>Tomáš Ebenlendr wrote:
>  
>
>>I think there is a 'bug' in the example. Or maybe I'm missing something.
>>    
>>
>
>Nope, it was intentional. :)
>
>  
>
>>On 17 Prosinec 2005, 18:40, Vesa Jääskeläinen napsal(a):
>>....
>>    
>>
>>>---
>>>/* Save possible old error message.  */
>>>grub_error_push ();
>>>
>>>/* Do your stuff here.  */
>>>call_possibly_failing_function ();
>>>
>>>if (grub_errno != GRUB_ERR_NONE)
>>>  {
>>>    /* Inform rest of the code that there is error (grub_errno
>>>       is set). */
>>>    return;
>>>  }
>>>
>>>/* Restore old error state by popping previous item from stack. */
>>>grub_error_pop ();
>>>---
>>>      
>>>
>>....
>>
>>When grub_errno != GRUB_ERR_NONE (branching into the 'if'), then
>>there is no grub_error_pop() in the example.
>>If the whole thing will be called in a loop, then unwanted
>>stack overflow can simply occur.
>>    
>>
>
>Error stack is statically allocated and it is protected so that it
>cannot overflow. Error stack is defined as being static to file
>kern/err.c so there is no direct access possible to error stack.
>
>Error stack is emptied in grub_print_error function. (And this is called
>before prompt is printed to screen)
>
>Idea of the error stack is to record all error messages before they are
>printed.
>
>Or did I miss your idea with the "unwanted stack overflow" ?
>
>Thanks,
>Vesa Jääskeläinen
>
>
>_______________________________________________
>Grub-devel mailing list
>Grub-devel@gnu.org
>http://lists.gnu.org/mailman/listinfo/grub-devel
>
>  
>

Re: problem in usage of grub_errno...

[Tomáš Ebenlendr]

> Error stack is statically allocated and it is protected so that it
> cannot overflow. Error stack is defined as being static to file
> kern/err.c so there is no direct access possible to error stack.
>
> Error stack is emptied in grub_print_error function. (And this is called
> before prompt is printed to screen)
>
> Idea of the error stack is to record all error messages before they are
> printed.
>
> Or did I miss your idea with the "unwanted stack overflow" ?

Okay. There is only minor issue then, that some loop over same
error pushing function may fill up the stack. But it is not problem
if first error (in the stack) and last error (in grub_errno) is printed
(at prompt time).

>
> Thanks,
> Vesa Jääskeläinen

-- 
                                            Tomas 'Ebi' Ebenlendr
                                            http://get.to/ebik

Re: problem in usage of grub_errno...

[Vesa Jääskeläinen]

Joel Buckley wrote:
> There is a second bug though...
> 
> A possible stack memory-leak is possible.  Replace the
> first line with the following to avoid the memory leak:
> 
>    if (grub_errno != GRUB_ERR_NONE) {
>       grub_error_push();
>    }
> 
> Otherwise, a useless push is done.

Then you need to counter protect possible pop or pop will destroy one
error message from stack.

Eg. If you omit push, you can't do a pop.

Now if you consistently use push/pop then there is no problem. Yes there
will be unnecessary GRUB_ERR_NONE in stack, but it only takes one entry.

Can you explain to me what is this your's memory leak :)

Because I fail to see it. Please study the code in more detail.

Re: problem in usage of grub_errno...

[Vesa Jääskeläinen]

Tomáš Ebenlendr wrote:
>> Error stack is statically allocated and it is protected so that it
>> cannot overflow. Error stack is defined as being static to file
>> kern/err.c so there is no direct access possible to error stack.
>>
>> Error stack is emptied in grub_print_error function. (And this is called
>> before prompt is printed to screen)
>>
>> Idea of the error stack is to record all error messages before they are
>> printed.
>>
>> Or did I miss your idea with the "unwanted stack overflow" ?
> 
> Okay. There is only minor issue then, that some loop over same
> error pushing function may fill up the stack. But it is not problem
> if first error (in the stack) and last error (in grub_errno) is printed
> (at prompt time).

Yes it is possible that some part might fill up the stack, that's way
there is a protection. When error is printed, whole stack will be dumped
to screen. So all error stack entries containing error code other than
GRUB_ERR_NONE will get displayed.

My idea is not to use this error stacking feature in all the place. Only
in places where there is a need to have a clean error state (grub_errno
== GRUB_ERR_NONE). This is because if grub_errno is being set, some
functions are required to fail.