bneradt commented on code in PR #13262:
URL: https://github.com/apache/trafficserver/pull/13262#discussion_r3414624248
##########
include/tscore/DiagsTypes.h:
##########
@@ -45,111 +45,356 @@
#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 the enable-state
+ * array 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 (DL_Fatal, DL_Alert, DL_Emergency).
+ *
+ * @pre Runs synchronously on the thread that emitted the terminal message,
+ * before the process exits. MUST NOT throw. MUST tolerate being invoked
+ * more than once per process (for example, from concurrent terminal
+ * emissions on different threads, or from a terminal emission inside the
+ * callback itself).
+ * @note Terminal-level emission is not async-signal-safe, so the callback
+ * MUST NOT be reached from a signal handler. Code paths that may emit at
+ * terminal level from signal context are bugs in the emitter, not a
+ * constraint this callback is expected to satisfy.
+ * @post On return, the process is terminated; control does not return to
+ * the emitter. DL_Emergency exits via the unrecoverable path; DL_Fatal
+ * and DL_Alert exit via the recoverable path.
+ * @par Thread safety
+ * No serialization is provided. The callback may run concurrently on
+ * multiple threads if multiple terminal emissions race; the callback
+ * itself is responsible for any required synchronization or idempotency.
+ */
using DiagsCleanupFunc = void (*)();
+/**
+ * @brief Bundles the two orthogonal pieces of Diags output configuration:
+ * the process-global per-tag enable state and the per-level output routing.
+ *
+ * The enable 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).
+ *
+ * The output routing is stored in @c outputs[], one entry per DiagsLevel.
+ *
+ * @par Thread safety
+ * The enable state is process-global (static storage). Reads via
+ * enabled() and writes via enabled(dtt, value) carry no internal
+ * synchronization; callers must provide external serialization when a
+ * write may race a read. The @c outputs[] array carries no internal
+ * synchronization either; the same serialization requirement applies.
+ */
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. The read carries no 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, the DbgCtl fast-path
+ * enable state is 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) hold tag_table_lock for
+ * diagslog, stdout, and stderr output. On non-FreeBSD platforms the lock is
+ * released before the syslog() call, so concurrent syslog output is not
+ * serialized against other emission calls. Reconfiguration writes to
+ * config.outputs do not acquire tag_table_lock, so concurrent emission
+ * and reconfiguration are a data race on config.outputs. Tag-table
+ * mutation methods (activate_taglist,
+ * deactivate_all) acquire tag_table_lock internally. Log-pointer swap
methods
+ * (reseat_diagslog, should_roll_diagslog) acquire tag_table_lock only for
+ * their pointer swap step; file operations run outside the lock.
+ * set_std_output (and should_roll_outputlog through it) additionally holds
+ * the lock across the dup2() rebinding of the standard stream descriptor on
+ * success. setup_diagslog() acquires no lock; callers must serialize it.
+ * Rolling-policy configuration methods (config_roll_diagslog,
+ * config_roll_outputlog) and dump() do not acquire any lock; see the
+ * per-method @par Thread safety paragraphs below for details.
+ */
class Diags : public DebugInterface
{
public:
+ /**
+ * @brief Construct a Diags instance with the given initial configuration.
+ *
+ * @param[in] prefix_string Tag prefix prepended to all debug output. Must
+ * be non-empty.
+ * @param[in] base_debug_tags Initial debug tag regex, or nullptr for none.
+ * @param[in] base_action_tags Initial action tag regex, or nullptr for none.
+ * @param[in] _diags_log BaseLogFile for diags.log output, or nullptr to
+ * suppress file logging. Ownership transfers to this Diags instance.
+ * @param[in] diags_log_perm File permission bits for diags.log, or -1 for
+ * the system default (LOGFILE_DEFAULT_PERMS).
+ * @param[in] output_log_perm File permission bits for stdout/stderr log
+ * files, or -1 for the system default.
+ * @pre prefix_string is non-empty; safe to construct before any thread
+ * reads via diags().
+ * @post magic == DIAGS_MAGIC; tag regex strings are stored; activated tag
+ * tables are empty until activate_taglist() is called (typically via
+ * reconfigure_diags()); diags_log is set and open if _diags_log is
+ * non-null and openable, otherwise diags_log is null.
+ * @par Errors
+ * Allocation failures abort via ink_abort. Log-open failures result in
+ * diags_log being null and are reported via log_log_error only when
+ * BASELOGFILE_DEBUG_MODE is enabled (off by default).
+ * @par Thread safety
+ * Single-threaded construction expected. After construction
+ * the instance may be installed via DiagsPtr::set and used concurrently.
+ */
Diags(std::string_view prefix_string, const char *base_debug_tags, const
char *base_action_tags, BaseLogFile *_diags_log,
int diags_log_perm = -1, int output_log_perm = -1);
+
+ /**
+ * @brief Destroy the Diags instance, closing all owned log files.
+ *
+ * @pre No thread is currently executing a method on this instance.
+ * @post All owned BaseLogFile handles are deleted (closing their FILE *).
+ * @par Errors
+ * None.
+ * @par Thread safety
+ * Single-threaded destruction. The caller must ensure no
+ * other thread holds or will acquire a reference to this instance.
+ */
virtual ~Diags();
+ /// Diagnostic log file. Owned by this instance. Read-only from outside
+ /// the tscore module; mutation via any path other than setup_diagslog /
+ /// reseat_diagslog is undefined behavior.
BaseLogFile *diags_log;
+
+ /// Standard-output redirection file. Owned by this instance.
+ /// Read-only from outside the tscore module.
BaseLogFile *stdout_log;
+
+ /// Standard-error redirection file. Owned by this instance.
+ /// Read-only from outside the tscore module.
BaseLogFile *stderr_log;
+ /// Sentinel. Always DIAGS_MAGIC for a live, properly constructed instance.
const unsigned int magic;
- DiagsConfigState config;
- DiagsShowLocation show_location;
- DiagsCleanupFunc cleanup_func;
+
+ /// Per-level output routing and tag-enable state. See DiagsConfigState.
+ DiagsConfigState config;
+
+ /// Controls whether source location is appended to emitted messages.
+ DiagsShowLocation show_location;
+
+ /// Optional cleanup callback invoked before terminal-level process exit.
+ /// May be nullptr (no cleanup). See DiagsCleanupFunc.
+ DiagsCleanupFunc cleanup_func;
///////////////////////////
// conditional debugging //
///////////////////////////
+ /**
+ * @brief Return the per-connection DEBUG_OVERRIDE flag for the current
+ * Continuation.
+ *
+ * @return True if ContFlags::DEBUG_OVERRIDE is set on the currently
+ * executing Continuation; false otherwise or if no Continuation is active.
+ * @pre None.
+ * @post No state change.
+ * @par Errors
+ * None.
+ * @par Thread safety
+ * Safe to call from any thread; reads a thread-local
+ * Continuation flag.
+ */
bool
get_override() const override
{
return get_cont_flag(ContFlags::DEBUG_OVERRIDE);
}
+ /**
+ * @brief Test whether the given IP endpoint matches the configured debug
+ * client IP.
+ *
+ * @param[in] test_ip Endpoint whose IP address is compared against the
+ * configured debug client IP. The port is ignored.
+ * @return True if the IP address of @a test_ip equals the configured
+ * debug client IP; false otherwise or if no debug client IP is set.
+ * @pre None.
+ * @post No state change.
+ * @par Errors
+ * None.
+ * @par Thread safety
+ * Not thread-safe. Reads debug_client_ip without
+ * synchronization; concurrent writes from the config-update callback are
+ * a data race.
+ */
bool
test_override_ip(IpEndpoint const &test_ip)
{
return this->debug_client_ip == test_ip;
}
- // It seems to make a big difference to performance (due to the caching of
the enabled flag) to call this
- // function first before doing anything else for debug output. This
includes entering blocks with static
- // DbgCtl instances, or other static variables with non-const
initialization. Static variables with
- // non-const initialization inside a function have a hidden flag that is
checked every time the containing
- // block is entered, to see if the variable has been initialized or not.
- //
+ /**
+ * @brief Test whether emission for the given tag mode is globally enabled.
Review Comment:
Alan told me, back in the day, not to do @brief because Sphinx was
configured for brief after the `/**`. Thus this instead:
```cpp
/** Test whether...
```
This applies to the other `@brief` uses in this file as well.
--
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]
