Copilot commented on code in PR #13091:
URL: https://github.com/apache/trafficserver/pull/13091#discussion_r3114060137
##########
include/proxy/http/HttpTransact.h:
##########
@@ -953,7 +953,13 @@ class HttpTransact
if (e != EIO) {
this->cause_of_death_errno = e;
}
- Dbg(_dbg_ctl, "Setting upstream connection failure %d to %d",
original_connect_result, this->current.server->connect_result);
+
+ if (_dbg_ctl.on()) {
+ std::string buf;
+ swoc::bwprint(buf, "Setting connect_result {::s} to {::s}",
swoc::bwf::Errno(e), swoc::bwf::Errno(original_connect_result),
+ swoc::bwf::Errno(this->current.server->connect_result));
+ Dbg(_dbg_ctl, "%s", buf.c_str());
Review Comment:
This header now uses `swoc::bwprint` and `swoc::bwf::Errno`, but
`HttpTransact.h` does not include the headers that define these formatting
helpers (e.g. `tsutil/ts_bw_format.h` / `swoc/bwf_base.h` + `swoc/bwf_ex.h`).
Because `HttpTransact.h` is included from translation units that don't already
include those headers (e.g. `src/proxy/ParentSelection.cc`), this change can
break compilation depending on include order. Add the necessary include(s) in
this header or switch to a logging approach that doesn't rely on transitive
includes.
##########
include/proxy/http/HttpTransact.h:
##########
@@ -953,7 +953,13 @@ class HttpTransact
if (e != EIO) {
this->cause_of_death_errno = e;
}
- Dbg(_dbg_ctl, "Setting upstream connection failure %d to %d",
original_connect_result, this->current.server->connect_result);
+
+ if (_dbg_ctl.on()) {
+ std::string buf;
+ swoc::bwprint(buf, "Setting connect_result {::s} to {::s}",
swoc::bwf::Errno(e), swoc::bwf::Errno(original_connect_result),
+ swoc::bwf::Errno(this->current.server->connect_result));
Review Comment:
The `swoc::bwprint` call has a format/argument mismatch: the format string
only has 2 placeholders (`{::s} ... {::s}`) but 3 `Errno(...)` arguments are
passed, and the first two args appear swapped vs the message text. This will
either fail to compile (depending on format checking) or produce an incorrect /
incomplete debug message. Update the format string and argument order so it
prints the intended values (e.g., original -> new, and optionally `e`
separately).
```suggestion
swoc::bwprint(buf, "Setting connect_result {::s} to {::s} for errno
{::s}",
swoc::bwf::Errno(original_connect_result),
swoc::bwf::Errno(this->current.server->connect_result),
swoc::bwf::Errno(e));
```
##########
include/proxy/http/HttpTransact.h:
##########
@@ -953,7 +953,13 @@ class HttpTransact
if (e != EIO) {
this->cause_of_death_errno = e;
}
- Dbg(_dbg_ctl, "Setting upstream connection failure %d to %d",
original_connect_result, this->current.server->connect_result);
+
+ if (_dbg_ctl.on()) {
+ std::string buf;
+ swoc::bwprint(buf, "Setting connect_result {::s} to {::s}",
swoc::bwf::Errno(e), swoc::bwf::Errno(original_connect_result),
+ swoc::bwf::Errno(this->current.server->connect_result));
+ Dbg(_dbg_ctl, "%s", buf.c_str());
Review Comment:
Inside `if (_dbg_ctl.on())` the code calls `Dbg(_dbg_ctl, ...)`, which
re-checks `_dbg_ctl.on()` again. Since the guard already ensures the tag is
enabled, consider calling `DbgPrint` directly here to avoid the redundant check
(or drop the outer `if` if you prefer the single `Dbg` guard).
```suggestion
DbgPrint(_dbg_ctl, "%s", buf.c_str());
```
--
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]
