refactor(proc-cap): centralize procfs start_time parsing#59
Conversation
Summary of ChangesHello @heidi-dang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the process start time parsing logic within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the procfs start_time parsing into a centralized helper function, which is a good improvement for code deduplication and testability. The overall logic for using the leader's start time to guard against PID reuse is sound. However, I've identified a critical issue in the new unit test that needs to be addressed, as it currently uses an incorrect test case. Additionally, I've suggested a couple of medium-severity improvements in the new parsing logic to align it better with modern C++ practices for resource management and efficiency.
| std::string line = | ||
| "1234 (my weird comm) R 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 999999 0 0"; |
There was a problem hiding this comment.
The test string for ParseStartTime appears to be incorrect. The starttime field in /proc/<pid>/stat is the 22nd field. In your parser, this corresponds to the 20th token after the closing parenthesis of the command name. The current test string has 999999 as the 21st token, which means the parser will actually read the 20th token (19) and the test will fail.
To fix this, 999999 should be the 20th token after (my weird comm). This involves removing the 19 token from the test string.
| std::string line = | |
| "1234 (my weird comm) R 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 999999 0 0"; | |
| std::string line = | |
| "1234 (my weird comm) R 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 999999 0 0"; |
| std::optional<uint64_t> read_proc_start_time_ticks(pid_t pid) { | ||
| char stat_path[256]; | ||
| snprintf(stat_path, sizeof(stat_path), "/proc/%d/stat", pid); | ||
| FILE* stat_file = fopen(stat_path, "r"); | ||
| if (!stat_file) | ||
| return std::nullopt; | ||
|
|
||
| char buf[4096]; | ||
| std::optional<uint64_t> result = std::nullopt; | ||
| if (!fgets(buf, sizeof(buf), stat_file)) { | ||
| fclose(stat_file); | ||
| return std::nullopt; | ||
| } | ||
| // Delegate to the parsing helper so tests can directly exercise it. | ||
| result = parse_start_time_from_stat_line(buf); | ||
|
|
||
| fclose(stat_file); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
The implementation of read_proc_start_time_ticks uses C-style file I/O (fopen, fgets, fclose). While correct in this simple case, using C++ streams and RAII (Resource Acquisition Is Initialization) is more idiomatic and generally safer, as it automatically handles resource cleanup, even in the presence of exceptions.
Consider refactoring to use std::ifstream, which would also simplify the code.
std::optional<uint64_t> read_proc_start_time_ticks(pid_t pid) {
// Note: this change requires including <fstream> and <string>,
// and removing <cstdio>.
std::ifstream stat_file("/proc/" + std::to_string(pid) + "/stat");
if (!stat_file) {
return std::nullopt;
}
std::string line;
if (!std::getline(stat_file, line)) {
return std::nullopt;
}
return parse_start_time_from_stat_line(line.c_str());
}| // parse token [t, s) | ||
| char tmp[64] = {0}; | ||
| int len = (int)(s - t); | ||
| if (len > 0 && len < (int)sizeof(tmp)) { | ||
| memcpy(tmp, t, len); | ||
| tmp[len] = '\0'; | ||
| uint64_t v = strtoull(tmp, nullptr, 10); | ||
| return v; | ||
| } | ||
| return std::nullopt; |
There was a problem hiding this comment.
The parsing logic currently copies the token for starttime into a temporary buffer before converting it to a uint64_t. This copy is unnecessary and can be avoided by parsing the number directly from the original buffer slice. Using strtoull with its endptr argument allows for in-place parsing and validation, which is more efficient.
// parse token [t, s)
char* end_ptr;
uint64_t v = strtoull(t, &end_ptr, 10);
if (end_ptr == s) {
return v;
}
return std::nullopt;
1 participant
Refactor only: deduplicates /proc//stat start_time parsing used by the leader start-time guard.
No behavioral change intended:
Verification: