From 871c3f1f993cef8ebd0f16d1de27358e36a2ee7d Mon Sep 17 00:00:00 2001
From: andre-braga <andrebraga@google.com>
Date: Thu, 20 Jun 2024 12:23:54 +0000
Subject: [PATCH] uefi:  Make TimeError more descriptive

When returning a TimeError, it'd be helpful to specify to the user which field
is invalid so that it can be handled accordingly, or at least communicated. This
change does this by adding bool fields to the TimeError struct, representing the
validity of each field of a Time struct.
---
 uefi/CHANGELOG.md         |   2 +
 uefi/src/table/runtime.rs | 100 ++++++++++++++++++++++++++++++++------
 2 files changed, 86 insertions(+), 16 deletions(-)

diff --git a/uefi/CHANGELOG.md b/uefi/CHANGELOG.md
index aa8ed737c..1ad46a6fa 100644
--- a/uefi/CHANGELOG.md
+++ b/uefi/CHANGELOG.md
@@ -14,6 +14,8 @@
 - Added `TryFrom<&[u8]>` for `DevicePathHeader`, `DevicePathNode` and `DevicePath`.
 - Added `ByteConversionError`.
 - Re-exported `CapsuleFlags`.
+- One can now specify in `TimeError` what fields of `Time` are outside its valid
+  range. `Time::is_valid` has been updated accordingly. 
 
 ## Changed
 - `SystemTable::exit_boot_services` is now `unsafe`. See that method's
diff --git a/uefi/src/table/runtime.rs b/uefi/src/table/runtime.rs
index e61076b65..f5945ac4a 100644
--- a/uefi/src/table/runtime.rs
+++ b/uefi/src/table/runtime.rs
@@ -371,19 +371,61 @@ pub struct TimeParams {
     pub daylight: Daylight,
 }
 
-/// Error returned by [`Time`] methods if the input is outside the valid range.
-#[derive(Copy, Clone, Debug, Default, Eq, PartialEq)]
-pub struct TimeError;
+/// Error returned by [`Time`] methods. A bool value of `true` means
+/// the specified field is outside of its valid range.
+#[allow(missing_docs)]
+#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
+pub struct TimeError {
+    pub year: bool,
+    pub month: bool,
+    pub day: bool,
+    pub hour: bool,
+    pub minute: bool,
+    pub second: bool,
+    pub nanosecond: bool,
+    pub timezone: bool,
+    pub daylight: bool,
+}
+
+#[cfg(feature = "unstable")]
+impl core::error::Error for TimeError {}
 
 impl Display for TimeError {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-        write!(f, "{self:?}")
+        if self.year {
+            writeln!(f, "year not within `1900..=9999`")?;
+        }
+        if self.month {
+            writeln!(f, "month not within `1..=12")?;
+        }
+        if self.day {
+            writeln!(f, "day not within `1..=31`")?;
+        }
+        if self.hour {
+            writeln!(f, "hour not within `0..=23`")?;
+        }
+        if self.minute {
+            writeln!(f, "minute not within `0..=59`")?;
+        }
+        if self.second {
+            writeln!(f, "second not within `0..=59`")?;
+        }
+        if self.nanosecond {
+            writeln!(f, "nanosecond not within `0..=999_999_999`")?;
+        }
+        if self.timezone {
+            writeln!(
+                f,
+                "time_zone not `Time::UNSPECIFIED_TIMEZONE` nor within `-1440..=1440`"
+            )?;
+        }
+        if self.daylight {
+            writeln!(f, "unknown bits set for daylight")?;
+        }
+        Ok(())
     }
 }
 
-#[cfg(feature = "unstable")]
-impl core::error::Error for TimeError {}
-
 impl Time {
     /// Unspecified Timezone/local time.
     const UNSPECIFIED_TIMEZONE: i16 = uefi_raw::time::Time::UNSPECIFIED_TIMEZONE;
@@ -404,11 +446,8 @@ impl Time {
             daylight: params.daylight,
             pad2: 0,
         });
-        if time.is_valid() {
-            Ok(time)
-        } else {
-            Err(TimeError)
-        }
+
+        time.is_valid().map(|_| time)
     }
 
     /// Create an invalid `Time` with all fields set to zero. This can
@@ -422,10 +461,39 @@ impl Time {
         Self(uefi_raw::time::Time::invalid())
     }
 
-    /// True if all fields are within valid ranges, false otherwise.
-    #[must_use]
-    pub fn is_valid(&self) -> bool {
-        self.0.is_valid()
+    /// `Ok()` if all fields are within valid ranges, `Err(TimeError)` otherwise.
+    pub fn is_valid(&self) -> core::result::Result<(), TimeError> {
+        let mut err = TimeError::default();
+        if !(1900..=9999).contains(&self.year()) {
+            err.year = true;
+        }
+        if !(1..=12).contains(&self.month()) {
+            err.month = true;
+        }
+        if !(1..=31).contains(&self.day()) {
+            err.day = true;
+        }
+        if self.hour() > 23 {
+            err.hour = true;
+        }
+        if self.minute() > 59 {
+            err.minute = true;
+        }
+        if self.second() > 59 {
+            err.second = true;
+        }
+        if self.nanosecond() > 999_999_999 {
+            err.nanosecond = true;
+        }
+        if self.time_zone().is_some() && !((-1440..=1440).contains(&self.time_zone().unwrap())) {
+            err.timezone = true;
+        }
+        // All fields are false, i.e., within their valid range.
+        if err == TimeError::default() {
+            Ok(())
+        } else {
+            Err(err)
+        }
     }
 
     /// Query the year.