[PATCH] kern/err.c + disk/raid.c error handling fixes

[PATCH] kern/err.c + disk/raid.c error handling fixes

[Felix Zielcke]

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

As requested by Robert on IRC
here are now my 2 little recent patches merged into one

2008-08-12  Felix Zielcke <fzielcke@z-51.de>
  
        * 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.

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

Index: kern/err.c
===================================================================
--- kern/err.c	(revision 1800)
+++ kern/err.c	(working copy)
@@ -21,6 +21,9 @@
 #include <grub/misc.h>
 #include <stdarg.h>
 
+#ifdef GRUB_UTIL
+#include <stdio.h>
+#endif
 #define GRUB_MAX_ERRMSG		256
 #define GRUB_ERROR_STACK_SIZE	10
 
@@ -121,7 +124,11 @@
   do
     {
       if (grub_errno != GRUB_ERR_NONE)
+#ifdef GRUB_UTIL
+	fprintf (stderr, "error: %s\n", grub_errmsg);
+#else
         grub_printf ("error: %s\n", grub_errmsg);
+#endif
     } 
   while (grub_error_pop ());
   

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Felix Zielcke]

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

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 <fzielcke@z-51.de>
  
        * 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.


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

Index: disk/raid.c
===================================================================
--- disk/raid.c	(revision 1800)
+++ disk/raid.c	(working copy)
@@ -581,6 +581,8 @@
 {
   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)
Index: kern/err.c
===================================================================
--- kern/err.c	(revision 1800)
+++ kern/err.c	(working copy)
@@ -21,6 +21,9 @@
 #include <grub/misc.h>
 #include <stdarg.h>
 
+#ifdef GRUB_UTIL
+#include <stdio.h>
+#endif
 #define GRUB_MAX_ERRMSG		256
 #define GRUB_ERROR_STACK_SIZE	10
 
@@ -121,7 +124,11 @@
   do
     {
       if (grub_errno != GRUB_ERR_NONE)
+#ifdef GRUB_UTIL
+	fprintf (stderr, "error: %s\n", grub_errmsg);
+#else
         grub_printf ("error: %s\n", grub_errmsg);
+#endif
     } 
   while (grub_error_pop ());

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Marco Gerards]

Felix Zielcke <fzielcke@z-51.de> writes:

> 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 <fzielcke@z-51.de>
>   
>         * kern/err.c [GRUB_UTIL]: Include <stdio.h>.

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

>           [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

>         * 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.

"...reset grub_errno.  Do not..."
(double space)

> Index: disk/raid.c
> ===================================================================
> --- disk/raid.c	(revision 1800)
> +++ disk/raid.c	(working copy)
> @@ -581,6 +581,8 @@
>  {

Please use -up for diff, you can use:
$ svn diff --diff-cmd diff -x -up

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

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Robert Millan]

On Tue, Aug 12, 2008 at 11:42:58PM +0200, Marco Gerards wrote:
> >           [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.

> 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 ;-)

-- 
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] kern/err.c + disk/raid.c error handling fixes

[Felix Zielcke]

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

On Tue, Aug 12, 2008 at 11:42:58PM +0200, Marco Gerards wrote:
> >         * 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.

> >"...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:
> > 
> > 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.

> > 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  <fzielcke@z-51.de>

        * 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.


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

Index: kern/err.c
===================================================================
--- kern/err.c	(Revision 1802)
+++ kern/err.c	(Arbeitskopie)
@@ -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
===================================================================
--- include/grub/err.h	(Revision 1802)
+++ include/grub/err.h	(Arbeitskopie)
@@ -63,6 +63,9 @@ grub_err_t EXPORT_FUNC(grub_error) (grub
 void EXPORT_FUNC(grub_fatal) (const char *fmt, ...) __attribute__ ((noreturn));
 void EXPORT_FUNC(grub_error_push) (void);
 int EXPORT_FUNC(grub_error_pop) (void);
+
+#ifndef GRUB_UTIL
 void EXPORT_FUNC(grub_print_error) (void);
+#endif
 
 #endif /* ! GRUB_ERR_HEADER */
Index: include/grub/util/misc.h
===================================================================
--- include/grub/util/misc.h	(Revision 1802)
+++ include/grub/util/misc.h	(Arbeitskopie)
@@ -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
===================================================================
--- util/misc.c	(Revision 1802)
+++ util/misc.c	(Arbeitskopie)
@@ -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;
+    }
+}

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Marco Gerards]

Felix Zielcke <fzielcke@z-51.de> writes:

> On Tue, Aug 12, 2008 at 11:42:58PM +0200, Marco Gerards wrote:
>> >         * 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" :)

>> >"...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:
>> > 
>> > 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.
>
>> > 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 :)

Great :-)

> 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  <fzielcke@z-51.de>
>
>         * include/kern/err.h [! GRUB_UTIL]: Disable.

Disable what?

BTW, this file does not exist.


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

Why?

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

If you included <grub/err.h> here...

>         * 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
> ===================================================================
> --- kern/err.c	(Revision 1802)
> +++ kern/err.c	(Arbeitskopie)
> @@ -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
> ===================================================================
> --- include/grub/err.h	(Revision 1802)
> +++ include/grub/err.h	(Arbeitskopie)
> @@ -63,6 +63,9 @@ grub_err_t EXPORT_FUNC(grub_error) (grub
>  void EXPORT_FUNC(grub_fatal) (const char *fmt, ...) __attribute__ ((noreturn));
>  void EXPORT_FUNC(grub_error_push) (void);
>  int EXPORT_FUNC(grub_error_pop) (void);
> +
> +#ifndef GRUB_UTIL
>  void EXPORT_FUNC(grub_print_error) (void);
> +#endif

Why does this have to be conditional?

>  #endif /* ! GRUB_ERR_HEADER */
> Index: include/grub/util/misc.h
> ===================================================================
> --- include/grub/util/misc.h	(Revision 1802)
> +++ include/grub/util/misc.h	(Arbeitskopie)
> @@ -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
> ===================================================================
> --- util/misc.c	(Revision 1802)
> +++ util/misc.c	(Arbeitskopie)
> @@ -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;
> +    }
> +}

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Felix Zielcke]

Am Mittwoch, den 13.08.2008, 11:47 +0200 schrieb Marco Gerards:
> Felix Zielcke <fzielcke@z-51.de> writes:
> 
> > On Tue, Aug 12, 2008 at 11:42:58PM +0200, Marco Gerards wrote:
> >> >         * 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

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Felix Zielcke]

Am Mittwoch, den 13.08.2008, 22:31 +0200 schrieb Felix Zielcke:
> Am Mittwoch, den 13.08.2008, 11:47 +0200 schrieb Marco Gerards:
> > Felix Zielcke <fzielcke@z-51.de> writes:
> > 
> > > On Tue, Aug 12, 2008 at 11:42:58PM +0200, Marco Gerards wrote:
> > >> >         * 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'

Sorry just copied the function from kern/err.c to util/misc.c without
changing grub_printf () to fprintf () so just ignore these 2 lines
please.

> 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
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Robert Millan]

On Wed, Aug 13, 2008 at 10:31:36PM +0200, Felix Zielcke wrote:
> 
> 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

This is because util/misc.c adds error handling to grub-mkimage, which
wasn't there before.

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.

-- 
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] kern/err.c + disk/raid.c error handling fixes

[Felix Zielcke]

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

Am Mittwoch, den 13.08.2008, 23:50 +0200 schrieb Robert Millan:
> 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.

Seems like I'm sometime really just too fast.
I should start to think more before I bother to change something :)

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.

But at least everything compiles now with that patch and yes I did
ran ./autogen.sh before ;)

2008-08-14  Felix Zielcke  <fzielcke@z-51.de>

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


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

Index: conf/common.rmk
===================================================================
--- conf/common.rmk	(Revision 1804)
+++ conf/common.rmk	(Arbeitskopie)
@@ -4,7 +4,7 @@
 sbin_UTILITIES += grub-probe
 util/grub-probe.c_DEPENDENCIES = grub_probe_init.h
 grub_probe_SOURCES = util/grub-probe.c	\
-	util/biosdisk.c	util/misc.c util/getroot.c		\
+	util/biosdisk.c	util/misc.c util/getroot.c util/err.c	\
 	kern/device.c kern/disk.c kern/err.c kern/misc.c	\
 	kern/parser.c kern/partition.c kern/file.c		\
 	\
Index: kern/err.c
===================================================================
--- kern/err.c	(Revision 1804)
+++ kern/err.c	(Arbeitskopie)
@@ -34,7 +34,7 @@ static struct
 } grub_error_stack_items[GRUB_ERROR_STACK_SIZE];
 
 static int grub_error_stack_pos;
-static int grub_error_stack_assert;
+int grub_error_stack_assert;
 
 grub_err_t
 grub_error (grub_err_t n, const char *fmt, ...)
@@ -112,7 +112,7 @@ grub_error_pop (void)
       return 0;
     }
 }
-
+#ifndef GRUB_UTIL
 void
 grub_print_error (void)
 {
@@ -132,3 +132,4 @@ grub_print_error (void)
       grub_error_stack_assert = 0;
     }
 }
+#endif
Index: include/grub/err.h
===================================================================
--- include/grub/err.h	(Revision 1804)
+++ include/grub/err.h	(Arbeitskopie)
@@ -58,6 +58,7 @@ grub_err_t;
 
 extern grub_err_t EXPORT_VAR(grub_errno);
 extern char EXPORT_VAR(grub_errmsg)[];
+extern int grub_error_stack_assert;
 
 grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *fmt, ...);
 void EXPORT_FUNC(grub_fatal) (const char *fmt, ...) __attribute__ ((noreturn));
Index: conf/common.rmk
===================================================================
--- conf/common.rmk	(Revision 1804)
+++ conf/common.rmk	(Arbeitskopie)
@@ -4,7 +4,7 @@
 sbin_UTILITIES += grub-probe
 util/grub-probe.c_DEPENDENCIES = grub_probe_init.h
 grub_probe_SOURCES = util/grub-probe.c	\
-	util/biosdisk.c	util/misc.c util/getroot.c		\
+	util/biosdisk.c	util/misc.c util/getroot.c util/err.c	\
 	kern/device.c kern/disk.c kern/err.c kern/misc.c	\
 	kern/parser.c kern/partition.c kern/file.c		\
 	\
Index: kern/err.c
===================================================================
--- kern/err.c	(Revision 1804)
+++ kern/err.c	(Arbeitskopie)
@@ -34,7 +34,7 @@ static struct
 } grub_error_stack_items[GRUB_ERROR_STACK_SIZE];
 
 static int grub_error_stack_pos;
-static int grub_error_stack_assert;
+int grub_error_stack_assert;
 
 grub_err_t
 grub_error (grub_err_t n, const char *fmt, ...)
@@ -112,7 +112,7 @@ grub_error_pop (void)
       return 0;
     }
 }
-
+#ifndef GRUB_UTIL
 void
 grub_print_error (void)
 {
@@ -132,3 +132,4 @@ grub_print_error (void)
       grub_error_stack_assert = 0;
     }
 }
+#endif
Index: include/grub/err.h
===================================================================
--- include/grub/err.h	(Revision 1804)
+++ include/grub/err.h	(Arbeitskopie)
@@ -58,6 +58,7 @@ grub_err_t;
 
 extern grub_err_t EXPORT_VAR(grub_errno);
 extern char EXPORT_VAR(grub_errmsg)[];
+extern int grub_error_stack_assert;
 
 grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *fmt, ...);
 void EXPORT_FUNC(grub_fatal) (const char *fmt, ...) __attribute__ ((noreturn));
Index: util/err.c
===================================================================
--- util/err.c	(Revision 0)
+++ util/err.c	(Revision 0)
@@ -0,0 +1,41 @@
+/* err.c - util error printing routine */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2002,2005,2007  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
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <stdio.h>
+#include <grub/err.h>
+
+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;
+    }
+}

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Felix Zielcke]

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

Am Donnerstag, den 14.08.2008, 00:39 +0200 schrieb Felix Zielcke:
> 
> 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  <fzielcke@z-51.de>

        * 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.


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

Index: conf/common.rmk
===================================================================
--- conf/common.rmk	(Revision 1804)
+++ conf/common.rmk	(Arbeitskopie)
@@ -4,7 +4,7 @@
 sbin_UTILITIES += grub-probe
 util/grub-probe.c_DEPENDENCIES = grub_probe_init.h
 grub_probe_SOURCES = util/grub-probe.c	\
-	util/biosdisk.c	util/misc.c util/getroot.c		\
+	util/biosdisk.c	util/misc.c util/getroot.c util/err.c	\
 	kern/device.c kern/disk.c kern/err.c kern/misc.c	\
 	kern/parser.c kern/partition.c kern/file.c		\
 	\
@@ -23,7 +23,7 @@ endif
 
 # For grub-fstest.
 util/grub-fstest.c_DEPENDENCIES = grub_fstest_init.h
-grub_fstest_SOURCES = util/grub-fstest.c util/hostfs.c util/misc.c 	\
+grub_fstest_SOURCES = util/grub-fstest.c util/hostfs.c util/misc.c util/err.c	\
 	kern/file.c kern/device.c kern/disk.c kern/err.c kern/misc.c	\
 	disk/host.c disk/loopback.c  normal/arg.c normal/misc.c		\
 	io/gzio.c lib/hexdump.c commands/blocklist.c commands/ls.c \
Index: kern/err.c
===================================================================
--- kern/err.c	(Revision 1804)
+++ kern/err.c	(Arbeitskopie)
@@ -34,7 +34,7 @@ static struct
 } grub_error_stack_items[GRUB_ERROR_STACK_SIZE];
 
 static int grub_error_stack_pos;
-static int grub_error_stack_assert;
+int grub_error_stack_assert;
 
 grub_err_t
 grub_error (grub_err_t n, const char *fmt, ...)
@@ -112,7 +112,7 @@ grub_error_pop (void)
       return 0;
     }
 }
-
+#ifndef GRUB_UTIL
 void
 grub_print_error (void)
 {
@@ -132,3 +132,4 @@ grub_print_error (void)
       grub_error_stack_assert = 0;
     }
 }
+#endif
Index: include/grub/err.h
===================================================================
--- include/grub/err.h	(Revision 1804)
+++ include/grub/err.h	(Arbeitskopie)
@@ -58,6 +58,7 @@ grub_err_t;
 
 extern grub_err_t EXPORT_VAR(grub_errno);
 extern char EXPORT_VAR(grub_errmsg)[];
+extern int grub_error_stack_assert;
 
 grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *fmt, ...);
 void EXPORT_FUNC(grub_fatal) (const char *fmt, ...) __attribute__ ((noreturn));
Index: util/err.c
===================================================================
--- util/err.c	(Revision 0)
+++ util/err.c	(Revision 0)
@@ -0,0 +1,41 @@
+/* err.c - util error printing routine */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 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
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <stdio.h>
+#include <grub/err.h>
+
+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;
+    }
+}

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Marco Gerards]

Felix Zielcke <fzielcke@z-51.de> writes:

> Am Donnerstag, den 14.08.2008, 00:39 +0200 schrieb Felix Zielcke:
>> 
>> 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

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

> 2008-08-14  Felix Zielcke  <fzielcke@z-51.de>
>
>         * 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?

>         * 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?

--
Marco

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Felix Zielcke]

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

Am Donnerstag, den 14.08.2008, 09:13 +0200 schrieb Marco Gerards: 
> So this patch isn't really required, but you want it for Lenny?

1218558734.19045.10.camel@fz.local
That's the one Robert and me thought could be something for lenny.

I have forgotten to merge the disk/raid.c changes which Robert wanted to
have in the last grub_print_error () patches.

As I already told you on IRC, I now start to believe like Robert that
the idea isn't that bad to have a function for the utils which prints to
stderr but it doestn't `exit ()' liek grub_util_error does.

Maybe not only raid.mod has a use for this but others too.

> > 2008-08-14  Felix Zielcke  <fzielcke@z-51.de>
> >
> >         * 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?

As already said on IRC
1218659496.25595.7.camel@fz.local

grub-mkimage would then need to have kern/err.c included.

> 
> >         * 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.

This whole problem, confuses me now myself.
Bean is rewriting the whole RAID code anyway and Robert and me would
like to have a simple and small patch for lenny.

For upstream (i.e. you ;)) I want a clean solution, that's why I did now
the RFC topic.
Maybe it's just better if I don't think now for lenny.

> 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?


Thanks to Robert for the hint with the copyright years.

As always with me, changelog and code is probable still not perfect ;)
but I think the genereal idea behind it is a step to come nearer to a
clean good solution for this whole problem
(1218699584.4065.26.camel@fz.local)

It does compile cleanly i.e. doestn't introduce any warnings at all and
it works with the 2 raid/disk.c changes from msgid I said at the
beginning of this mail.

2008-08-14  Felix Zielcke  <fzielcke@z-51.de>

        * 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.


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

Index: kern/err.c
===================================================================
--- kern/err.c	(Revision 1804)
+++ kern/err.c	(Arbeitskopie)
@@ -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
===================================================================
--- kern/misc.c	(Revision 1804)
+++ kern/misc.c	(Arbeitskopie)
@@ -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
===================================================================
--- include/grub/err.h	(Revision 1804)
+++ include/grub/err.h	(Arbeitskopie)
@@ -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
@@ -64,5 +64,6 @@ void EXPORT_FUNC(grub_fatal) (const char
 void EXPORT_FUNC(grub_error_push) (void);
 int EXPORT_FUNC(grub_error_pop) (void);
 void EXPORT_FUNC(grub_print_error) (void);
+int EXPORT_FUNC(grub_err_printf) (const char *fmt, ...) __attribute__ ((format (printf, 1, 2)));
 
 #endif /* ! GRUB_ERR_HEADER */
Index: util/misc.c
===================================================================
--- util/misc.c	(Revision 1804)
+++ util/misc.c	(Arbeitskopie)
@@ -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;
+  int ret;
+  
+  va_start (ap, fmt);
+  ret = vfprintf (stderr, fmt, ap);
+  va_end (ap);
+
+  return ret;
+}
 void *
 xmalloc (size_t size)
 {

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Robert Millan]

On Thu, Aug 14, 2008 at 01:02:30PM +0200, Felix Zielcke wrote:
> +int
> +grub_err_printf (const char *fmt, ...)
> +{
> +  va_list ap;
> +  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?

-- 
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] kern/err.c + disk/raid.c error handling fixes

[Vesa Jääskeläinen]

Robert Millan wrote:
> On Thu, Aug 14, 2008 at 01:02:30PM +0200, Felix Zielcke wrote:
>> +int
>> +grub_err_printf (const char *fmt, ...)
>> +{
>> +  va_list ap;
>> +  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?
> 

Please use grub's own string formatter.

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Marco Gerards]

Hi Felix,

Felix Zielcke <fzielcke@z-51.de> writes:

[...]

> 2008-08-14  Felix Zielcke  <fzielcke@z-51.de>
>
>         * include/grub.err.h (grub_err_printf): New function prototype.

grub.err.h -> grub/err.h

>         * 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.

Looks fine to me.


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

> Index: kern/err.c
> ===================================================================
> --- kern/err.c	(Revision 1804)
> +++ kern/err.c	(Arbeitskopie)
> @@ -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
> ===================================================================
> --- kern/misc.c	(Revision 1804)
> +++ kern/misc.c	(Arbeitskopie)
> @@ -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
> ===================================================================
> --- include/grub/err.h	(Revision 1804)
> +++ include/grub/err.h	(Arbeitskopie)
> @@ -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
> @@ -64,5 +64,6 @@ void EXPORT_FUNC(grub_fatal) (const char
>  void EXPORT_FUNC(grub_error_push) (void);
>  int EXPORT_FUNC(grub_error_pop) (void);
>  void EXPORT_FUNC(grub_print_error) (void);
> +int EXPORT_FUNC(grub_err_printf) (const char *fmt, ...) __attribute__ ((format (printf, 1, 2)));

Can you break the line somehow?  It is too long.


>  #endif /* ! GRUB_ERR_HEADER */
> Index: util/misc.c
> ===================================================================
> --- util/misc.c	(Revision 1804)
> +++ util/misc.c	(Arbeitskopie)
> @@ -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;
> +  int ret;
> +  
> +  va_start (ap, fmt);
> +  ret = vfprintf (stderr, fmt, ap);
> +  va_end (ap);
> +
> +  return ret;
> +}
>  void *
>  xmalloc (size_t size)

Missing newline between the functions above.

--
Marco

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Felix Zielcke]

Am Donnerstag, den 14.08.2008, 20:27 +0200 schrieb Marco Gerards:

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

Commited.
Hurray.

Re: [PATCH] kern/err.c + disk/raid.c error handling fixes

[Robert Millan]

On Thu, Aug 14, 2008 at 12:39:16AM +0200, Felix Zielcke wrote:
> --- util/err.c	(Revision 0)
> +++ util/err.c	(Revision 0)
> @@ -0,0 +1,41 @@
> +/* err.c - util error printing routine */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2002,2005,2007  Free Software Foundation, Inc.

Remember to update copyright headers (in modified files as well)

-- 
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] kern/err.c + disk/raid.c error handling fixes

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Tue, Aug 12, 2008 at 11:42:58PM +0200, Marco Gerards wrote:
>> >           [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.

That might be better than adding fprintf to kern/ IMO.

>> 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 ;-)

Now if only everyone would use this... :-)

--
Marco