change grub_print_error to use stderr for the utils

Here's now the patch Robert requested. grub_print_error shouldn't use stdout for the utils.

Why?

# grub-install /dev/sda
Unknown extra argument `Unsupported'.

This is with Bean's or mine lastest RAID patch.
/dev/sda isn't on a raid at all.
There are 4 disks attached to the VM which have a RAID 6 on it. But the RAID 6 isn't even assembled, i.e. /proc/mdstat says nothing.

As always, the changelog isn't probable that perfect for you, so please comment :)

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

* kern/err.c [GRUB_UTIL]: Include <stdio.h>. (grub_print_error) [GRUB_UTIL]: Use fprintf (stderr, ...) instead of grub_printf.

Index: kern/err.c
===================================================================
@@ -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");
+#else
         grub_printf ("error: %s\n", grub_errmsg);
+#endif
     } 
   while (grub_error_pop ());
   

Am Dienstag, den 12.08.2008, 17:30 +0200 schrieb Felix Zielcke:

Bah and I attached the broken one not the working one. So please use that one.

Index: kern/err.c
===================================================================
@@ -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 ());
   

Am Dienstag, den 12.08.2008, 18:36 +0300 schrieb Vesa Jääskeläinen:

Just see my 2nd mail, I noticed this even before I saw yours.

IIRC we usually put the [FOO] before the (bar).

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

Am Dienstag, den 12.08.2008, 17:41 +0200 schrieb Robert Millan:

2008-07-30 Robert Millan <[email protected]>

* include /grub/powerpc/ieee1275/kernel.h [!ASM_FILE] (grub_prefix):
New variable reference.

2008-05-05 Christian Franke <[email protected]>

* util/grub-mkdevicemap.c (get_floppy_disk_name) [__CYGWIN__]:
Add Cygwin device names.

But because I have more to do with you Robert then with Christian ;)

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.

Actually, both syntaxes are used. See:

http://www.gnu.org/prep/standards/html_node/Conditional-Changes.html#Conditional-Changes

Please do not use the current changelog as a manual, only for examples. The GCS is the manual :-)

--
Marco

Am Dienstag, den 12.08.2008, 17:30 +0200 schrieb Felix Zielcke:

Ok this is only clear for Bean, Robert and me :)
So here's a longer explaination for this.

The Unsupported comes from here

fz@fz:~/grub/grub2.svn$ grep -r "Unsupported" *
disk/raid.c:		  "Unsupported RAID version: %d.%d",
disk/raid.c:		  "Unsupported RAID level: %d",
disk/raid.c:		  "Unsupported RAID5 layout: %d",

In my case it's Unsupported RAID level: 6

The problem is grub_print_error () uses stdout even for grub-probe which is used by the grub-install script
So instead of ext2 or whatever grub-probe would display for grub-install, grub-install get's Unsupported.

It isn't right to use stdout for errors in the utils. real grub of course doestn't care about stdout or stderr.

If there is another way to do this, I would prefer this.

--
Marco

Hello,

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

Now with remaking that grub_print_error () patch I began to doubt if that `GRUB fears raid 6 so much that it goes to rescue mode if it just sees one which it doestn't need to access at all' should be changed for lenny.
Robert confirmed my doubts on IRC i.e. he said, it's better to discuss the whole thing first.

This is my first mail about this:
Message-Id: <[email protected]>

just using grub_dprintf isn't that great, I'd like to have at least `grub-probe -vv' show the unknown RAID level message.

The other patch, which changes grub_print_error () to use stderr and let the raid.mod itself print the error instead of giving it to the upper layer isn't that great either.

Real GRUB shows for every RAID 6 disk once `unknown RAID level 6' e.g. in my VMware testcase with 4 disks 4 times.

grub-install /dev/sda with raid 6 not even assembled i.e. empty /proc/mdstat shows it 6 times, I think once for each time grub-probe gets called.

By the way my last patch isn't even compiling because I forgot to make some other stuff from kern/err.c avaible to it i.e. grub_error_stack_assert is declared as static but used in grub_print_error ()
and there's another problem grub-mkimage includes util/misc.c but not kern/err.c so the other functions and variables needed by it aren't avaible.

So please comment this.

So what do you propose? :-) You lost me midway since you change subjects a bit often ;(

--
Marco

Am Mittwoch, den 13.08.2008, 20:41 +0200 schrieb Marco Gerards:

Oh sorry for that this time I only changed [PATCH] to [RFC] but yeah this whole topic isn't in one subject :(

But the important things I already said in the [RFC] mail, or in the mail with the msgid I said, e.g. the link to our bug report.

I wanted a small and simple fix for this problem which could go into Lenny.
I think more a mix of grub_dprint for real GRUB and showing an error in grub-probe IF a device which GRUB needs to access contains an unsupported RAID level or unsupported version of superblock. By the way that `unsupported version' is pretty useless to check for because each version (0.90, 1.0, 1.1 , 1.2) stores it at different position. Maybe 0.90 and one 1.x has the same one but then still 2 which hasn't.

With Bean's raid_6.diff (i.e. the one before he just sent now) this `unknown RAID level 6' message is shown even more then with mine. I think 15-20 times maybe even more on a grub-install /dev/sda1 I haven't tried it on real GRUB.

As Robert already (i think more indirectly) said we shouldn't fix things on different codebases.
So either we just wait for Bean to commit his changes and address the problems there or we try to address this problem just with the current one and Bean has to adjust his patch then.

Maybe it's better to have that bug report not fixed in lenny, but probable that's more a discussion with just Robert.

I haven't tried yet but Bean said he implemented Raid 6 now so the whole problem isn't that important then anymore.
But this won't be for sure in lenny :(

Well, ok. So thus we are indeed fixing a problem in Lenny inside GRUB? On IRC someone said this was *not* the case. I am very confused...

--
Marco

Am Donnerstag, den 14.08.2008, 09:07 +0200 schrieb Marco Gerards:

Honourly I want that whole topic discussed with everyone who is subscribed here and not just with you Marco, but it's very kind that at least you reply :)

Yeah sorry it is very confusing.
I want (wanted ?) to have that problem _for_ lenny fixed.

The problem (in current SVN without Bean's RAID rewrite, this won't be in lenny anway) is:

As soon as real GRUB _sees_ one disk wich has a mdraid raid level 6 (or e.g. 10, it's not supported either) super block then it goes to rescue mode.

First patch from me: <[email protected]>

Problem: The messages get totally lost for the average user, i.e. even grub-probe --vv says nothing, that RAID 6 isn't supported.

Robert suggested (20080811142856.GB15950@thorin) to handle it with grub_print_error () and grub_errno = GRUB_ERR_NONE in the raid.mod itself.

Problem: With that way + my fprintf stderr change, it's displayed in real GRUB once for every disk.
In grub-install /dev/sda1, where that isn't on RAID at all it's shown 6 times, I _think_ once for every grub-probe call.

Bean's raid_6.diff ([email protected]) and my fprintf change

Problem: The message on grub-install is now shown much more then just 6 times as with mine, I think more then 20 times.

I haven't tried Bean's lastest patch with an unsupported RAID level like 10 now that he has RAID 6 implemented.
But the problem is still there.

I'd like to have that unknown RAID level message shown only once per array not for every disk.

The `grub_print_error fprintf (stderr ..)' is probable the right first step, but it isn't a 100% perfect fix for this.

Seems like I just have to try lastest patch from Bean, if he changed something and then maybe complain at his topic about this.

For lenny Robert and me need a simple small patch to fix this. Maybe it's just better to have it for lenny unfixed.

Hey, at least you get a reply. I can't say the same about many topics I start :P

And I do not want to compromise the code quality because of a release schedule ;-)

[...]

Ah! ;-)

--
Marco

Am Donnerstag, den 14.08.2008, 10:03 +0200 schrieb Marco Gerards:

Yep, that's why I said that this is very kind of you :)
But at least Robert and Bean should say something about that. Bean is rewritting the whole RAID code anyway and Robert has at least to decide if this could go into lenny or not

Yep right Robert just wanted to have it reviewed by you which i can totally understand.
Maybe I should say that we even have to stay with SVN 2008-07-24 i.e. no new upstream versions during the freeze.
It doestn't get automatically to lenny if this is now commited upstream :(

Would all of you please stop talking about debian and lenny? This is about a bug in GRUB. :-/

The only thing I said, is that it is easier for us (debian) if the bug is fixed before Bean's RAID refactoring. But the problem will still be there after that is merged.

Anyway, please let's not waste time in this discussion. It's only a matter on which patch goes first. If Bean wants to checkin his code first, then it just means we'll need to adjust the raid.c part a bit, and that's all.

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

Am Donnerstag, den 14.08.2008, 19:54 +0200 schrieb Robert Millan:

Yep, I think so now too that this should be first fixed upstream and then we can talk about lenny.

The grub_print_error change is now commited.
The disk/raid.c change you proposed is in

Date: Mon, 11 Aug 2008 23:34:28 +0200
Message-Id: <[email protected]>

I don't mind if that or Bean's get commited first.

I still haven't tried his lastest patch, now that it includes RAID 6 support the message is anyway for more users not at all shown.

But I still don't like it that the message is shown for every disk instead of once for the whole array.
But that's a different problem and well for this we can just reuse that [RFC] topic by me to discuss this then :)

Am Donnerstag, den 14.08.2008, 19:54 +0200 schrieb Robert Millan:

This is with the current SVN without any patches except Marco's atap scsi one.
This is on my real system, i.e. with my 2 SATA RAID 0 disks.

Does it get now clear why I want a discussion about this whole topic?

fz:~# update-grub
        error: out of disk
        error: out of disk
        Updating /boot/grub/grub.cfg ...
        error: out of disk
        error: out of disk
        error: out of disk
        Found Debian background: debian-blueish-wallpaper-640x480.png
        error: out of disk
        error: out of disk
        error: out of disk
        error: out of disk
        Found linux image: /boot/vmlinuz-2.6.26-1-amd64
        Found initrd image: /boot/initrd.img-2.6.26-1-amd64
        error: out of disk
        error: out of disk
        error: out of disk
        error: out of disk
        done
        fz:~# grub-install /dev/sda
        error: out of disk
        error: out of disk
        error: out of disk
        error: out of disk
        error: out of disk
        error: out of disk
        error: out of disk
        Installation finished. No error reported.
        This is the contents of the device map /boot/grub/device.map.
        Check if this is correct or not. If any of the lines is incorrect,
        fix it and re-run the script `grub-install'.
        (hd0)	/dev/hda
        (hd1)	/dev/sda
        (hd2)	/dev/sdb

This just means someone is reporting this error and shouldn't. It's a bug.

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

Am Sonntag, den 17.08.2008, 17:07 +0200 schrieb Robert Millan:

Hi Robert,

Marco already said on IRC before I even send the mail that the error shouldn't be displayed if a dmraid is detected on it. Maybe too complicated and the error shouldn't be shown at all. The error won't be shown on real GRUB anyway.

Please don't get me wrong, I don't want to complain about the already commited code.

Now with Bean's lastest patch with even RAID 10 support there is never for mdraid's now shown "unknown RAID level".
And my original intention was, to have it only shown once per array instead of once per disk on array.

But I don't know what error messages can currently appear or will in the future.
I only want a good solution for the long term :)

Maybe just grub-install and update-grub should be a little changed to supress repeating the errors that much, i.e. not on every grub-probe run?

Hiding errors is a very bad practice. And hiding them because the output is ugly or because something is reporting false positives is even worse IMHO.

This "out of disk" sounds like a false positive indeed. It's a bug, somewhere. Something is handling errors in a place where this particular error shouldn't be reported. Maybe it should handle them somewhere else, or maybe they should be handled selectively.

Why not?

That's difficult to archieve due to current design, but sounds to me as a minor problem in comparison with what we had before.

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

Am Sonntag, den 17.08.2008, 17:34 +0200 schrieb Robert Millan:

This is because of my nForce 4 Hardware RAID 0 (i.e. dmraid). Linux sees 2 disks (sda,sdb) and the partition table of the first is for the 2 disks combined.
Both disk have only 80 GB but both combined have 160 GB.

Oh I should have said in my case.
The BIOS presents GRUB only one (hd0) for the combined 2 disks. Whereas the ata.mod from Marco sees both (well it fails to detect one but in theory)
The BIOS does handle the RAID 0 for booting, but just for that. As soon as the OS takes over it has to handle it.

Yes it isn't that important,
as I already said I only want to find a clean solution for the long term.
The current way is better then before, but at least I don't like it to have a error message shown more then needed.

This doestn't look that good either ;)

[...]
error: out of disk
Installation finished. No error reported.

If I understood correctly, nvidia's is a _software_ RAID solution which is implemented in Linux, in BIOS and in Windows (via non-native drivers), and is marketed as if it were hardware RAID.

Anyway, this doesn't answer the question on where is this "out of disk" error issued, and why.

Ah, I see. Sounds like a very nasty situation. We'll probably need special logic at install time to sort this out? At least, just to skip raid.mod inclussion and use UUIDs for the rest.

Anyway, how is this related to the "out of disk" error?

This "No error reported" seems to assume non-critical errors aren't possible. How about we remove that part of the message?

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

Am Sonntag, den 17.08.2008, 18:37 +0200 schrieb Robert Millan:

Bah normally I even say myself to this `fake hardware RAID'. But yes, this is neither a real hardware RAID controller nor like real software RAID like mdraid on Linux.

This is just from grub-probe -vv / where / is even on my IDE disk.

/home/fz/grub/grub2-1.96+20080816/kern/disk.c:368: Reading `hd1,2'... /home/fz/grub/grub2-1.96+20080816/kern/disk.c:375: Read out of range:
sector 0x100f9b45 (out of disk). /home/fz/grub/grub2-1.96+20080816/kern/disk.c:312: Closing `hd1,2'. /home/fz/grub/grub2-1.96+20080816/partmap/pc.c:143: partition 2: flag 0x0, type 0x0, start 0x0, len 0x0 /home/fz/grub/grub2-1.96+20080816/partmap/pc.c:143: partition 3: flag 0x0, type 0x0, start 0x0, len 0x0 /home/fz/grub/grub2-1.96+20080816/kern/disk.c:312: Closing `hd1'.
error: out of disk

Yes not very easy.

The partiton table is for both disks.
It says there are 160 GB, but grub-probe sees only 2 80 GB one.

Yes, would be at least better and I don't have yet a different idea.

Hi,

Actually, "out of disk" error is normal. raid needs to scan all devices, some of them is virtual, like (host), it will cause problem when trying to read from them.

In my raid patch, I just ignore error code GRUB_ERR_OUT_OF_RANGE.

Bean