pidwait: optimize & cross-platform by FurryR · Pull Request #400 · uutils/procps

FurryR · 2025-04-24T15:25:13Z

Resolves#399 (partially).

Needs to be reviewed before merge as it contains untested code. However it seems that at least the implementation for Linux works fine.

Signed-off-by: FurryR <awathefox@gmail.com>

Krysztal112233 · 2025-04-25T07:09:14Z

Thank you for your contribution, I will start reviewing soon :)

cakebaker · 2025-04-28T12:48:05Z

Hm, at least on my machine (using Linux) your changes make the test test_non_matching_pattern fail as it runs into a timeout:

failures:
---- test_pidwait::test_non_matching_pattern stdout ----
bin: "/home/dho/projects/procps/target/debug/procps"
run: /home/dho/projects/procps/target/debug/procps pidwait THIS_PATTERN_DOES_NOT_MATCH
bin: "/home/dho/projects/procps/target/debug/procps"
run: /home/dho/projects/procps/target/debug/procps pidwait DOES_NOT_MATCH
thread 'test_pidwait::test_non_matching_pattern' panicked at /home/dho/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/uutests-0.0.30/src/lib/util.rs:1912:35:
called `Result::unwrap()` on an `Err` value: Custom { kind: Other, error: "wait: Timeout of '30s' reached" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Signed-off-by: FurryR <awathefox@gmail.com>

FurryR · 2025-05-31T02:54:39Z

Fixed a bunch of things, now tests can be passed on Linux [REDACTED] 5.15.167.4-microsoft-standard-WSL2 #1 SMP Tue Nov 5 00:21:55 UTC 2024 x86_64 GNU/Linux

Output of cargo test:


running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 142 tests
test test_free::test_count_zero ... ok
test test_free::test_invalid_arg ... ok
test test_free::test_total ... ok
test test_pgrep::test_help ... ok
test test_pgrep::test_invalid_arg ... ok
test test_free::test_no_args ... ok
test test_free::test_count_line ... ok
test test_pgrep::test_count_with_non_matching_pattern ... ok
test test_pgrep::test_count_with_matching_pattern ... FAILED
test test_pgrep::test_does_not_match_pid ... ok
test test_pgrep::test_current_user ... ok
test test_pgrep::test_list_full_process_with_empty_cmdline ... FAILED
test test_pgrep::test_full ... ok
test test_pgrep::test_lone_require_handler_is_allowed ... ok
test test_free::test_lohi ... ok
test test_free::test_committed ... ok
test test_pgrep::test_invalid_signal ... ok
test test_pgrep::test_invalid_username ... ok
test test_pgrep::test_no_args ... ok
test test_pgrep::test_invalid_group_name ... ok
test test_pgrep::test_invalid_regex ... ok
test test_free::test_seconds_zero ... ok
test test_free::test_wide ... ok
test test_pgrep::test_non_matching_pattern ... ok
test test_pgrep::test_does_not_match_current_process ... ok
test test_pgrep::test_nonexisting_pgroup ... ok
test test_free::test_line ... ok
test test_pgrep::test_newest_non_matching_pattern ... ok
test test_free::test_count ... ok
test test_pgrep::test_older_non_matching_pattern ... ok
test test_pgrep::test_older_matching_pattern ... ok
test test_pgrep::test_oldest_non_matching_pattern ... ok
test test_pgrep::test_parent_non_matching_parent ... ok
test test_pgrep::test_list_full ... ok
test test_pgrep::test_delimiter_last_wins ... ok
test test_pgrep::test_nonexisting_session ... ok
test test_pgrep::test_runstates_invalid_runstate ... ok
test test_pgrep::test_parent_multiple_parents ... ok
test test_pgrep::test_too_many_patterns ... ok
test test_pgrep::test_newest ... ok
test test_pidof::test_find_init ... ignored, fails in CI
test test_pgrep::test_signal_that_never_matches ... ok
test test_pgrep::test_delimiter ... ok
test test_pgrep::test_require_handler ... ok
test test_pidof::test_invalid_arg ... ok
test test_pidof::test_no_args ... ok
test test_pgrep::test_terminal_invalid_terminal ... ok
test test_pgrep::test_parent ... ok
test test_pgrep::test_older ... ok
test test_pidof::test_find_kthreadd_only_with_w_flag ... FAILED
test test_pidof::test_no_pid_found ... ok
test test_pidof::test_omit_pid ... FAILED
test test_pidof::test_quiet ... FAILED
test test_pgrep::test_terminal_multiple_terminals ... FAILED
test test_pgrep::test_pgroup ... ok
test test_pgrep::test_oldest ... ok
test test_pidwait::test_no_args ... ok
test test_pidwait::test_invalid_arg ... ok
test test_pidwait::test_too_many_patterns ... ok
test test_pidof::test_separator ... FAILED
test test_pgrep::test_ignore_case ... ok
test test_pkill::test_invalid_arg ... ok
test test_pidof::test_threads ... ok
test test_pkill::test_help ... ok
test test_pkill::test_no_args ... ok
test test_pkill::test_too_many_patterns ... ok
test test_pkill::test_too_long_pattern ... ok
test test_pidof::test_check_root_accepted ... FAILED
test test_pidof::test_single_shot ... FAILED
test test_pgrep::test_too_long_pattern ... ok
test test_pmap::test_invalid_arg ... ok
test test_pgrep::test_runstates ... ok
test test_pmap::test_non_existing_pid ... ok
test test_pmap::test_no_args ... ok
test test_ps::test_invalid_arg ... ok
test test_pgrep::test_session ... ok
test test_pmap::test_non_existing_and_existing_pid ... ok
test test_pidwait::test_non_matching_pattern ... ok
test test_pkill::test_non_matching_pattern ... ok
test test_pidof::test_script ... ok
test test_pkill::test_inverse ... ok
test test_pmap::test_permission_denied ... ok
test test_pmap::test_extended_permission_denied ... ok
test test_pwdx::test_invalid_arg ... ok
test test_pmap::test_device_permission_denied ... ok
test test_pmap::test_existing_pid ... ok
test test_pwdx::test_no_args ... ok
test test_slabtop::test_invalid_arg ... ok
test test_slabtop::test_help ... ok
test test_slabtop::test_without_args_as_non_root ... FAILED
test test_pgrep::test_unknown_terminal ... FAILED
test test_slabtop::test_once_as_non_root ... FAILED
test test_pgrep::test_valid_regex ... ok
test test_snice::test_no_process_selected ... ok
test test_sysctl::linux::test_continues_on_error ... ok
test test_pmap::test_multiple_existing_pids ... ok
test test_sysctl::linux::test_get_key_only ... ok
test test_pmap::test_device ... ok
test test_sysctl::test_invalid_arg ... ok
test test_free::test_unit ... ok
test test_slabtop::test_once_as_root ... ok
test test_tload::test_invalid_arg ... ok
test test_pgrep::test_terminal ... ok
test test_sysctl::linux::test_get_simple ... ok
test test_top::test_invalid_arg ... ok
test test_top::test_conflict_arg ... ok
test test_snice::test_no_args ... ok
test test_vmstat::test_header ... ok
test test_vmstat::test_active ... ok
test test_vmstat::test_invalid_arg ... ok
test test_sysctl::linux::test_ignoring_errors ... ok
test test_vmstat::test_invalid_number ... ok
test test_vmstat::test_simple ... ok
test test_vmstat::test_wide_mode ... ok
test test_vmstat::test_unit ... ok
test test_sysctl::linux::test_get_value_only ... ok
test test_w::test_invalid_arg ... ok
test test_w::test_help ... ok
test test_pmap::test_extended ... ok
test test_watch::test_invalid_arg ... ok
test test_vmstat::test_invalid_unit ... ok
test test_w::test_output_format ... FAILED
test test_w::test_option_short ... ok
test test_watch::test_invalid_interval ... ok
test test_w::test_no_header ... ok
test test_slabtop::test_without_args_as_root ... ok
test test_pwdx::test_non_existing_pid ... ok
test test_pwdx::test_multiple_valid_pids ... ok
test test_pwdx::test_valid_pid ... ok
test test_pgrep::test_threads ... ok
test test_ps::test_select_all_processes ... ok
test test_pwdx::test_invalid_pid ... ok
test test_pwdx::test_non_existing_and_existing_pid ... ok
test test_pwdx::test_invalid_and_valid_pid ... ok
test test_ps::test_code_mapping ... ok
test test_watch::test_no_interval ... ok
test test_watch::test_valid_interval ... ok
test test_watch::test_interval_environment_variable ... ok
test test_top::test_arg_p ... ok
test test_vmstat::test_no_first ... ok
test test_watch::test_valid_interval_comma ... ok
test test_top::test_flag_user ... ok
failures:
---- test_pgrep::test_count_with_matching_pattern stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps pgrep -c kthreadd
thread 'test_pgrep::test_count_with_matching_pattern' panicked at tests/by-util/test_pgrep.rs:245:14:
Command was expected to succeed. code: 1
stdout = 0
stderr = note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- test_pgrep::test_list_full_process_with_empty_cmdline stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps pgrep kthreadd --list-full
thread 'test_pgrep::test_list_full_process_with_empty_cmdline' panicked at tests/by-util/test_pgrep.rs:234:10:
Command was expected to succeed. code: 1
stdout = stderr = ---- test_pidof::test_find_kthreadd_only_with_w_flag stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps pidof kthreadd
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps pidof -w kthreadd
thread 'test_pidof::test_find_kthreadd_only_with_w_flag' panicked at tests/by-util/test_pidof.rs:31:43:
Command was expected to succeed. code: 1
stdout = stderr = ---- test_pidof::test_omit_pid stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps pidof -o=1000 -w kthreadd
thread 'test_pidof::test_omit_pid' panicked at tests/by-util/test_pidof.rs:84:56:
Command was expected to succeed. code: 1
stdout = stderr = ---- test_pidof::test_quiet stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps pidof kthreadd -q -w
thread 'test_pidof::test_quiet' panicked at tests/by-util/test_pidof.rs:47:10:
Command was expected to succeed. code: 1
stdout = stderr = ---- test_pgrep::test_terminal_multiple_terminals stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps pgrep --terminal=tty1,? kthreadd
thread 'test_pgrep::test_terminal_multiple_terminals' panicked at tests/by-util/test_pgrep.rs:283:10:
Command was expected to succeed. code: 1
stdout = stderr = ---- test_pidof::test_separator stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps pidof -S separator -w kthreadd kthreadd
thread 'test_pidof::test_separator' panicked at tests/by-util/test_pidof.rs:98:14:
Command was expected to succeed. code: 1
stdout = stderr = ---- test_pidof::test_check_root_accepted stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps pidof -w --check-root kthreadd
thread 'test_pidof::test_check_root_accepted' panicked at tests/by-util/test_pidof.rs:58:10:
Command was expected to succeed. code: 1
stdout = stderr = ---- test_pidof::test_single_shot stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps pidof -s -w kthreadd kthreadd kthreadd
thread 'test_pidof::test_single_shot' panicked at tests/by-util/test_pidof.rs:67:14:
Command was expected to succeed. code: 1
stdout = stderr = ---- test_slabtop::test_without_args_as_non_root stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps slabtop
thread 'test_slabtop::test_without_args_as_non_root' panicked at tests/by-util/test_slabtop.rs:29:10:
'slabtop: No such file or directory
' does not contain 'Permission denied'
---- test_pgrep::test_unknown_terminal stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps pgrep --terminal=?
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps pgrep --terminal=? kthreadd
thread 'test_pgrep::test_unknown_terminal' panicked at tests/by-util/test_pgrep.rs:291:53:
Command was expected to succeed. code: 1
stdout = stderr = ---- test_slabtop::test_once_as_non_root stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps slabtop -o
thread 'test_slabtop::test_once_as_non_root' panicked at tests/by-util/test_slabtop.rs:57:14:
'slabtop: No such file or directory
' does not contain 'Permission denied'
---- test_w::test_output_format stdout ----
bin: "/home/furryr/procps/target/debug/procps"
run: /home/furryr/procps/target/debug/procps w --no-header
thread 'test_w::test_output_format' panicked at tests/by-util/test_w.rs:80:46:
Command was expected to succeed. code: 1
stdout = stderr = w: failed to fetch user info: No such process (os error 3)
failures:
test_pgrep::test_count_with_matching_pattern
test_pgrep::test_list_full_process_with_empty_cmdline
test_pgrep::test_terminal_multiple_terminals
test_pgrep::test_unknown_terminal
test_pidof::test_check_root_accepted
test_pidof::test_find_kthreadd_only_with_w_flag
test_pidof::test_omit_pid
test_pidof::test_quiet
test_pidof::test_separator
test_pidof::test_single_shot
test_slabtop::test_once_as_non_root
test_slabtop::test_without_args_as_non_root
test_w::test_output_format
test result: FAILED. 128 passed; 13 failed; 1 ignored; 0 measured; 0 filtered out; finished in 1.21s

Requiring further tests on Windows and OpenBSD.

Signed-off-by: FurryR <awathefox@gmail.com>

sorry that is my fault. Signed-off-by: FurryR <awathefox@gmail.com>

Signed-off-by: FurryR <awathefox@gmail.com> Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>

FurryR · 2025-06-09T03:48:24Z

Sorry for replying late. I am working on it right now.

Signed-off-by: FurryR <awathefox@gmail.com>

cakebaker · 2025-06-11T15:39:44Z

+pub(crate) fn wait(
+ procs: &[ProcessInformation],
+ timeout: Option<Duration>,
+) -> Result<Option<()>, std::io::Error> {


What's the reason for using Option<()>? Why not use ()?

I thought that it would be better if we can pass exceptions rather than just unwrap.

Hm, I don't understand what you mean :| I think we are talking about different things. Maybe my question was too imprecise?
My question is: why do you use Result<Option<()>, std::io::Error> instead of Result<(), std::io::Error> as return type?

I think using Option<()> is unnecessary because the meaning of this Option is to indicate something there or nothing there, and it's clear that this function doesn't need that meaning; it only needs to know whether an error occurred

Option<()> here:
None means that there is no such process.
Ok(()) means that the process exists and the function waited for the process.
It is probably useful for future uses, as you can detect if the function really did wait. idk.

cakebaker · 2025-06-11T15:42:48Z

+ procs: &[ProcessInformation],
+ timeout: Option<Duration>,
+) -> Result<Option<()>, std::io::Error> {
+ if !procs.is_empty() {


A detail: I would drop the ! and swap the if and else blocks, it's a little bit easier to read.

Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>

Krysztal112233 · 2025-06-18T11:59:19Z

+use std::time::Duration;
+use uu_pgrep::process::ProcessInformation;
+
+pub fn wait(procs: &[ProcessInformation], timeout: Option<Duration>) -> Result<Option<()>> {


same as
https://github.com/uutils/procps/pull/400/files#r2154416501

Krysztal112233 · 2025-06-18T11:59:38Z

+use std::time::Duration;
+use uu_pgrep::process::ProcessInformation;
+
+pub fn wait(procs: &[ProcessInformation], timeout: Option<Duration>) -> Result<Option<()>> {


same
https://github.com/uutils/procps/pull/400/files#r2154416501

Krysztal112233 · 2025-06-18T12:04:47Z

I noticed that there are some unsafe block in this file, and cloud you please add comments to explain why it's safety here? :)
https://github.com/uutils/coreutils/blob/main/CONTRIBUTING.md#unsafe

pidwait: optimize & cross-platform
7ae897c
Signed-off-by: FurryR <awathefox@gmail.com>

Krysztal112233 self-requested a review April 25, 2025 07:08

cakebaker reviewed Apr 28, 2025
View reviewed changes

Comment threadsrc/uu/pidwait/Cargo.toml Outdated

FurryR added 3 commits May 31, 2025 10:45

fix(pidwait): add length check for procs
7e2813b
Signed-off-by: FurryR <awathefox@gmail.com>

chore(pidwait): use workspace windows-sys crate
dc7b000
Signed-off-by: FurryR <awathefox@gmail.com>

Merge branch 'main' into main
903fef8

FurryR requested a review from cakebaker May 31, 2025 02:54

FurryR added 2 commits May 31, 2025 17:27

fix(pidwait): sync with master
c41708d
Signed-off-by: FurryR <awathefox@gmail.com>

chore(pidwait): add newline at eof
5029113
sorry that is my fault. Signed-off-by: FurryR <awathefox@gmail.com>

cakebaker reviewed Jun 1, 2025
View reviewed changes

Comment threadsrc/uu/pidwait/src/wait.rs Outdated

cakebaker reviewed Jun 1, 2025
View reviewed changes

Comment threadsrc/uu/pidwait/src/wait.rs Outdated

cakebaker reviewed Jun 1, 2025
View reviewed changes

Comment threadsrc/uu/pidwait/src/wait.rs Outdated

Krysztal112233and others added 2 commits June 7, 2025 03:15

Merge branch 'main' into main
df176fe

chore(pidwait): make clippy happy
1836a9d
Signed-off-by: FurryR <awathefox@gmail.com> Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>

FurryR added 2 commits June 9, 2025 12:07

chore(pidwait): update structure
ce291dd
Signed-off-by: FurryR <awathefox@gmail.com>

chore(pidwait): styling
76310ac
Signed-off-by: FurryR <awathefox@gmail.com>

FurryR requested a review from cakebaker June 9, 2025 04:10

cakebaker reviewed Jun 11, 2025
View reviewed changes

Comment threadsrc/uu/pidwait/src/platform/linux.rs Outdated

refactor(pidwait): merge suggestions
1e9a3dd
Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>

Krysztal112233 requested changes Jun 18, 2025
View reviewed changes

Krysztal112233 mentioned this pull request May 29, 2026
pidwait: replace busy-loop with pidfd_open and poll on Linux #711
Merged

Uh oh!

Conversation

FurryR commented Apr 24, 2025

Uh oh!

Krysztal112233 commented Apr 25, 2025

Uh oh!

cakebaker commented Apr 28, 2025

Uh oh!

Uh oh!

FurryR commented May 31, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FurryR commented Jun 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FurryRMay 29, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FurryRMay 29, 2026•
edited
Loading