Improve Time struct

* Add a `TimeParams` struct with public fields corresponding to all the
  non-padding fields of `Time`, and change `Time::new` to take that as
  input. That improves callsites of `Time::new` so that you can see what
  the numeric values correspond to. See `known_disk.rs` for an example.

* Add `Time::invalid` for use with `File::set_info`. When setting file
  info, a fully-zero-time indicates the attribute should not be
  updated. I think ideally we'd have `Time` always represent a valid
  value, and use `Option<Time>::None` to represent an invalid time
  similar to what we do with `Handle`s, but I don't think Rust currently
  allows us to indicate that all-zero-Time can be used for the
  Option::None niche.

* Add `Time::is_invalid` to check that all fields are in the valid range.

* Update Time's docstring to be more general. Although `EFI_TIME` in the
  spec says it's for the current time, it's used elsewhere too (like
  `EFI_FILE_INFO`).

* Fix the docstring for `Daylight`, which was accidentally copied from
  `MemoryAttribute`.

Author: nicholasbishop

CHANGELOG.md

@@ -5,6 +5,12 @@
 ### Added
 
 - Added `FileHandle::into_directory` and `FileHandle::into_regular_file`.
+- Added `TimeParams`, `Time::invalid`, and `Time::is_invalid`.
+
+### Changed
+
+- `Time::new` now takes a single `TimeParams` argument so that date and
+  time fields can be explicitly named at the call site.
 
 ## uefi-macros - [Unreleased]
 

src/proto/media/file/info.rs

@@ -375,6 +375,7 @@ impl FileProtocolInfo for FileSystemVolumeLabel {}
 mod tests {
     use super::*;
     use crate::alloc_api::vec;
+    use crate::table::runtime::TimeParams;
     use crate::table::runtime::{Daylight, Time};
     use crate::CString16;
 
@@ -394,9 +395,20 @@ mod tests {
 
         let file_size = 123;
         let physical_size = 456;
-        let create_time = Time::new(1970, 1, 1, 0, 0, 0, 0, 0, Daylight::IN_DAYLIGHT);
-        let last_access_time = Time::new(1971, 1, 1, 0, 0, 0, 0, 0, Daylight::IN_DAYLIGHT);
-        let modification_time = Time::new(1972, 1, 1, 0, 0, 0, 0, 0, Daylight::IN_DAYLIGHT);
+        let tp = TimeParams {
+            year: 1970,
+            month: 1,
+            day: 1,
+            hour: 0,
+            minute: 0,
+            second: 0,
+            nanosecond: 0,
+            time_zone: None,
+            daylight: Daylight::IN_DAYLIGHT,
+        };
+        let create_time = Time::new(tp).unwrap();
+        let last_access_time = Time::new(TimeParams { year: 1971, ..tp }).unwrap();
+        let modification_time = Time::new(TimeParams { year: 1972, ..tp }).unwrap();
         let attribute = FileAttribute::READ_ONLY;
         let name = CString16::try_from("test_name").unwrap();
         let info = FileInfo::new(

src/table/runtime.rs

@@ -263,7 +263,7 @@ impl Debug for RuntimeServices {
     }
 }
 
-/// The current time information
+/// Date and time representation.
 #[derive(Copy, Clone)]
 #[repr(C)]
 pub struct Time {
@@ -280,92 +280,151 @@ pub struct Time {
     _pad2: u8,
 }
 
+/// Input parameters for [`Time::new`].
+#[derive(Copy, Clone)]
+pub struct TimeParams {
+    /// Year in the range `1900..=9999`.
+    pub year: u16,
+
+    /// Month in the range `1..=12`.
+    pub month: u8,
+
+    /// Day in the range `1..=31`.
+    pub day: u8,
+
+    /// Hour in the range `0.=23`.
+    pub hour: u8,
+
+    /// Minute in the range `0..=59`.
+    pub minute: u8,
+
+    /// Second in the range `0..=59`.
+    pub second: u8,
+
+    /// Fraction of a second represented as nanoseconds in the range
+    /// `0..=999_999_999`.
+    pub nanosecond: u32,
+
+    /// Offset in minutes from UTC in the range `-1440..=1440`, or
+    /// local time if `None`.
+    pub time_zone: Option<i16>,
+
+    /// Daylight savings time information.
+    pub daylight: Daylight,
+}
+
 bitflags! {
-    /// Flags describing the capabilities of a memory range.
+    /// A bitmask containing daylight savings time information.
     pub struct Daylight: u8 {
-        /// Time is affected by daylight savings time
+        /// Time is affected by daylight savings time.
         const ADJUST_DAYLIGHT = 0x01;
-        /// Time has been adjusted for daylight savings time
+        /// Time has been adjusted for daylight savings time.
         const IN_DAYLIGHT = 0x02;
     }
 }
 
+/// Error returned by [`Time`] methods if the input is outside the valid range.
+#[derive(Copy, Clone, Debug, Default, Eq, PartialEq)]
+pub struct TimeError;
+
 impl Time {
     /// Unspecified Timezone/local time.
     const UNSPECIFIED_TIMEZONE: i16 = 0x07ff;
 
-    /// Build an UEFI time struct
-    #[allow(clippy::too_many_arguments)]
-    pub fn new(
-        year: u16,
-        month: u8,
-        day: u8,
-        hour: u8,
-        minute: u8,
-        second: u8,
-        nanosecond: u32,
-        time_zone: i16,
-        daylight: Daylight,
-    ) -> Self {
-        assert!((1900..=9999).contains(&year));
-        assert!((1..=12).contains(&month));
-        assert!((1..=31).contains(&day));
-        assert!(hour <= 23);
-        assert!(minute <= 59);
-        assert!(second <= 59);
-        assert!(nanosecond <= 999_999_999);
-        assert!((time_zone >= -1440 && time_zone <= 1440) || time_zone == 2047);
+    /// Create a `Time` value. If a field is not in the valid range,
+    /// [`TimeError`] is returned.
+    pub fn new(params: TimeParams) -> core::result::Result<Self, TimeError> {
+        let time = Self {
+            year: params.year,
+            month: params.month,
+            day: params.day,
+            hour: params.hour,
+            minute: params.minute,
+            second: params.second,
+            _pad1: 0,
+            nanosecond: params.nanosecond,
+            time_zone: params.time_zone.unwrap_or(Self::UNSPECIFIED_TIMEZONE),
+            daylight: params.daylight,
+            _pad2: 0,
+        };
+        if time.is_valid() {
+            Ok(time)
+        } else {
+            Err(TimeError)
+        }
+    }
+
+    /// Create an invalid `Time` with all fields set to zero. This can
+    /// be used with [`FileInfo`] to indicate a field should not be
+    /// updated when calling [`File::set_info`].
+    ///
+    /// [`FileInfo`]: uefi::proto::media::file::FileInfo
+    /// [`File::set_info`]: uefi::proto::media::file::File::set_info
+    pub fn invalid() -> Self {
         Self {
-            year,
-            month,
-            day,
-            hour,
-            minute,
-            second,
+            year: 0,
+            month: 0,
+            day: 0,
+            hour: 0,
+            minute: 0,
+            second: 0,
             _pad1: 0,
-            nanosecond,
-            time_zone,
-            daylight,
+            nanosecond: 0,
+            time_zone: 0,
+            daylight: Daylight::empty(),
             _pad2: 0,
         }
     }
 
-    /// Query the year
+    /// True if all fields are within valid ranges, false otherwise.
+    pub fn is_valid(&self) -> bool {
+        (1900..=9999).contains(&self.year)
+            && (1..=12).contains(&self.month)
+            && (1..=31).contains(&self.day)
+            && self.hour <= 23
+            && self.minute <= 59
+            && self.second <= 59
+            && self.nanosecond <= 999_999_999
+            && ((-1440..=1440).contains(&self.time_zone)
+                || self.time_zone == Self::UNSPECIFIED_TIMEZONE)
+    }
+
+    /// Query the year.
     pub fn year(&self) -> u16 {
         self.year
     }
 
-    /// Query the month
+    /// Query the month.
     pub fn month(&self) -> u8 {
         self.month
     }
 
-    /// Query the day
+    /// Query the day.
     pub fn day(&self) -> u8 {
         self.day
     }
 
-    /// Query the hour
+    /// Query the hour.
     pub fn hour(&self) -> u8 {
         self.hour
     }
 
-    /// Query the minute
+    /// Query the minute.
     pub fn minute(&self) -> u8 {
         self.minute
     }
 
-    /// Query the second
+    /// Query the second.
     pub fn second(&self) -> u8 {
         self.second
     }
 
-    /// Query the nanosecond
+    /// Query the nanosecond.
     pub fn nanosecond(&self) -> u32 {
         self.nanosecond
     }
 
-    /// Query the time offset in minutes from UTC, or None if using local time
+    /// Query the time offset in minutes from UTC, or None if using local time.
     pub fn time_zone(&self) -> Option<i16> {
         if self.time_zone == 2047 {
             None
@@ -374,7 +433,7 @@ impl Time {
         }
     }
 
-    /// Query the daylight savings time information
+    /// Query the daylight savings time information.
     pub fn daylight(&self) -> Daylight {
         self.daylight
     }

uefi-test-runner/src/proto/media/known_disk.rs

@@ -5,7 +5,7 @@ use uefi::proto::media::file::{
 };
 use uefi::proto::media::fs::SimpleFileSystem;
 use uefi::table::boot::{OpenProtocolAttributes, OpenProtocolParams};
-use uefi::table::runtime::{Daylight, Time};
+use uefi::table::runtime::{Daylight, Time, TimeParams};
 use uefi::CString16;
 
 /// Test directory entry iteration.
@@ -78,17 +78,37 @@ fn test_existing_file(directory: &mut Directory) {
     let info = file.get_info::<FileInfo>(&mut info_buffer).unwrap();
     assert_eq!(info.file_size(), 15);
     assert_eq!(info.physical_size(), 512);
-    assert_eq!(
-        *info.create_time(),
-        Time::new(2000, 1, 24, 0, 0, 0, 0, 2047, Daylight::empty())
-    );
+    let tp = TimeParams {
+        year: 2000,
+        month: 1,
+        day: 24,
+        hour: 0,
+        minute: 0,
+        second: 0,
+        nanosecond: 0,
+        time_zone: None,
+        daylight: Daylight::empty(),
+    };
+    assert_eq!(*info.create_time(), Time::new(tp).unwrap());
     assert_eq!(
         *info.last_access_time(),
-        Time::new(2001, 2, 25, 0, 0, 0, 0, 2047, Daylight::empty())
+        Time::new(TimeParams {
+            year: 2001,
+            month: 2,
+            day: 25,
+            ..tp
+        })
+        .unwrap()
     );
     assert_eq!(
         *info.modification_time(),
-        Time::new(2002, 3, 26, 0, 0, 0, 0, 2047, Daylight::empty())
+        Time::new(TimeParams {
+            year: 2002,
+            month: 3,
+            day: 26,
+            ..tp
+        })
+        .unwrap()
     );
     assert_eq!(info.attribute(), FileAttribute::empty());
     assert_eq!(