Copilot commented on code in PR #3517:
URL: https://github.com/apache/brpc/pull/3517#discussion_r3949796776
##########
src/bvar/default_variables.cpp:
##########
@@ -20,6 +20,7 @@
#include <unistd.h> // getpagesize
#include <sys/types.h>
#include <sys/resource.h> // getrusage
+#include <sys/utsname.h> // uname
Review Comment:
ReadVersion() now logs strerror(errno) but this file does not include
<string.h>/<cstring>, which can cause build failures under strict C++ warnings
(strerror undeclared). Add the proper header include near the other C headers.
##########
src/bvar/default_variables.cpp:
##########
@@ -617,11 +618,28 @@ static void get_cmdline(std::ostream& os, void*) {
struct ReadVersion {
std::string content;
ReadVersion() {
- std::ostringstream oss;
- if (butil::read_command_output(oss, "uname -ap") != 0) {
- LOG(ERROR) << "Fail to read kernel version";
+ struct utsname buf;
+ if (uname(&buf) != 0) {
+ LOG(ERROR) << "Fail to read kernel version, errno=" << errno
+ << " (" << strerror(errno) << ")";
return;
}
+#if defined(__APPLE__) && (defined(__aarch64__) || defined(__arm64__))
+ const char* processor = "arm";
+#elif defined(__APPLE__) && defined(__x86_64__)
+ const char* processor = "i386";
+#else
+ const char* processor = buf.machine;
+#endif
+ const char* hardware_platform = buf.machine;
+ std::ostringstream oss;
Review Comment:
The previous implementation used `uname -ap` (userland uname), which can
produce processor/hardware-platform values that are not available via the
`uname()` syscall. The current fallback (`processor`/`hardware_platform` =
buf.machine) is a best-effort and may not be output-equivalent on some systems
(e.g., where `uname -p` reports "unknown"). If strict backward compatibility is
required, consider matching coreutils behavior more closely or clarifying the
compatibility guarantee (and test expectations).
##########
test/bvar_variable_unittest.cpp:
##########
@@ -462,6 +463,42 @@ TEST_F(VariableTest, dtor_waits_for_inflight_describe) {
ASSERT_TRUE(destructed.load());
}
+
+TEST_F(VariableTest, uname_returns_valid_kernel_info) {
Review Comment:
Test name is misleading: it primarily validates the exported
`kernel_version` bvar contains `uname()` fields, not that `uname()` itself
returns valid info. Renaming the test would make failures easier to interpret.
--
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]
