Skip to content
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

feat: Add associated constants UTIME_OMIT UTIME_NOW to TimeSpec #1879

Merged
merged 5 commits into from
Dec 10, 2023

Conversation

SteveLauC
Copy link
Member

Since we can add some breaking changes in the following 0.27 release, I made this PR.

What this PR does

  1. Change the type of atime, mtime arguments of utimenstat(2) and futimens(2) to Option<&TimeSpec>, when set to None, the corresponding timestamp remains unchanged.
  2. Add two tests for feature 1
  3. Re-export libc::UTIME_NOW in nix::sys::stat module so that users can directly use it when they want to update timestamps to the special value Now
  4. Change the URL of gethostname(2) in README.md to the UNIX one (from Open Group)

Fixes #1834

cc @sargun

@SteveLauC
Copy link
Member Author

redox does not support UTIME_NOW and UIME_OMIT, I will file an issue in their GitLab repo to ask if they have any plan to support this feature.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Do you think it would be more natural to change the arguments into an enum like this:

pub enum UtimeSpec {
    Omit,
    Now,
    Set(TimeSpec)
}

Or would that just be unnecessarily complicated?

src/sys/stat.rs Outdated
Comment on lines 414 to 421
libc::timespec {
tv_sec: 0,
tv_nsec: libc::UTIME_OMIT,
},
libc::timespec {
tv_sec: 0,
tv_nsec: libc::UTIME_OMIT,
},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
libc::timespec {
tv_sec: 0,
tv_nsec: libc::UTIME_OMIT,
},
libc::timespec {
tv_sec: 0,
tv_nsec: libc::UTIME_OMIT,
},
libc::timespec {
tv_sec: 0,
tv_nsec: libc::UTIME_OMIT,
..
},
libc::timespec {
tv_sec: 0,
tv_nsec: libc::UTIME_OMIT,
..
},

See #1886 for the reason why.

@SteveLauC
Copy link
Member Author

Do you think it would be more natural to change the arguments into an enum like this:

pub enum UtimeSpec {
    Omit,
    Now,
    Set(TimeSpec)
}

Or would that just be unnecessarily complicated?

Honestly, this interface looks great, a better way to go!

src/sys/stat.rs Outdated Show resolved Hide resolved
src/sys/stat.rs Outdated Show resolved Hide resolved
src/sys/stat.rs Outdated Show resolved Hide resolved
src/sys/time.rs Outdated Show resolved Hide resolved
src/sys/stat.rs Outdated Show resolved Hide resolved
@SteveLauC
Copy link
Member Author

SteveLauC commented Dec 10, 2022

redox does not support UTIME_NOW and UIME_OMIT, I will file an issue in their GitLab repo to ask if they have any plan to support this feature.

Issue filed.


This PR involves two syscalls:

  1. utimensat(2)
  2. futimens(2)

Redox does not support utimensat(2).

For futimens(2), since redox does not have UTIME_NOW and UTIME_OMIT, I leave the previous implementation unchanged for it:

#[inline]
#[cfg(target_os = "redox")]
pub fn futimens(fd: RawFd, atime: &TimeSpec, mtime: &TimeSpec) -> Result<()> {
    let times: [libc::timespec; 2] = [*atime.as_ref(), *mtime.as_ref()];
    let res = unsafe { libc::futimens(fd, &times[0]) };

CHANGELOG.md entry added.

@SteveLauC SteveLauC changed the title feat(#1384): change timestamps args of utimensat and futimens to be o… feat(#1384): make time args of utimensat and futimens optional Dec 10, 2022
@SteveLauC SteveLauC changed the title feat(#1384): make time args of utimensat and futimens optional refactor(#1384): a new TimestampSpec type for the time args of utimensat and futimens Dec 10, 2022
@SteveLauC SteveLauC changed the title refactor(#1384): a new TimestampSpec type for the time args of utimensat and futimens refactor(#1834): a new TimestampSpec type for the time args of utimensat and futimens Dec 10, 2022
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

This looks good. But before we merge it, I just have to ask a question: does the TimestampSpec abstraction create too much overhead? I think it's nice and ergonomic. But a zero-cost alternative would simply be to leave the old interface as it was, and add constants like this:

pub const UTIME_OMIT: libc::timespec {tv_sec: 0, tv_nsec: libc::UTIME_OMIT}

that the user could pass in to futimens.

src/sys/stat.rs Outdated Show resolved Hide resolved
@SteveLauC
Copy link
Member Author

I just have to ask a question: does the TimestampSpec abstraction create too much overhead?

Let's compare these two interfaces:

NEW interface
// To leave timestamps unchanged
futimens(&file, &TimestampSpec::Omit, &TimestampSpec::Omit).unwrap();
// To update timestamps to NOW
futimens(&file, &TimestampSpec::Now, &TimestampSpec::Now).unwrap();
// To set timestamps to specific values, e.g., UNIX epoch
let timespec = TimeSpec::zero();
futimens(
    &file,
    &TimestampSpec::Set(timespec),
    &TimestampSpec::Set(timespec),
)
.unwrap();
OLD interface, with UTIME_OMIT and UTIME_NOW defined:
// To leave timestamps unchanged
futimens(
    &file,
    &TimeSpec::from_timespec(UTIME_OMIT),
    &TimeSpec::from_timespec(UTIME_OMIT),
)
.unwrap();
// To update timestamps to NOW
futimens(
    &file,
    &TimeSpec::from_timespec(UTIME_NOW),
    &TimeSpec::from_timespec(UTIME_NOW),
)
.unwrap();
// To set timestamps to specific values, e.g., UNIX epoch
let timespec = TimeSpec::zero();
futimens(&file, &timespec, &timespec).unwrap();

IMO, the new interface looks pretty concise and expressive. The old interface, if we define UTIME_OMIT and UTIME_NOW like this:

pub const UTIME_OMIT: TimeSpec = TimeSpec::new(0, libc::UTIME_OMIT);
pub const UTIME_NOW: TimeSpec = TimeSpec::new(0, libc::UTIME_NOW);

Then it could be more concise than the new one, but less expressive IMO:

futimens(&file, &UTIME_OMIT, &UTIME_OMIT).unwrap();
futimens(&file, &UTIME_NOW, &UTIME_NOW).unwrap();

@SteveLauC
Copy link
Member Author

Update on the that Redox (relibc) issue, reply from developers of Redox:

Since they're part of POSIX and don't seem to conflict with any core redox functionality, yes.

@tertsdiepraam
Copy link
Contributor

I just ran into this issue, so I'm glad to see it's being worked on :)

A third option for the API might be to define associated constants for TimeSpec, which I think would be zero-cost, fairly intuitive and a non-breaking change:

impl TimeSpec {
    pub const NOW: TimeSpec = TimeSpec::new(0, libc::UTIME_NOW);
    pub const OMIT: TimeSpec = TimeSpec::new(0, libc::UTIME_OMIT);
}

It would also be more self-documenting than reexporting UTIME_NOW/OMIT, because it would show up in the docs for TimeSpec.

It would look like this when used:

// To leave timestamps unchanged
futimens(&file, &TimeSpec::OMIT, &TimeSpec::OMIT).unwrap();
// To update timestamps to NOW
futimens(&file, &TimeSpec::NOW, &TimeSpec::NOW).unwrap();
// To set timestamps to specific values, e.g., UNIX epoch
let timespec = TimeSpec::zero();
futimens(
    &file,
    &timespec,
    &timespec,
)
.unwrap();

@SteveLauC
Copy link
Member Author

SteveLauC commented Dec 9, 2023

Revisiting this PR, I think defining 2 types for a function isn't quite suitable, we should define 2 associated constants for them

BTW, this is a duplicate PR of #1103, I am sorry that I didn't notice it while opening this PR

Update: the author of #1103 has removed his fork, so I closed that PR

@SteveLauC SteveLauC changed the title refactor(#1834): a new TimestampSpec type for the time args of utimensat and futimens feat: Add associated constants UTIME_OMIT UTIME_NOW for TimeSpec Dec 9, 2023
@SteveLauC SteveLauC changed the title feat: Add associated constants UTIME_OMIT UTIME_NOW for TimeSpec feat: Add associated constants UTIME_OMIT UTIME_NOW to TimeSpec Dec 9, 2023
@@ -376,6 +376,9 @@ pub fn lutimes<P: ?Sized + NixPath>(

/// Change the access and modification times of the file specified by a file descriptor.
///
/// If you want to set the timestamp to now, use `TimeSpec::UTIME_NOW`. Use
Copy link
Member Author

Choose a reason for hiding this comment

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

Cannot use "[`TimeSpec::UTIME_NOW`]" because redox does not have it

@SteveLauC SteveLauC added this pull request to the merge queue Dec 10, 2023
Merged via the queue into nix-rust:master with commit 1c189d9 Dec 10, 2023
35 checks passed
@SteveLauC SteveLauC deleted the utimensat-futimens branch December 10, 2023 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the doc for utimensat(3) and futimens(3)
3 participants