Re: problem in usage of grub_errno...

[Vesa Jääskeläinen]

Yoshinori K. Okuji wrote:
> On Saturday 10 December 2005 11:05 am, Vesa Jääskeläinen wrote:
>> 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;
> 
> Please do not change it in this way. The error subsystem of GRUB is based on 
> the idea of exceptions, that is to say, passing an error to higher levels 
> until it is caught explicitly. This is similar to C++, Java, Python or Ruby. 
> Unfortunately, C does not support exceptions directly, so we must emulate it. 
> So, what we must do is not to ignore an error where it happens.

[snip]

> In other words, if you ignore an error set by calling a function and pass the 
> control to another, it violates the semantics. So, strictly speaking, you 
> must choose either of these when an error is set:
> 
> - deciding to ignore the error explicitly by setting GRUB_ERRNO to 
> GRUB_ERR_NONE
> 
> - handling the error by returning to the higher level or dealing with the 
> error  locally (e.g. printing the error)
> 
> We (at least I) sometimes violate this rule by intention when knowing that it 
> is safe to ignore an error, simply because it is too heavy to deal with 
> errors all the time.

Perhaps we could put this information somewhere on wiki? :)

It could help a bit new developers and would also give some guidelines
how error handling should be implemented.

> In your situation, things are a bit complicated. In my opinion, all you 
> should 
> do is to save error information in a temporary place and reset it afterwards. 
> For example, you can define a function like this:
> 
> void
> grub_error_push (void)
> {
>   /* Push the current error in a stack and clear GRUB_ERRNO.  */
>   ...
> }
> 
> void
> grub_error_pop (void)
> {
>   /* Pop the previous error and set GRUB_ERRNO.  */
>   ...
> }
> 
> I don't know if this should be stack-based or flat. For example, you can 
> allocate space in a function and pass the pointer to a function which saves 
> current error information. In this case, functions should be named 
> grub_error_save and grub_error_restore.
> 
> The difficulty is that it is not very safe to use a heap to allocate space to 
> store error information, because such a memory allocation can generate 
> another error (due to memory shortage). The possibility is low, but we must 
> still consider it.
> 
> One way is to allocate such space in advance. For example, if we use the 
> stack-based approach (personally I prefer this for consistency), instead of 
> allocating space dynamically, we can allocate it statically. That is, at 
> linking time or at initialization time. This way limits the maximum number of 
> slots arbitrarily, but it should be enough for the reality (e.g. 10).
> 
> What do you think?

Sounds good.

Enhancing the error handling could solve this problem. Stack based
approach here sounds more reasonable.

I think we should also modify grub_print_error () in way that it would
print out all messages from stack too. Perhaps add some kind of boolean
information to pop function to tell whether there are more entries in stack.

Something like this:
--
do {
  if (grub_errno != GRUB_ERR_NONE)
    grub_printf (...)
} while (grub_error_pop ());
--

In case of stack is empty, grub_error_pop would set grub_errno to
GRUB_ERR_NONE and return zero. If there is still entries left grub_errno
and error string would be copied from stack and non-zero value would be
returned.

If the stack is about to be filled full, it could show some kind of
assert message to user and keep stack at it's current state. One option
could be that if there is room for one error message when push comes it
would put assert message there and then refuse to push more messages. So
new error messages would override latests ones. Most likely the first
error message is the most important.

Only thing here is what is preferred order to display error messages. It
would be more traditional to display first error message first and then
error message that came after that. Stack model for using it sounds
reasonable, but for printing it, reverse order is more reasonable.

Now if we think about the font manager issue.

First it should search from it's cache for fonts and if it could not
find it then do the following:

1. grub_error_push ()
2. read glyph from disk
3. if there is error, ->leave & return dummy glyph (but do not release font)
4. if all went correctly, grub_error_pop ()

This would leave last error message to stack and then newest one from
read glyph message is also shown to user. Assuming that there is
understandable glyphs :).

This would require that either fonts are validated at load time or then
assumption that fonts will contain correct information. If there is only
couple of displayable fonts corrupted then at least some of the message
would be readable by the user.

How this sounds to you ?