JosiahWI commented on code in PR #13254:
URL: https://github.com/apache/trafficserver/pull/13254#discussion_r3390131556
##########
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) {
return false;
}
-
- off_t size = buf.st_size;
- if (outputlog_rolling_size != -1 && size >=
static_cast<off_t>(outputlog_rolling_size) * BYTES_IN_MB) {
- // 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()) {
- char *oldname = ats_strdup(stdout_log->get_name());
- log_log_trace("in %s(), oldname=%s\n", __func__, oldname);
- set_std_output(StdStream::STDOUT, oldname);
-
- // if stderr and stdout are redirected to the same place, we should
- // update the stderr_log object as well
- if (!strcmp(oldname, stderr_log->get_name())) {
- log_log_trace("oldname == stderr_log->get_name()\n");
- set_std_output(StdStream::STDERR, oldname);
- need_consider_stderr = false;
- }
- ats_free(oldname);
- ret_val = true;
- }
- }
}
if (outputlog_rolling_enabled == RollingEnabledValues::ROLL_ON_TIME ||
outputlog_rolling_enabled ==
RollingEnabledValues::ROLL_ON_TIME_OR_SIZE) {
- time_t now = time(nullptr);
- if (outputlog_rolling_interval != -1 && (now - outputlog_time_last_roll)
>= outputlog_rolling_interval) {
- // 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()) {
- 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);
-
- // if stderr and stdout are redirected to the same place, we should
- // update the stderr_log object as well
- if (!strcmp(oldname, stderr_log->get_name())) {
- log_log_trace("oldname == stderr_log->get_name()\n");
- set_std_output(StdStream::STDERR, oldname);
- need_consider_stderr = false;
- }
- ats_free(oldname);
- ret_val = true;
- }
- }
+ ret_val = roll_streams_on_interval(outputlog_rolling_interval);
}
}
- // 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.
- if (ret_val) {
- ink_assert(!need_consider_stderr);
Review Comment:
Instead of asserting that this flag was set, I am asserting at an earlier
point that the condition for setting the flag is met. The code was originally
designed to be a little easier to change if more output stream flexibility was
needed. Since that obviously hasn't happened for a long time, and was only a
small amount of code, I have removed the extra logic flow and this flag
entirely, simplifying the code.
--
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]
