Copilot commented on code in PR #13254:
URL: https://github.com/apache/trafficserver/pull/13254#discussion_r3390249795
##########
src/tscore/Diags.cc:
##########
@@ -810,3 +703,162 @@ Diags::rebind_std_stream(StdStream stream, int new_fd)
}
return false;
}
+
+bool
+Diags::roll_diags_on_filesize(int mb_threshold)
+{
+ // if we can't even check the file, we can forget about rotating
+ struct stat buf;
+ if (fstat(fileno(diags_log->m_fp), &buf) != 0) {
+ throw std::runtime_error{"could not stat diags log"};
+ }
+
+ off_t size = buf.st_size;
+ if (mb_threshold == -1 || size < static_cast<off_t>(mb_threshold) *
BYTES_IN_MB) {
+ return false;
+ }
+
+ fflush(diags_log->m_fp);
+
+ if (!diags_log->roll()) {
+ return false;
+ }
+
+ if (std::unique_ptr<BaseLogFile> new_logfile{remake_logfile(diags_log)};
new_logfile) {
+ swap_diagslog(new_logfile.release());
+ }
+ return true;
+}
+
+bool
+Diags::roll_diags_on_interval(int interval)
+{
+ time_t now = time(nullptr);
+ if (interval == -1 || (now - diagslog_time_last_roll) < interval) {
+ return false;
+ }
+
+ fflush(diags_log->m_fp);
+
+ if (!diags_log->roll()) {
+ return false;
+ }
+
+ diagslog_time_last_roll = now;
+ if (std::unique_ptr<BaseLogFile> new_logfile{remake_logfile(diags_log)};
new_logfile) {
+ swap_diagslog(new_logfile.release());
+ }
+ return true;
+}
+
+bool
+Diags::roll_streams_on_filesize(int mb_threshold)
+{
+ // if we can't even check the file, we can forget about rotating
+ struct stat buf;
+ if (fstat(fileno(stdout_log->m_fp), &buf) != 0) {
+ throw std::runtime_error{"could not stat output log"};
+ }
+
+ off_t size = buf.st_size;
+ if (mb_threshold == -1 || size < static_cast<off_t>(mb_threshold) *
BYTES_IN_MB) {
+ return false;
+ }
+
+ // since usually stdout and stderr are the same file on disk, we should just
+ // play it safe and just flush both BaseLogFiles
+ if (stderr_log->is_init()) {
+ fflush(stderr_log->m_fp);
+ }
+ fflush(stdout_log->m_fp);
+
+ if (!stdout_log->roll()) {
+ return false;
+ }
+
+ char *oldname = ats_strdup(stdout_log->get_name());
+ log_log_trace("in %s(), oldname=%s\n", __func__, oldname);
+ set_std_output(StdStream::STDOUT, oldname);
+
+ // This assertion has to be true since log rolling for traffic.out is only
ever enabled
+ // (and useful) when traffic_server is NOT running in stand alone mode. If
traffic_server
+ // is NOT running in stand alone mode, then stderr and stdout SHOULD ALWAYS
be pointing
+ // to the same file (traffic.out).
+ //
+ // If for some reason, someone wants the feature to have stdout pointing to
some file on
+ // disk, and stderr pointing to a different file on disk, and then also
wants both files to
+ // rotate according to the (same || different) scheme, it would not be
difficult to add
+ // some more config options in records.yaml and said feature into this
function.
+ ink_assert(0 == strcmp(stdout_log->get_name(), stderr_log->get_name()));
+ log_log_trace("oldname == stderr_log->get_name()\n");
+ set_std_output(StdStream::STDERR, oldname);
Review Comment:
This relies on `ink_assert()` to enforce the stdout/stderr sameness
assumption, but `ink_assert()` is compiled out in non-debug builds
(include/tscore/ink_assert.h). In release builds this will always redirect
stderr to the stdout log even if they were configured differently, which is a
behavioral change vs the previous conditional update.
##########
src/tscore/Diags.cc:
##########
@@ -546,53 +541,16 @@ Diags::should_roll_diagslog()
if (diags_log && diags_log->is_init()) {
if (diagslog_rolling_enabled == RollingEnabledValues::ROLL_ON_SIZE ||
diagslog_rolling_enabled ==
RollingEnabledValues::ROLL_ON_TIME_OR_SIZE) {
- // if we can't even check the file, we can forget about rotating
- struct stat buf;
- if (fstat(fileno(diags_log->m_fp), &buf) != 0) {
+ try {
+ ret_val = roll_diags_on_filesize(diagslog_rolling_size);
+ } catch (std::runtime_error const &e) {
Review Comment:
The caught exception variable `e` is unused; this can trigger
`-Wunused-variable` warnings (often treated as errors). Catch by reference
without a name (or log `e.what()`).
##########
src/tscore/Diags.cc:
##########
@@ -810,3 +703,162 @@ Diags::rebind_std_stream(StdStream stream, int new_fd)
}
return false;
}
+
+bool
+Diags::roll_diags_on_filesize(int mb_threshold)
+{
+ // if we can't even check the file, we can forget about rotating
+ struct stat buf;
+ if (fstat(fileno(diags_log->m_fp), &buf) != 0) {
+ throw std::runtime_error{"could not stat diags log"};
+ }
+
+ off_t size = buf.st_size;
+ if (mb_threshold == -1 || size < static_cast<off_t>(mb_threshold) *
BYTES_IN_MB) {
+ return false;
+ }
+
+ fflush(diags_log->m_fp);
+
+ if (!diags_log->roll()) {
+ return false;
+ }
+
+ if (std::unique_ptr<BaseLogFile> new_logfile{remake_logfile(diags_log)};
new_logfile) {
+ swap_diagslog(new_logfile.release());
+ }
+ return true;
+}
+
+bool
+Diags::roll_diags_on_interval(int interval)
+{
+ time_t now = time(nullptr);
+ if (interval == -1 || (now - diagslog_time_last_roll) < interval) {
+ return false;
+ }
+
+ fflush(diags_log->m_fp);
+
+ if (!diags_log->roll()) {
+ return false;
+ }
+
+ diagslog_time_last_roll = now;
+ if (std::unique_ptr<BaseLogFile> new_logfile{remake_logfile(diags_log)};
new_logfile) {
+ swap_diagslog(new_logfile.release());
+ }
+ return true;
+}
+
+bool
+Diags::roll_streams_on_filesize(int mb_threshold)
+{
+ // if we can't even check the file, we can forget about rotating
+ struct stat buf;
+ if (fstat(fileno(stdout_log->m_fp), &buf) != 0) {
+ throw std::runtime_error{"could not stat output log"};
+ }
+
+ off_t size = buf.st_size;
+ if (mb_threshold == -1 || size < static_cast<off_t>(mb_threshold) *
BYTES_IN_MB) {
+ return false;
+ }
+
+ // since usually stdout and stderr are the same file on disk, we should just
+ // play it safe and just flush both BaseLogFiles
+ if (stderr_log->is_init()) {
+ fflush(stderr_log->m_fp);
+ }
+ fflush(stdout_log->m_fp);
+
+ if (!stdout_log->roll()) {
+ return false;
+ }
+
+ char *oldname = ats_strdup(stdout_log->get_name());
+ log_log_trace("in %s(), oldname=%s\n", __func__, oldname);
+ set_std_output(StdStream::STDOUT, oldname);
+
+ // This assertion has to be true since log rolling for traffic.out is only
ever enabled
+ // (and useful) when traffic_server is NOT running in stand alone mode. If
traffic_server
+ // is NOT running in stand alone mode, then stderr and stdout SHOULD ALWAYS
be pointing
+ // to the same file (traffic.out).
+ //
+ // If for some reason, someone wants the feature to have stdout pointing to
some file on
+ // disk, and stderr pointing to a different file on disk, and then also
wants both files to
+ // rotate according to the (same || different) scheme, it would not be
difficult to add
+ // some more config options in records.yaml and said feature into this
function.
+ ink_assert(0 == strcmp(stdout_log->get_name(), stderr_log->get_name()));
+ log_log_trace("oldname == stderr_log->get_name()\n");
+ set_std_output(StdStream::STDERR, oldname);
+
+ ats_free(oldname);
+ return true;
+}
+
+bool
+Diags::roll_streams_on_interval(int interval)
+{
+ time_t now = time(nullptr);
+ if (interval == -1 || (now - outputlog_time_last_roll) < interval) {
+ return false;
+ }
+
+ // since usually stdout and stderr are the same file on disk, we should just
+ // play it safe and just flush both BaseLogFiles
+ if (stderr_log->is_init()) {
+ fflush(stderr_log->m_fp);
+ }
+ fflush(stdout_log->m_fp);
+
+ if (!stdout_log->roll()) {
+ return false;
+ }
+
+ outputlog_time_last_roll = now;
+ char *oldname = ats_strdup(stdout_log->get_name());
+ log_log_trace("in %s, oldname=%s\n", __func__, oldname);
+ set_std_output(StdStream::STDOUT, oldname);
+
+ // This assertion has to be true since log rolling for traffic.out is only
ever enabled
+ // (and useful) when traffic_server is NOT running in stand alone mode. If
traffic_server
+ // is NOT running in stand alone mode, then stderr and stdout SHOULD ALWAYS
be pointing
+ // to the same file (traffic.out).
+ //
+ // If for some reason, someone wants the feature to have stdout pointing to
some file on
+ // disk, and stderr pointing to a different file on disk, and then also
wants both files to
+ // rotate according to the (same || different) scheme, it would not be
difficult to add
+ // some more config options in records.yaml and said feature into this
function.
+ ink_assert(0 == strcmp(stdout_log->get_name(), stderr_log->get_name()));
+ log_log_trace("oldname == stderr_log->get_name()\n");
+ set_std_output(StdStream::STDERR, oldname);
Review Comment:
Same issue as the size-based path: `ink_assert()` is compiled out in release
builds, but the code unconditionally redirects stderr. Preserve the previous
conditional update to avoid changing behavior when stdout/stderr differ.
##########
src/tscore/Diags.cc:
##########
@@ -48,6 +48,8 @@
#include "tscore/Regression.h"
#include "tscore/Diags.h"
#include "ts/ts.h"
+
+#include <memory>
#include <string_view>
Review Comment:
`std::runtime_error` is used in this translation unit, but `<stdexcept>`
isn't included. Relying on transitive includes can break builds when headers
change; add the proper standard header explicitly.
##########
src/tscore/Diags.cc:
##########
@@ -634,83 +591,19 @@ Diags::should_roll_outputlog()
if (stdout_log->is_init()) {
if (outputlog_rolling_enabled == RollingEnabledValues::ROLL_ON_SIZE ||
outputlog_rolling_enabled ==
RollingEnabledValues::ROLL_ON_TIME_OR_SIZE) {
- // if we can't even check the file, we can forget about rotating
- struct stat buf;
- if (fstat(fileno(stdout_log->m_fp), &buf) != 0) {
+ try {
+ ret_val = roll_streams_on_filesize(outputlog_rolling_size);
+ } catch (std::runtime_error &e) {
Review Comment:
The caught exception variable `e` is unused and the catch is non-const.
Catch by `const` reference without a name (or log `e.what()`) to avoid warnings
and allow catching temporaries.
--
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]
