Refactor libc pipe tests to use utility functions for error handling and data operations #4768
Conversation
…and data operations
|
Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two. |
There was a problem hiding this comment.
Thanks a lot! This looks great.
However, there are some read calls here you did not convert because we did not have the right helpers for that. I think we should explore adding more helpers so that we don't need to do a second pass over all the tests later.
| let mut buf4: [u8; 5] = [0; 5]; | ||
| let res = unsafe { libc::read(fds[0], buf4.as_mut_ptr().cast(), buf4.len() as libc::size_t) }; | ||
| assert!(res > 0 && res <= 3); |
There was a problem hiding this comment.
We should think about a way to also cover this case without having to directly invoke read (or read_all below).
I am imagining functions like this:
/// Fill `dest` by repeatedly reading from `fd`. Error if not enough bytes could be read.
fn read_all_into_slice(fd: libc::c_int, dest: &mut [u8]) -> Result<(), libc::ssize_t>;
/// Read up to `dest.len()` many bytes from `fd`. Returns a pair of the part of `dest` that has been
/// read into, and the tail that was left untouched.
fn read_into_slice(fd: libc::c_int, dest: &mut [u8]) -> Result<(&mut [u8], &mut [u8]), libc::ssize_t>;Can you add such functions to tests/utils/libc.rs and use them here?
(Please let me know if this is too much, then I can take over for this part.)
There was a problem hiding this comment.
It seems clear. I will do changes very soon because currently I am busy with my exams but don't worry, I will do it!
Do you want to use these functions instead of read and read_all in all libc tests? also maybe adding them to all PRs do some conflicts so is there a way to avoid this? maybe open a separate pr?
There was a problem hiding this comment.
This PR should use functions like that for all read in the pipe tests. Once this lands, the function is then available for the other PRs, so that should avoid conflicts.
tests/pass-dep/libc/libc-pipe.rs
Outdated
| let data = b"abcde"; | ||
| write_all_from_slice(fds[1], data).unwrap(); |
There was a problem hiding this comment.
| let data = b"abcde"; | |
| write_all_from_slice(fds[1], data).unwrap(); | |
| write_all_from_slice(fds[1], b"abcde").unwrap(); |
Generally, if data is only used exactly once, then just inline it. This occurs some more times below.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
I can't tell if you intend to make more changes here or not -- as indicated above, I will wait until you mark this PR as ready before I do another review. |
3 participants
Related to #4725