Implement fd_filestat_set_size using libstd #47
Conversation
marmistrz
force-pushed
the
fd_filestat_set_size
branch
from
b1c5cb4 to
b6d30cc
Compare
src/hostcalls_impl/fs.rs
Outdated
| @@ -766,11 +766,7 @@ pub(crate) fn fd_filestat_set_size( | |||
| .and_then(|fe| fe.fd_object.descriptor.as_file())?; | |||
|
|
|||
| let st_size = dec_filesize(st_size); | |||
| if st_size > i64::max_value() as u64 { | |||
| return Err(host::__WASI_E2BIG); | |||
There was a problem hiding this comment.
Hmm, are you sure it is wise to remove this check? At a quick glance, std::fs::File::set_len which internally calls std::sys::*::fs::File::truncate does not perform this check. I might be mistaken here though! Please correct me if that's the case. If I'm right, however, I think we should leave this check in the code for better error signalling to the Wasm app. @sunfishcode @alexcrichton thoughts?
There was a problem hiding this comment.
You're right: https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/fs.rs#L552-L559
But isn't it a bug in libstd that a silent overflow occurs instead of an error being reported? (off64_t is int64_t on my system)
There was a problem hiding this comment.
Yes if libstd isn't doing a proper cast check then libstd should be where this error is generated for sure, sounds like a bug!
There was a problem hiding this comment.
Could we then leave the check as is for now and in the meantime, submit a bug and a patch to libstd? Sounds like the best option to me in the short term :-)
There was a problem hiding this comment.
I'll submit the bug and make the patch to libstd tomorrow :)
There was a problem hiding this comment.
@marmistrz after you submit the patch and bring back the check in wasi-common, could you add a comment with a link to the libstd’s patch explaining that when it lands and stabilises we’ll remove it altogether?
marmistrz
force-pushed
the
fd_filestat_set_size
branch
from
2349dad to
48cf790
Compare
|
I rebased the change upon the new revision of #46. |
|
Could you rebase onto master? Otherwise, the commit this PR is based on will be repeated again after we merge it seems :-) |
The check will be removed when rust-lang/rust#63326 is fixed
marmistrz
force-pushed
the
fd_filestat_set_size
branch
from
48cf790 to
6e57865
Compare
|
Done. |
|
LGTM! |
This PR bases on #42 & #46.