Copilot commented on code in PR #3517:
URL: https://github.com/apache/brpc/pull/3517#discussion_r3948664312
##########
src/bvar/default_variables.cpp:
##########
@@ -617,11 +618,27 @@ 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) {
+ struct utsname buf;
+ if (uname(&buf) != 0) {
LOG(ERROR) << "Fail to read kernel version";
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;
+ oss << buf.sysname << ' ' << buf.nodename << ' '
+ << buf.release << ' ' << buf.version << ' '
+ << buf.machine << ' ' << processor;
+#if !defined(__APPLE__)
Review Comment:
`GNU/Linux` is appended for all non-Apple platforms, which will produce
incorrect output on non-Linux targets (e.g., FreeBSD) and may differ from
`uname -ap` output on some Linux variants (e.g., Android). To preserve
compatibility with the previous `popen("uname -ap")` behavior, gate this suffix
more precisely (e.g., `__linux__`, and consider `__ANDROID__` separately) or
omit the OS suffix on platforms where it cannot be determined reliably via
`uname()`.
##########
src/bvar/default_variables.cpp:
##########
@@ -617,11 +618,27 @@ 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) {
+ struct utsname buf;
+ if (uname(&buf) != 0) {
LOG(ERROR) << "Fail to read kernel version";
return;
}
Review Comment:
When `uname()` fails, the log message drops the reason. Including `errno`
(or a strerror equivalent) would make operational debugging significantly
easier while preserving the same control flow.
##########
test/bvar_variable_unittest.cpp:
##########
@@ -462,6 +463,48 @@ TEST_F(VariableTest, dtor_waits_for_inflight_describe) {
ASSERT_TRUE(destructed.load());
}
+
+TEST_F(VariableTest, uname_returns_valid_kernel_info) {
+ struct utsname buf;
+ ASSERT_EQ(0, uname(&buf));
+
+ // Each field should be non-empty
+ ASSERT_GT(strlen(buf.sysname), 0u);
+ ASSERT_GT(strlen(buf.nodename), 0u);
+ ASSERT_GT(strlen(buf.release), 0u);
+ ASSERT_GT(strlen(buf.version), 0u);
+ ASSERT_GT(strlen(buf.machine), 0u);
+
+ // Build the string the same way ReadVersion does in default_variables.cpp
+#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
+ std::ostringstream oss;
+ oss << buf.sysname << ' ' << buf.nodename << ' '
+ << buf.release << ' ' << buf.version << ' '
+ << buf.machine << ' ' << processor;
+#if !defined(__APPLE__)
+ oss << " GNU/Linux";
+#endif
+ oss << '\n';
+ std::string content = oss.str();
+
+ // The result should contain all key fields
+ ASSERT_NE(content.find(buf.sysname), std::string::npos);
+ ASSERT_NE(content.find(buf.release), std::string::npos);
+ ASSERT_NE(content.find(buf.machine), std::string::npos);
+
+ // On Linux, sysname should be "Linux"; on macOS, "Darwin"
+#if defined(__linux__)
+ ASSERT_STREQ(buf.sysname, "Linux");
Review Comment:
The test duplicates the string-formatting logic but does not actually match
`ReadVersion` (notably missing the `hardware_platform` field and spacing), so
it can drift without catching regressions in the `kernel_version` bvar output.
Prefer asserting on the real exported bvar value (or a shared helper used by
both production and test) so the test verifies the behavior this PR is changing
rather than re-implementing a slightly different formatter.
--
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]
