-
Notifications
You must be signed in to change notification settings - Fork 38
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
Implement node deletions #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what happens if I try to delete an explicit mapping? Does it crash?
src/nodes/mod.rs
Outdated
let result = try_path(path, |p| | ||
unistd::fchownat(None, p, uid, gid, unistd::FchownatFlags::NoFollowSymlink)); | ||
if result.is_ok() { | ||
attr.uid = uid.map_or_else(|| attr.uid, u32::from); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does map_or
work here? attr.uid is probably Copy, so it shouldn't cause an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it works. Done.
src/nodes/mod.rs
Outdated
let atime = atime.unwrap_or_else(|| conv::timespec_to_timeval(attr.atime)); | ||
let mtime = mtime.unwrap_or_else(|| conv::timespec_to_timeval(attr.mtime)); | ||
if attr.kind == fuse::FileType::Symlink { | ||
// TODO(jmmv): Should use futimensat to avoid following symlinks but this function is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of this. We should at some point conditionalize and this path should fail if you try to do this to a symlink.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I've actually just changed this now to return an error instead and added a test.
If the caller is trying to change the times of a symlink, the kernel is the one doing the symlink resolution and invoking setattr
on the target, so we rarely should get here. (There is a test that validates changing the times of a symlink's target.)
I've also added a test. (I have a pending PR for nix to add a lutimes
wrapper, which should let me fix this properly later.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About what happens when we try to delete an explicit mapping: this is tested by TestReadWrite_Remove/MappedDirCannotBeRemoved
and TestReadWrite_Remove/ScaffoldDirCannotBeRemoved
, both of which get controlled errors.
Separately, I also have another pending PR for nix to expose time_t
and suseconds_t
, which are necessary to fix the current build failures we observe on Linux in Travis. Will have to wait to merge this PR until that one is submitted.
src/nodes/mod.rs
Outdated
let result = try_path(path, |p| | ||
unistd::fchownat(None, p, uid, gid, unistd::FchownatFlags::NoFollowSymlink)); | ||
if result.is_ok() { | ||
attr.uid = uid.map_or_else(|| attr.uid, u32::from); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it works. Done.
src/nodes/mod.rs
Outdated
let atime = atime.unwrap_or_else(|| conv::timespec_to_timeval(attr.atime)); | ||
let mtime = mtime.unwrap_or_else(|| conv::timespec_to_timeval(attr.mtime)); | ||
if attr.kind == fuse::FileType::Symlink { | ||
// TODO(jmmv): Should use futimensat to avoid following symlinks but this function is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I've actually just changed this now to return an error instead and added a test.
If the caller is trying to change the times of a symlink, the kernel is the one doing the symlink resolution and invoking setattr
on the target, so we rarely should get here. (There is a test that validates changing the times of a symlink's target.)
I've also added a test. (I have a pending PR for nix to add a lutimes
wrapper, which should let me fix this properly later.)
No new push? |
I think I got distracted with necessary FTR, this PR requires nix-rust/nix#968 in order to get the Linux builds working. And nix-rust/nix#967 will hopefully let me address the issue of updating the times of symlinks. |
410aaf9
to
8c2bdc3
Compare
In order to implement unlink and rmdir, which will come later, we must prepare to deal with the case of applying updates to a delete node via still-open handles. To do this, avoid doing a getattr immediately after a setattr (because it may not be possible once the underlying file is gone), and instead compute what the updated attributes should be. This is not completely accurate because of ctime, but it's good enough for now. Also, make the input path to setattr optional so that we can ask this function to only update the in-memory properties.
To support operations on deleted nodes, this changes File and Symlink so that their underlying path is optional (Dir already had this). When None we treat these nodes as deleted, which means that some operations become impossible on them and others, like setattr, only have affect in-memory node data.
The setattr hook should be using futimensat to update the timestamps of the node so that we could support the case of modifying those of a symlink (without following it). Unfortunately, we currently cannot do so because our testing environment does not have this system call available in all platforms... so just return EOPNOTSUPP for now (instead of doing the wrong thing and following the symlink).
Alright, nix-rust/nix#968 was merged into nix so I have updated |
This PR is a complex because of all the required refactoring of
setattr
, but the implementation of the actual unlinking operations is pretty simple once that is done. Let me know if you want me to send the two commits separately for review.Also, I've left a couple of TODOs in place because I just can't figure out what the type of the lambda should be. Clippy implies that I can remove the redundant closure, but that's not true.
And... we are almost feature complete! The most significant thing left is the ability to apply reconfigurations.
Thanks