Marco Gerards wrote: > Vesa Jääskeläinen <[EMAIL PROTECTED]> 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. _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel