Merge pull request #556 from phip1611/read-entry-return-code

documentation and code improvements for Status, Error, and read()

Author: phip1611

uefi/src/data_types/strs.rs

@@ -337,7 +337,7 @@ impl CStr16 {
     }
 
     /// Writes each [`Char16`] as a [`char`] (4 bytes long in Rust language) into the buffer.
-    /// It is up the the implementer of [`core::fmt::Write`] to convert the char to a string
+    /// It is up to the implementer of [`core::fmt::Write`] to convert the char to a string
     /// with proper encoding/charset. For example, in the case of [`alloc::string::String`]
     /// all Rust chars (UTF-32) get converted to UTF-8.
     ///

uefi/src/proto/console/text/output.rs

@@ -110,7 +110,7 @@ impl<'boot> Output<'boot> {
         (self.query_mode)(self, index, &mut columns, &mut rows).into_with_val(|| (columns, rows))
     }
 
-    /// Returns the the current text mode.
+    /// Returns the current text mode.
     pub fn current_mode(&self) -> Result<Option<OutputMode>> {
         match self.data.mode {
             -1 => Ok(None),

uefi/src/proto/media/file/dir.rs

@@ -50,11 +50,13 @@ impl Directory {
         FileInfo::assert_aligned(buffer);
 
         // Read the directory entry into the aligned storage
-        self.0.read(buffer).map(|size| {
-            if size != 0 {
-                unsafe { Some(FileInfo::from_uefi(buffer.as_mut_ptr().cast::<c_void>())) }
-            } else {
+        self.0.read(buffer).map(|read_bytes| {
+            // 0 read bytes signals that the last directory entry was read
+            let last_directory_entry_read = read_bytes == 0;
+            if last_directory_entry_read {
                 None
+            } else {
+                unsafe { Some(FileInfo::from_uefi(buffer.as_mut_ptr().cast::<c_void>())) }
             }
         })
     }

uefi/src/proto/media/file/mod.rs

@@ -182,7 +182,7 @@ pub trait File: Sized {
     fn is_directory(&self) -> Result<bool>;
 }
 
-// Internal File helper methods to access the funciton pointer table.
+// Internal File helper methods to access the function pointer table.
 trait FileInternal: File {
     fn imp(&mut self) -> &mut FileImpl {
         unsafe { &mut *self.handle().0 }
@@ -286,6 +286,19 @@ pub(super) struct FileImpl {
     ) -> Status,
     close: extern "efiapi" fn(this: &mut FileImpl) -> Status,
     delete: extern "efiapi" fn(this: &mut FileImpl) -> Status,
+    /// # Read from Regular Files
+    /// If `self` is not a directory, the function reads the requested number of bytes from the file
+    /// at the file’s current position and returns them in `buffer`. If the read goes beyond the end
+    /// of the file, the read length is truncated to the end of the file. The file’s current
+    /// position is increased by the number of bytes returned.
+    ///
+    /// # Read from Directory
+    /// If `self` is a directory, the function reads the directory entry at the file’s current
+    /// position and returns the entry in `buffer`. If the `buffer` is not large enough to hold the
+    /// current directory entry, then `EFI_BUFFER_TOO_SMALL` is returned and the current file
+    /// position is not updated. `buffer_size` is set to be the size of the buffer needed to read
+    /// the entry. On success, the current position is updated to the next directory entry. If there
+    /// are no more directory entries, the read returns a zero-length buffer.
     read: unsafe extern "efiapi" fn(
         this: &mut FileImpl,
         buffer_size: &mut usize,

uefi/src/proto/media/file/regular.rs

@@ -21,7 +21,7 @@ impl RegularFile {
         Self(handle)
     }
 
-    /// Read data from file
+    /// Read data from file.
     ///
     /// Try to read as much as possible into `buffer`. Returns the number of bytes that were
     /// actually read.
@@ -38,10 +38,15 @@ impl RegularFile {
     ///                                      and the required buffer size is provided as output.
     pub fn read(&mut self, buffer: &mut [u8]) -> Result<usize, Option<usize>> {
         let mut buffer_size = buffer.len();
-        unsafe { (self.imp().read)(self.imp(), &mut buffer_size, buffer.as_mut_ptr()) }.into_with(
+        let status =
+            unsafe { (self.imp().read)(self.imp(), &mut buffer_size, buffer.as_mut_ptr()) };
+
+        status.into_with(
             || buffer_size,
             |s| {
                 if s == Status::BUFFER_TOO_SMALL {
+                    // `buffer_size` was updated to the required buffer size by the underlying read
+                    // function.
                     Some(buffer_size)
                 } else {
                     None

uefi/src/result/mod.rs

@@ -14,8 +14,8 @@ pub use self::status::Status;
 /// Almost all UEFI operations provide a status code as an output which
 /// indicates either success, a warning, or an error. This type alias maps
 /// [`Status::SUCCESS`] to the `Ok` variant (with optional `Output` data), and
-/// maps both warning and error statuses to the `Err` variant (with optional
-/// `ErrData`).
+/// maps both warning and error statuses to the `Err` variant of type [`Error`],
+/// which may carry optional inner `ErrData`.
 ///
 /// Warnings are treated as errors by default because they generally indicate
 /// an abnormal situation.

uefi/src/result/status.rs

@@ -12,6 +12,12 @@ newtype_enum! {
 /// enum, as injecting an unknown value in a Rust enum is undefined behaviour.
 ///
 /// For lack of a better option, we therefore model them as a newtype of usize.
+///
+/// For a convenient integration into the Rust ecosystem, there are multiple
+/// factory methods to convert a Status into a [`uefi::Result`]:
+/// - [`Status::into_with`]
+/// - [`Status::into_with_val`]
+/// - [`Status::into_with_err`]
 #[must_use]
 pub enum Status: usize => {
     /// The operation completed successfully.
@@ -122,7 +128,10 @@ impl Status {
         self.0 & ERROR_BIT != 0
     }
 
-    /// Converts this status code into a result with a given value.
+    /// Converts this status code into a [`uefi::Result`] with a given `Ok` value.
+    ///
+    /// If the status does not indicate success, the status representing the specific error
+    /// code is embedded into the `Err` variant of type [`uefi::Error`].
     #[inline]
     pub fn into_with_val<T>(self, val: impl FnOnce() -> T) -> Result<T, ()> {
         if self.is_success() {
@@ -132,7 +141,10 @@ impl Status {
         }
     }
 
-    /// Converts this status code into a result with a given error payload
+    /// Converts this status code into a [`uefi::Result`] with a given `Err` payload.
+    ///
+    /// If the status does not indicate success, the status representing the specific error
+    /// code is embedded into the `Err` variant of type [`uefi::Error`].
     #[inline]
     pub fn into_with_err<ErrData: Debug>(
         self,
@@ -145,7 +157,10 @@ impl Status {
         }
     }
 
-    /// Convert this status code into a result with a given value and error payload
+    /// Convert this status code into a result with a given `Ok` value and `Err` payload.
+    ///
+    /// If the status does not indicate success, the status representing the specific error
+    /// code is embedded into the `Err` variant of type [`uefi::Error`].
     #[inline]
     pub fn into_with<T, ErrData: Debug>(
         self,

xtask/src/device_path/node.rs

@@ -165,7 +165,7 @@ impl Node {
                     }
 
                     // It's not trivial to nicely format the DST data since
-                    // the the slice might be unaligned. Treat it as a byte
+                    // the slice might be unaligned. Treat it as a byte
                     // slice instead.
                     quote!({
                         let ptr = addr_of!(#field_val);