Copilot commented on code in PR #13667:
URL: https://github.com/apache/trafficserver/pull/13667#discussion_r3981260980
##########
src/iocore/net/P_UnixNetVConnection.h:
##########
@@ -304,6 +307,39 @@ UnixNetVConnection::set_mptcp_state()
#endif
}
+// Copy the TCP_INFO fields ATS reports out of the kernel.
+inline bool
+UnixNetVConnection::get_tcp_info(TcpInfoSnapshot &info) const
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
+ struct tcp_info tinfo;
+ int tinfo_len = sizeof(tinfo);
+ int const fd = con.sock.get_fd();
Review Comment:
`tinfo` is not zero-initialized. If the kernel returns a shorter `TCP_INFO`
payload than `sizeof(tinfo)` (or only partially fills it), the later field
reads can observe uninitialized data. Initializing the struct defensively
avoids undefined behavior and makes debug output reliable.
##########
src/iocore/net/P_UnixNetVConnection.h:
##########
@@ -304,6 +307,39 @@ UnixNetVConnection::set_mptcp_state()
#endif
}
+// Copy the TCP_INFO fields ATS reports out of the kernel.
+inline bool
+UnixNetVConnection::get_tcp_info(TcpInfoSnapshot &info) const
+{
+#if defined(TCP_INFO) && defined(HAVE_STRUCT_TCP_INFO)
+ struct tcp_info tinfo;
+ int tinfo_len = sizeof(tinfo);
+ int const fd = con.sock.get_fd();
+
+ if (0 != safe_getsockopt(fd, IPPROTO_TCP, TCP_INFO, &tinfo, &tinfo_len)) {
+ Dbg(_dbg_ctl_socket_tcp_info, "failed getsockopt(%d, TCP_INFO): %s", fd,
strerror(errno));
+ return false;
+ }
+ info.rtt = tinfo.tcpi_rtt;
+ info.rttvar = tinfo.tcpi_rttvar;
+ info.snd_cwnd = tinfo.tcpi_snd_cwnd;
+#if HAVE_STRUCT_TCP_INFO_TCPI_TOTAL_RETRANS
+ info.retrans = tinfo.tcpi_total_retrans;
+#elif HAVE_STRUCT_TCP_INFO___TCPI_RETRANS
+ // FreeBSD spells the cumulative count differently; __tcpi_retrans is the
+ // currently outstanding count, which is not what this reports.
+ info.retrans = tinfo.tcpi_snd_rexmitpack;
+#endif
Review Comment:
If neither `HAVE_STRUCT_TCP_INFO_TCPI_TOTAL_RETRANS` nor
`HAVE_STRUCT_TCP_INFO___TCPI_RETRANS` is defined, `info.retrans` is left at its
default value but the function still returns true, which can silently misreport
retransmits. Treat this as unsupported and fail the sample so callers log `-1`
consistently.
##########
include/proxy/http/HttpSM.h:
##########
@@ -531,8 +531,11 @@ class HttpSM : public Continuation, public
PluginUserArgs<TS_USER_ARGS_TXN>
// do_api_callout_internal()
bool hooks_set = false;
std::optional<bool> mptcp_state; // Don't initialize, that marks it as "not
defined".
- const char *server_protocol = "-";
- int server_transact_count = 0;
+ /// TCP_INFO for the current origin response, sampled after successful
header parsing.
+ /// Cleared when starting another origin attempt or reading another response
header.
+ std::optional<TcpInfoSnapshot> server_tcp_info;
Review Comment:
`HttpSM` now stores `std::optional<TcpInfoSnapshot>`, which requires the
complete `TcpInfoSnapshot` definition at this point. The type is currently
available only via an indirect include chain (e.g., `PreWarmManager.h` ->
`NetVConnection.h`), which is brittle if include dependencies change; include
`iocore/net/TcpInfoSnapshot.h` directly in this header.
##########
tests/gold_tests/logging/verify_origin_tcp_info.py:
##########
@@ -0,0 +1,78 @@
+# 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.
+"""Validate origin TCP_INFO access-log fields after replay traffic
completes."""
+
+import argparse
+from pathlib import Path
+import time
+
+
+def verify(log_path: Path, mode: str) -> None:
+ expected_keys = {
+ 'disabled': {'miss'},
+ 'enabled': {'miss', 'hit', 'guard', 'oversized', 'malformed'},
+ 'retry': {'retry'},
+ }[mode]
+ # Wait for the asynchronous log writer, rather than sleeping a fixed time.
+ deadline = time.monotonic() + 15
+ while True:
+ lines = log_path.read_text().splitlines() if log_path.exists() else []
+ if len(lines) >= len(expected_keys):
+ break
+ if time.monotonic() >= deadline:
+ raise AssertionError(f'Timed out waiting for access-log records
for {expected_keys}: {lines}')
+ time.sleep(0.1)
Review Comment:
The log file can be read while the async logger is still flushing, which can
produce a partially written final line; `splitlines()` will include it and the
subsequent strict `len(fields) == 6` check can fail intermittently. Filter out
incomplete lines while waiting so the test only proceeds once full records are
present.
--
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]
