[ipcs, ipcrm] lots of review fixes, add tests #430
Conversation
jgarzik
added
bug
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for the ipcs and ipcrm utilities and includes numerous code review fixes to improve correctness, error handling, and POSIX compliance.
- Adds 13 tests for
ipcscovering various command-line options and facility types - Adds 8 tests for
ipcrmcovering error cases and multiple IPC object removal - Refactors
ipcsto properly handle Linux and macOS platform differences using/proc/sysvipcon Linux - Improves
ipcrmto support multiple IPC objects per invocation and better error reporting
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| sys/tests/sys-tests.rs | Registers new test modules for ipcs and ipcrm |
| sys/tests/ipcs/mod.rs | Adds comprehensive tests for ipcs functionality including facility filters and print options |
| sys/tests/ipcrm/mod.rs | Adds tests for ipcrm error handling and multi-object removal |
| sys/ipcs.rs | Major refactoring: adds platform-specific IPC enumeration, improves output formatting, adds internationalization |
| sys/ipcrm.rs | Changes arguments from Option to Vec for multi-object support, improves error handling and messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| println!("{}", header); | ||
| println!("{}:", gettext("Semaphores")); |
There was a problem hiding this comment.
Same issue: the section label should be printed before the header line, not after it.
| println!("{}", header); | |
| println!("{}:", gettext("Semaphores")); | |
| println!("{}:", gettext("Semaphores")); | |
| println!("{}", header); |
sys/ipcs.rs
Outdated
| &owner[..owner.len().min(8)], | ||
| &group[..group.len().min(8)] |
There was a problem hiding this comment.
Same UTF-8 slicing issue: this can panic with multi-byte characters. Use character-aware truncation.
| } | ||
|
|
||
| #[test] | ||
| fn ipcrm_no_args() { |
There was a problem hiding this comment.
[nitpick] The test name ipcrm_no_args and its comment describe different expected behaviors. The function name suggests testing with no arguments, but the comment states "no IPC objects to remove is not an error". Consider renaming the test to ipcrm_no_args_succeeds to clarify that the test verifies successful execution with no arguments.
| fn ipcrm_no_args() { | |
| fn ipcrm_no_args_succeeds() { |
| println!("{}", header); | ||
| println!("{}:", gettext("Shared Memory")); |
There was a problem hiding this comment.
Same issue as with message queues: the section label should be printed before the header line, not after it.
| println!("{}", header); | |
| println!("{}:", gettext("Shared Memory")); | |
| println!("{}:", gettext("Shared Memory")); | |
| println!("{}", header); |
No description provided.