Re: problem in usage of grub_errno...

[Vesa Jääskeläinen]

Marco Gerards wrote:
> Vesa Jääskeläinen <address@hidden> 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.