Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isolate-check-environment: check SMT and improve scaling_governor check #173

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pobrn
Copy link

@pobrn pobrn commented Dec 30, 2024

Please see the commit messages.

@gollux
Copy link
Member

gollux commented Dec 30, 2024

Overall, I like the changes, but I would prefer the if not to fail when cat returns a string with spaces. Please add a pair of quotes.

@pobrn
Copy link
Author

pobrn commented Dec 30, 2024

Sorry, can you specify where?

@gollux
Copy link
Member

gollux commented Dec 30, 2024

Eh, shell rules have bitten me again :)

I meant this:

if val=$(cat /sys/devices/system/cpu/smt/active 2>/dev/null) ; then

But surprisingly enough, even though word splitting is done in echo $(cat something), it is not done in a=$(cat something), so your code is actually safe. This is actually documented in man bash:

A variable may be assigned to by a statement [...] All values undergo tilde expansion, parameter and variable expansion, command substitution, arithmetic expansion, and quote removal [...] Word splitting and pathname expansion are not performed.

Still, I think that adding explicit quotes makes the code more obvious.

When a CPU is offline, reading its `scaling_governor` file will
return EBUSY, which makes the check fail. Simply skip the check
if the name of the scaling governor could not be read.
Check if simultaneous multithreading is enabled,
and if so, suggest disabling it.
@pobrn pobrn force-pushed the isolate_check_env_impr branch from 21c176d to ee2d8ca Compare December 30, 2024 21:11
@pobrn
Copy link
Author

pobrn commented Dec 31, 2024

Still, I think that adding explicit quotes makes the code more obvious.

Added " around both assignments; but note that there are other checks that have no ".

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.

2 participants