Handle UEFI warnings more cleanly (#60)

UEFI warnings represent non-fatal failures. We used to handle them as total failures, now we let the user choose how he wants to handle them.

Author: HadrienG2

Cargo.toml

@@ -8,6 +8,7 @@ edition = "2018"
 
 [dependencies]
 bitflags = "1"
+log = { version = "0.4", default-features = false }
 ucs2 = "0.1"
 
 [workspace]

src/error/completion.rs

@@ -0,0 +1,105 @@
+use super::Status;
+use log::warn;
+
+/// This type is used when an UEFI operation has completed, but some non-fatal
+/// problems may have been encountered along the way
+#[must_use]
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub enum Completion<T> {
+    /// The operation completed without problems
+    Success(T),
+
+    /// The operation completed, but some non-fatal issues were encountered
+    Warning(T, Status),
+}
+
+impl<T> Completion<T> {
+    /// Split the completion into a (status, value) pair
+    pub fn split(self) -> (T, Status) {
+        match self {
+            Completion::Success(res) => (res, Status::SUCCESS),
+            Completion::Warning(res, stat) => (res, stat),
+        }
+    }
+
+    /// Access the inner value, logging the warning if there is any
+    pub fn log(self) -> T {
+        match self {
+            Completion::Success(res) => res,
+            Completion::Warning(res, stat) => {
+                log_warning(stat);
+                res
+            }
+        }
+    }
+
+    /// Assume that no warning occured, panic if not
+    pub fn unwrap(self) -> T {
+        match self {
+            Completion::Success(res) => res,
+            Completion::Warning(_, w) => {
+                unwrap_failed("Called `Completion::unwrap()` on a `Warning` value", w)
+            }
+        }
+    }
+
+    /// Assume that no warning occured, panic with provided message if not
+    pub fn expect(self, msg: &str) -> T {
+        match self {
+            Completion::Success(res) => res,
+            Completion::Warning(_, w) => unwrap_failed(msg, w),
+        }
+    }
+
+    /// Transform the inner value without unwrapping the Completion
+    pub fn map<U>(self, f: impl Fn(T) -> U) -> Completion<U> {
+        match self {
+            Completion::Success(res) => Completion::Success(f(res)),
+            Completion::Warning(res, stat) => Completion::Warning(f(res), stat),
+        }
+    }
+
+    /// Merge this completion with a success or warning status
+    ///
+    /// Since this type only has storage for one warning, if two warnings must
+    /// be stored, one of them will be spilled into the logs.
+    ///
+    pub fn with_status(self, extra_status: Status) -> Self {
+        match self {
+            Completion::Success(res) => {
+                if extra_status.is_success() {
+                    Completion::Success(res)
+                } else {
+                    Completion::Warning(res, extra_status)
+                }
+            }
+            Completion::Warning(res, stat) => {
+                if extra_status.is_success() {
+                    Completion::Warning(res, stat)
+                } else {
+                    log_warning(stat);
+                    Completion::Warning(res, extra_status)
+                }
+            }
+        }
+    }
+}
+
+impl<T> From<T> for Completion<T> {
+    fn from(res: T) -> Self {
+        Completion::Success(res)
+    }
+}
+
+// These are separate functions to reduce the code size of the methods
+#[inline(never)]
+#[cold]
+fn unwrap_failed(msg: &str, warning: Status) -> ! {
+    panic!("{}: {:?}", msg, warning)
+}
+
+#[inline(never)]
+#[cold]
+fn log_warning(warning: Status) {
+    warn!("Encountered UEFI warning: {:?}", warning)
+}

src/error/mod.rs

@@ -1,8 +1,55 @@
-use core::result;
-
 /// Definition of UEFI's standard status codes
 mod status;
 pub use self::status::Status;
 
+/// Completions are used to model operations which have completed, but may have
+/// encountered non-fatal errors ("warnings") along the way
+mod completion;
+pub use self::completion::Completion;
+
 /// Return type of many UEFI functions.
-pub type Result<T> = result::Result<T, Status>;
+pub type Result<T> = core::result::Result<Completion<T>, Status>;
+
+/// Extension trait for Result which helps dealing with UEFI's warnings
+pub trait ResultExt<T> {
+    /// Treat warnings as errors
+    fn warning_as_error(self) -> core::result::Result<T, Status>;
+
+    /// Ignore warnings, keeping a trace of them in the logs
+    fn log_warning(self) -> core::result::Result<T, Status>;
+
+    /// Expect success without warnings, panic otherwise
+    fn unwrap_success(self) -> T;
+
+    /// Expect success without warnings, panic with provided message otherwise
+    fn expect_success(self, msg: &str) -> T;
+
+    /// Transform the inner output, if any
+    fn map_inner<U>(self, f: impl Fn(T) -> U) -> Result<U>;
+}
+
+impl<T> ResultExt<T> for Result<T> {
+    fn warning_as_error(self) -> core::result::Result<T, Status> {
+        match self {
+            Ok(Completion::Success(v)) => Ok(v),
+            Ok(Completion::Warning(_, s)) => Err(s),
+            Err(s) => Err(s),
+        }
+    }
+
+    fn log_warning(self) -> core::result::Result<T, Status> {
+        self.map(|completion| completion.log())
+    }
+
+    fn unwrap_success(self) -> T {
+        self.unwrap().unwrap()
+    }
+
+    fn expect_success(self, msg: &str) -> T {
+        self.expect(msg).expect(msg)
+    }
+
+    fn map_inner<U>(self, f: impl Fn(T) -> U) -> Result<U> {
+        self.map(|completion| completion.map(f))
+    }
+}

src/error/status.rs

@@ -1,4 +1,4 @@
-use super::Result;
+use super::{Completion, Result};
 use core::ops;
 use ucs2;
 
@@ -129,9 +129,10 @@ impl Status {
     where
         F: FnOnce() -> T,
     {
-        // FIXME: Is that the best way to handle warnings?
         if self.is_success() {
-            Ok(f())
+            Ok(Completion::Success(f()))
+        } else if self.is_warning() {
+            Ok(Completion::Warning(f(), self))
         } else {
             Err(self)
         }
@@ -146,7 +147,7 @@ impl Into<Result<()>> for Status {
 }
 
 impl ops::Try for Status {
-    type Ok = ();
+    type Ok = Completion<()>;
     type Error = Status;
 
     fn into_result(self) -> Result<()> {

src/lib.rs

@@ -24,8 +24,6 @@
 //! therefore all the network protocols will be unavailable.
 
 #![feature(optin_builtin_traits)]
-#![feature(min_const_fn)]
-#![feature(tool_lints)]
 #![feature(try_trait)]
 #![no_std]
 // Enable some additional warnings and lints.
@@ -37,7 +35,7 @@ mod data_types;
 pub use self::data_types::{Event, Guid, Handle};
 
 mod error;
-pub use self::error::{Result, Status};
+pub use self::error::{Completion, Result, ResultExt, Status};
 
 pub mod table;
 

src/prelude.rs

@@ -2,7 +2,7 @@
 //!
 //! This includes the system table types, `Status` codes, etc.
 
-pub use crate::Status;
+pub use crate::{ResultExt, Status};
 
 // Import the basic table types.
 pub use crate::table::{boot::BootServices, runtime::RuntimeServices, SystemTable};

src/proto/console/gop.rs

@@ -24,7 +24,7 @@
 //! but in practice there might be some extra padding used for efficiency.
 
 use core::{ptr, slice};
-use crate::{Result, Status};
+use crate::{Completion, Result, Status};
 
 /// Provides access to the video hardware's frame buffer.
 ///
@@ -71,7 +71,7 @@ impl GraphicsOutput {
     }
 
     /// Returns information about all available graphics modes.
-    pub fn modes<'a>(&'a self) -> impl Iterator<Item = Mode> + 'a {
+    pub fn modes<'a>(&'a self) -> impl Iterator<Item = Completion<Mode>> + 'a {
         ModeIter {
             gop: self,
             current: 0,
@@ -397,7 +397,7 @@ struct ModeIter<'a> {
 }
 
 impl<'a> Iterator for ModeIter<'a> {
-    type Item = Mode;
+    type Item = Completion<Mode>;
 
     fn next(&mut self) -> Option<Self::Item> {
         let index = self.current;

src/proto/console/pointer/mod.rs

@@ -37,9 +37,8 @@ impl Pointer {
         let mut pointer_state = unsafe { mem::uninitialized() };
 
         match (self.get_state)(self, &mut pointer_state) {
-            Status::SUCCESS => Ok(Some(pointer_state)),
-            Status::NOT_READY => Ok(None),
-            error => Err(error),
+            Status::NOT_READY => Ok(None.into()),
+            other => other.into_with(|| Some(pointer_state)),
         }
     }
 

src/proto/console/serial.rs

@@ -1,7 +1,7 @@
 //! Abstraction over byte stream devices, also known as serial I/O devices.
 
 use bitflags::bitflags;
-use crate::{Result, Status};
+use crate::{Completion, Result, Status};
 
 /// Provides access to a serial I/O device.
 ///
@@ -83,14 +83,14 @@ impl Serial {
     /// Returns the number of bytes actually written to the device.
     /// In the case of a timeout, this number will be smaller than
     /// the buffer's size.
+    ///
+    /// Unlike UEFI, we handle timeouts as warnings, not errors
     pub fn write(&mut self, data: &[u8]) -> Result<usize> {
         let mut buffer_size = data.len();
 
-        let status = (self.write)(self, &mut buffer_size, data.as_ptr());
-
-        match status {
-            Status::SUCCESS | Status::TIMEOUT => Ok(buffer_size),
-            err => Err(err),
+        match (self.write)(self, &mut buffer_size, data.as_ptr()) {
+            s @ Status::TIMEOUT => Ok(Completion::Warning(buffer_size, s)),
+            other => other.into_with(|| buffer_size),
         }
     }
 
@@ -99,14 +99,14 @@ impl Serial {
     /// Returns the number of bytes actually read from the device.
     /// In the case of a timeout or buffer overrun, this number will be smaller
     /// than the buffer's size.
+    ///
+    /// Unlike UEFI, we handle timeouts as warnings, not errors
     pub fn read(&mut self, data: &mut [u8]) -> Result<usize> {
         let mut buffer_size = data.len();
 
-        let status = (self.read)(self, &mut buffer_size, data.as_mut_ptr());
-
-        match status {
-            Status::SUCCESS | Status::TIMEOUT => Ok(buffer_size),
-            err => Err(err),
+        match (self.read)(self, &mut buffer_size, data.as_mut_ptr()) {
+            s @ Status::TIMEOUT => Ok(Completion::Warning(buffer_size, s)),
+            other => other.into_with(|| buffer_size),
         }
     }
 

src/proto/console/text/input.rs

@@ -34,9 +34,8 @@ impl Input {
         let mut key = unsafe { mem::uninitialized() };
 
         match (self.read_key_stroke)(self, &mut key) {
-            Status::SUCCESS => Ok(Some(key)),
-            Status::NOT_READY => Ok(None),
-            error => Err(error),
+            Status::NOT_READY => Ok(None.into()),
+            other => other.into_with(|| Some(key)),
         }
     }
 

src/proto/console/text/output.rs

@@ -1,5 +1,6 @@
 use core::fmt;
-use crate::{Result, Status};
+use crate::prelude::*;
+use crate::{Completion, Result, Status};
 
 /// Interface for text-based output devices.
 ///
@@ -53,7 +54,7 @@ impl Output {
 
     /// Returns an iterator of all supported text modes.
     // TODO: fix the ugly lifetime parameter.
-    pub fn modes<'a>(&'a mut self) -> impl Iterator<Item = OutputMode> + 'a {
+    pub fn modes<'a>(&'a mut self) -> impl Iterator<Item = Completion<OutputMode>> + 'a {
         let max = self.data.max_mode;
         OutputModeIter {
             output: self,
@@ -74,8 +75,7 @@ impl Output {
     /// alternative to this method.
     fn query_mode(&self, index: i32) -> Result<(usize, usize)> {
         let (mut columns, mut rows) = (0, 0);
-        (self.query_mode)(self, index, &mut columns, &mut rows)?;
-        Ok((columns, rows))
+        (self.query_mode)(self, index, &mut columns, &mut rows).into_with(|| (columns, rows))
     }
 
     /// Sets a mode as current.
@@ -86,8 +86,8 @@ impl Output {
     /// Returns the the current text mode.
     pub fn current_mode(&self) -> Result<OutputMode> {
         let index = self.data.mode;
-        let dims = self.query_mode(index)?;
-        Ok(OutputMode { index, dims })
+        self.query_mode(index)
+            .map_inner(|dims| OutputMode { index, dims })
     }
 
     /// Make the cursor visible or invisible.
@@ -148,7 +148,9 @@ impl fmt::Write for Output {
             buf[*i] = 0;
             *i = 0;
 
-            self.output_string(buf.as_ptr()).map_err(|_| fmt::Error)
+            self.output_string(buf.as_ptr())
+                .warning_as_error()
+                .map_err(|_| fmt::Error)
         };
 
         // This closure converts a character to UCS-2 and adds it to the buffer,
@@ -217,15 +219,15 @@ struct OutputModeIter<'a> {
 }
 
 impl<'a> Iterator for OutputModeIter<'a> {
-    type Item = OutputMode;
+    type Item = Completion<OutputMode>;
 
     fn next(&mut self) -> Option<Self::Item> {
         let index = self.current;
         if index < self.max {
             self.current += 1;
 
-            if let Ok(dims) = self.output.query_mode(index) {
-                Some(OutputMode { index, dims })
+            if let Ok(dims_completion) = self.output.query_mode(index) {
+                Some(dims_completion.map(|dims| OutputMode { index, dims }))
             } else {
                 self.next()
             }