Fix data types for CUTime and CSTime stat fields #403 #404

vykulakov · 2021-07-29T11:39:03Z

These two stat fields (CUTime and CSTime) in the /proc/[pid]/stat file should have the signed long data type according to the documentation. But currently, in the code, their data type is just unsigned int. This commit fixes it and adds more tests.

See for details:

Signed-off-by: Vyacheslav Kulakov [email protected]

These two stat fields (CUTime and CSTime) in the /proc/[pid]/stat file should have the signed long data type according to the documentation. But currently in the code their data type is just unsigned int. This commit fixes it and adds more tests. See for details: * https://man7.org/linux/man-pages/man5/proc.5.html * https://man7.org/linux/man-pages/man3/scanf.3.html Signed-off-by: Vyacheslav Kulakov <[email protected]>

Signed-off-by: Vyacheslav Kulakov <[email protected]>

Signed-off-by: Vyacheslav Kulakov [email protected] Signed-off-by: Vyacheslav Kulakov <[email protected]>

discordianfish

Thanks, LGTM!

vykulakov · 2021-07-30T11:41:24Z

It seems I cannot merge this PR. Do we need to wait for yet another approvement?

discordianfish · 2021-08-02T12:06:57Z

Yeah we usually need 2 LGTMs. @SuperQ can you have a look?

SuperQ · 2021-08-09T15:20:55Z

Hrm, we just recently changed these from int to int64 intentionally. We had issues like prometheus/node_exporter#2110.

vykulakov · 2021-08-10T09:32:42Z

Hrm, we just recently changed these from int to int64 intentionally. We had issues like prometheus/node_exporter#2110.

@SuperQ yep, It was me who changed it to int64. But this was too much for the ignored field as they should be just unsigned, that's it. I reread the docs and found that we may be a bit more accurate with specifying data types for the fields. Also, there are tests for cases like in the issue you mentioned. So all should be fine.

Or I may just revert those types for the ignored fields to int64. But it affects nothings.

vykulakov · 2021-08-10T10:58:49Z

@SuperQ Actually, the most important thing in the previous PR #402 was replacing int with uint for most of the ignored fields instead of replacing int with int64. I used the 64-bit versions of types because didn't know some C data types aspects that I know now. Sorry for that.

SuperQ · 2021-08-16T17:07:24Z

@vykulakov Ok, the only thing I'm worried about is that even tho it says one thing in the procfs docs, I did some digging into the code in the kernel. I'm not so good at C, but it seems like some of the data types are "long long", hard-coding some of these values as 64-bit. But, again, I'm no kernel expert.

vykulakov · 2021-08-17T08:05:55Z

@SuperQ could you point me to this place in the kernel, please? It'll save some time for me.

SuperQ · 2021-08-18T09:24:39Z

I think the actual print definition is https://github.com/torvalds/linux/blob/614cb2751d3150850d459bee596c397f344a7936/fs/proc/array.c#L571-L610.

Everything there is marked long long, or 64-bit. It seems like the man page is out of date compared to the kernel. Maybe the underlying data type is just a long, but it's now unconditionally printed as a long long.

This seems to be the commit that changed the output.

proc_stat.go

Signed-off-by: Vyacheslav Kulakov [email protected] Signed-off-by: Vyacheslav Kulakov <[email protected]>

vykulakov · 2021-08-30T20:19:04Z

I think the actual print definition is https://github.com/torvalds/linux/blob/614cb2751d3150850d459bee596c397f344a7936/fs/proc/array.c#L571-L610.

Everything there is marked long long, or 64-bit. It seems like the man page is out of date compared to the kernel. Maybe the underlying data type is just a long, but it's now unconditionally printed as a long long.

This seems to be the commit that changed the output.

That means that all other fields get affected again. I mean on 32-bit systems most of the fields will take 4 bytes instead of 8 bytes as in that kernel change.

So this PR doesn't make much sense now as it should change all the fields instead just CUTime and CSTime.

SuperQ

LGTM, Thanks!

dswarbrick · 2022-09-06T00:19:47Z

proc_stat_test.go

+		{name: "waited for children user time", want: math.MinInt64, have: s.CUTime},
+		{name: "waited for children system time", want: math.MaxInt64, have: s.CSTime},


Using MinInt64 / MaxInt64 here causes tests to fail on 32-bit archs, since these are untyped constants, being assigned to machine-native ints (in struct a few lines up).

Additionally, the supplied test data will simply overflow the ProcStat.CUTime and ProcStat.CSTime members, since they are also declared as machine-native ints. Either the struct needs to explicitly use int64 types, or you need to use 32-bit safe test data.

Incidentally, this is not the first time that there has been a test regression for 32-bit arch in this package. It would be nice if the CI pipeline would also test on 32-bit archs, as I presume they are still officially supported.

Makes sense, do you want to open an issue for testing on 32 bit archs? We need to see how to make that possible (let alone who has time to work on it) but agreed that we should track that at least.

I was able to reproduce the 32-bit test failure simply by exporting GOARCH=386 before running the tests.

…etheus#404) * Fix data types for CUTime and CSTime stat fields prometheus#403 These two stat fields (CUTime and CSTime) in the /proc/[pid]/stat file should have the signed long data type according to the documentation. But currently in the code their data type is just unsigned int. This commit fixes it and adds more tests. See for details: * https://man7.org/linux/man-pages/man5/proc.5.html * https://man7.org/linux/man-pages/man3/scanf.3.html Signed-off-by: Vyacheslav Kulakov <[email protected]>

vykulakov added 3 commits July 29, 2021 14:51

Fix test names

850087e

Signed-off-by: Vyacheslav Kulakov <[email protected]>

Sign the commit

6315b9c

Signed-off-by: Vyacheslav Kulakov [email protected] Signed-off-by: Vyacheslav Kulakov <[email protected]>

discordianfish approved these changes Jul 30, 2021

View reviewed changes

discordianfish requested a review from SuperQ August 2, 2021 12:06

timja mentioned this pull request Aug 13, 2021

Fix data types for ignored stat fields #401 #402

Merged

discordianfish mentioned this pull request Aug 14, 2021

Crashes on Mac M1 arm64 prometheus/node_exporter#2120

Closed

SuperQ requested changes Aug 26, 2021

View reviewed changes

proc_stat.go Outdated Show resolved Hide resolved

Revert types for the ignored fields

f892ce9

Signed-off-by: Vyacheslav Kulakov [email protected] Signed-off-by: Vyacheslav Kulakov <[email protected]>

SuperQ approved these changes Aug 30, 2021

View reviewed changes

SuperQ merged commit 21d221e into prometheus:master Aug 30, 2021

dswarbrick reviewed Sep 6, 2022

View reviewed changes

dswarbrick mentioned this pull request Sep 6, 2022

TestProcStatLimits: Int overflow on 32 bits arches #432

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix data types for CUTime and CSTime stat fields #403 #404

Fix data types for CUTime and CSTime stat fields #403 #404

vykulakov commented Jul 29, 2021 •

edited

Loading

discordianfish left a comment

vykulakov commented Jul 30, 2021

discordianfish commented Aug 2, 2021

SuperQ commented Aug 9, 2021 •

edited

Loading

vykulakov commented Aug 10, 2021

vykulakov commented Aug 10, 2021

SuperQ commented Aug 16, 2021

vykulakov commented Aug 17, 2021 •

edited

Loading

SuperQ commented Aug 18, 2021

vykulakov commented Aug 30, 2021

SuperQ left a comment

dswarbrick Sep 6, 2022 •

edited

Loading

discordianfish Sep 6, 2022

dswarbrick Sep 6, 2022

		{name: "waited for children user time", want: math.MinInt64, have: s.CUTime},
		{name: "waited for children system time", want: math.MaxInt64, have: s.CSTime},

Fix data types for CUTime and CSTime stat fields #403 #404

Fix data types for CUTime and CSTime stat fields #403 #404

Conversation

vykulakov commented Jul 29, 2021 • edited Loading

discordianfish left a comment

Choose a reason for hiding this comment

vykulakov commented Jul 30, 2021

discordianfish commented Aug 2, 2021

SuperQ commented Aug 9, 2021 • edited Loading

vykulakov commented Aug 10, 2021

vykulakov commented Aug 10, 2021

SuperQ commented Aug 16, 2021

vykulakov commented Aug 17, 2021 • edited Loading

SuperQ commented Aug 18, 2021

vykulakov commented Aug 30, 2021

SuperQ left a comment

Choose a reason for hiding this comment

dswarbrick Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

discordianfish Sep 6, 2022

Choose a reason for hiding this comment

dswarbrick Sep 6, 2022

Choose a reason for hiding this comment

vykulakov commented Jul 29, 2021 •

edited

Loading

SuperQ commented Aug 9, 2021 •

edited

Loading

vykulakov commented Aug 17, 2021 •

edited

Loading

dswarbrick Sep 6, 2022 •

edited

Loading