Copilot commented on code in PR #3517:
URL: https://github.com/apache/brpc/pull/3517#discussion_r3950261974
##########
src/bvar/default_variables.cpp:
##########
@@ -617,12 +619,13 @@ 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;
}
Review Comment:
The error log reads `errno` twice (for the integer and again for the
message). `errno` is not guaranteed to remain unchanged across intervening
calls (including within logging/streaming), so the message could mismatch the
printed numeric value.
Save `errno` to a local before logging, and reuse it for both parts of the
message (using `berror(saved_errno)` which is already used elsewhere in this
file).
##########
src/bvar/default_variables.h:
##########
@@ -0,0 +1,60 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#ifndef BVAR_DEFAULT_VARIABLES_H
+#define BVAR_DEFAULT_VARIABLES_H
+
+#include <sys/utsname.h> // struct utsname
+#include <sstream> // std::ostringstream
+#include <string> // std::string
+
+namespace bvar {
+
+// Build the value of the `kernel_version` bvar from a uname(2) result.
+// The field layout mirrors `uname -ap` on the major platforms:
+// Linux : sysname nodename release version machine processor machine
GNU/Linux
+// macOS : sysname nodename release version machine processor
+//
+// This is intentionally a header-only helper so that it is shared by both
+// default_variables.cpp and the unit tests. default_variables.o is stripped
+// from unit-test binaries (see BVAR_NOT_LINK_DEFAULT_VARIABLES in
+// variable.cpp), so keeping the formatting logic here lets tests exercise the
+// exact production formatter without depending on that object being linked.
+inline std::string make_kernel_version_string(const struct utsname& buf) {
Review Comment:
This adds a new inline helper in the public `bvar` namespace from an
installed header (`src/` headers are copied into the install include tree).
That effectively commits to `make_kernel_version_string()` as part of the
library API surface.
Consider moving this into `bvar::detail` (and/or a `bvar/detail/...` header)
and updating call sites accordingly, so downstream users don’t start depending
on an internal formatting helper.
##########
test/bvar_variable_unittest.cpp:
##########
@@ -462,6 +464,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(std::strlen(buf.sysname), 0u);
+ ASSERT_GT(std::strlen(buf.nodename), 0u);
+ ASSERT_GT(std::strlen(buf.release), 0u);
+ ASSERT_GT(std::strlen(buf.version), 0u);
+ ASSERT_GT(std::strlen(buf.machine), 0u);
+
+ // Exercise the exact formatter that backs the kernel_version bvar. It is a
+ // header-only helper shared with default_variables.cpp, so this validates
+ // the real production formatting without depending on default_variables.o
+ // being linked into the unit-test binary: that object is stripped via
+ // BVAR_NOT_LINK_DEFAULT_VARIABLES, so the bvar is not registered here and
+ // describe_exposed("kernel_version") would return nothing.
+ const std::string content = bvar::make_kernel_version_string(buf);
+ ASSERT_FALSE(content.empty());
+
+ // The formatted value should contain all the key uname fields.
+ ASSERT_NE(content.find(buf.sysname), std::string::npos);
+ ASSERT_NE(content.find(buf.nodename), std::string::npos);
+ ASSERT_NE(content.find(buf.release), std::string::npos);
+ ASSERT_NE(content.find(buf.version), std::string::npos);
+ ASSERT_NE(content.find(buf.machine), std::string::npos);
+
+ // The trailing newline must be preserved to match the previous
+ // popen("uname -ap") output that this bvar used to expose.
+ ASSERT_EQ('\n', content[content.size() - 1]);
+
+ // On Linux, sysname is "Linux" and the OS suffix is appended; on macOS,
+ // sysname is "Darwin" and there is no OS suffix (both match `uname -ap`).
+#if defined(__linux__)
+ ASSERT_STREQ(buf.sysname, "Linux");
+ ASSERT_NE(content.find("GNU/Linux"), std::string::npos);
Review Comment:
This Linux-only assertion block will also compile on Android (where
`__linux__` is defined), but `make_kernel_version_string()` intentionally does
*not* append `GNU/Linux` when `__ANDROID__` is defined. As written, the test
would fail on Android builds.
Match the test’s platform condition to the production formatter (exclude
`__ANDROID__`, or add an Android-specific branch).
--
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]
