Copilot commented on code in PR #3517:
URL: https://github.com/apache/brpc/pull/3517#discussion_r3950796040
##########
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);
+#elif defined(__APPLE__)
+ ASSERT_STREQ(buf.sysname, "Darwin");
+ ASSERT_EQ(content.find("GNU/Linux"), std::string::npos);
+#endif
Review Comment:
This test will likely fail on Android builds: `make_kernel_version_string()`
intentionally omits the `GNU/Linux` suffix when `__ANDROID__` is defined, but
the test asserts it for all `__linux__` targets. Update the `__linux__` branch
to exclude Android (e.g., `defined(__linux__) && !defined(__ANDROID__)`) or add
an Android-specific expectation.
##########
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:
This code uses `errno` and `strerror()` but the diff context doesn’t show a
direct include for the corresponding headers in this translation unit. To avoid
relying on transitive includes (which can break with unrelated include
refactors), include the appropriate headers explicitly (e.g., `<cerrno>` and
`<cstring>` or `<errno.h>` and `<string.h>`).
##########
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:
Adding `make_kernel_version_string` to a new header under `src/bvar/` makes
it easy for other code to include and start depending on it, which can
unintentionally expand the public API surface. If this helper is meant to be
internal-only for `default_variables.cpp` + tests, consider moving it to an
explicitly-internal header/location (or an internal namespace) so consumers
don’t treat it as a supported API.
##########
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
Review Comment:
The log message grammar is a bit off; consider changing \"Fail to read
kernel version\" to \"Failed to read kernel version\" for
clarity/professionalism (keeping the errno details as-is).
--
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]
