Copilot commented on code in PR #13262:
URL: https://github.com/apache/trafficserver/pull/13262#discussion_r3408297165
##########
include/tscore/DiagsTypes.h:
##########
@@ -45,111 +45,336 @@
#include "tscore/ink_mutex.h"
#include "tsutil/Regex.h"
+/// Sentinel value stored in Diags::magic to detect heap corruption or
+/// use-after-free of a Diags instance. Diags::magic == DIAGS_MAGIC for
+/// any properly constructed, living instance.
#define DIAGS_MAGIC 0x12345678
-#define BYTES_IN_MB 1000000
-enum DiagsTagType {
- DiagsTagType_Debug = 0, // do not renumber --- used as array index
- DiagsTagType_Action = 1
-};
+/// Bytes-per-megabyte (decimal, not binary). Used by configuration
+/// parsing to convert MB-valued knobs into byte counts.
+#define BYTES_IN_MB 1000000
+/**
+ * @brief Selects which of the two tag tables a Diags operation addresses:
+ * debug tags (controlling Dbg/Diag emission) or action tags (controlling
+ * is_action_tag_set conditional code paths).
+ *
+ * @note Numeric values are used as array indices into
+ * DiagsConfigState::_enabled and Diags::activated_tags. Do not renumber.
+ */
+enum DiagsTagType { DiagsTagType_Debug = 0, DiagsTagType_Action = 1 };
+
+/**
+ * @brief Per-DiagsLevel output destination configuration.
+ *
+ * Each boolean controls whether messages at the owning level are emitted
+ * to the corresponding sink. Multiple sinks may be set simultaneously;
+ * the message is emitted to all enabled sinks in unspecified order.
+ *
+ * @par Thread safety
+ * Carries no internal synchronization. Callers reading or
+ * modifying Diags::config.outputs[] must respect the concurrency contract
+ * of the containing DiagsConfigState.
+ */
struct DiagsModeOutput {
bool to_stdout;
bool to_stderr;
bool to_syslog;
bool to_diagslog;
};
+/**
+ * @brief Identifies which standard stream Diags::set_std_output reseats.
+ *
+ * STDOUT targets the standard output stream; STDERR targets the standard
+ * error stream.
+ */
enum StdStream { STDOUT = 0, STDERR };
+/**
+ * @brief Selects the rolling policy for a managed log file.
+ *
+ * - NO_ROLLING — never roll; file grows without bound.
+ * - ROLL_ON_TIME — roll when the configured time interval elapses.
+ * - ROLL_ON_SIZE — roll when the configured size threshold is
exceeded.
+ * - ROLL_ON_TIME_OR_SIZE — roll when either condition is satisfied.
+ * - INVALID_ROLLING_VALUE — sentinel; any code path receiving this value MUST
+ * treat it as NO_ROLLING. MUST be the largest enumerator so that range
checks
+ * can detect out-of-range configuration values.
+ */
enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE,
ROLL_ON_TIME_OR_SIZE, INVALID_ROLLING_VALUE };
+/// Count of valid DiagsLevel values; equal to DL_Undefined and used to
+/// size DiagsConfigState::outputs[].
#define DiagsLevel_Count DL_Undefined
+/// True if the given DiagsLevel causes process termination after emission.
+/// Terminal levels are DL_Fatal and above (excluding DL_Undefined).
#define DiagsLevel_IsTerminal(_l) (((_l) >= DL_Fatal) && ((_l) < DL_Undefined))
-// Cleanup Function Prototype - Called before ink_fatal to
-// cleanup process state
+/**
+ * @brief Cleanup callback invoked before process termination on a terminal
+ * DiagsLevel (Fatal, Alert, Emergency).
+ *
+ * @pre The callback runs on the thread that emitted the terminal message.
+ * It MUST NOT throw. It MUST be async-signal-safe if terminal-level
+ * emission can occur from a signal handler.
+ * @post On return, the process continues into the exit path for the terminal
+ * level: DL_Fatal and DL_Alert invoke ink_fatal_va; DL_Emergency invokes
+ * ink_emergency_va (a distinct, stronger exit path).
+ * @par Thread safety
+ * Only one callback is installed per Diags instance.
+ * The callback is invoked at most once per process.
+ */
using DiagsCleanupFunc = void (*)();
+/**
+ * @brief Process-global enable state for the two tag tables.
+ *
+ * Exposes two static methods to read and write the per-tag enable state.
+ * The state for each DiagsTagType is one of:
+ * 0 — disabled (no emission),
+ * 1 — enabled for all callers,
+ * 2 — enabled only when ContFlags::DEBUG_OVERRIDE is set on the
+ * current Continuation (per-connection debug override),
+ * 3 — treated identically to 1 (always-enabled).
+ */
Review Comment:
DiagsConfigState’s doxygen says it represents “process-global enable state”,
but the type also carries per-instance output routing via outputs[]. This is
potentially confusing for readers (especially in tests where multiple Diags
instances could exist). Consider updating the brief to reflect both
responsibilities: per-instance outputs plus process-global tag enable state.
##########
include/tscore/DiagsTypes.h:
##########
@@ -45,111 +45,336 @@
#include "tscore/ink_mutex.h"
#include "tsutil/Regex.h"
+/// Sentinel value stored in Diags::magic to detect heap corruption or
+/// use-after-free of a Diags instance. Diags::magic == DIAGS_MAGIC for
+/// any properly constructed, living instance.
#define DIAGS_MAGIC 0x12345678
-#define BYTES_IN_MB 1000000
-enum DiagsTagType {
- DiagsTagType_Debug = 0, // do not renumber --- used as array index
- DiagsTagType_Action = 1
-};
+/// Bytes-per-megabyte (decimal, not binary). Used by configuration
+/// parsing to convert MB-valued knobs into byte counts.
+#define BYTES_IN_MB 1000000
+/**
+ * @brief Selects which of the two tag tables a Diags operation addresses:
+ * debug tags (controlling Dbg/Diag emission) or action tags (controlling
+ * is_action_tag_set conditional code paths).
+ *
+ * @note Numeric values are used as array indices into
+ * DiagsConfigState::_enabled and Diags::activated_tags. Do not renumber.
+ */
+enum DiagsTagType { DiagsTagType_Debug = 0, DiagsTagType_Action = 1 };
+
+/**
+ * @brief Per-DiagsLevel output destination configuration.
+ *
+ * Each boolean controls whether messages at the owning level are emitted
+ * to the corresponding sink. Multiple sinks may be set simultaneously;
+ * the message is emitted to all enabled sinks in unspecified order.
+ *
+ * @par Thread safety
+ * Carries no internal synchronization. Callers reading or
+ * modifying Diags::config.outputs[] must respect the concurrency contract
+ * of the containing DiagsConfigState.
+ */
struct DiagsModeOutput {
bool to_stdout;
bool to_stderr;
bool to_syslog;
bool to_diagslog;
};
+/**
+ * @brief Identifies which standard stream Diags::set_std_output reseats.
+ *
+ * STDOUT targets the standard output stream; STDERR targets the standard
+ * error stream.
+ */
enum StdStream { STDOUT = 0, STDERR };
+/**
+ * @brief Selects the rolling policy for a managed log file.
+ *
+ * - NO_ROLLING — never roll; file grows without bound.
+ * - ROLL_ON_TIME — roll when the configured time interval elapses.
+ * - ROLL_ON_SIZE — roll when the configured size threshold is
exceeded.
+ * - ROLL_ON_TIME_OR_SIZE — roll when either condition is satisfied.
+ * - INVALID_ROLLING_VALUE — sentinel; any code path receiving this value MUST
+ * treat it as NO_ROLLING. MUST be the largest enumerator so that range
checks
+ * can detect out-of-range configuration values.
+ */
enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE,
ROLL_ON_TIME_OR_SIZE, INVALID_ROLLING_VALUE };
+/// Count of valid DiagsLevel values; equal to DL_Undefined and used to
+/// size DiagsConfigState::outputs[].
#define DiagsLevel_Count DL_Undefined
+/// True if the given DiagsLevel causes process termination after emission.
+/// Terminal levels are DL_Fatal and above (excluding DL_Undefined).
#define DiagsLevel_IsTerminal(_l) (((_l) >= DL_Fatal) && ((_l) < DL_Undefined))
-// Cleanup Function Prototype - Called before ink_fatal to
-// cleanup process state
+/**
+ * @brief Cleanup callback invoked before process termination on a terminal
+ * DiagsLevel (Fatal, Alert, Emergency).
+ *
+ * @pre The callback runs on the thread that emitted the terminal message.
+ * It MUST NOT throw. It MUST be async-signal-safe if terminal-level
+ * emission can occur from a signal handler.
+ * @post On return, the process continues into the exit path for the terminal
+ * level: DL_Fatal and DL_Alert invoke ink_fatal_va; DL_Emergency invokes
+ * ink_emergency_va (a distinct, stronger exit path).
+ * @par Thread safety
+ * Only one callback is installed per Diags instance.
+ * The callback is invoked at most once per process.
+ */
using DiagsCleanupFunc = void (*)();
+/**
+ * @brief Process-global enable state for the two tag tables.
+ *
+ * Exposes two static methods to read and write the per-tag enable state.
+ * The state for each DiagsTagType is one of:
+ * 0 — disabled (no emission),
+ * 1 — enabled for all callers,
+ * 2 — enabled only when ContFlags::DEBUG_OVERRIDE is set on the
+ * current Continuation (per-connection debug override),
+ * 3 — treated identically to 1 (always-enabled).
+ */
class DiagsConfigState
{
public:
+ /**
+ * @brief Return the current enable state for the given tag type.
+ *
+ * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+ * @return 0 (disabled), 1 (enabled), 2 (DEBUG_OVERRIDE mode), or 3
+ * (always-enabled, same effect as 1).
+ * @pre None.
+ * @post No state change.
+ * @par Errors
+ * None.
+ * @par Thread safety
+ * Not thread-safe. Reads _enabled without synchronization;
+ * concurrent writes from a reconfiguration thread are a data race.
+ */
static int
enabled(DiagsTagType dtt)
{
return _enabled[dtt];
}
+ /**
+ * @brief Set the enable state for the given tag type.
+ *
+ * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+ * @param[in] new_value 0, 1, 2, or 3 (see class contract); 3 behaves
+ * identically to 1.
+ * @pre new_value is in [0, 3]; values outside this range produce
+ * unspecified behavior.
+ * @post enabled(dtt) returns new_value on all subsequent calls. When
+ * dtt == DiagsTagType_Debug and the value changes, DbgCtl::_config_mode
+ * is also updated via a relaxed atomic store so that DbgCtl-based checks
+ * remain in sync without acquiring a lock.
+ * @par Errors
+ * None.
+ * @par Thread safety
+ * Must be called with external serialization against
+ * concurrent enabled(dtt) reads.
+ */
static void enabled(DiagsTagType dtt, int new_value);
- DiagsModeOutput outputs[DiagsLevel_Count]; // where each level prints
+ /// Per-level output routing. Each element controls where messages at
+ /// the corresponding DiagsLevel are sent. See DiagsModeOutput.
+ DiagsModeOutput outputs[DiagsLevel_Count];
private:
static int _enabled[2]; // one debug, one action
};
-//////////////////////////////////////////////////////////////////////////////
-//
-// class Diags
-//
-// The Diags class is used for global configuration of the run-time
-// diagnostics system. This class provides the following services:
-//
-// * run-time notices, debugging, warnings, errors
-// * debugging tags to selectively enable & disable diagnostics
-// * action tags to selectively enable & disable code paths
-// * configurable output to stdout, stderr, syslog, error logs
-// * interface to supporting on-the-fly reconfiguration
-//
-//////////////////////////////////////////////////////////////////////////////
-
+/**
+ * @brief Active diagnostic emission and tag-table state for the process.
+ *
+ * Owns the diags.log, stdout, and stderr BaseLogFile handles, the per-level
+ * output routing configuration, the regex tables for debug and action tags,
+ * and the rolling policy for the diagnostic and standard-output logs.
+ * All diagnostic emission macros (Status, Note, Warning, Error, Fatal, Alert,
+ * Emergency, Diag) route through the process-global Diags instance installed
+ * via DiagsPtr.
+ *
+ * @par Thread safety
+ * Concurrent emission calls (print, log, error) are serialized
+ * against each other via tag_table_lock. Reconfiguration writes to
+ * config.outputs (e.g., via DiagsConfig::reconfigure_diags) do not acquire
Review Comment:
This thread-safety note says concurrent emission calls are serialized via
tag_table_lock, but Diags::print_va() explicitly releases tag_table_lock before
the syslog() call on non-FreeBSD platforms. Please qualify this statement so it
matches the platform-specific locking behavior (syslog outside the lock except
on FreeBSD).
##########
src/tscore/Diags.cc:
##########
@@ -57,6 +57,12 @@ using namespace swoc::literals;
void
DiagsConfigState::enabled(DiagsTagType dtt, int new_value)
{
+ // NOTE: proxy.config.diags.debug.enabled is validated as [0-3] in
+ // RecordsConfig.cc, so new_value may be 3 even though the API contract
+ // documents [0, 2]. Value 3 is not range-checked here; it reaches
+ // _enabled[dtt] and DbgCtl::_config_mode unchanged. In Diags::on(),
+ // the test (config.enabled(mode) & 1) makes value 3 behave identically
+ // to value 1 (always-enabled).
Review Comment:
The new NOTE says the API contract documents the debug enable range as [0,
2], but this PR updates the DiagsConfigState doxygen contract to include 0–3.
Please update this NOTE so it doesn’t contradict the current header
documentation.
--
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]
