Skip to content

uefi: fs: Add file times plumbing #138918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ayush1325
Copy link
Contributor

  • Add plumbing to allow conversions to and from UEFI Time to Rust SystemTime.
  • Also add FileTimes implementation.

cc @nicholasbishop

r? @petrochenkov

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 25, 2025
@petrochenkov
Copy link
Contributor

(I'll initially mark these PRs as waiting on author in the sense that, that they are waiting for the author to summon @nicholasbishop and get a review. It can be marked as ready when there's a review.)
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 25, 2025
- Add plumbing to allow conversions to and from UEFI Time to Rust
  SystemTime.
- Also add FileTimes implementation.

Signed-off-by: Ayush Singh <[email protected]>
@Ayush1325
Copy link
Contributor Author

@rustbot label +O-UEFI

@rustbot rustbot added the O-UEFI UEFI label Apr 11, 2025
Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried about the edge cases here. It's not that uncommon to end up with a system time of 1970-1-1 if the devices clock is uninitialised. If the current timezone is positive, this might lead to a panic when converting it to a UNIX time. Considering the insanely large bounds afforded by secs being 64-bit, using a signed integer in the conversion algorithm would allow converting dates before the UNIX epoch as well.

Aside from that, the checked methods on SystemTime operate on the assumption that the operation will fail if the time is not representable in the operating system format – but this is currently not the case: UEFI allows times from the year 1900 up to and including the year 9999, whereas a Duration since the UNIX epoch allows representing times from 1970 up to the heat death of the universe, but not before that. I think it would be better to use the UEFI time representation for SystemTime and only convert it into a Duration for the addition/subtraction operations.

let secs = if timezone == r_efi::efi::UNSPECIFIED_TIMEZONE {
dur.as_secs()
} else {
dur.as_secs().checked_add_signed(-timezone as i64).unwrap()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use subtraction here, just like the formula in the UEFI spec.

Suggested change
dur.as_secs().checked_add_signed(-timezone as i64).unwrap()
dur.as_secs().checked_sub_signed(timezone as i64).expect("times should be representable as local UEFI times")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked_sub_signed is currently unstable. I can add the feature if that is fine though.

let remaining_secs = secs % SECS_IN_DAY;
let z = days + 719468;
let era = z / 146097;
let doe = z - (era * 146097);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clear by operator precedence, and the source doesn't use parenthesis here either.

Suggested change
let doe = z - (era * 146097);
let doe = z - era * 146097;

@Ayush1325
Copy link
Contributor Author

I'm a bit worried about the edge cases here. It's not that uncommon to end up with a system time of 1970-1-1 if the devices clock is uninitialised. If the current timezone is positive, this might lead to a panic when converting it to a UNIX time. Considering the insanely large bounds afforded by secs being 64-bit, using a signed integer in the conversion algorithm would allow converting dates before the UNIX epoch as well.

Aside from that, the checked methods on SystemTime operate on the assumption that the operation will fail if the time is not representable in the operating system format – but this is currently not the case: UEFI allows times from the year 1900 up to and including the year 9999, whereas a Duration since the UNIX epoch allows representing times from 1970 up to the heat death of the universe, but not before that. I think it would be better to use the UEFI time representation for SystemTime and only convert it into a Duration for the addition/subtraction operations.

Yes, I did come to the same conclusion. I think the reason I initially went with duration was to have a simple implementation for all the function implemented on SystemTime (since all the results there are in Duration). I will try representing SystemTime using UEFI Time internally and see how it looks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-UEFI UEFI S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants