-
Notifications
You must be signed in to change notification settings - Fork 19
v1: fix a regression when reading cpuacct.usage_all #47
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
base: main
Are you sure you want to change the base?
Conversation
When reading cpuacct.usage_all, we assume each line contains exactly three columns; Lines with a different number of columns should be skipped. However, a regression was introduced when the column count exceeds three. The parameter 'n' in strings.SplitN was changed to 3, causing the split result to always contain exactly three elements—any additional columns are concatenated into the third field. This breaks subsequent unit parsing. Reported-by: vimiix <[email protected]> Signed-off-by: lifubang <[email protected]>
|
Even though cgroupv1 is being deprecated, this regression should still be addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do better:
- Check if the first line (that we currently discard) starts with "cpu user system", and if not, bail out (
usageKernelMode, usageUserMode, nil). - When reading, just ignore the 4th and subsequent values instead of discarding the whole line.
This way, if a newer kernel will add something (which I very much doubt because cgroup v1 is over) we are ready for it.
Or, we can just fix the regression (using this patch as is).
I don't know if anyone has done localization for these strings.
However, what if someone adds extra columns before the user and system fields? In that case, assuming the first three columns are valid becomes unreliable. So I think we should skip reading if there are more than 3 columns, because we can't trust these values. |
|
FWIW, if this kind of change breaks us we should report it as a kernel regression. |
Not talking about localization here. We've seen that someone patched the kernel to add more values. We only know about the first three, so instead of rejecting the file with more than 3 (which is what the current code as well as your PR does), we can check that the first three values are as expected, and use those if they are, or return empty stats if there's something unexpected in there.
This is why I wanted to check that the first line starts with "cpu user system" (and not because of localization). If it does -- we can probably use the first 3 numbers. If it doesn't -- we return empty stats.
|
It's not in the upstream kernel. |
When reading cpuacct.usage_all, we assume each line contains exactly three columns; Lines with a different number of columns should be skipped.
However, a regression was introduced when the column count exceeds three. The parameter 'n' in strings.SplitN was changed to 3, causing the split result to always contain exactly three elements—any additional columns are concatenated into the third field. This breaks subsequent unit parsing.
Fixes: #46
Reported-by: vimiix [email protected]