-
Notifications
You must be signed in to change notification settings - Fork 13k
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
extending filesystem support for Hermit #115984
Conversation
r? @thomcc (rustbot has picked a reviewer for you, use r? to override) |
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
Cc: @mkroening |
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.
I'll give it a more thorough pass later this weekend. It mostly looks fine.
library/std/src/sys/hermit/fs.rs
Outdated
|
||
macro_rules! offset_ptr { | ||
($entry_ptr:expr, $field:ident) => {{ | ||
const OFFSET: isize = { |
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.
I think you can use https://doc.rust-lang.org/nightly/std/mem/macro.offset_of.html here.
library/std/src/sys/hermit/fs.rs
Outdated
} else { | ||
#[allow(deref_nullptr)] | ||
{ | ||
ptr::addr_of!((*ptr::null::<dirent>()).$field) |
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.
This is very gnarly, even if the branch is never taken....
library/std/src/sys/hermit/fs.rs
Outdated
} | ||
}; | ||
|
||
macro_rules! offset_ptr { |
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.
I'm not sure why this macro exists. Most/all of the use sites could be fine with another construct (either addr_of, or just (*p).field
), no?
The only place I see something like it being needed is for the name field of the dirent (since it might not be entirely there) but at that point you can just use offset_of and casting, no?
Hm, you are right. I will revise it. |
The Since we control both sides of this API, we might be able to avoid having to use it. |
I see, so that code is mostly copied from sys::unix? Hm... That's probably fine, then. It would be nice if it could be somehow reused, admittedly. |
☔ The latest upstream changes (presumably #116029) made this pull request unmergeable. Please resolve the merge conflicts. |
Hm, I agree, that this will be useful. However, the handling of error numbers are different and also a few system calls (e.g. |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #116956) made this pull request unmergeable. Please resolve the merge conflicts. |
Rebased on latest @thomcc, are you okay with the macro for now? |
☔ The latest upstream changes (presumably #118046) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #117285) made this pull request unmergeable. Please resolve the merge conflicts. |
@thomcc We revised the system call to read directory entries. I hope that interface is now more understandable. |
This comment has been minimized.
This comment has been minimized.
2e7dba1
to
f461c4a
Compare
I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks. r? libs |
☔ The latest upstream changes (presumably #121800) made this pull request unmergeable. Please resolve the merge conflicts. |
Some changes occurred in engine.rs, potentially modifying the public API of
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor This PR modifies If appropriate, please update Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
The new interface has some similarities to Linux system call getdents64. The system call reads several dirent64 structures. At the end of each dirent64 is stored the name of the file. The length of file name is implictly part of dirent64 because d_reclen contains size of dirent64 plus the length of the file name.
@@ -48,6 +48,11 @@ impl FileDesc { | |||
pub fn set_nonblocking(&self, _nonblocking: bool) -> io::Result<()> { | |||
unsupported() | |||
} | |||
|
|||
pub fn fstat(&self, stat: *mut abi::stat) -> io::Result<()> { | |||
cvt(unsafe { abi::fstat(self.fd.as_raw_fd(), stat) })?; |
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.
...Why did I have to trace through three different directories just to discover that this is referring to the hermit_abi
crate? That is so confusing.
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.
Excuse me for my late response.
I am a little bit confused. Why do you have trace three different directories? Do you mean I should move fstat
from from fd.rs
to fs.rs
?
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.
Or do you want that rename abi
to hermit-abi
? Maybe it is easier to understand...
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.
Ah, I was a bit confused by the levels of indirection in general, but yes, I think probably using hermit_abi
might be a bit clearer.
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.
Ok, I would prefer to create an additional PR for these changes because I have to touch every file in library/std/src/sys/pal/hermit/
. In addition, it doesn't belong to this PR.
@bors r+ rollup |
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#115984 (extending filesystem support for Hermit) - rust-lang#120144 (privacy: Stabilize lint `unnameable_types`) - rust-lang#122807 (Add consistency with phrases "meantime" and "mean time") - rust-lang#123089 (Add invariant to VecDeque::pop_* that len < cap if pop successful) - rust-lang#123595 (Documentation fix) - rust-lang#123625 (Stop exporting `TypeckRootCtxt` and `FnCtxt`.) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#115984 - hermit-os:fuse, r=m-ou-se extending filesystem support for Hermit Extending `std` to create, change and read a directory for Hermit. Hermit is a tier 3 platform and this PR changes only files, wich are related to the tier 3 platform.
Take up suggestion from the discussions within rust-lang#115984 to increase readability.
Take up suggestion from the discussions within rust-lang#115984 to increase readability.
revise the interpretation of ReadDir for HermitOS HermitOS supports getdents64. As under Linux, the dirent64 entry `d_off` is not longer used, because its definition is not clear. Instead of `d_off` the entry `d_reclen` is used to determine the end of the dirent64 entry. In addition, take up `@workingjubilee` suggestion from the discussions in rust-lang#115984 to increase the readability. Hermit is a tier 3 platform and this PR changes only files, wich are related to the tier 3 platform.
Rollup merge of rust-lang#124304 - hermit-os:fuse, r=joboet revise the interpretation of ReadDir for HermitOS HermitOS supports getdents64. As under Linux, the dirent64 entry `d_off` is not longer used, because its definition is not clear. Instead of `d_off` the entry `d_reclen` is used to determine the end of the dirent64 entry. In addition, take up `@workingjubilee` suggestion from the discussions in rust-lang#115984 to increase the readability. Hermit is a tier 3 platform and this PR changes only files, wich are related to the tier 3 platform.
Extending
std
to create, change and read a directory for Hermit.Hermit is a tier 3 platform and this PR changes only files, wich are related to the tier 3 platform.