Copilot commented on code in PR #13262:
URL: https://github.com/apache/trafficserver/pull/13262#discussion_r3404132926
##########
include/tscore/DiagsTypes.h:
##########
@@ -142,14 +228,26 @@ 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.
+ /// Does not acquire the internal lock. Reads
DiagsConfigState::enabled,
+ /// which has a known data race with concurrent writes (see
+ /// DiagsConfigState::enabled note).
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.
+ /// @param mode DiagsTagType_Debug or DiagsTagType_Action.
+ /// @return true if the global enable is set and tag matches the active
pattern.
+ /// @note Thread-safe; acquires the internal lock to read the tag regex.
Review Comment:
The tag parameter is documented as non-null, but the implementation allows
nullptr (tag_activated(nullptr) returns true), making on(nullptr, mode)
equivalent to the fast-path global enable check. The docs should match the
actual behavior.
##########
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 documentation 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 it after multiple threads start
logging is a C++ data race; the contract should restrict this to
single-threaded startup (or the flag must become atomic).
##########
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.
+/// @note Thread-safe for reading; the pointer is set once during
single-threaded
+/// initialization and never changes thereafter.
Review Comment:
The diags() documentation has a contradictory contract: it says
DiagsPtr::set() must have been called, but also says the function may return
nullptr if not yet initialized. Update the doc to consistently describe the
nullptr behavior.
##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +391,162 @@ 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.
Review Comment:
activate_taglist() is documented as accepting nullptr to clear the active
pattern, but the implementation treats nullptr as a no-op (the active regex
remains unchanged). Clearing is done by deactivate_all().
##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +391,162 @@ 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.
+ /// @pre taglist, if non-null, must be a valid regex pattern.
+ /// @post Subsequent on(tag, mode) calls match against the new pattern.
+ /// If this is the global Diags instance, DbgCtl's tag cache is
updated.
+ /// @note Thread-safe. The regex is compiled outside the internal lock; only
+ /// the pointer swap is performed under the lock.
void activate_taglist(const char *taglist, DiagsTagType mode =
DiagsTagType_Debug);
+ /// @brief Clears the active tag filter, disabling all tag-based output for
mode.
+ ///
+ /// @param mode DiagsTagType_Debug or DiagsTagType_Action.
+ /// @post on(tag, mode) returns false for all tags until activate_taglist()
+ /// is called again. If this is the global Diags instance, DbgCtl's
+ /// cache is updated.
+ /// @note Thread-safe. Acquires the internal lock for the pointer reset.
void deactivate_all(DiagsTagType mode = DiagsTagType_Debug);
+ /// @brief Opens blf for writing.
+ ///
+ /// @param blf A BaseLogFile pointing to the desired log path, or nullptr
+ /// (treated as a no-op).
+ /// @return true if blf was nullptr or was opened successfully.
+ /// false if blf could not be opened (blf is deleted and an error
+ /// is written to stderr).
+ /// @pre If blf is non-null, it must not already be open.
+ /// @post On success, blf is open and ready for writes. The caller is
+ /// responsible for assigning blf to diags_log.
+ /// On failure, blf is freed; diags_log is unchanged.
+ /// @note Thread safety: intended for single-threaded initialization.
+ /// For runtime reseat, use reseat_diagslog() instead.
bool setup_diagslog(BaseLogFile *blf);
+
+ /// @brief Configures automatic rolling for the diagnostics log.
+ ///
+ /// @param re Rolling mode (NO_ROLLING, ROLL_ON_TIME, ROLL_ON_SIZE,
+ /// ROLL_ON_TIME_OR_SIZE).
+ /// @param ri Roll interval in seconds for time-based rolling (-1 to
disable).
+ /// @param rs Roll size threshold in megabytes for size-based rolling (-1 to
disable).
+ /// @post should_roll_diagslog() uses the new configuration on the next call.
+ /// @note Call during initialization or single-threaded configuration only.
void config_roll_diagslog(RollingEnabledValues re, int ri, int rs);
+
+ /// @brief Configures automatic rolling for stdout and stderr output logs.
+ ///
+ /// @param re Rolling mode.
+ /// @param ri Roll interval in seconds (-1 to disable).
+ /// @param rs Roll size threshold in megabytes (-1 to disable).
+ /// @post should_roll_outputlog() uses the new configuration on the next
call.
+ /// @note Call during initialization or single-threaded configuration only.
void config_roll_outputlog(RollingEnabledValues re, int ri, int rs);
+
+ /// @brief Reopens the diagnostics log at its current configured path.
+ ///
+ /// Closes the current file descriptor and opens a new one at the same path,
+ /// allowing the log to continue writing to a freshly created file after an
+ /// external tool has renamed or moved the previous one.
+ ///
+ /// @return true if diags_log was initialized (regardless of whether the
+ /// reopen succeeded). false if diags_log is nullptr or
uninitialized.
+ /// @pre diags_log may or may not point to an existing file on disk.
+ /// @post On success: diags_log points to a new file opened at the original
+ /// path. The old file descriptor is closed.
+ /// On failure to reopen: diags_log is unchanged; the old descriptor
+ /// remains in use.
+ /// @note Not concurrency-safe with respect to should_roll_diagslog().
+ /// Reads diags_log outside the internal lock before acquiring it for
+ /// the pointer swap. Callers must ensure reseat and roll operations
+ /// are externally serialized. Concurrent logging via print_va() is
+ /// safe because print_va() holds the internal lock for all accesses.
bool reseat_diagslog();
+
+ /// @brief Checks rolling conditions and rolls the diagnostics log if due.
+ ///
+ /// Evaluates the configured rolling mode (time, size, or both). If the
+ /// condition is met, renames the current log file with a timestamp suffix
+ /// and opens a new file at the original path.
+ ///
+ /// @return true if the log was rolled. false if no rolling was needed,
+ /// or if diags_log is nullptr or uninitialized.
+ /// @note Not concurrency-safe with respect to reseat_diagslog().
+ /// Reads diags_log outside the internal lock before acquiring it for
+ /// the pointer swap. Callers must ensure roll and reseat operations
+ /// are externally serialized. Concurrent logging via print_va() is
+ /// safe because print_va() holds the internal lock for all accesses.
bool should_roll_diagslog();
+
+ /// @brief Checks rolling conditions and rolls stdout/stderr output logs if
due.
+ ///
+ /// Evaluates rolling mode for the output logs. When stdout and stderr share
+ /// the same file, both pointers are updated atomically.
+ ///
+ /// @return true if any output log was rolled. false otherwise.
+ /// @note Pointer swaps are performed under the internal lock via
+ /// set_std_output(). File inspection (fstat, fflush) occurs outside
+ /// the lock. Callers must ensure no concurrent set_std_output() call
+ /// is in progress.
bool should_roll_outputlog();
+ /// @brief Redirects stdout or stderr to the specified file.
+ ///
+ /// Opens a new BaseLogFile at file, then uses dup2() to redirect the
+ /// corresponding OS file descriptor. The previous log handle is closed.
+ ///
+ /// @param stream StdStream::STDOUT or StdStream::STDERR.
+ /// @param file Path to the new output file. Must not be empty.
+ /// @return true if the file was opened and the redirect succeeded.
+ /// false if the file could not be opened (an error is written to
+ /// stderr and the log handle is set to nullptr).
Review Comment:
The return-value docs claim failures are reported by writing an error to
stderr, but the implementation uses log_log_error(), which is compiled out by
default (BASELOGFILE_DEBUG_MODE=0). The docs should avoid guaranteeing stderr
output.
##########
include/tscore/DiagsTypes.h:
##########
@@ -189,9 +312,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.
+ ///
+ /// @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.
+ /// @note Thread-safe. Acquires the internal lock for file writes. On most
+ /// platforms the lock is released before syslog writes; on FreeBSD the
+ /// lock is held across syslog as well.
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 and treats it as a pure global-enable check (no tag filtering).
Update the parameter contract to avoid misleading API docs.
##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +391,162 @@ 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.
+ /// @pre taglist, if non-null, must be a valid regex pattern.
+ /// @post Subsequent on(tag, mode) calls match against the new pattern.
+ /// If this is the global Diags instance, DbgCtl's tag cache is
updated.
+ /// @note Thread-safe. The regex is compiled outside the internal lock; only
+ /// the pointer swap is performed under the lock.
void activate_taglist(const char *taglist, DiagsTagType mode =
DiagsTagType_Debug);
+ /// @brief Clears the active tag filter, disabling all tag-based output for
mode.
+ ///
+ /// @param mode DiagsTagType_Debug or DiagsTagType_Action.
+ /// @post on(tag, mode) returns false for all tags until activate_taglist()
+ /// is called again. If this is the global Diags instance, DbgCtl's
+ /// cache is updated.
+ /// @note Thread-safe. Acquires the internal lock for the pointer reset.
void deactivate_all(DiagsTagType mode = DiagsTagType_Debug);
+ /// @brief Opens blf for writing.
+ ///
+ /// @param blf A BaseLogFile pointing to the desired log path, or nullptr
+ /// (treated as a no-op).
+ /// @return true if blf was nullptr or was opened successfully.
+ /// false if blf could not be opened (blf is deleted and an error
+ /// is written to stderr).
Review Comment:
The return-value docs claim failures are reported by writing an error to
stderr, but the implementation uses log_log_error(), which is compiled out by
default (BASELOGFILE_DEBUG_MODE=0). The docs should avoid guaranteeing stderr
output.
##########
include/tscore/DiagsTypes.h:
##########
@@ -203,6 +349,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 and treats it as a pure global-enable check (no
tag filtering). Update the parameter contract to avoid misleading API docs.
--
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]
