Copilot commented on code in PR #13262:
URL: https://github.com/apache/trafficserver/pull/13262#discussion_r3408439517
##########
include/tscore/DiagsTypes.h:
##########
@@ -45,111 +45,353 @@
#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.
Review Comment:
This comment says output is emitted to all enabled sinks in "unspecified
order", but Diags::print_va() has a defined order (diagslog → stdout → stderr →
syslog). The documentation should match the current behavior (or explicitly say
the current order may change).
##########
src/tscore/Diags.cc:
##########
@@ -750,7 +713,7 @@ Diags::set_std_output(StdStream stream, const char *file)
log_log_error("[Warning]: %s is currently not bound to anything\n",
target_stream);
delete new_log;
lock();
- *current = nullptr;
+ *current = nullptr; // old_log is not freed here
unlock();
return false;
Review Comment:
On failure to open the new log file, this code sets the stdout/stderr log
pointer to nullptr and leaks old_log. It also leaves the underlying stdio FD
still bound to the previous file, so the pointer no longer reflects reality and
later code (e.g. should_roll_outputlog() assertions / dereferences) can crash.
Keep the existing BaseLogFile on failure and just return false.
##########
src/tscore/Diags.cc:
##########
@@ -759,7 +722,7 @@ Diags::set_std_output(StdStream stream, const char *file)
log_log_error("[Warning]: %s is currently not bound to anything\n",
target_stream);
delete new_log;
lock();
- *current = nullptr;
+ *current = nullptr; // old_log is not freed here
unlock();
return false;
Review Comment:
Same issue as the earlier failure path: setting *current to nullptr on
open/is_open failure leaks old_log and can break invariants
(stdout_log/stderr_log expected non-null) while the stdio FD is still bound to
the old file. Leave the existing BaseLogFile intact on failure.
##########
include/tscore/DiagsTypes.h:
##########
@@ -45,111 +45,353 @@
#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 be async-signal-safe if
+ * installed in a process that may emit terminal-level messages from a
+ * signal handler. 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).
Review Comment:
Requiring the cleanup callback to be async-signal-safe is misleading here:
Diags::error_va()/print_va() take locks, call vfprintf/syslog, etc., and are
not async-signal-safe. The contract should instead warn that terminal
diagnostics (and therefore this callback) are not safe to invoke from a signal
handler.
--
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]
