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

lib/vfscore: Accept negative timestamps #991

Closed
wants to merge 1 commit into from

Conversation

andreittr
Copy link
Contributor

Description of changes

Previously vfs code would reject timestamps that are more than 1 second before the epoch as invalid. This is wrong; unix time is defined as a signed offset and as such can and must support dates before the epoch. This change relaxes the overly-strict validity check, allowing the full range of valid file timestamps to be used.

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): N/A
  • Platform(s): N/A
  • Application(s): N/A

Additional configuration

Test with:

struct utimbuf t = {-1, -1};
int r = utime("file", &t);
if (r) perror("Bug in utime");

@andreittr andreittr requested a review from a team as a code owner July 24, 2023 21:01
@razvand razvand self-assigned this Aug 2, 2023
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Aug 2, 2023
@razvand razvand added area/lib Internal Unikraft Microlibrary kind/quick-fix Issue is a quick fix lib/vfscore VFS Core Interface lang/c Issues or PRs to do with C/C++ labels Aug 5, 2023
@razvand razvand removed the request for review from a team August 5, 2023 05:36
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

@andreittr the changes make sense, but shouldn't they be made in is_timeval_valid too (since that is the function that is eventually called by the utime syscall)?

Previously vfs code would reject timestamps that are more than 1 second
before the epoch as invalid. This is wrong; unix time is defined as a
signed offset and as such can and must support dates before the epoch.
This change relaxes the overly-strict validity check, allowing the full
range of valid file timestamps to be used.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@andreittr
Copy link
Contributor Author

@andreittr the changes make sense, but shouldn't they be made in is_timeval_valid too (since that is the function that is eventually called by the utime syscall)?

Good catch, thanks! Updated with fix.

@unikraft-bot
Copy link
Member

Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA commit checkpatch
9f11dc8 lib/vfscore: Accept negative timestamps

@StefanJum StefanJum self-requested a review August 7, 2023 15:23
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

All good, thank you.
Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 9, 2023
@andreittr andreittr deleted the ttr/timespec-neg branch August 9, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary ci/merged Merged by CI kind/quick-fix Issue is a quick fix lang/c Issues or PRs to do with C/C++ lib/vfscore VFS Core Interface
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

4 participants