bryancall commented on code in PR #13550:
URL: https://github.com/apache/trafficserver/pull/13550#discussion_r3786416673
##########
src/tscore/Layout.cc:
##########
@@ -161,15 +161,15 @@ Layout::Layout(std::string_view const _prefix)
if ((len + 1) > PATH_NAME_MAX) {
ink_fatal("TS_ROOT environment variable is too big: %d, max %d\n",
len, PATH_NAME_MAX - 1);
}
- path = env_path;
+ path = std::move(env_path);
while (path.back() == '/') {
path.pop_back();
}
Review Comment:
Following up: a dedicated change for this is in progress, so it is not going
to ride along here.
Two extra details worth recording while looking at it. There is a second
route to the same problem that does not need an empty TS_ROOT at all: the loop
pops until the string does not end in a slash, so a TS_ROOT of "/" or "///"
empties the string partway through and then calls back() on it again. And
src/tscore/runroot.cc has the same unguarded shape in get_parent_yaml_path(),
where whole_path.back() is tested before any emptiness check, even though the
loop three lines below it does check for empty.
The guard itself is one token, but the open question is what an empty or
all-slashes TS_ROOT should mean. Falling through to the compile time prefix
would silently ignore what the operator asked for, and every layout path is
derived from this value, so I would rather fail loudly the way the too-big case
just above already does. That decision is the reason this belongs in its own
change rather than in a cleanup that is meant to be free of behavior changes.
--
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]
