Skip to content

Implement chcpu#226

Closed
alxndrv wants to merge 12 commits intouutils:mainfrom
alxndrv:add-chcpu
Closed

Implement chcpu#226
alxndrv wants to merge 12 commits intouutils:mainfrom
alxndrv:add-chcpu

Conversation

@alxndrv
Copy link
Copy Markdown
Contributor

@alxndrv alxndrv commented Feb 17, 2025

This PR implements pretty much the entirety of chcpu.

Some things still missing:

  • Tests: I'm not entirely sure how to properly test something which is so tightly coupled to system internals.
  • Exit codes: chcpu should return a non-zero exit code in some edge cases. This implementation does check for most such cases, but pretty much always returns a success exit-code. I'm guessing that uucore::error::UResult can be used to solve this somehow without having to manually call std::process::exit().

@alxndrv alxndrv marked this pull request as ready for review February 17, 2025 16:44
@sylvestre
Copy link
Copy Markdown
Contributor

in general, to test such things, you either try to mock up
and you do your best to look at the error management

@alxndrv
Copy link
Copy Markdown
Contributor Author

alxndrv commented Feb 17, 2025

@sylvestre Thanks for the review, I'll implement your suggestions a bit later. Since we're on the topic of deduping: there's some code here that's copy-pasted from my work on lscpu (the parse_cpu_list() function). Should there be some kind of "lib" crate for common stuff like this? These kinds of CPU lists come up in a bunch of Linux utils and theres no sense in implementing it all over again every time. There's other similar examples: for example the mount-related utils all have to parse mount entries from /proc/<pid>/mountinfo and could benefit from having a shared lib.

@koutheir koutheir mentioned this pull request Feb 26, 2025
@sylvestre
Copy link
Copy Markdown
Contributor

Should there be some kind of "lib" crate for common stuff like this?

yes, we try to publish content in uucore
https://docs.rs/uucore/latest/uucore/

@sylvestre sylvestre requested a review from Copilot March 8, 2025 12:42
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR implements the new chcpu utility for managing CPU states via enabling, disabling, configuring, and dispatching mode management. Key changes include:

  • Adding package configuration and build targets for chcpu.
  • Implementing core CPU management functions with system file interaction and error handling placeholders.
  • Providing documentation and updating the global Cargo.toml feature flags.

Reviewed Changes

File Description
src/uu/chcpu/Cargo.toml Defines package configuration for chcpu.
src/uu/chcpu/src/chcpu.rs Implements CPU management functionality (enable, disable, etc.).
src/uu/chcpu/chcpu.md Adds documentation for the chcpu command.
src/uu/chcpu/src/main.rs Defines the main entry point for chcpu.
Cargo.toml Updates feature flags to include chcpu.

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (2)

src/uu/chcpu/src/chcpu.rs:186

  • [nitpick] On failure to trigger a CPU rescan, handling the error with a proper exit code would improve reliability instead of only printing a message.
Err(e) => println!("Failed to trigger rescan of CPUs: {}", e.kind()),

src/uu/chcpu/src/chcpu.rs:230

  • [nitpick] Consider returning an appropriate error with a non-zero exit code when the dispatching mode is unsupported, following the TODO guidance.
if !path.exists() {

Comment on lines +69 to +89
fn is_online(&self) -> bool {
fs::read_to_string(self.get_path().join("online"))
.map(|content| match content.trim() {
"0" => false,
"1" => true,
other => panic!("Unrecognized CPU online state: {}", other),
})
// Just in case the caller forgot to check `is_hotpluggable` first,
// instead of panicing that the file doesn't exist, return true
// This is because a non-hotpluggable CPU is assumed to be always online
.unwrap_or(true)
Copy link

Copilot AI Mar 8, 2025

Choose a reason for hiding this comment

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

Calling panic! on an unexpected CPU online state can cause abrupt termination. Consider returning an error or handling the unexpected state more gracefully.

Suggested change
fn is_online(&self) -> bool {
fs::read_to_string(self.get_path().join("online"))
.map(|content| match content.trim() {
"0" => false,
"1" => true,
other => panic!("Unrecognized CPU online state: {}", other),
})
// Just in case the caller forgot to check `is_hotpluggable` first,
// instead of panicing that the file doesn't exist, return true
// This is because a non-hotpluggable CPU is assumed to be always online
.unwrap_or(true)
fn is_online(&self) -> Result<bool, String> {
fs::read_to_string(self.get_path().join("online"))
.map(|content| match content.trim() {
"0" => Ok(false),
"1" => Ok(true),
other => Err(format!("Unrecognized CPU online state: {}", other)),
})
// Just in case the caller forgot to check `is_hotpluggable` first,
// instead of panicing that the file doesn't exist, return Ok(true)
// This is because a non-hotpluggable CPU is assumed to be always online
.unwrap_or(Ok(true))

Copilot uses AI. Check for mistakes.
Comment on lines +52 to +61
fn read_cpu_file(&self, file: &str) -> String {
fs::read_to_string(self.get_path().join(file)).unwrap()
Copy link

Copilot AI Mar 8, 2025

Choose a reason for hiding this comment

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

Using unwrap here may cause an unexpected panic on I/O error. It would be better to propagate the error using Result to handle read failures gracefully.

Suggested change
fn read_cpu_file(&self, file: &str) -> String {
fs::read_to_string(self.get_path().join(file)).unwrap()
fn read_cpu_file(&self, file: &str) -> std::io::Result<String> {
fs::read_to_string(self.get_path().join(file))

Copilot uses AI. Check for mistakes.
@alxndrv alxndrv force-pushed the add-chcpu branch 3 times, most recently from 0ce313e to fbc1e6e Compare March 10, 2025 17:38
@alxndrv
Copy link
Copy Markdown
Contributor Author

alxndrv commented Mar 10, 2025

Updated the implementation to make use of UResults in order to return correct non-zero exit codes in most failure scenarios. Also added some basic test scenarios and cleaned up the code a bit.

@alxndrv alxndrv requested a review from sylvestre March 10, 2025 17:49
@alxndrv
Copy link
Copy Markdown
Contributor Author

alxndrv commented Mar 10, 2025

Should there be some kind of "lib" crate for common stuff like this?

yes, we try to publish content in uucore https://docs.rs/uucore/latest/uucore/

In regards to this, good to know. I can look into adding the shared CPU list parsing functionality to that later, and then replace the implementations present in util-linux in a separate PR. Does that sound fine?

@sylvestre
Copy link
Copy Markdown
Contributor

I am really really sorry but I took this #234 instead. it was more complete :(

sorry again :(

@sylvestre sylvestre closed this Mar 21, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (f91b55d) to head (88131fa).
Report is 8 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff     @@
##   main   #226   +/-   ##
===========================
===========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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