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

config-linux: update type of LinuxCPU.Idle to *int64 #1146

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

wineway
Copy link
Contributor

@wineway wineway commented Apr 24, 2022

the config values should contain a value that means "do not modify it" within its range of values. for int64 in other cases, we usually use 0 to express this meaning, but in cpu.idle, 0 can not be regarded as this meaning.

by changing the type from int64to *int64, we can use nil solve this problem

@wineway
Copy link
Contributor Author

wineway commented Apr 24, 2022

😂I found this when I try to implement it, if we keep this type, we can only choose a value like -1 to mark it does not need to modify, but -1 might be used by the kernel in the future, and also ugly

and add cpu.idle to example in config.md

Signed-off-by: wineway <[email protected]>
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@wineway wineway changed the title config-linux: update cpu.Idle type of LinuxCPU to *int64 config-linux: update type of LinuxCPU.Idle to *int64 Apr 24, 2022
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

LGTM (and matches the description in https://github.com/opencontainers/runtime-spec/blob/2cf6663ca299f8454318ea7c797a8039d013fe8c/config-linux.md#cpu, which IMO doesn't need any updates beyond the OPTIONAL it already has)

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.

4 participants