Add statx shim for linux target #1101
Conversation
pvdrz
changed the title
|
Hmm apparently OSX uses Edit: I implemented Edit2: Apparently the specifics of the |
pvdrz
force-pushed
the
stat-shim
branch
7 times, most recently
from
cd958e5 to
61272f0
Compare
src/shims/fs.rs
Outdated
| // This argument should be a `c_int` but the syscall API just returns a pointer sized | ||
| // integer. | ||
| let flags: i32 = this.try_from_ptr_sized_operand(flags_op)?; |
There was a problem hiding this comment.
Where is that statx call site? I seems just wrong to me to pass an isize when the syscall says it should be a c_int.
There was a problem hiding this comment.
So the reason these are all isize is that they all get cast right here. That is very odd.
@gnzlbg any idea why libstd turns all syscall parameters to c_long, even when the official syscall docs use other types such as "int" or "unsinged"?
There was a problem hiding this comment.
Ehm, no idea, the first argument to glibc's syscall is a c_long specifying the number of arguments, but the rest of the arguments are a C variadic list of arguments, and they should have the correct types of the syscall. If the syscall expects an c_int, and you pass it an isize, then that would have the wrong argument size on, e.g., 64-bit linux, so that seems wrong. Maybe it doesn't "matter" in practice, e.g., due to how the calling convention handles these arguments, but still, looks like a bug. cc @Amanieu
There was a problem hiding this comment.
The first argument seems to be the syscall number, not the number of arguments?
If the syscall expects an c_int, and you pass it an isize, then that would have the wrong argument size on, e.g., 64-bit linux, so that seems wrong.
I think that is exactly what is happening. All parameters of all syscalls going through that macro are passed as c_long, no matter their actual type.
src/shims/fs.rs
Outdated
| // The `mode` field specifies the type of the file and the permissions over the file for | ||
| // the owner, its group and other users. Given that we can only provide the file type | ||
| // without using platform specific methods, we only set the bits corresponding to the file | ||
| // type. This argument should be an `__u16` but the syscall API just returns a pointer sized |
There was a problem hiding this comment.
"the syscall API" doesn't return anything here, does it?
There was a problem hiding this comment.
No, that's just bad wording from my part. "provides" maybe?
There was a problem hiding this comment.
Thus is not about "the syscall API" at all. The thing is that syscall is an untyped function so all types are determined by the caller, and the particular caller we are working with here decided to make everything an isize. (I am still trying to figure out why.)
This is not a universal truth for all syscalls.
|
I think I grilled you long enough, let's go ahead and land this. :) @bors r+ |
|
📌 Commit b0c7625 has been approved by |
Add statx shim for linux target This is an attempt to fix: #999 (for linux only) Currently there is one problem that I haven't been able to solve. `std::fs::metadata` fails because the creation time is not available even though it is provided in the shim code. In order to inform the caller that the field was provided, the `stx_flag` field must have the bits of `STATX_BTIME` set (which they are). The creation time is in the `stx_btime` field of the `statx` struct (see [1]). The relevant code in `libstd` is here (probably?): https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/fs.rs#L322 Another important point is that we are just providing the fields that are available in "all" platforms (this is, without using any platform specific traits or so). This can be improved later. References: [1] Man page: http://man7.org/linux/man-pages/man2/statx.2.html [2] libc `statx` struct: https://docs.rs/libc/0.2.63/libc/struct.statx.html Edit: The problem is that my filesystem is not providing it and I thought all filesystems could provide it. I changed the code so it only provides those dates if they are available. now we are ready to go. r? @RalfJung @oli-obk
|
☀️ Test successful - checks-travis, status-appveyor |
This is an attempt to fix: #999 (for linux only)
Currently there is one problem that I haven't been able to solve.
std::fs::metadatafails because the creation time is not available even though it is provided in the shim code.In order to inform the caller that the field was provided, the
stx_flagfield must have the bits ofSTATX_BTIMEset (which they are). The creation time is in thestx_btimefield of thestatxstruct (see [1]). The relevant code inlibstdis here (probably?): https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/fs.rs#L322Another important point is that we are just providing the fields that are available in "all" platforms (this is, without using any platform specific traits or so). This can be improved later.
References:
[1] Man page: http://man7.org/linux/man-pages/man2/statx.2.html
[2] libc
statxstruct: https://docs.rs/libc/0.2.63/libc/struct.statx.htmlEdit: The problem is that my filesystem is not providing it and I thought all filesystems could provide it. I changed the code so it only provides those dates if they are available. now we are ready to go.
r? @RalfJung @oli-obk