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

[Marco Gerards <mgerards@xs4all.nl>]

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;
> +    }
> +}