-
Notifications
You must be signed in to change notification settings - Fork 402
Implement posix_fallocate with set_len() functionality
#4664
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
base: master
Are you sure you want to change the base?
Implement posix_fallocate with set_len() functionality
#4664
Conversation
|
Thank you for contributing to Miri! |
| } | ||
|
|
||
| // only macos doesn't support `posix_fallocate` | ||
| "posix_fallocate" if &*this.tcx.sess.target.os != "macos" => { |
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 sure if this is the correct way, from what I could find online, only macos doesn't implement posix_fallocate.
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.
If that is the case then the test should also be for all OSes except macos.
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.
Also, please check pipe2 for how we make a shim available only on some OSes. Please do not use a match guard.
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.
Thanks for the PR!
| let ebadf = Scalar::from_i32(this.eval_libc_i32("EBADF")); | ||
| let einval = Scalar::from_i32(this.eval_libc_i32("EINVAL")); | ||
| let enodev = Scalar::from_i32(this.eval_libc_i32("ENODEV")); |
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 it's better to just inline these where they are needed; the error path shouldn't take so much space on the happy path.
Also, you can use eval_libc instead of Scalar::from_i32(eval_libc_i32).
| return interp_ok(einval); | ||
| } | ||
|
|
||
| if file.writable { |
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.
Please make this more like
if !file.writable { return error; }to avoid rightwards drift
| Err(err) => return this.io_error_to_errnum(err), | ||
| }; | ||
| let new_size = match offset.checked_add(len) { | ||
| Some(size) => size.try_into().unwrap(), // We just checked negative `offset` and `len`. |
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.
Please change this code so it is clear what type we are converting this into.
Also the comment does not explain why the size would fit into the target type.
| // `posix_fallocate` only specifies increasing the file size. | ||
| if current_size < new_size { | ||
| let result = file.file.set_len(new_size); | ||
| let result = this.try_unwrap_io_result(result.map(|_| 0i32))?; |
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 can set the errno. Didn't you say the function does not do that?
| Ok(metadata) => metadata.len(), | ||
| Err(err) => return this.io_error_to_errnum(err), | ||
| }; | ||
| let new_size = match offset.checked_add(len) { |
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.
IIUC, if this operation overflows we should return EFBIG.
should close #4464.
I used this man page for the implementation.
Changes included in this pr:
posix_fallocateandposix_fallocate64(same as truncate versions because of libc::off_t)unix/foreign_items.pass-dep/libc/libc-fs.rs.