uucore::pipes: remove pipe and use pipe_with_size(MAX_ROOTLESS_PIPE_SIZE)#12284
uucore::pipes: remove pipe and use pipe_with_size(MAX_ROOTLESS_PIPE_SIZE)#12284blixygetir wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Linux/Android pipe usage to call pipe_with_size(MAX_ROOTLESS_PIPE_SIZE) directly and makes pipe_with_size public in uucore::pipes.
Changes:
- Exposes
uucore::pipes::pipe_with_size. - Replaces selected
pipe()usages inyes,wc, and acommtest withpipe_with_size(MAX_ROOTLESS_PIPE_SIZE).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/uucore/src/lib/features/pipes.rs |
Makes pipe_with_size public. |
src/uu/yes/src/yes.rs |
Uses pipe_with_size(MAX_ROOTLESS_PIPE_SIZE) for fast-path pipes. |
src/uu/wc/src/count_fast.rs |
Uses pipe_with_size(MAX_ROOTLESS_PIPE_SIZE) for splice broker pipe creation. |
tests/by-util/test_comm.rs |
Updates anonymous pipe test to use pipe_with_size(MAX_ROOTLESS_PIPE_SIZE). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn pipe_with_size(s: usize) -> std::io::Result<(File, File)> { | ||
| let (read, write) = rustix::pipe::pipe()?; | ||
| if s > KERNEL_DEFAULT_PIPE_SIZE { | ||
| let _ = fcntl_setpipe_size(&read, s); |
There was a problem hiding this comment.
This error is deliberately ignored since it is NOT requirement for splice fast-path. I'll add different fn pipe_exact and change doc later.
Please ignore AI's review.
| fn test_comm_anonymous_pipes() { | ||
| use std::{io::Write, os::fd::AsRawFd, process}; | ||
| use uucore::pipes::pipe; | ||
| use uucore::pipes::{pipe_with_size, MAX_ROOTLESS_PIPE_SIZE}; |
| #[inline] | ||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| fn pipe_with_size(s: usize) -> std::io::Result<(File, File)> { | ||
| pub fn pipe_with_size(s: usize) -> std::io::Result<(File, File)> { |
| fn test_comm_anonymous_pipes() { | ||
| use std::{io::Write, os::fd::AsRawFd, process}; | ||
| use uucore::pipes::pipe; | ||
| use uucore::pipes::{pipe_with_size, MAX_ROOTLESS_PIPE_SIZE}; |
There was a problem hiding this comment.
For test_comm, we don't need too large pipe. We should rustix::pipe::pipe directly at here.
There was a problem hiding this comment.
And cfg should be gated to unix too.
|
Would you wait merging #12254 ? Then you can remove |
|
GNU testsuite comparison: |
|
Sure |
|
Another option is adding const generics to determine changing pipe size is optional or not. |
|
I think the only difference if I use generics for size would be an ? at the end, separating the functions via the requirement of a size parameter would make the code more readable and explicit. |
|
opened #12285 |
Personally, I don't want to have many |
|
Sounds fair |
Fixes #12271
I have changes to the files using uucore:pipe replacing it with pipe_size_max(MAX_ROOTLESS_PIPE_SIZE). I've made this function public in the pipe.rs file.