Copilot commented on code in PR #3517:
URL: https://github.com/apache/brpc/pull/3517#discussion_r3939858950
##########
src/bvar/default_variables.cpp:
##########
@@ -617,11 +618,15 @@ 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;
}
+ std::ostringstream oss;
+ oss << buf.sysname << ' ' << buf.nodename << ' '
+ << buf.release << ' ' << buf.version << ' '
+ << buf.machine << ' ' << buf.machine;
content.append(oss.str());
Review Comment:
This change claims to keep output equivalent to `uname -ap`, but it (1)
drops the trailing newline that the previous popen-based implementation
preserved, and (2) uses buf.machine for both `-m` and `-p` (on macOS `uname -p`
can differ, e.g. `arm` vs `arm64`). This makes `kernel_version` output
observably different for existing scrapers.
##########
test/bvar_variable_unittest.cpp:
##########
@@ -19,6 +19,7 @@
#include <pthread.h> // pthread_*
#include <unistd.h> // usleep
+#include <sys/utsname.h> // uname
Review Comment:
The new test calls strlen() but this file doesn’t include
<string.h>/<cstring>, which can cause build failures depending on the standard
library headers pulled in indirectly.
##########
test/bvar_variable_unittest.cpp:
##########
@@ -462,6 +463,37 @@ 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
+ std::ostringstream oss;
+ oss << buf.sysname << ' ' << buf.nodename << ' '
+ << buf.release << ' ' << buf.version << ' '
+ << buf.machine << ' ' << buf.machine;
+ std::string content = oss.str();
Review Comment:
The added unit test only validates that uname() returns non-empty fields and
then reconstructs the production string; it doesn’t exercise the actual
`kernel_version` bvar path in default_variables.cpp (nor verify the output
matches the previous `uname -ap` behavior). Consider adding a dedicated test
that forces linking/exposing of default variables and asserts the
`kernel_version` value format.
--
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]
