You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "traeak (via GitHub)" <gi...@apache.org> on 2026/10/14 15:41:40 UTC
[PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
traeak opened a new pull request, #13837:
URL: https://github.com/apache/trafficserver/pull/13837
A '#include' with no file name built a std::string from a null pointer, which crashes with libc++ and throws an unhelpful basic_string error with libstdc++. Reject it with a clear message, and keep the nested load error when an included file fails.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6195154598
[approve ci clang-analyzer]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6199952619
[approve ci autest 3]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6195681926
[approve ci clang-analyzer]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on code in PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#discussion_r4275668810
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -251,17 +251,27 @@ NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::strings
std::ifstream file(fileName);
if (file.is_open()) {
while (std::getline(file, line)) {
- if (line[0] == '#') {
- tok = strtok_r(const_cast<char *>(line.c_str()), sep, &last);
- if (tok != nullptr && strcmp(tok, "#include") == 0) {
- std::string f = strtok_r(nullptr, sep, &last);
+ std::string_view line_view{line};
+ if (line_view.starts_with('#')) {
+ const auto directive_end = line_view.find_first_of(separators);
Review Comment:
NO
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6202067265
[approve ci clang-analyzer]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6193741629
[approve ci clang-analyzer]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#discussion_r4274354380
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -251,17 +251,27 @@ NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::strings
std::ifstream file(fileName);
if (file.is_open()) {
while (std::getline(file, line)) {
- if (line[0] == '#') {
- tok = strtok_r(const_cast<char *>(line.c_str()), sep, &last);
- if (tok != nullptr && strcmp(tok, "#include") == 0) {
- std::string f = strtok_r(nullptr, sep, &last);
+ std::string_view line_view{line};
+ if (line_view.starts_with('#')) {
+ const auto directive_end = line_view.find_first_of(separators);
+ if (line_view.substr(0, directive_end) == "#include") {
+ if (directive_end == std::string_view::npos) {
+ throw std::invalid_argument("'#include' without a file name in '" + fileName + "'");
+ }
+ const auto include_start = line_view.find_first_not_of(separators, directive_end);
+ if (include_start == std::string_view::npos) {
+ throw std::invalid_argument("'#include' without a file name in '" + fileName + "'");
+ }
Review Comment:
The same `std::invalid_argument` message is constructed in two branches. To reduce duplication and keep future changes consistent, consider centralizing this into a single helper/constant (or one guarded condition) that throws the message once.
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -195,10 +196,9 @@ void
NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::stringstream &doc,
std::unordered_set<std::string> &include_once)
{
- const char *sep = " \t";
- char *tok, *last;
- struct stat buf;
- std::string line;
+ constexpr std::string_view separators = " \t";
Review Comment:
`separators` does not include `'\r'`, so CRLF files can break directive detection/filename extraction (e.g., `#include foo.yaml\r` yields `f == "foo.yaml\r"`; `#include\r` won’t match `"#include"` and won’t be rejected). Consider including `'\r'` in `separators` so Windows line endings don’t bypass validation or corrupt the include filename.
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -251,17 +251,27 @@ NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::strings
std::ifstream file(fileName);
if (file.is_open()) {
while (std::getline(file, line)) {
- if (line[0] == '#') {
- tok = strtok_r(const_cast<char *>(line.c_str()), sep, &last);
- if (tok != nullptr && strcmp(tok, "#include") == 0) {
- std::string f = strtok_r(nullptr, sep, &last);
+ std::string_view line_view{line};
+ if (line_view.starts_with('#')) {
+ const auto directive_end = line_view.find_first_of(separators);
Review Comment:
`separators` does not include `'\r'`, so CRLF files can break directive detection/filename extraction (e.g., `#include foo.yaml\r` yields `f == "foo.yaml\r"`; `#include\r` won’t match `"#include"` and won’t be rejected). Consider including `'\r'` in `separators` so Windows line endings don’t bypass validation or corrupt the include filename.
##########
src/proxy/http/remap/unit-tests/test_NextHopStrategyFactory.cc:
##########
@@ -40,6 +40,18 @@
SCENARIO("factory tests loading yaml configs", "[loadConfig]")
{
+ GIVEN("Loading a strategy.yaml with a bare '#include' directive.")
Review Comment:
The test text references `strategy.yaml`, but the PR title/description refer to `strategies.yaml`, and the fixture file used is `include-missing-strategy.yaml`. Consider aligning the wording so the scenario description matches the actual config name being tested, reducing confusion when reading test output.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6191639966
agreed on the TS_SRC_DIR I will take that out.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6192503303
[approve ci debian]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak merged PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#discussion_r4273180693
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -254,14 +254,18 @@ NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::strings
if (line[0] == '#') {
tok = strtok_r(const_cast<char *>(line.c_str()), sep, &last);
if (tok != nullptr && strcmp(tok, "#include") == 0) {
- std::string f = strtok_r(nullptr, sep, &last);
+ const char *include = strtok_r(nullptr, sep, &last);
+ if (include == nullptr) {
+ throw std::invalid_argument("'#include' without a file name in '" + fileName + "'");
+ }
+ std::string f = include;
Review Comment:
Parsing with `strtok_r(const_cast<char*>(line.c_str()), ...)` mutates the buffer returned by `std::string::c_str()`, which is undefined behavior in C++. Since this code is being touched, consider switching to a non-mutating parse approach (e.g., trim + prefix check + substring, `std::string_view` tokenization, or `std::istringstream`) to avoid UB and make the include parsing more robust.
##########
src/proxy/http/remap/unit-tests/include-missing-strategy.yaml:
##########
@@ -0,0 +1,37 @@
+# @file
+#
+# Unit test data strategy.yaml file for testing the NextHopStrategyFactory
+#
+# @section license License
+#
+# 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.
+#
+# @section details Details
+#
+#
+# unit test strategy.yaml with a '#include' directive missing its file name
+#
+#include
+#
+strategies:
+ - strategy: 'strategy-1'
+ policy: consistent_hash
+ hash_key: cache_key
+ go_direct: false
+ groups:
+ - *g1
+ scheme: http
Review Comment:
This fixture includes an undefined YAML alias (`*g1`). It currently won’t matter because the loader throws on the bare `#include` before YAML parse, but it makes the test data fragile (future refactors to include handling could cause the test to fail for the wrong reason). To keep the fixture focused, either remove the `strategies` block entirely (since the test only checks that nothing loads) or define a local anchor for `g1` so the YAML is internally valid.
##########
src/proxy/http/remap/unit-tests/test_NextHopStrategyFactory.cc:
##########
@@ -40,6 +40,17 @@
SCENARIO("factory tests loading yaml configs", "[loadConfig]")
{
+ GIVEN("Loading a strategy.yaml with a bare '#include' directive.")
+ {
+ NextHopStrategyFactory nhf(TS_SRC_DIR "/include-missing-strategy.yaml");
+
+ THEN("no strategies are loaded")
+ {
+ REQUIRE(nhf.strategies_loaded == false);
+ REQUIRE(nhf.strategyInstance("strategy-1") == nullptr);
+ }
+ }
Review Comment:
This test validates the failure outcome, but not the new behavior promised by the PR description (a clear error for bare `#include`). Consider asserting that the emitted error/exception text contains something like `'#include' without a file name` (e.g., by capturing logs or by exposing/returning the load error) so regressions don’t silently pass with a different/less helpful failure mode.
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -254,14 +254,18 @@ NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::strings
if (line[0] == '#') {
tok = strtok_r(const_cast<char *>(line.c_str()), sep, &last);
if (tok != nullptr && strcmp(tok, "#include") == 0) {
- std::string f = strtok_r(nullptr, sep, &last);
+ const char *include = strtok_r(nullptr, sep, &last);
+ if (include == nullptr) {
+ throw std::invalid_argument("'#include' without a file name in '" + fileName + "'");
+ }
+ std::string f = include;
if (include_once.find(f) == include_once.end()) {
include_once.insert(f);
// try to load included file.
try {
loadConfigFile(f, doc, include_once);
} catch (std::exception &ex) {
- throw std::invalid_argument("Unable to open included file '" + f + "' from '" + fileName + "'");
+ throw std::invalid_argument("Unable to load included file '" + f + "' from '" + fileName + "': " + ex.what());
}
Review Comment:
The catch should take `const std::exception&` (exceptions are typically caught by const reference). Also, if retaining more diagnostic context is important, consider preserving the original exception via `std::throw_with_nested` (instead of only flattening into a new message), while still keeping the friendly top-level message.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on code in PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#discussion_r4275672384
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -195,10 +196,9 @@ void
NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::stringstream &doc,
std::unordered_set<std::string> &include_once)
{
- const char *sep = " \t";
- char *tok, *last;
- struct stat buf;
- std::string line;
+ constexpr std::string_view separators = " \t";
Review Comment:
NO
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on code in PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#discussion_r4276072865
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -251,17 +251,27 @@ NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::strings
std::ifstream file(fileName);
if (file.is_open()) {
while (std::getline(file, line)) {
- if (line[0] == '#') {
- tok = strtok_r(const_cast<char *>(line.c_str()), sep, &last);
- if (tok != nullptr && strcmp(tok, "#include") == 0) {
- std::string f = strtok_r(nullptr, sep, &last);
+ std::string_view line_view{line};
+ if (line_view.starts_with('#')) {
+ const auto directive_end = line_view.find_first_of(separators);
+ if (line_view.substr(0, directive_end) == "#include") {
+ if (directive_end == std::string_view::npos) {
+ throw std::invalid_argument("'#include' without a file name in '" + fileName + "'");
+ }
+ const auto include_start = line_view.find_first_not_of(separators, directive_end);
+ if (include_start == std::string_view::npos) {
+ throw std::invalid_argument("'#include' without a file name in '" + fileName + "'");
+ }
Review Comment:
taken
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6196147264
[approve ci autest 0]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6199959018
[approve ci clang-analyzer]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on code in PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#discussion_r4274215677
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -254,14 +254,18 @@ NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::strings
if (line[0] == '#') {
tok = strtok_r(const_cast<char *>(line.c_str()), sep, &last);
if (tok != nullptr && strcmp(tok, "#include") == 0) {
- std::string f = strtok_r(nullptr, sep, &last);
+ const char *include = strtok_r(nullptr, sep, &last);
+ if (include == nullptr) {
+ throw std::invalid_argument("'#include' without a file name in '" + fileName + "'");
+ }
+ std::string f = include;
if (include_once.find(f) == include_once.end()) {
include_once.insert(f);
// try to load included file.
try {
loadConfigFile(f, doc, include_once);
} catch (std::exception &ex) {
- throw std::invalid_argument("Unable to open included file '" + f + "' from '" + fileName + "'");
+ throw std::invalid_argument("Unable to load included file '" + f + "' from '" + fileName + "': " + ex.what());
}
Review Comment:
taken
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6193715406
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6199948518
[approve ci autest 2]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6201519458
[approve ci autest 3]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#discussion_r4276233642
##########
src/proxy/http/remap/unit-tests/test_NextHopStrategyFactory.cc:
##########
@@ -40,6 +40,18 @@
SCENARIO("factory tests loading yaml configs", "[loadConfig]")
{
+ GIVEN("Loading a strategy.yaml with a bare '#include' directive.")
+ {
+ REQUIRE_NOTHROW(YAML::LoadFile(TS_SRC_DIR "/include-missing-strategy.yaml"));
+ NextHopStrategyFactory nhf(TS_SRC_DIR "/include-missing-strategy.yaml");
+
+ THEN("the bare directive is rejected and no strategies load")
+ {
+ REQUIRE(nhf.strategies_loaded == false);
+ REQUIRE(nhf.strategyInstance("strategy-1") == nullptr);
+ }
+ }
Review Comment:
`TS_SRC_DIR` is used unconditionally in this new test block, but the surrounding file already indicates it may be optional (there is an existing `#ifdef TS_SRC_DIR` later). If `TS_SRC_DIR` is not defined in some build configurations, this block will fail to compile. Wrap this new `GIVEN` block in the same `#ifdef TS_SRC_DIR` pattern (or otherwise provide a fallback path) to keep the unit tests buildable across configurations.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on code in PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#discussion_r4274224693
##########
src/proxy/http/remap/unit-tests/test_NextHopStrategyFactory.cc:
##########
@@ -40,6 +40,17 @@
SCENARIO("factory tests loading yaml configs", "[loadConfig]")
{
+ GIVEN("Loading a strategy.yaml with a bare '#include' directive.")
+ {
+ NextHopStrategyFactory nhf(TS_SRC_DIR "/include-missing-strategy.yaml");
+
+ THEN("no strategies are loaded")
+ {
+ REQUIRE(nhf.strategies_loaded == false);
+ REQUIRE(nhf.strategyInstance("strategy-1") == nullptr);
+ }
+ }
Review Comment:
changed will be picked up in next review
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6198094399
[approve ci autest 1]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6193748003
[approve ci osx]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on code in PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#discussion_r4282866740
##########
src/proxy/http/remap/unit-tests/test_NextHopStrategyFactory.cc:
##########
@@ -40,6 +40,18 @@
SCENARIO("factory tests loading yaml configs", "[loadConfig]")
{
+ GIVEN("Loading a strategy.yaml with a bare '#include' directive.")
+ {
+ REQUIRE_NOTHROW(YAML::LoadFile(TS_SRC_DIR "/include-missing-strategy.yaml"));
+ NextHopStrategyFactory nhf(TS_SRC_DIR "/include-missing-strategy.yaml");
+
+ THEN("the bare directive is rejected and no strategies load")
+ {
+ REQUIRE(nhf.strategies_loaded == false);
+ REQUIRE(nhf.strategyInstance("strategy-1") == nullptr);
+ }
+ }
Review Comment:
TS_SRC_DIR is always defined for this target under CMake
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6200777641
[approve ci autest 2]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "jrushford (via GitHub)" <gi...@apache.org>.
jrushford commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6201204724
LGTM
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on code in PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#discussion_r4274207353
##########
src/proxy/http/remap/unit-tests/include-missing-strategy.yaml:
##########
@@ -0,0 +1,37 @@
+# @file
+#
+# Unit test data strategy.yaml file for testing the NextHopStrategyFactory
+#
+# @section license License
+#
+# 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.
+#
+# @section details Details
+#
+#
+# unit test strategy.yaml with a '#include' directive missing its file name
+#
+#include
+#
+strategies:
+ - strategy: 'strategy-1'
+ policy: consistent_hash
+ hash_key: cache_key
+ go_direct: false
+ groups:
+ - *g1
+ scheme: http
Review Comment:
test made more realistic
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on code in PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#discussion_r4274204774
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -254,14 +254,18 @@ NextHopStrategyFactory::loadConfigFile(const std::string &fileName, std::strings
if (line[0] == '#') {
tok = strtok_r(const_cast<char *>(line.c_str()), sep, &last);
if (tok != nullptr && strcmp(tok, "#include") == 0) {
- std::string f = strtok_r(nullptr, sep, &last);
+ const char *include = strtok_r(nullptr, sep, &last);
+ if (include == nullptr) {
+ throw std::invalid_argument("'#include' without a file name in '" + fileName + "'");
+ }
+ std::string f = include;
Review Comment:
replaced with std::string_view operations
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nexthop: reject a bare #include in strategies.yaml [trafficserver]
Posted by "traeak (via GitHub)" <gi...@apache.org>.
traeak commented on PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#issuecomment-6195158304
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org