-
Notifications
You must be signed in to change notification settings - Fork 320
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
Change stat data types to match the related kernel data types #403
Comments
Well, I made a mistake in my conclusion. In C they just specify the minimum size of data types like You may check the Wikipedia for details and other resources:
As you may guess, they do the same in the Go for the As a result, the existent code to parse the stat fields work perfectly except for the fields |
I'll prepare a fix for those two fields. |
* Fix data types for CUTime and CSTime stat fields #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 <kulakov.home@gmail.com>
…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 <kulakov.home@gmail.com>
ACK, I believe this can be closed now. |
All seems to be fine so far, closing the issue. |
…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 <kulakov.home@gmail.com>
The man page for
/proc/[pid]/stat
describes data types that should be used to parse particular values in this stat file. So the following information about data types may be obtained from there:Here things like
%d
and%lu
are scanf(3) format specifiers (according to the man page). The full list of the used above specifiers is:%d
- forint
;%u
- forunsigned int
;%ld
- forlong
;%lu
- forunsigned long
;%llu
- forunsigned long long
;%s
- matches a sequence of non-white-space characters;%c
- matches a sequence of characters whose length is specified by the maximum field width (default 1).But in the code we have the following (just a few examples):
So instead of parsing
UTime
intounsigned long
(according to the man page), the library is trying to parse it intouint
that may beuint32
on machines with the 32-bit arch. As a result, we may get the value out of range errors. WithCUTime
it is even worse: instead ofsigned long
the library is trying to useuint
so any negative values may raise errors too.Such errors have been already faced in real use. Check #401 for details.
I propose to fix the field types according to the list above. It will break compatibility with any old code but as the library is only on the pre-1.0 stage it can be done easily. Additionally, in golang_client such values are converted into
float64
so the future migration may happen unnoticed.Useful links:
The text was updated successfully, but these errors were encountered: