kern/err.c + disk/raid.c error handling fixes

Am Dienstag, den 12.08.2008, 18:28 +0200 schrieb Felix Zielcke:

Ok naming the old patch kern_err.diff and the new one raid_err.diff isn't a good idea :(

2008-08-12 Felix Zielcke <[email protected]>

* kern/err.c [GRUB_UTIL]: Include <stdio.h>.

[GRUB_UTIL] (grub_print_error): Use fprintf (stderr, ...) instead of grub_printf.

* disk/raid.c (GRUB_MOD_INIT): Use grub_print_error() to show RAID errors and reset grub_errno. Do not give errors to the upper layer.

raid.c +2 -0

   grub_device_iterate (&grub_raid_scan_device);
   grub_disk_dev_register (&grub_raid_dev);
+  grub_print_error ();
+  grub_errno = GRUB_ERR_NONE;
 }
 
 GRUB_MOD_FINI(raid)

Quoted 2 lines

[GRUB_UTIL] (grub_print_error): Use fprintf (stderr, ...) instead of grub_printf.

Why?

It would be better to make a util/misc.c:grub_print_error instead of this

Please use -up for diff, you can use:

$ svn diff --diff-cmd diff -x -up

Quoted 1 line

* kern/err.c [GRUB_UTIL]: Include <stdio.h>.

Please don't do this. Why do you want this?

Quoted 1 line

"...reset grub_errno. Do not..."

(double space)

Quoted 2 lines

It would be better to make a util/misc.c:grub_print_error instead of this

Problem is that kern/err.c is already included in grub-probe. So then we would have to add util/misc.c:grub_print_error _and_ disable grub_print_error.

Quoted 2 lines

Please use -up for diff, you can use:

$ svn diff --diff-cmd diff -x -up

Thanks for the tip, I've been looking for this ;-)

Quoted 3 lines · Marco Gerards

Quoted 1 line

* kern/err.c [GRUB_UTIL]: Include <stdio.h>.

Please don't do this. Why do you want this?

It's needed for fprintf, and fprintf is only useful for GRUB_UTIL.

Quoted 2 lines

Quoted 1 line

"...reset grub_errno. Do not..."

(double space)

Right, I should really pay more attention to these small little details.

Am Mittwoch, den 13.08.2008, 00:40 +0200 schrieb Robert Millan:

Quoted 5 lines

Quoted 2 lines

It would be better to make a util/misc.c:grub_print_error instead of this

Problem is that kern/err.c is already included in grub-probe. So then we would have to add util/misc.c:grub_print_error _and_ disable grub_print_error.

Thanks for your hint.

Quoted 5 lines

Quoted 2 lines

Please use -up for diff, you can use:

$ svn diff --diff-cmd diff -x -up

Thanks for the tip, I've been looking for this ;-)

I made now a svn-diff alias for this :)

I hope it's now better, but I was unsure about the `new function' in changelog it isn't really a new one more a modified one.

2008-08-13 Felix Zielcke <[email protected]>

* include/kern/err.h [! GRUB_UTIL]: Disable.

* kern/err.c [! GRUB_UTIL] (grub_print_error): Likewise.

* include/util/misc.h (grub_print_error): New function prototype.

* util/misc.c (grub_print_error): New function.

* disk/raid.c (GRUB_MOD_INIT): Use grub_print_error() to show RAID errors and reset grub_errno. Do not give errors to the upper layer.

Index: kern/err.c
===================================================================

err.c +2 -0

@@ -113,6 +113,7 @@ grub_error_pop (void)
     }
 }
 
+#ifndef GRUB_UTIL
 void
 grub_print_error (void)
 {

@@ -132,3 +133,4 @@ grub_print_error (void)
       grub_error_stack_assert = 0;
     }
 }
+#endif

Index: include/grub/err.h
===================================================================

grub/err.h +0 -0

 #endif /* ! GRUB_ERR_HEADER */

Index: include/grub/util/misc.h
===================================================================

grub/util/misc.h +1 -0

@@ -55,4 +55,5 @@ void grub_util_write_image_at (const voi
 			       FILE *out);
 char *grub_util_get_disk_name (int disk, char *name);
 
+void grub_print_error (void);
 #endif /* ! GRUB_UTIL_MISC_HEADER */

Index: util/misc.c
===================================================================

misc.c +20 -0

@@ -300,3 +300,23 @@ grub_arch_sync_caches (void *address __a
 		       grub_size_t len __attribute__ ((unused)))
 {
 }
+
+void
+grub_print_error (void)
+{
+  /* Print error messages in reverse order. First print active error message
+     and then empty error stack.  */
+  do
+    {
+      if (grub_errno != GRUB_ERR_NONE)
+        fprintf (stderr ,"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)
+    {
+      fprintf (stderr, "assert: error stack overflow detected!\n");
+      grub_error_stack_assert = 0;
+    }
+}

Quoted 6 lines · Felix Zielcke

Quoted 3 lines · Marco Gerards

Quoted 1 line

* kern/err.c [GRUB_UTIL]: Include <stdio.h>.

Please don't do this. Why do you want this?

It's needed for fprintf, and fprintf is only useful for GRUB_UTIL.

I understood that. With this I meant "changing the kernel for GRUB_UTIL" :)

Am Mittwoch, den 13.08.2008, 11:47 +0200 schrieb Marco Gerards:

Quoted 11 lines

Quoted 6 lines · Felix Zielcke

Quoted 3 lines · Marco Gerards

Quoted 1 line

* kern/err.c [GRUB_UTIL]: Include <stdio.h>.

Please don't do this. Why do you want this?

It's needed for fprintf, and fprintf is only useful for GRUB_UTIL.

I understood that. With this I meant "changing the kernel for GRUB_UTIL" :)

I just did it first that way, because we need a simple and small patch for lenny (i.e. debian-release)

If I do it like you prefer duplicating it in util/misc.c then I need to remove the static from `int grub_error_stack_assert;' and having it prefixed with extern on grub/err.h

Yes this change is no problem, grub-probe now compiles fine But grub-mkimage not:

grub_mkimage-util_misc.o: In function `grub_print_error':
/home/fz/grub/grub2.svn/util/misc.c:315: undefined reference to grub_error_pop' /home/fz/grub/grub2.svn/util/misc.c:312: undefined reference to grub_errno' /home/fz/grub/grub2.svn/util/misc.c:313: undefined reference to grub_errmsg' /home/fz/grub/grub2.svn/util/misc.c:313: undefined reference to grub_printf' /home/fz/grub/grub2.svn/util/misc.c:315: undefined reference to grub_error_pop' /home/fz/grub/grub2.svn/util/misc.c:320: undefined reference to grub_printf'

collect2: ld returned 1 exit status
make: *** [grub-mkimage] Error 1

So for me it seems kern/err.c has to be either doubled for the utils or, just like I did first, with just changing 2 grub_printf to fprintf for GRUB_UTIL

_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Yes this change is no problem, grub-probe now compiles fine But grub-mkimage not:

grub_mkimage-util_misc.o: In function `grub_print_error':
/home/fz/grub/grub2.svn/util/misc.c:315: undefined reference to grub_error_pop' /home/fz/grub/grub2.svn/util/misc.c:312: undefined reference to grub_errno' /home/fz/grub/grub2.svn/util/misc.c:313: undefined reference to grub_errmsg' /home/fz/grub/grub2.svn/util/misc.c:315: undefined reference to grub_error_pop'
collect2: ld returned 1 exit status
make: *** [grub-mkimage] Error 1

If we can't do this in kern/err.c (although I really think it'd be simpler), adding a new util/err.c that can be used by grub-probe but not grub-mkimage would solve this.

This time dear Marco I didn't bother to read through the changelog, it might be still not perfect and I'm even tired now.

Am Donnerstag, den 14.08.2008, 00:39 +0200 schrieb Felix Zielcke:

Quoted 3 lines

This time dear Marco I didn't bother to read through the changelog, it might be still not perfect and I'm even tired now.

I should have better gone to bed instead of making quickly a patch for this.
Well 2nd try now.
I forgot grub-fstest and I think I know now why you said `why' to one entry ;)
And I forgot to change the copyright year to 2008 for the new shiny util/err.c

2008-08-14 Felix Zielcke <[email protected]>

* include/grub/err.h (grub_error_stack_assert): New variable declaration.
* kern/err.c (grub_error_stack_assert): Remove static. (grub_print_error): Disable it for [GRUB_UTIL] because ...
* util/err.c: New file with grub_print_error () using fprintf (stderr, ...).

* conf/common.rmk: Add it for grub-probe.

So this patch isn't really required, but you want it for Lenny?

Quoted 8 lines

2008-08-14 Felix Zielcke <[email protected]>

* include/grub/err.h (grub_error_stack_assert): New variable declaration.
* kern/err.c (grub_error_stack_assert): Remove static. (grub_print_error): Disable it for [GRUB_UTIL] because ...
* util/err.c: New file with grub_print_error () using fprintf (stderr, ...).

Can you just use misc.c?

Quoted 2 lines

* conf/common.rmk: Add it for grub-probe.

I have been thinking of an alternative way to fix this. A bit of a compromise between what you want and what I want and perhaps cleaner.

You could define a function grub_err_printf or something like that. Which you use in grub_print_error. You can redefine it for GRUB_UTIL to grub_util_error and to grub_print otherwise.

I didn't study the code. Will this be easier and cleaner?

misc.c +4 -0

+int
+grub_err_printf (const char *fmt, ...)
+{
+  va_list ap;

Quoted 4 lines · Felix Zielcke

misc.c +4 -0

+int
+grub_err_printf (const char *fmt, ...)
+{
+  va_list ap;

misc.c grub_util_error (const char *fmt, ...) · line 90

Quoted 10 lines

misc.c +8 -0

+  int ret;
+  
+  va_start (ap, fmt);
+  ret = vfprintf (stderr, fmt, ap);
+  va_end (ap);
+
+  return ret;
+}
 void *
 xmalloc (size_t size)

Please separate functions with an empty line. Anyway, no need to send a new patch just for this.

Marco, is it fine to you now?

2008-08-14 Felix Zielcke <[email protected]>

* include/grub.err.h (grub_err_printf): New function prototype.

* util/misc.c (grub_err_printf): New function.
* kern/misc.c [! GRUB_UTIL] (grub_err_printf): New alias for grub_printf.
* kern/err.c (grub_print_error): Use grub_err_printf.

Index: kern/err.c
===================================================================

err.c +3 -3

@@ -1,7 +1,7 @@
 /* err.c - error handling routines */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2005,2007  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2005,2007,2008  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

@@ -121,14 +121,14 @@ grub_print_error (void)
   do
     {
       if (grub_errno != GRUB_ERR_NONE)
-        grub_printf ("error: %s\n", grub_errmsg);
+        grub_err_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_err_printf ("assert: error stack overflow detected!\n");
       grub_error_stack_assert = 0;
     }
 }

Index: kern/misc.c
===================================================================

misc.c +4 -0

@@ -133,6 +133,10 @@ grub_printf (const char *fmt, ...)
 
   return ret;
 }  
+#ifndef GRUB_UTIL
+int grub_err_printf (const char *fmt, ...)
+__attribute__ ((alias("grub_printf")));
+#endif
 
 void
 grub_real_dprintf (const char *file, const int line, const char *condition,

Index: include/grub/err.h
===================================================================

grub/err.h +1 -1

@@ -1,7 +1,7 @@
 /* err.h - error numbers and prototypes */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2005,2007  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2005,2007,2008 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

grub/err.h +0 -0

 #endif /* ! GRUB_ERR_HEADER */

Index: util/misc.c
===================================================================

misc.c +5 -1

@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2003,2005,2006,2007  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2003,2005,2006,2007,2008  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

@@ -74,6 +74,18 @@ grub_util_error (const char *fmt, ...)
   exit (1);
 }
 
+int
+grub_err_printf (const char *fmt, ...)
+{
+  va_list ap;

This patch looks clean enough for me :-). If you can correct the following (and previous ;)) comments, it is ready to be committed.

Quoted 6 lines · Marco Gerards

Quoted 2 lines

[GRUB_UTIL] (grub_print_error): Use fprintf (stderr, ...) instead of grub_printf.

Why?

It would be better to make a util/misc.c:grub_print_error instead of this

Problem is that kern/err.c is already included in grub-probe. So then we would have to add util/misc.c:grub_print_error _and_ disable grub_print_error.

Quoted 2 lines

Please use -up for diff, you can use:

$ svn diff --diff-cmd diff -x -up

Thanks for the tip, I've been looking for this ;-)