Fixes #121: ignore EFI_DEVICE_ERROR in output_string

[imtsuki]

This PR fixes #121.

Signed-off-by: imtsuki me@qjx.app

[HadrienG2]

Thanks for your PR!

---

It does not seem right to unconditionally add a special case somewhere in the depths of a general-purpose UEFI binding, only for the purpose of working around a bug of one specific, if highly popular UEFI implementation. From the UEFI specification wording...

The device reported an error while attempting to output the text.

...another UEFI implementation could just as well decide to emit this error to signal a very serious condition, like a dead hardware link preventing the text output from reaching the user.

Therefore, please consider making the workaround conditional on an off-by-default run-time switch (could just be a static AtomicBool with a function to flip it on), which the user can enable as they wish, e.g. after checking the UEFI implementation's version information.

---

Also, if this is a general output_string() problem, you'll want to put the workaround inside the output_string() implementation, rather than in the fmt::Write implementation that calls output_string().

[josephlr]

@imtsuki when you hit this bug, does the text still get output event if the device errors?

One alternative might just be to retry (a few times?) on getting this errror.

[imtsuki]

@HadrienG2 Thanks for your review!

...another UEFI implementation could just as well decide to emit this error to signal a very serious condition, like a dead hardware link preventing the text output from reaching the user.

It's frustrating that UEFI spec lacks documentation about this. But considering the fact that before the loader application is executed, UEFI has already done some self-test and output some text to the console (BDS phase), a potentially fatal error could only happen in the BDS phase and then our program will never be executed.

Also, even if a fatal error did happen in the TSL phase, it should not be considered as fatal. This is a more general case what logging's role is: logging only aids to debug the main program, and it should not cause the main program to crash.

Therefore, I think a more acceptable workaround to fix this bug might be ignoring the Result here:

uefi-rs/src/logger.rs

Lines 56 to 61 in 006c388

	fn log(&self, record: &log::Record) {
	if let Some(mut ptr) = self.writer {
	let writer = unsafe { ptr.as_mut() };
	DecoratedLog::write(writer, record.level(), record.args()).unwrap();
	}
	}

As logging is a much higher, convenient API exposed by this crate, not from UEFI spec. And we leave the low-level fmt::Write implementation for SimpleTextOutput protocol as-is.

[imtsuki]

@josephlr outputting to console is still functioning. Otherwise, I won't see the panic message😀

I've tried both methods. Retrying leads to duplicate text output and ignoring will drop some text. What's worse, the amount of text dropped or duplicated is different in the graphic console and the serial console, in case both are connected. I'm afraid there is no such an elegant workaround to fix this.

[HadrienG2]

Thanks for clarifying that the VirtualBox UEFI implementation does drop out text. Your initial issue made it sound like the error code was unjustified, while it is now pretty clear that it reports a real self-diagnosed problem in that UEFI implementation.

Please report a VirtualBox bug about this with a reduced version of your reproducer. Text I/O bugs in firmwares are very serious, because they deprive low-level devs from what is often their only (or at least most accessible) diagnosis tool. They should not be ignored, they must be fixed.

---

Further, I agree that logging errors being unconditionally fatal does not sound right. In my opinion, what we're really facing here an interface design issue of the Log interface from the log crate, which gives no other option than to panic or ignore output errors. I will take care of reporting this one.

Until the log crate fixes their interface (if ever), I agree that ignoring the output Result is probably the least bad solution. Please add a FIXME comment clarifying this situation (so that people experiencing log data loss can more easily diagnose what's going on), and then this PR is ready to merge IMO.

[HadrienG2]

Thank you!

[HadrienG2]

Opened a Log issue: rust-lang/log#382 .