Copilot commented on code in PR #12977:
URL: https://github.com/apache/trafficserver/pull/12977#discussion_r2943825958
##########
plugins/header_rewrite/operators.h:
##########
@@ -487,8 +487,8 @@ class OperatorSetPluginCntl : public Operator
}
private:
- PluginCtrl _name;
- int _value;
+ PluginCtrl _name{PluginCtrl::TIMEZONE}; // always overwritten by initialize()
+ int _value{0};
Review Comment:
The comment says _name is "always overwritten by initialize()", but
initialize() does not overwrite it when the control name is unknown (it only
logs an error). With the new default initializer (TIMEZONE), this can mask
parse failures and lead to unintended behavior; consider using an explicit
invalid default (or a separate validity flag) and updating/removing the
misleading comment.
##########
plugins/header_rewrite/operators.cc:
##########
@@ -1232,6 +1232,8 @@ OperatorSetPluginCntl::initialize(Parser &p)
} else {
TSError("[%s] Unknown value for INBOUND_IP_SOURCE control: %s",
PLUGIN_NAME, value.c_str());
}
+ } else {
+ TSError("[%s] Unknown plugin control name: %s", PLUGIN_NAME, name.c_str());
}
Review Comment:
OperatorSetPluginCntl::initialize() logs an error on unknown control
name/value but leaves the operator in a state that will still execute using the
default-initialized _name/_value. That means an invalid config can silently set
TIMEZONE to 0 (TIMEZONE_LOCAL) or set an unintended INBOUND_IP_SOURCE. Consider
marking the operator invalid on parse errors (e.g., add an INVALID enum /
_valid flag) and make exec() a no-op (or otherwise prevent execution) when
initialization did not successfully parse a supported control/value.
##########
plugins/experimental/ja4_fingerprint/ja4.h:
##########
@@ -53,8 +53,8 @@ class TLSClientHelloSummary
public:
using difference_type =
std::iterator_traits<std::vector<std::uint16_t>::iterator>::difference_type;
- Protocol protocol;
- std::uint16_t TLS_version{0}; // 0 is not the default, this is only to not
have it un-initialized.
+ Protocol protocol{Protocol::TLS}; // always overwritten by caller
Review Comment:
The new default initializer sets protocol to Protocol::TLS but the comment
says it is "always overwritten by caller". If callers rely on the default (or
forget to set it), this comment becomes misleading; either remove/adjust the
comment or enforce explicit initialization (e.g., via a constructor parameter)
if a default is not intended.
--
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]
