Skip to content

Conversation

@hulxv
Copy link
Contributor

@hulxv hulxv commented Dec 17, 2025

Related to #4725

@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2025

Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two.
Please remember to not force-push to the PR branch except when you need to rebase due to a conflict or when the reviewer asks you for it.

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Dec 17, 2025
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

View changes since this review

Comment on lines 40 to 42
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Dec 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 22, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@RalfJung
Copy link
Member

RalfJung commented Jan 2, 2026

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.

@hulxv
Copy link
Contributor Author

hulxv commented Jan 2, 2026

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Jan 2, 2026
@hulxv hulxv force-pushed the refactor/simplify-libc-tests/libc-pipe branch from 2d0df3d to 26d5417 Compare January 2, 2026 20:46
@hulxv
Copy link
Contributor Author

hulxv commented Jan 2, 2026

Sorry, It seems the test fails without noticing it. it's fixed now

@hulxv hulxv force-pushed the refactor/simplify-libc-tests/libc-pipe branch from 26d5417 to e824c37 Compare January 2, 2026 20:56
@RalfJung RalfJung force-pushed the refactor/simplify-libc-tests/libc-pipe branch from e824c37 to 7c2da1b Compare January 3, 2026 15:24
@RalfJung
Copy link
Member

RalfJung commented Jan 3, 2026

Thanks! I made some further small tweaks, this is good to land now. :)

@RalfJung RalfJung enabled auto-merge January 3, 2026 15:25
@RalfJung RalfJung dismissed their stale review January 3, 2026 15:25

resolved

@RalfJung RalfJung added this pull request to the merge queue Jan 3, 2026
Merged via the queue into rust-lang:master with commit 19ed096 Jan 3, 2026
13 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label Jan 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants