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: allow vfs implementations to specify m/b/a-times in file metadata #63

Merged
merged 14 commits into from
Mar 9, 2024

Conversation

kartonrad
Copy link
Contributor

This small change adds the file modification birth and access times to the VfsMetadata struct.

These times are encapsulated in Option<>, as they are not guaranteed to be available even in std::fs::metadata.

I have implemented the metadata timestamps for PhysicalFS, though it may be possible to support them for the in memory database as well.

Motivation

I am planning on writing a minimal vfs implementation for MTS (Media Transfer Protocol).
I don't know yet if this is going to work, especially since there are few crates for MTS - but maybe i can make it work.

In the final product i will need access to the mtime field.

@kartonrad
Copy link
Contributor Author

Nooo i forgot to run cargo fmt
Maybe tomorrow

@manuel-woelker
Copy link
Owner

Hi, thanks for the PR! Definitely useful!
I think adding some unit tests for the time fields is probably a good idea, even though the implementation is fairly simple.
Having support for timestamp information in MemoryFS would be quite neat as well!

@kartonrad
Copy link
Contributor Author

kartonrad commented Feb 24, 2024

I think adding some unit tests for the time fields is probably a good idea, even though the implementation is fairly simple.

Good Idea! I don't know if we should necessarily expect ModifyTimes to update when we write to a file - maybe that isnt guaranteed on all operating systems...
Do you have an idea of how to test that?

Having support for timestamp information in MemoryFS would be quite neat as well!

I just looked into it, and realized the trait definition immutably borrows the Filesystem
I could use a ref-cel i guess - what do you think

@manuel-woelker
Copy link
Owner

Build looks good now 👍

Yeah, a lot of systems also mount with noatime for performance reasons.
I think the best approach might be to set the times manually via the "normal" std::fs mechanisms (on systems that support it) and then verify that the VFS abstraction returns the correct values.

It might make sense to have a way to modify these times as well, that would certainly make testing easier.

Regarding the MemoryFS implementation, I think this should work without refcell. AsyncMemoryFile would need those additional fields to track the values, and then copy them out in metadata(). But it also might make sense to tackle that in a separate PR.

@kartonrad
Copy link
Contributor Author

kartonrad commented Feb 27, 2024

The diff has grown 😯

I've added your suggestions to the non-async version
Namely:

  • The optional set_creation/modification/access_time functions (path.rs and filesystem.rs)
    • should these remain optional? or do we force downstream libraries to think about this issue
  • Implementations for physical and memory, as well as redirections in altroot and overlay fs
  • Tests in the test_vfs!() macro, that only run fully if the call to set_..._time doesnt return VfsErrorKind::NotSupported
    • do these tests meet your standard?

@kartonrad
Copy link
Contributor Author

kartonrad commented Feb 27, 2024

Regarding the Pipeline: MSRV would now be 1.75.0
image

@kartonrad
Copy link
Contributor Author

i could certaily do the work to implement this in async too sometime...

@kartonrad
Copy link
Contributor Author

I have added the neccesary tests to async_vfs
I have added the needed methods in path.rs, and redirected the calls in overlay and altroot

However i have not been able to find a function in async_std that works like the new std::fs::File::set_times
And i have not implemented the behaviour for async memory.rs

So the tests right now are all passing, because they recieve VfsErrorKind::NotSupported

@manuel-woelker
Copy link
Owner

Thanks for the updates! I will take a closer look later this week.

Copy link
Owner

@manuel-woelker manuel-woelker left a comment

Choose a reason for hiding this comment

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

Looking good, some small suggestions.

The merge conflicts with master only touch the imports and should be fairly simple to resolve.

If you have any questions, let me know!

Cargo.toml Outdated Show resolved Hide resolved
src/impls/embedded.rs Outdated Show resolved Hide resolved
src/impls/memory.rs Outdated Show resolved Hide resolved
src/impls/memory.rs Show resolved Hide resolved
src/test_macros.rs Outdated Show resolved Hide resolved
@kartonrad
Copy link
Contributor Author

Feel free to resolve the threads that i have fixed.

@manuel-woelker manuel-woelker merged commit dca06ac into manuel-woelker:master Mar 9, 2024
4 checks passed
@manuel-woelker
Copy link
Owner

Merged! Thanks again for the great work, especially getting all the finer details sorted out 👍

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.

2 participants