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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
