Copilot commented on code in PR #13032:
URL: https://github.com/apache/trafficserver/pull/13032#discussion_r4258442996
##########
include/shared/rpc/yaml_codecs.h:
##########
@@ -63,11 +63,33 @@ template <> struct convert<shared::rpc::JSONRPCError> {
static bool
decode(Node const &node, shared::rpc::JSONRPCError &error)
{
+ using DataEntry = shared::rpc::JSONRPCError::DataEntry;
+
error.code = helper::try_extract<int32_t>(node, "code");
error.message = helper::try_extract<std::string>(node, "message");
- if (auto data = node["data"]) {
- for (auto &&err : data) {
- error.data.emplace_back(helper::try_extract<int32_t>(err, "code"),
helper::try_extract<std::string>(err, "message"));
+ // Read only: the same tree is printed as received in json output.
+ if (auto const data = node["data"]; data && !data.IsSequence()) {
+ // Not a list of annotations: nothing in it can be read as one.
+ error.data.push_back({0, DataEntry::INVALID_SEVERITY, {}});
+ } else if (data) {
+ for (auto const &err : data) {
+ DataEntry entry;
+ entry.code = helper::try_extract<int32_t>(err, "code");
+ entry.message = helper::try_extract<std::string>(err, "message");
+ if (!err.IsMap()) {
+ entry.severity = DataEntry::INVALID_SEVERITY;
+ } else if (auto const severity = err["severity"]) {
+ entry.severity = DataEntry::INVALID_SEVERITY;
+ // A quoted value is text, not a severity.
+ if (severity.IsScalar() && severity.Tag() != "!") {
+ try {
+ entry.severity = severity.as<int32_t>();
+ } catch (YAML::Exception const &) {
+ // Not an integer: stays INVALID_SEVERITY.
+ }
+ }
+ }
Review Comment:
The “quoted value is text” rule relies on `severity.Tag() != "!"` to
distinguish JSON/YAML strings from numbers. That’s a YAML-CPP behavior detail
and may not be robust across YAML-CPP versions/build options (and it’s not
obvious that `Tag()` will reliably differ for `"0"` vs `0`). If this check
doesn’t hold, `"severity": "0"` could be accepted as `0` and incorrectly
graded/printed. A more reliable approach is to validate the node’s resolved
type/tag explicitly (e.g., accept only nodes resolved as an integer scalar) and
treat all other scalar forms as invalid.
##########
src/traffic_ctl/TrafficCtlStatus.h:
##########
@@ -20,10 +20,87 @@ limitations under the License.
*/
#pragma once
+#include <algorithm>
+#include <exception>
+#include <optional>
+#include <string_view>
+
+#include "mgmt/rpc/jsonrpc/error/RPCError.h"
+#include "shared/rpc/RPCRequests.h"
+#include "shared/rpc/yaml_codecs.h"
+#include "swoc/string_view_util.h"
+#include "tsutil/ts_errata.h"
+
constexpr int CTRL_EX_OK = 0;
// EXIT_FAILURE can also be used.
constexpr int CTRL_EX_ERROR = 2;
constexpr int CTRL_EX_UNIMPLEMENTED = 3;
constexpr int CTRL_EX_TEMPFAIL = 75; ///< Temporary failure — operation in
progress, retry later (EX_TEMPFAIL from sysexits.h).
-extern int App_Exit_Status_Code; //!< Global variable to store the exit status
code of the application.
+extern int App_Exit_Status_Code; //!< Global variable
to store the exit status code of the application.
+extern swoc::Errata::severity_type App_Exit_Level_Error; //!< Lowest
annotation severity that makes a server error exit non zero.
+
+/// Thrown by a command step that already printed a server error and graded it
into @c App_Exit_Status_Code, so the command
+/// stops there and the error is not reported a second time.
+struct ServerErrorReported : std::exception {
+ const char *
+ what() const noexcept override
+ {
+ return "server error already reported";
+ }
+};
+
+/// Parse an @c --error-level value: a severity name (case-insensitive), or
"warning" as an alias of "warn".
+inline std::optional<swoc::Errata::severity_type>
+parse_error_level(std::string_view name)
+{
+ if (strcasecmp(name, std::string_view{"warning"}) == 0) {
+ return swoc::Errata::severity_type(ERRATA_WARN);
+ }
+ for (size_t i = 0; i < Severity_Names.size(); ++i) {
+ if (strcasecmp(name, Severity_Names[i]) == 0) {
+ return swoc::Errata::severity_type(i);
+ }
+ }
+ return std::nullopt;
+}
Review Comment:
`strcasecmp` is used with `std::string_view`, but there’s no visible include
for the standard/POSIX `strcasecmp` declaration (typically `<strings.h>`), and
the overload set for `(std::string_view, std::string_view)` is not guaranteed.
This can lead to compilation errors or unintended overload resolution depending
on platform/toolchain. Prefer using an explicit, project-provided
case-insensitive comparison utility that operates on string views (e.g., a SWOC
helper), or include the correct header and compare `c_str()`-style strings
explicitly.
##########
src/mgmt/rpc/jsonrpc/unit_tests/test_basic_protocol.cc:
##########
@@ -21,11 +21,15 @@
#include <catch2/catch_test_macros.hpp> /* catch unit-test framework */
#include <tsutil/ts_bw_format.h>
+#include <tsutil/ts_errata.h>
#include "mgmt/rpc/jsonrpc/JsonRPCManager.h"
#include "mgmt/rpc/jsonrpc/JsonRPC.h"
#include "mgmt/rpc/handlers/common/ErrorUtils.h"
+// Defined in src/tsutil/ts_diags.cc; traffic_server calls it at startup.
+void Initialize_Errata_Settings();
Review Comment:
Forward-declaring `Initialize_Errata_Settings()` inside the test file risks
signature drift and makes it harder to track ownership/visibility (especially
if the function later moves into a namespace or gains a header declaration).
Prefer including the header that declares it (if one exists), or introducing a
small test-only header for this declaration so it’s shared and kept consistent.
##########
src/traffic_ctl/traffic_ctl.cc:
##########
@@ -382,6 +388,21 @@ main([[maybe_unused]] int argc, const char **argv)
signal_register_handler(SIGINT, handle_signal);
auto args = parser.parse(argv);
+
+ // ArgParser refuses it given more than once. Given without a value it is
an error, not the default.
+ if (auto const &error_level = args.get("error-level"); error_level) {
+ if (error_level.size() == 0) {
+ throw std::runtime_error("--error-level needs a value. Use one of
diag, debug, status, note, warn, error, fatal, alert, "
+ "emergency");
+ }
+ if (auto const level = parse_error_level(error_level.value()); level) {
+ App_Exit_Level_Error = *level;
+ } else {
+ throw std::runtime_error("Unknown error level: " + error_level.value()
+
+ ". Use one of diag, debug, status, note,
warn, error, fatal, alert, emergency");
+ }
Review Comment:
The accepted-level list is duplicated in multiple runtime error strings (and
also exists in help/docs). This is easy to let drift from `parse_error_level()`
(e.g., if severity names change, or aliases are added). Consider centralizing
the canonical list (e.g., build it from `Severity_Names` plus the `warning`
alias) and reuse it for both validation and error messages.
--
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]
