Copilot commented on code in PR #13262:
URL: https://github.com/apache/trafficserver/pull/13262#discussion_r3403470774
##########
include/tscore/Regression.h:
##########
@@ -101,4 +101,10 @@ int rprintf(RegressionTest *t, const char *format,
...);
int rperf(RegressionTest *t, const char *tag, double val);
const char *regression_status_string(int status);
+/// @brief Forces all diagnostic output to stderr regardless of routing config.
+///
+/// @pre May be called at any time after initialization.
+/// @post All subsequent print_va() calls write to stderr unconditionally,
+/// in addition to any other configured outputs.
+/// @note Intended for regression test harnesses only. Not reversible.
Review Comment:
The new contract says this can be called "at any time after initialization",
but the implementation toggles a non-atomic global flag that is read
concurrently in Diags::print_va(). Calling this after multiple threads start
logging can introduce a C++ data race; the comment should reflect the intended
single-threaded startup usage (or the flag should be made atomic).
##########
include/tscore/DiagsTypes.h:
##########
@@ -142,14 +222,24 @@ class Diags : public DebugInterface
// 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 Returns true if diagnostic output is globally enabled for mode.
+ ///
+ /// @param mode DiagsTagType_Debug or DiagsTagType_Action.
+ /// @return true if output for mode is enabled (globally or via IP override).
+ /// @note This is the fast-path check. It does not perform tag matching.
+ /// Thread-safe; reads atomic state without acquiring the internal
lock.
Review Comment:
This note says the fast-path check "reads atomic state", but
config.enabled() reads a non-atomic int. The documentation should avoid
claiming atomic/thread-safe behavior here.
##########
include/tscore/DiagsTypes.h:
##########
@@ -160,10 +250,24 @@ class Diags : public DebugInterface
// low-level tag inquiry functions //
/////////////////////////////////////
- // This does regex match against the tag.
- //
+ /// @brief Returns true if tag matches the active regex for mode.
+ ///
+ /// @param tag Tag string to match. If nullptr, returns true unconditionally.
+ /// @param mode DiagsTagType_Debug or DiagsTagType_Action.
+ /// @return true if tag matches the compiled regex, or if no regex is set, or
+ /// if tag is nullptr.
Review Comment:
tag_activated() is documented as returning true when no regex is set, but
the implementation returns false when the regex pointer is null (disabling all
tag-based output for that mode). This is a functional mismatch in the API docs.
##########
include/tscore/DiagsTypes.h:
##########
@@ -189,9 +304,32 @@ class Diags : public DebugInterface
va_end(ap);
}
+ /// @brief Core variadic implementation for diagnostic output.
+ ///
+ /// Formats and writes the message to all outputs configured for level.
+ /// Syslog writes occur after the internal lock is released.
Review Comment:
This comment states syslog writes happen after releasing the internal lock,
but Diags::print_va keeps the lock held across the syslog call on FreeBSD (#if
defined(freebsd)). The doc should not claim unconditional lock release ordering.
##########
include/tscore/DiagsTypes.h:
##########
@@ -189,9 +304,32 @@ class Diags : public DebugInterface
va_end(ap);
}
+ /// @brief Core variadic implementation for diagnostic output.
+ ///
+ /// Formats and writes the message to all outputs configured for level.
+ /// Syslog writes occur after the internal lock is released.
+ ///
+ /// @param tag Optional tag label. May be nullptr.
+ /// @param level Diagnostic severity level. Must be < DiagsLevel_Count.
+ /// @param loc Optional source location. May be nullptr.
+ /// @param fmt printf-style format string. Must not be nullptr.
+ /// @param ap Argument list for fmt.
+ /// @pre level < DiagsLevel_Count.
+ /// @post Message is written to all enabled outputs for level. Syslog, if
+ /// configured, is written outside the internal lock.
+ /// @note Thread-safe. Acquires the internal lock for file writes.
void print_va(const char *tag, DiagsLevel level, const SourceLocation *loc,
const char *fmt, va_list ap) const override;
- /// Print the log message only if tag is enabled.
+ /// @brief Conditionally outputs a diagnostic message if tag is enabled.
+ ///
+ /// Equivalent to: if (on(tag, DiagsTagType_Debug)) print(tag, level, loc,
fmt, ...)
+ ///
+ /// @param tag Tag to check. Must not be nullptr.
Review Comment:
The log() documentation says tag must not be nullptr, but the implementation
permits nullptr (it becomes a pure global-enable check). Update the doc to
match the actual behavior.
##########
include/tscore/DiagsTypes.h:
##########
@@ -48,39 +48,60 @@
#define DIAGS_MAGIC 0x12345678
#define BYTES_IN_MB 1000000
+/// Selects whether a tag applies to debug output or action-control output.
+/// Values are used as array indices; do not renumber.
enum DiagsTagType {
DiagsTagType_Debug = 0, // do not renumber --- used as array index
DiagsTagType_Action = 1
};
+/// Per-level output routing: controls which sinks receive messages at a given
+/// diagnostic level.
struct DiagsModeOutput {
bool to_stdout;
bool to_stderr;
bool to_syslog;
bool to_diagslog;
};
+/// Identifies which standard stream is the target of a redirect operation.
enum StdStream { STDOUT = 0, STDERR };
+/// Controls when the Diags subsystem automatically rolls (renames and reopens)
+/// a log file.
enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE,
ROLL_ON_TIME_OR_SIZE, INVALID_ROLLING_VALUE };
#define DiagsLevel_Count DL_Undefined
#define DiagsLevel_IsTerminal(_l) (((_l) >= DL_Fatal) && ((_l) < DL_Undefined))
-// Cleanup Function Prototype - Called before ink_fatal to
-// cleanup process state
+/// Prototype for an optional cleanup callback invoked before process exit
+/// when a terminal-level diagnostic (DL_Fatal, DL_Alert, DL_Emergency) is
emitted.
using DiagsCleanupFunc = void (*)();
class DiagsConfigState
{
public:
+ /// @brief Returns the enabled state for the given tag type.
+ ///
+ /// @param dtt DiagsTagType_Debug or DiagsTagType_Action.
+ /// @return 0 if disabled, 1 if globally enabled, 2 if IP-override enabled.
+ /// @note Intended as a fast-path check. Thread-safe for reads; writes use
+ /// the setter overload.
Review Comment:
This note claims DiagsConfigState::enabled() is thread-safe for reads, but
the backing storage is a plain static int array (not atomic). Without external
synchronization, concurrent writes via the setter would be a data race; the
comment should not promise thread safety.
##########
include/tscore/DiagsTypes.h:
##########
@@ -107,29 +128,88 @@ class DiagsConfigState
class Diags : public DebugInterface
{
public:
+ /// @brief Initializes the Diags logging subsystem.
+ ///
+ /// @param prefix_string Non-empty string prepended to every log line.
+ /// @param base_debug_tags Initial debug tag regex pattern, or nullptr to
disable
+ /// debug output. Vertical-bar-separated (e.g., "http|cache").
+ /// @param base_action_tags Initial action tag regex pattern, or nullptr to
disable.
+ /// @param _diags_log BaseLogFile for the primary diagnostics log, or nullptr
+ /// if no diags log is desired.
+ /// @param diags_log_perm File permissions for the diags log (-1 to inherit).
+ /// @param output_log_perm File permissions for stdout/stderr redirects (-1
to inherit).
+ /// @pre prefix_string must not be empty.
+ /// @post stdout_log and stderr_log are initialized and open. diags_log is
open
+ /// if _diags_log was non-null and the file was accessible; otherwise
nullptr.
+ /// Rolling is disabled by default. cleanup_func is nullptr.
+ /// @note Thread safety: call from the main thread during single-threaded
startup.
+ /// No concurrent logging may occur until construction is complete.
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 Closes all log file handles and releases tag resources.
+ ///
+ /// @pre No concurrent logging or configuration changes may be in progress.
+ /// @post All log file handles are closed and freed. Tag regex objects are
released.
+ /// @note Thread safety: call only during shutdown when no other threads are
+ /// actively calling logging methods.
virtual ~Diags();
+ /// Primary diagnostics log file (e.g., diags.log).
+ /// May be nullptr if no diagnostics log was configured or if the file could
+ /// not be opened at initialization. Replaced atomically under the internal
+ /// lock during log rotation and reseat operations. Do not cache this pointer
+ /// across calls that may trigger rotation.
BaseLogFile *diags_log;
+
+ /// stdout redirect log file. Never nullptr after construction.
+ /// Replaced atomically under the internal lock during set_std_output()
+ /// and output log rolling. Do not cache this pointer.
BaseLogFile *stdout_log;
+
+ /// stderr redirect log file. Never nullptr after construction.
+ /// Replaced atomically under the internal lock during set_std_output()
+ /// and output log rolling. Do not cache this pointer.
BaseLogFile *stderr_log;
Review Comment:
stderr_log is documented as "Never nullptr after construction", but
set_std_output() explicitly sets stderr_log to nullptr on failure. The field
comment should allow nullptr after failed redirection.
##########
include/tscore/DiagsTypes.h:
##########
@@ -142,14 +222,24 @@ class Diags : public DebugInterface
// 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 Returns true if diagnostic output is globally enabled for mode.
+ ///
+ /// @param mode DiagsTagType_Debug or DiagsTagType_Action.
+ /// @return true if output for mode is enabled (globally or via IP override).
+ /// @note This is the fast-path check. It does not perform tag matching.
+ /// Thread-safe; reads atomic state without acquiring the internal
lock.
bool
on(DiagsTagType mode = DiagsTagType_Debug) const
{
return (config.enabled(mode) & 1) || (config.enabled(mode) == 2 &&
this->get_override());
}
- // Returns true if tag is enabled for mode.
- //
+ /// @brief Returns true if the given tag is enabled for mode.
+ ///
+ /// @param tag Tag string to check. Must not be nullptr.
Review Comment:
The documentation says tag "Must not be nullptr", but the implementation
allows nullptr (tag_activated(nullptr) returns true), making on(nullptr)
equivalent to the global enable check. The comment should match the actual
behavior.
##########
include/tscore/DiagsTypes.h:
##########
@@ -189,9 +304,32 @@ class Diags : public DebugInterface
va_end(ap);
}
+ /// @brief Core variadic implementation for diagnostic output.
+ ///
+ /// Formats and writes the message to all outputs configured for level.
+ /// Syslog writes occur after the internal lock is released.
+ ///
+ /// @param tag Optional tag label. May be nullptr.
+ /// @param level Diagnostic severity level. Must be < DiagsLevel_Count.
+ /// @param loc Optional source location. May be nullptr.
+ /// @param fmt printf-style format string. Must not be nullptr.
+ /// @param ap Argument list for fmt.
+ /// @pre level < DiagsLevel_Count.
+ /// @post Message is written to all enabled outputs for level. Syslog, if
+ /// configured, is written outside the internal lock.
Review Comment:
This postcondition claims syslog output is written outside the internal
lock, but on FreeBSD the lock is released only after the syslog write. Please
qualify this statement to match the platform-specific implementation.
##########
include/tscore/DiagsTypes.h:
##########
@@ -203,6 +341,15 @@ class Diags : public DebugInterface
}
}
+ /// @brief Variadic version of log().
+ ///
+ /// @param tag Tag to check. Must not be nullptr.
Review Comment:
The log_va() documentation says tag must not be nullptr, but the
implementation permits nullptr (it becomes a pure global-enable check). Update
the doc to match the actual behavior.
##########
include/tscore/Diags.h:
##########
@@ -61,12 +61,26 @@ class DiagsPtr
{
public:
friend Diags *diags();
- static void set(Diags *new_ptr);
+
+ /// @brief Registers a Diags instance as the global singleton.
+ ///
+ /// @param new_ptr The Diags instance to register. Must not be nullptr.
+ /// @pre Called during single-threaded process initialization only.
+ /// @post diags() returns new_ptr. The instance is also registered with the
+ /// DebugInterface subsystem.
+ /// @note Not thread-safe. Must be called exactly once before any logging
occurs.
+ static void set(Diags *new_ptr);
private:
static Diags *_diags_ptr;
};
+/// @brief Returns the global Diags singleton.
+///
+/// @pre DiagsPtr::set() must have been called with a valid Diags instance.
+/// @return Pointer to the global Diags instance, or nullptr if not yet
initialized.
Review Comment:
The `@pre` says DiagsPtr::set() "must" have been called with a valid
instance, but the very next line says diags() may return nullptr if not
initialized. These two lines contradict each other; either relax the
precondition or drop the nullptr wording.
##########
include/tscore/DiagsTypes.h:
##########
@@ -107,29 +128,88 @@ class DiagsConfigState
class Diags : public DebugInterface
{
public:
+ /// @brief Initializes the Diags logging subsystem.
+ ///
+ /// @param prefix_string Non-empty string prepended to every log line.
+ /// @param base_debug_tags Initial debug tag regex pattern, or nullptr to
disable
+ /// debug output. Vertical-bar-separated (e.g., "http|cache").
+ /// @param base_action_tags Initial action tag regex pattern, or nullptr to
disable.
+ /// @param _diags_log BaseLogFile for the primary diagnostics log, or nullptr
+ /// if no diags log is desired.
+ /// @param diags_log_perm File permissions for the diags log (-1 to inherit).
+ /// @param output_log_perm File permissions for stdout/stderr redirects (-1
to inherit).
+ /// @pre prefix_string must not be empty.
+ /// @post stdout_log and stderr_log are initialized and open. diags_log is
open
+ /// if _diags_log was non-null and the file was accessible; otherwise
nullptr.
+ /// Rolling is disabled by default. cleanup_func is nullptr.
+ /// @note Thread safety: call from the main thread during single-threaded
startup.
+ /// No concurrent logging may occur until construction is complete.
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 Closes all log file handles and releases tag resources.
+ ///
+ /// @pre No concurrent logging or configuration changes may be in progress.
+ /// @post All log file handles are closed and freed. Tag regex objects are
released.
+ /// @note Thread safety: call only during shutdown when no other threads are
+ /// actively calling logging methods.
virtual ~Diags();
+ /// Primary diagnostics log file (e.g., diags.log).
+ /// May be nullptr if no diagnostics log was configured or if the file could
+ /// not be opened at initialization. Replaced atomically under the internal
+ /// lock during log rotation and reseat operations. Do not cache this pointer
+ /// across calls that may trigger rotation.
BaseLogFile *diags_log;
+
+ /// stdout redirect log file. Never nullptr after construction.
+ /// Replaced atomically under the internal lock during set_std_output()
+ /// and output log rolling. Do not cache this pointer.
BaseLogFile *stdout_log;
Review Comment:
stdout_log is documented as "Never nullptr after construction", but
set_std_output() explicitly sets stdout_log to nullptr on failure. The field
comment should allow nullptr after failed redirection.
##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +383,150 @@ class Diags : public DebugInterface
va_end(ap);
}
+ /// @brief Variadic implementation of error().
+ ///
+ /// @param level Diagnostic severity level.
+ /// @param loc Optional source location. May be nullptr.
+ /// @param fmt printf-style format string.
+ /// @param ap Argument list for fmt.
+ /// @post Message is written. Process exits if level is terminal.
+ /// @note Thread-safe for the output step. Virtual to allow test overrides.
virtual void error_va(DiagsLevel level, const SourceLocation *loc, const
char *fmt, va_list ap) const;
+ /// @brief Writes a human-readable summary of the current Diags
configuration.
+ ///
+ /// @param fp Output FILE stream. Defaults to stdout.
+ /// @pre fp must be a valid open stream.
+ /// @post A formatted table of enabled states, tag patterns, and output
+ /// routing is written to fp.
+ /// @note Not intended for production code paths. Configuration is read
+ /// without the internal lock; output is informational only.
void dump(FILE *fp = stdout) const;
+ /// @brief Replaces the active tag filter with the compiled form of taglist.
+ ///
+ /// @param taglist Vertical-bar-separated regex pattern (e.g., "http|dns"),
+ /// or nullptr to clear the active pattern.
+ /// @param mode DiagsTagType_Debug or DiagsTagType_Action.
Review Comment:
activate_taglist() is documented as accepting nullptr "to clear the active
pattern", but the implementation treats nullptr as a no-op (it does not clear
anything). Clearing is done by deactivate_all().
--
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]
