Copilot commented on code in PR #13262:
URL: https://github.com/apache/trafficserver/pull/13262#discussion_r3408235874
##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +513,228 @@ class Diags : public DebugInterface
va_end(ap);
}
+ /**
+ * @brief va_list form of error().
+ *
+ * @param[in] level DiagsLevel for routing and terminal handling.
+ * @param[in] loc Source location, or nullptr.
+ * @param[in] fmt Non-null printf-format string.
+ * @param[in] ap Initialized va_list. Consumed; caller MUST NOT reuse
+ * without va_end + va_start.
+ * @pre fmt is non-null; ap is initialized.
+ * @post Same as error(). For terminal levels, invokes cleanup_func (if
+ * set) then exits the process; does not return.
+ * @errors None signaled; I/O errors absorbed.
+ * @thread-safety Safe to call concurrently.
+ */
virtual void error_va(DiagsLevel level, const SourceLocation *loc, const
char *fmt, va_list ap) const;
+ /**
+ * @brief Print the current Diags configuration to fp.
+ *
+ * @param[in] fp Destination FILE *; defaults to stdout.
+ * @pre fp is a valid open stream.
+ * @post Configuration summary written to fp.
+ * @errors I/O errors on fp are not signaled.
+ * @thread-safety Not thread-safe. Reads config fields (_enabled,
+ * outputs, base_debug_tags, base_action_tags) without holding
+ * tag_table_lock. Concurrent reconfiguration may produce
+ * interleaved or inconsistent output.
+ */
void dump(FILE *fp = stdout) const;
+ /**
+ * @brief Enable tags matching the given PCRE2 pattern for the given mode.
+ *
+ * @param[in] taglist PCRE2 regex string, or nullptr. If nullptr, this call
+ * is a no-op and the previous pattern (if any) is unchanged.
+ * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+ * @pre taglist is null or a valid PCRE2 pattern string. The regex engine is
+ * PCRE2, not POSIX ERE; PCRE2-specific syntax (lookaheads, named groups,
+ * etc.) is accepted.
+ * @post If taglist is non-null: the new pattern unconditionally replaces the
+ * previous one. If the pattern fails to compile, the new (empty) Regex
+ * object still replaces the previous one — no tags will match until a
+ * valid pattern is installed. For DiagsTagType_Debug, if this is the
+ * process-global Diags instance, all registered DbgCtl objects are
+ * updated immediately to reflect the new pattern via DbgCtl::update.
+ * @errors Invalid regex is silently accepted; compile failure is not
+ * signaled. The previous pattern is NOT retained on compile failure.
+ * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+ * emission, but serialized with other reconfiguration methods.
+ */
void activate_taglist(const char *taglist, DiagsTagType mode =
DiagsTagType_Debug);
+ /**
+ * @brief Disable all tags for the given mode.
+ *
+ * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+ * @pre None.
+ * @post No tag matches for mode; activated_tags[mode] is null.
+ * @errors None.
+ * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+ * emission, but serialized with other reconfiguration methods.
+ */
void deactivate_all(DiagsTagType mode = DiagsTagType_Debug);
+ /**
+ * @brief Open the given BaseLogFile for use as a diagnostic log destination.
+ *
+ * Does NOT assign diags_log; the caller is responsible for that assignment
+ * on success. On failure, blf is deleted before returning.
+ *
+ * @param[in] blf Pointer to a constructed but not-yet-opened BaseLogFile,
+ * or nullptr (null is a no-op that returns true). When non-null, ownership
+ * transfers unconditionally: on success the caller must assign blf to
+ * diags_log; on failure blf has been deleted.
+ * @return True if blf was opened successfully (or blf is nullptr); false if
+ * the open failed (blf is deleted before returning false).
+ * @pre blf is null or points to a BaseLogFile that has not yet been opened.
+ * @post On true return: blf (if non-null) is open; caller must assign it to
+ * diags_log. On false return: blf has been deleted; diags_log is
unchanged.
+ * @errors Open failures cause a false return and an internal log_log_error
+ * diagnostic at LL_Error.
+ * @thread-safety Caller MUST serialize with all other reconfiguration
+ * methods. reseat_diagslog() acquires tag_table_lock only for the
+ * pointer swap that follows this call; setup_diagslog() itself is not
+ * called under the lock. Direct callers must provide equivalent
+ * serialization for the pointer swap.
+ */
bool setup_diagslog(BaseLogFile *blf);
+
+ /**
+ * @brief Configure the rolling policy for diags.log.
+ *
+ * @param[in] re Rolling policy (see RollingEnabledValues).
+ * @param[in] ri Rolling interval in seconds (used by time-based policies).
+ * @param[in] rs Rolling size threshold in bytes (used by size-based
+ * policies).
+ * @pre None.
+ * @post Rolling policy fields are updated in the calling thread.
+ * @errors None.
+ * @thread-safety No lock is acquired. The caller must ensure no concurrent
+ * calls to should_roll_diagslog() occur during reconfiguration.
+ */
void config_roll_diagslog(RollingEnabledValues re, int ri, int rs);
+
+ /**
+ * @brief Configure the rolling policy for the output log (traffic.out).
+ *
+ * @param[in] re Rolling policy.
+ * @param[in] ri Rolling interval in seconds.
+ * @param[in] rs Rolling size threshold in bytes.
+ * @pre None.
+ * @post Rolling policy fields are updated in the calling thread.
+ * @errors None.
+ * @thread-safety No lock is acquired. The caller must ensure no concurrent
+ * calls to should_roll_outputlog() occur during reconfiguration.
+ */
void config_roll_outputlog(RollingEnabledValues re, int ri, int rs);
+
+ /**
+ * @brief Close the current diags.log and reopen it at the configured path.
+ *
+ * Intended for use after an external log rotation tool has renamed or
+ * removed the active diags.log. The implementation:
+ * 1. Flushes the current file's buffered output.
+ * 2. Captures the configured filename from the active BaseLogFile.
+ * 3. Constructs a new BaseLogFile at that filename. The filename is
+ * passed to the OS verbatim — symlinks are re-resolved at each call.
+ * 4. On success: atomically swaps the new file into place under
+ * tag_table_lock and destroys the previous BaseLogFile (closing its
+ * FILE *).
+ * 5. On failure: leaves the previous file in place.
+ *
+ * @return False if diags_log is null or not yet initialized (safe no-op).
+ * True in all other cases, including when the internal reopen fails —
+ * reopen failures are not reflected in the return value and are
+ * observable only via the internal diagnostic trace log.
+ * @pre A Diags instance is active and diags_log is initialized
+ * (is_init() == true). If this precondition is not met, returns false
+ * without performing a reopen — this is the safe no-op path for calls
+ * that arrive before the diagnostics subsystem is initialized.
+ * @post On success: diags_log is a fresh BaseLogFile at the configured
+ * path; the previous BaseLogFile (and its FILE *) is destroyed; all
+ * subsequent emission goes to the new file.
+ * On failure: diags_log is unchanged; the newly allocated BaseLogFile
+ * is deleted before it returns.
+ * @errors Open failures are not directly signaled via the return value;
+ * failure is reported to the internal diagnostic trace log only.
+ * @thread-safety The swap in step 4 is performed under tag_table_lock.
+ * Concurrent emission either observes the pre-swap or post-swap log;
+ * no message is lost or written to a destroyed FILE *.
+ * @note When diagnostic output is disabled or redirected to a non-file
+ * sink (e.g., syslog), diags_log is null or uninitialized and the
+ * initialization guard causes an early false return. SIGUSR2 is a
+ * safe no-op in that configuration.
+ */
bool reseat_diagslog();
+
+ /**
+ * @brief Roll diags.log if the current rolling policy condition is met.
+ *
+ * @return True if the underlying file was renamed (rolled); false if no
+ * roll condition was triggered, or if fstat failed. Note: true is
+ * returned even when the subsequent reopen of the new log file fails.
+ * @pre None.
+ * @post If the rolling condition is met: the current diags.log is flushed,
+ * rolled (renamed), and replaced by a new BaseLogFile at the same path.
+ * If not: no state change. fstat failure causes an early false return
+ * without rolling.
+ * @errors None signaled. fstat and reopen failures silently suppress the
+ * replacement; the rolled state of the original file is not reversed.
+ * @thread-safety tag_table_lock is acquired only during the BaseLogFile
+ * pointer swap. The rolling-condition checks and file operations execute
+ * without a lock; the caller must ensure no concurrent reconfiguration
+ * (see config_roll_diagslog()).
+ */
bool should_roll_diagslog();
+
+ /**
+ * @brief Roll stdout_log and stderr_log if the current rolling policy
+ * condition is met.
+ *
+ * @return True if any output log was rolled; false otherwise.
+ * @pre stdout_log and stderr_log are non-null.
+ * @post If the rolling condition is met: affected logs are flushed, rolled,
+ * and replaced by new BaseLogFile instances at the same paths.
+ * If not: no state change. fstat failure causes an early false return.
+ * @errors None signaled. fstat failures silently suppress the roll.
+ * @thread-safety Same as should_roll_diagslog(); see
config_roll_outputlog().
+ */
bool should_roll_outputlog();
+ /**
+ * @brief Reseat the named standard stream to a file at the given path.
+ *
+ * @param[in] stream STDOUT or STDERR (see StdStream).
+ * @param[in] file Non-null path string. Passed verbatim to the OS open
+ * call; symlinks are re-resolved at each call. An empty string causes an
+ * immediate false return without modifying any state.
+ * @return True on success; false if file is empty, the file could not be
+ * opened, or the resulting FILE * is null.
+ * @pre A Diags instance is active; file is non-null (passing null is UB —
+ * it is passed directly to strcmp without a null guard).
+ * @post On success: the new file is open and bound as the named stream;
+ * the previous BaseLogFile is deleted.
+ * On failure: the stream pointer is set to nullptr — the previous
+ * binding is NOT preserved. The previous BaseLogFile is NOT freed
+ * (leaked on the failure path).
+ * @errors File-open failures cause a false return and internal
+ * log_log_error messages; the named stream is left unbound (nullptr).
+ * @thread-safety Acquires tag_table_lock internally on both success and
+ * failure paths.
+ */
bool set_std_output(StdStream stream, const char *file);
- const char *base_debug_tags; // internal copy of default debug tags
- const char *base_action_tags; // internal copy of default action tags
+ /// Initial debug tag regex string; copied at construction. May be nullptr.
+ const char *base_debug_tags;
+
+ /// Initial action tag regex string; copied at construction. May be nullptr.
+ const char *base_action_tags;
+ /// Optional IP address for per-connection debug override. When set,
+ /// on(mode) returns true for connections whose remote IP matches this.
IpAddr debug_client_ip;
Review Comment:
The `debug_client_ip` field comment implies `on(mode)` becomes true for
matching remote IPs, but the IP match is only used to set
ContFlags::DEBUG_OVERRIDE and only affects emission when
`DiagsConfigState::enabled(mode) == 2` (DEBUG_OVERRIDE mode). As written, this
overstates the field’s effect.
##########
src/tscore/Diags.cc:
##########
@@ -714,12 +698,6 @@ Diags::should_roll_outputlog()
return ret_val;
}
-/*
- * Sets up a BaseLogFile for the specified file. Then it binds the specified
standard steam
- * to the aforementioned BaseLogFile.
- *
- * Returns true on successful binding and setup, false otherwise
- */
bool
Diags::set_std_output(StdStream stream, const char *file)
{
Review Comment:
If you do change `set_std_output()` to preserve the old binding on failure
(recommended), please also update the new doxygen contract in
`include/tscore/DiagsTypes.h` which currently documents the failure behavior as
unbinding + leaking the old BaseLogFile.
##########
include/tscore/DiagsTypes.h:
##########
@@ -45,111 +45,317 @@
#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.
+ *
+ * @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 (=0) targets the standard output stream; STDERR targets the
+ * standard error stream. Values are used as array indices; do not renumber.
+ */
Review Comment:
The StdStream enum comment says the values are used as array indices / must
not be renumbered, but there are no array-index uses (only control-flow checks
like `if (stream == StdStream::STDOUT)`). This is misleading documentation.
##########
src/tscore/Diags.cc:
##########
@@ -320,8 +314,9 @@ Diags::print_va(const char *debug_tag, DiagsLevel
diags_level, const SourceLocat
//
// This routine inquires if a particular <tag> in the tag table of
// type <mode> is activated, returning true if it is, false if it
-// isn't. If <tag> is nullptr, true is returned. The call uses a lock
-// to get atomic access to the tag tables.
+// isn't. If <tag> is nullptr, true is returned. The active regex
+// pointer is snapshotted under tag_table_lock; regex execution runs
Review Comment:
The block comment header immediately above this section still shows `bool
Diags::tag_activated(char * tag, ...)`, but the actual function is `bool
Diags::tag_activated(const char *tag, DiagsTagType mode) const`. Please update
the signature in the comment to match the code.
##########
src/tscore/Diags.cc:
##########
@@ -714,12 +698,6 @@ Diags::should_roll_outputlog()
return ret_val;
}
-/*
- * Sets up a BaseLogFile for the specified file. Then it binds the specified
standard steam
- * to the aforementioned BaseLogFile.
- *
- * Returns true on successful binding and setup, false otherwise
- */
bool
Diags::set_std_output(StdStream stream, const char *file)
{
Review Comment:
`set_std_output()`’s error handling currently nulls out
`stdout_log`/`stderr_log` and returns without freeing the previous BaseLogFile.
This leaks memory and can violate later assumptions (e.g.,
`should_roll_outputlog()` asserts these pointers are non-null). Consider
leaving the existing binding intact on failure (and adjust the warning text
accordingly).
##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +513,228 @@ class Diags : public DebugInterface
va_end(ap);
}
+ /**
+ * @brief va_list form of error().
+ *
+ * @param[in] level DiagsLevel for routing and terminal handling.
+ * @param[in] loc Source location, or nullptr.
+ * @param[in] fmt Non-null printf-format string.
+ * @param[in] ap Initialized va_list. Consumed; caller MUST NOT reuse
+ * without va_end + va_start.
+ * @pre fmt is non-null; ap is initialized.
+ * @post Same as error(). For terminal levels, invokes cleanup_func (if
+ * set) then exits the process; does not return.
+ * @errors None signaled; I/O errors absorbed.
+ * @thread-safety Safe to call concurrently.
+ */
virtual void error_va(DiagsLevel level, const SourceLocation *loc, const
char *fmt, va_list ap) const;
+ /**
+ * @brief Print the current Diags configuration to fp.
+ *
+ * @param[in] fp Destination FILE *; defaults to stdout.
+ * @pre fp is a valid open stream.
+ * @post Configuration summary written to fp.
+ * @errors I/O errors on fp are not signaled.
+ * @thread-safety Not thread-safe. Reads config fields (_enabled,
+ * outputs, base_debug_tags, base_action_tags) without holding
+ * tag_table_lock. Concurrent reconfiguration may produce
+ * interleaved or inconsistent output.
+ */
void dump(FILE *fp = stdout) const;
+ /**
+ * @brief Enable tags matching the given PCRE2 pattern for the given mode.
+ *
+ * @param[in] taglist PCRE2 regex string, or nullptr. If nullptr, this call
+ * is a no-op and the previous pattern (if any) is unchanged.
+ * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
Review Comment:
`activate_taglist()` is documented as a complete no-op when `taglist ==
nullptr`, but the implementation still runs `DbgCtl::update()` for the
process-global Diags instance in debug mode, which can be an observable (and
potentially expensive) side effect. The parameter documentation should reflect
that only the pattern is unchanged.
--
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]
