Copilot commented on code in PR #3517:
URL: https://github.com/apache/brpc/pull/3517#discussion_r3948403895
##########
src/bvar/default_variables.cpp:
##########
@@ -617,11 +618,26 @@ 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
+ std::ostringstream oss;
+ oss << buf.sysname << ' ' << buf.nodename << ' '
+ << buf.release << ' ' << buf.version << ' '
+ << buf.machine << ' ' << processor;
+#if !defined(__APPLE__)
+ oss << " GNU/Linux";
Review Comment:
The constructed output does not appear equivalent to `uname -ap` on common
Linux distros. `uname -a` (and therefore `uname -ap`) typically includes an
additional “hardware platform” field (often a third arch token) before the OS
string (e.g., `... x86_64 x86_64 x86_64 GNU/Linux`). The new formatting emits
only two arch tokens (`machine` and `processor`) plus the OS string, which is a
user-visible bvar output change and contradicts the PR description’s “output
format remains equivalent” claim. Consider updating the formatting to match the
previous command output more closely (e.g., include an additional field
consistent with what `uname -a` would have produced on Linux), or update the PR
description / add documentation stating the output format intentionally changed.
##########
src/bvar/default_variables.cpp:
##########
@@ -617,11 +618,26 @@ 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:
On `uname()` failure, the log message drops the failure cause (errno).
Including `errno`/`strerror(errno)` would make this actionable for debugging
(especially in constrained/container environments).
##########
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';
Review Comment:
This test validates `uname()` and a reimplementation of the formatting, but
it does not exercise the actual `kernel_version` bvar path (i.e., that the
bvar’s `describe()`/value uses the new `ReadVersion` output). As written, the
test can still pass even if `ReadVersion` formatting diverges (or if
`kernel_version` returns something else entirely). Consider asserting against
the observable bvar output (e.g., fetching the `kernel_version` variable’s
described value) and comparing it to an expected string/pattern derived from
`uname()`.
##########
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));
Review Comment:
This test validates `uname()` and a reimplementation of the formatting, but
it does not exercise the actual `kernel_version` bvar path (i.e., that the
bvar’s `describe()`/value uses the new `ReadVersion` output). As written, the
test can still pass even if `ReadVersion` formatting diverges (or if
`kernel_version` returns something else entirely). Consider asserting against
the observable bvar output (e.g., fetching the `kernel_version` variable’s
described value) and comparing it to an expected string/pattern derived from
`uname()`.
##########
test/bvar_variable_unittest.cpp:
##########
@@ -19,7 +19,8 @@
#include <pthread.h> // pthread_*
#include <unistd.h> // usleep
-
+#include <sys/utsname.h> // uname
+#include <string.h> // strlen
Review Comment:
Prefer C++ headers/functions here (`#include <cstring>` and `std::strlen`)
rather than the C header `<string.h>` and global `strlen`, to keep the test
consistent with C++ style and avoid namespace pollution.
##########
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);
Review Comment:
Prefer C++ headers/functions here (`#include <cstring>` and `std::strlen`)
rather than the C header `<string.h>` and global `strlen`, to keep the test
consistent with C++ style and avoid namespace pollution.
--
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]
