Copilot commented on code in PR #3517:
URL: https://github.com/apache/brpc/pull/3517#discussion_r3951526308
##########
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 fail on Android: `__linux__` is defined so it enters the
Linux branch, but `make_kernel_version_string()` intentionally does *not*
append `" GNU/Linux"` when `__ANDROID__` is defined. Update the preprocessor
checks (and/or assertions) to treat `__ANDROID__` separately (e.g.,
`defined(__linux__) && !defined(__ANDROID__)` for the `"GNU/Linux"` assertion).
##########
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:
The formatter claims to “mirror `uname -ap`”, but `uname(2)` doesn’t provide
`processor` (`-p`) or `hardware platform` (`-i`) fields, so filling them with
`buf.machine` may change the observable output on some systems (e.g., where
`uname -p` reports `unknown` or differs from `-m`). If strict backward output
compatibility is required, consider either (a) explicitly documenting this as a
best-effort approximation (and updating the PR description/comments
accordingly), or (b) adjusting the formatting to match your previous production
behavior more precisely on the platforms you support.
##########
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:
`errno` is read twice and `strerror(errno)` may observe a different value if
`errno` changes between evaluations; also this block requires the appropriate
headers for `errno`/`strerror` to be reliably available. Capture `errno` into a
local `int err = errno;` before logging and use `err` for both the numeric
value and message; ensure the file includes the correct headers (e.g.,
`<cerrno>` and `<cstring>` or equivalents) if they aren’t already included
elsewhere in this translation unit.
##########
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
Review Comment:
The formatter claims to “mirror `uname -ap`”, but `uname(2)` doesn’t provide
`processor` (`-p`) or `hardware platform` (`-i`) fields, so filling them with
`buf.machine` may change the observable output on some systems (e.g., where
`uname -p` reports `unknown` or differs from `-m`). If strict backward output
compatibility is required, consider either (a) explicitly documenting this as a
best-effort approximation (and updating the PR description/comments
accordingly), or (b) adjusting the formatting to match your previous production
behavior more precisely on the platforms you support.
##########
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) {
+#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(__linux__) && !defined(__ANDROID__)
+ // `uname -a` appends the hardware platform and the operating-system
+ // identifier on Linux; the hardware platform equals `machine` here.
+ oss << ' ' << buf.machine << " GNU/Linux";
+#endif
Review Comment:
The formatter claims to “mirror `uname -ap`”, but `uname(2)` doesn’t provide
`processor` (`-p`) or `hardware platform` (`-i`) fields, so filling them with
`buf.machine` may change the observable output on some systems (e.g., where
`uname -p` reports `unknown` or differs from `-m`). If strict backward output
compatibility is required, consider either (a) explicitly documenting this as a
best-effort approximation (and updating the PR description/comments
accordingly), or (b) adjusting the formatting to match your previous production
behavior more precisely on the platforms you support.
--
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]
