Copilot commented on code in PR #3602:
URL: https://github.com/apache/brpc/pull/3602#discussion_r4259737151
##########
src/bvar/default_variables.cpp:
##########
@@ -107,25 +110,76 @@ static bool read_proc_status(ProcStat &stat) {
}
return true;
#elif defined(OS_MACOSX)
- // TODO(zhujiashun): get remaining state in MacOS.
- memset(&stat, 0, sizeof(stat));
- static pid_t pid = getpid();
- std::ostringstream oss;
- char cmdbuf[128];
- snprintf(cmdbuf, sizeof(cmdbuf),
- "ps -p %ld -o pid,ppid,pgid,sess"
- ",tpgid,flags,pri,nice | tail -n1", (long)pid);
- if (butil::read_command_output(oss, cmdbuf) != 0) {
- LOG(ERROR) << "Fail to read stat";
- return false;
+ const pid_t pid = getpid();
+ stat.pid = pid;
+ stat.ppid = getppid();
+ stat.pgrp = getpgrp();
+ const pid_t session = getsid(0);
+ stat.session = session >= 0 ? session : 0;
+ errno = 0;
+ const int nice = getpriority(PRIO_PROCESS, 0);
+ if (errno == 0)
+ stat.nice = nice;
+ struct proc_bsdinfo bsd = {};
Review Comment:
`stat` is no longer zero-initialized on macOS (the previous implementation
did `memset(&stat, 0, sizeof(stat))`). If any of the native API calls fail,
fields like `state`, `flags`, `priority`, `num_threads`, fault counters, etc.
can retain stale values from prior invocations. Reintroduce deterministic
initialization at the start of the macOS branch (e.g., zero-initialize `stat`
before setting individual fields).
##########
src/bvar/default_variables.cpp:
##########
@@ -232,24 +286,19 @@ static bool read_proc_memory(ProcMemory &m) {
return true;
#elif defined(OS_MACOSX)
// TODO(zhujiashun): get remaining memory info in MacOS.
- memset(&m, 0, sizeof(m));
- static pid_t pid = getpid();
- static int64_t pagesize = getpagesize();
- std::ostringstream oss;
- char cmdbuf[128];
- snprintf(cmdbuf, sizeof(cmdbuf), "ps -p %ld -o rss=,vsz=", (long)pid);
- if (butil::read_command_output(oss, cmdbuf) != 0) {
- LOG(ERROR) << "Fail to read memory state";
+ mach_task_basic_info_data_t info = {};
+ mach_msg_type_number_t count = MACH_TASK_BASIC_INFO_COUNT;
+ const kern_return_t result = task_info(
+ mach_task_self(), MACH_TASK_BASIC_INFO,
+ reinterpret_cast<task_info_t>(&info), &count);
+ if (result != KERN_SUCCESS) {
+ LOG_ONCE(WARNING) << "Cannot read own task memory, Mach error=" <<
result;
return false;
}
Review Comment:
On failure, the function now returns `false` without clearing `m`, whereas
the previous code path zeroed the struct before attempting collection. This can
leak stale memory values to callers that ignore the boolean or reuse the same
`ProcMemory` instance. Clear/initialize `m` before the `task_info` call (or at
least before returning `false`).
##########
src/bvar/default_variables.cpp:
##########
@@ -232,24 +286,19 @@ static bool read_proc_memory(ProcMemory &m) {
return true;
#elif defined(OS_MACOSX)
// TODO(zhujiashun): get remaining memory info in MacOS.
- memset(&m, 0, sizeof(m));
- static pid_t pid = getpid();
- static int64_t pagesize = getpagesize();
- std::ostringstream oss;
- char cmdbuf[128];
- snprintf(cmdbuf, sizeof(cmdbuf), "ps -p %ld -o rss=,vsz=", (long)pid);
- if (butil::read_command_output(oss, cmdbuf) != 0) {
- LOG(ERROR) << "Fail to read memory state";
+ mach_task_basic_info_data_t info = {};
+ mach_msg_type_number_t count = MACH_TASK_BASIC_INFO_COUNT;
+ const kern_return_t result = task_info(
+ mach_task_self(), MACH_TASK_BASIC_INFO,
+ reinterpret_cast<task_info_t>(&info), &count);
+ if (result != KERN_SUCCESS) {
+ LOG_ONCE(WARNING) << "Cannot read own task memory, Mach error=" <<
result;
Review Comment:
This log line prints only the numeric `kern_return_t`, which is difficult to
interpret operationally. Include the human-readable Mach error string as well
(e.g., via `mach_error_string(result)`) so failures can be diagnosed from logs
without needing to map error codes manually.
##########
src/bvar/default_variables.cpp:
##########
@@ -350,29 +399,19 @@ static int get_fd_count(int limit) {
for (; dr.Next() && count <= limit + 3; ++count) {}
return count - 3 /* skipped ., .. and the fd in dr*/;
#elif defined(OS_MACOSX)
- // TODO(zhujiashun): following code will cause core dump with some
- // probability under mac when program exits. Fix it.
- /*
- static pid_t pid = getpid();
- std::ostringstream oss;
- char cmdbuf[128];
- snprintf(cmdbuf, sizeof(cmdbuf),
- "lsof -p %ld | grep -v \"txt\" | wc -l", (long)pid);
- if (butil::read_command_output(oss, cmdbuf) != 0) {
- LOG(ERROR) << "Fail to read open files";
- return -1;
+ if (limit <= 0) {
+ return 0;
}
- const std::string& result = oss.str();
- int count = 0;
- if (sscanf(result.c_str(), "%d", &count) != 1) {
- PLOG(WARNING) << "Fail to sscanf";
+ std::vector<proc_fdinfo> fds(limit);
+ errno = 0;
+ // This query opens no additional FD.
+ const int bytes = proc_pidinfo(getpid(), PROC_PIDLISTFDS, 0,
+ fds.data(), fds.size() *
sizeof(proc_fdinfo));
+ if (bytes < 0 || (bytes == 0 && errno != 0)) {
+ PLOG(WARNING) << "Fail to list own file descriptors";
return -1;
}
- // skipped . and first column line
- count = count - 2;
- return std::min(count, limit);
- */
- return 0;
+ return bytes / sizeof(proc_fdinfo);
Review Comment:
The return value assumes `bytes` is always a clean multiple of
`sizeof(proc_fdinfo)`. If `proc_pidinfo` ever returns a partial size (or an
unexpected value), this will silently truncate and miscount. Add a guard that
validates `bytes % sizeof(proc_fdinfo) == 0` (and handle/report otherwise), and
consider clamping to `limit` defensively even though the provided buffer is
sized to `limit`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
