JosiahWI commented on code in PR #13686:
URL: https://github.com/apache/trafficserver/pull/13686#discussion_r4016420747
##########
src/api/InkAPITest.cc:
##########
@@ -946,7 +946,7 @@ synserver_vc_accept(TSCont contp, TSEvent event, void *data)
TSAssert(s->magic == MAGIC_ALIVE);
if (event == TS_EVENT_NET_ACCEPT_FAILED) {
- if (s && s->accept_port != SYNSERVER_DUMMY_PORT) {
+ if (s->accept_port != SYNSERVER_DUMMY_PORT) {
Review Comment:
We can assume `s` is not `nullptr` because of the magic value assertion on
line 946.
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -106,8 +106,10 @@ DbgCtl dbg_ctl{"rpc.test.client"};
bool
try_setup_rpc_test_paths(fs::path const &base, std::string &error)
{
- auto const dir_template = (base / rpc_test_dir_template).string();
- auto const socket_path = (fs::path{dir_template} /
rpc_test_socket_name).string();
+ fs::path const template_path = base / rpc_test_dir_template;
Review Comment:
Missing include for `fs::path` (http/remap/PluginDso.h). I don't think we
should depend on http/remap just for a namespace alias. Other tests have their
own `namespace fs = swoc::file`; consider doing that for consistency (and
including the correct libswoc header).
##########
src/proxy/http/remap/unit-tests/test_PluginFactory.cc:
##########
@@ -395,7 +395,7 @@ SCENARIO("loading plugins", "[plugin][core]")
WHEN("config using nonexisting absolute plugin file name")
{
- fs::path relativeExistingPath = pluginName;
+ fs::path relativeExistingPath = std::move(pluginName);
Review Comment:
Missing `<utility>` include.
##########
src/api/InkAPITest.cc:
##########
@@ -2084,8 +2084,14 @@ REGRESSION_TEST(SDK_API_TSfopen)(RegressionTest *test,
int /* atype ATS_UNUSED *
// Create unique tmp _file_name_, do not use any TS file_name
snprintf(write_file_name, PATH_NAME_MAX, "/tmp/%sXXXXXX",
ts::filename::PLUGIN);
- int write_file_fd; // this file will be reopened below
- if ((write_file_fd = mkstemp(write_file_name)) <= 0) {
+
+ // Narrow the umask across mkstemp so the temporary file cannot land in /tmp
+ // group or world accessible, whatever the process umask happens to be.
+ const mode_t old_umask = umask(S_IRWXG | S_IRWXO);
+ int write_file_fd = mkstemp(write_file_name); // this file will be
reopened below
+
+ umask(old_umask);
Review Comment:
Copilot is right. The `umask` calls here are redundant.
##########
src/api/InkAPITest.cc:
##########
@@ -7996,9 +8002,7 @@ load(const char *append_string)
p = TSIOBufferBlockWriteStart(blk, &avail);
ink_strlcpy(p, append_string, avail);
- if (append_string != nullptr) {
- TSIOBufferProduce(append_buffer, strlen(append_string));
- }
+ TSIOBufferProduce(append_buffer, strlen(append_string));
Review Comment:
Nit: consider asserting `nullptr != append_string` at the top of the
function.
##########
src/proxy/http/remap/unit-tests/test_PluginFactory.cc:
##########
@@ -395,7 +395,7 @@ SCENARIO("loading plugins", "[plugin][core]")
WHEN("config using nonexisting absolute plugin file name")
{
- fs::path relativeExistingPath = pluginName;
+ fs::path relativeExistingPath = std::move(pluginName);
Review Comment:
If you're going to fix this here, please do the `std::move` in the other
sections as well for consistency.
##########
src/records/unit_tests/test_ConfigRegistry.cc:
##########
@@ -30,7 +30,7 @@ using config::ConfigRegistry;
using config::ConfigSource;
// Shared no-op handler for test registrations
-static config::ConfigReloadHandler noop_handler = [](ConfigContext) {};
+static config::ConfigReloadHandler noop_handler = [](ConfigContext const &) {};
Review Comment:
How does this work? `config::ConfigReloadHandler` has type
`std::function<void(ConfigContext)>`.
--
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]
