Revisit decision to ignore I/O errors in logging about uefi-rs HOT 6 CLOSED

So, the conclusion of rust-lang/log#382 was that faillible logging should probably be a responsibility of individual Log implementations, rather than the log crate itself.

Here is a selection of some interesting points that were raised:

• A global and universally agreed upon Log trait cannot support everyone's individual choice Error type, so it would need to follow the evil fmt::Error design.
• In loggers that support both faillible and infaillible operation, the choice of logging policy should probably be in the hands of the "application" code that is configuring the logger, rather than those of the "library" code that is attempting to log things. However, a faillible Log trait that returns a Result on every log operation would actually take us in the latter direction.

Therefore, I think the best course of action is to provide a uefi-services configuration flag that states whether log errors are fatal or ignored (in Rust tradition, they should probably be fatal by default).

I don't think there is a clear use case for being able to switch dynamically between fatal and ignored log errors, so the way I would spontaneously interface this is via a configuration flag at uefi-services instantiation time. This will require a new uefi_services::init_with_config(st: &SystemTable<Boot>, config: ServicesConfiguration) method (of which uefi_services::init() will become a mere alias) since we never needed such services configuration before and did not plan for it in the API.

Any further opinion, @GabrielMajeri and @imtsuki?

from uefi-rs.

@GabrielMajeri I would also leave as much of firmware bug workarounds as possible to users of uefi-rs, rather than uefi-rs itself, unless we end up in a situation where there is no other sensible choice.

I don't think this is incompatible with the idea of a run-time configuration switch however. The idea would be that the uefi-rs user would check firmware version information very early in the efi_main entry point, before setting up uefi-services accordingly. I don't know if it is possible to detect the broken firmware with the implementation versioning information that UEFI exposes though, an experiment from @imtsuki would be desirable here.

At the current stage, I don't think going for a callback-based solution would be justified, given the lack of clear use case for doing something other than ignoring the error or panicking, and how unergonomic it is to set up a nontrivial callback before the global memory allocator has been initialized.

from uefi-rs.

So, since there was no reaction from @imtsuki regarding the feasibility of detecting the broken VBox firmware, I'll implement the feature flag based approach at some point in the coming days.

from uefi-rs.

I think this is a good idea. How about a uefi-services feature called ignore-log-errors or so, not enabled by default.

It sound reasonable for downstream users to configure this depending on whether they care a lot about what gets printed to the screen or if they have other outputs.

from uefi-rs.

Indeed, a compile-time switch via a feature flag would also work.

I guess the decision between the two designs could be guided by whether there is a use case for choosing whether to ignore log errors or not at runtime. Would it be sensible, for example, to check the UEFI firmware version information and use that to enable the workaround only on affected versions of VirtualBox?

from uefi-rs.

I'm not sure if we can detect broken firmware, or even if we should do it.

By leaving it as a compile-time feature, it's up to the crate users to decide if they want to handle output errors or not.

Alternatively, we could have an optional global, static callback for handling errors, given when initializing the logger.

If it's None, we abort. Otherwise we call it to handle errors.

from uefi-rs.