Copilot commented on code in PR #12799:
URL: https://github.com/apache/trafficserver/pull/12799#discussion_r2686930259
##########
src/traffic_server/traffic_server.cc:
##########
@@ -755,7 +755,7 @@ initialize_jsonrpc_server()
// jsonrpcServer object.
ink_assert(jsonrpcServer == nullptr);
std::string msg;
- return {false, swoc::bwprint(msg, "Server failed: '{}'", ex.what())};
+ return {false, swoc::bwprint(msg, "Error: '{}'", ex.what())};
Review Comment:
Changing "Server failed" to "Error" reduces the clarity of the error message
and contributes to redundant error labeling when this message is used in the
fprintf statement at lines 1998-2001. The original "Server failed: '...'" was
more descriptive and less redundant than "Error: '...'" when embedded in
"[ERROR] JSONRPC server could not be started because: '...'". Consider
reverting this change or removing the "Error:" prefix entirely since the
context is already provided by the outer message.
```suggestion
return {false, swoc::bwprint(msg, "Server failed: '{}'", ex.what())};
```
##########
src/traffic_server/traffic_server.cc:
##########
@@ -1995,7 +1995,10 @@ main(int /* argc ATS_UNUSED */, const char **argv)
if (!command_flag) { // No need if we are going into command mode.
// JSONRPC server and handlers
if (auto &&[ok, msg] = initialize_jsonrpc_server(); !ok) {
- Warning("JSONRPC server could not be started.\n Why?: '%s' ...
Continuing without it.", msg.c_str());
+ fprintf(stderr,
+ "[ERROR] JSONRPC server could not be started because: '%s', ATS
will start without it, but traffic_ctl will not be "
+ "available.\n",
Review Comment:
The error message structure will result in redundant "Error:" labels. The
inner message from initialize_jsonrpc_server() returns "Error: 'Socket name is
too long.'" (line 758), which then gets embedded in the outer fprintf statement
as "because: 'Error: ...'". This creates awkward double-labeling like "[ERROR]
JSONRPC server could not be started because: 'Error: ...'". The change at line
758 from "Server failed" to "Error" makes this worse compared to the original
implementation.
```suggestion
"[ERROR] JSONRPC server could not be started: %s\n"
"ATS will start without it, but traffic_ctl will not be
available.\n",
```
##########
include/mgmt/rpc/server/CommBase.h:
##########
@@ -36,7 +36,7 @@ struct BaseCommInterface {
virtual std::string const &name() const = 0;
};
-enum class InternalError { MAX_TRANSIENT_ERRORS_HANDLED = 1, POLLIN_ERROR,
PARTIAL_READ, FULL_BUFFER };
+enum class InternalError { MAX_TRANSIENT_ERRORS_HANDLED = 1, POLLIN_ERROR,
PARTIAL_READ, FULL_BUFFER, SOCKET_NAME_TOO_LONG };
Review Comment:
The enum name SOCKET_NAME_TOO_LONG is misleading because it's used for both
empty socket paths and paths that are too long (see IPCSocketServer.cc line
145). Consider renaming to something more accurate like INVALID_SOCKET_PATH or
SOCKET_PATH_INVALID, or create separate enum values for each condition.
```suggestion
enum class InternalError { MAX_TRANSIENT_ERRORS_HANDLED = 1, POLLIN_ERROR,
PARTIAL_READ, FULL_BUFFER, INVALID_SOCKET_PATH };
```
##########
src/mgmt/rpc/server/CommBase.cc:
##########
@@ -44,6 +44,8 @@ CommInternalErrorCategory::message(int ev) const
return {"No more data to be read, but the buffer contains some invalid?
data."};
case rpc::comm::InternalError::FULL_BUFFER:
return {"Buffer's full."};
+ case rpc::comm::InternalError::SOCKET_NAME_TOO_LONG:
+ return {"Socket name is too long."};
Review Comment:
The error message "Socket name is too long." doesn't provide helpful context
about the actual limits or what action to take. Consider providing more
actionable information, such as "RPC socket path exceeds maximum length of
[limit] characters" or include guidance on how to resolve the issue.
```suggestion
return {"RPC socket path exceeds the maximum length supported by the
operating system; use a shorter directory path or socket filename."};
```
##########
src/mgmt/rpc/server/IPCSocketServer.cc:
##########
@@ -139,15 +139,15 @@ IPCSocketServer::configure(YAML::Node const ¶ms)
std::error_code
IPCSocketServer::init()
{
+ std::error_code ec; // Flag possible errors.
// Need to run some validations on the pathname to avoid issue. Normally
this would not be an issue, but some tests may fail on
// this.
if (_conf.sockPathName.empty() || _conf.sockPathName.size() > sizeof
_serverAddr.sun_path) {
- Dbg(dbg_ctl, "Invalid unix path name, check the size.");
- return std::make_error_code(static_cast<std::errc>(ENAMETOOLONG));
+ Dbg(dbg_ctl, "Invalid unix path name, check the size. Empty or too long.");
+ ec = InternalError::SOCKET_NAME_TOO_LONG;
+ return ec;
Review Comment:
The error code SOCKET_NAME_TOO_LONG is used for both empty socket paths and
paths that are too long (see the condition at line 145). This single error code
masks which specific problem occurred, making it harder for users to diagnose
the issue. Consider using the error message or debug log to distinguish between
these two cases, or use separate error codes.
--
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]
