trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jpe...@apache.org
Subject [12/17] git commit: TS-2302: convert Log::error_log into a generic prefedined LogObject
Date Thu, 31 Oct 2013 15:18:31 GMT
TS-2302: convert Log::error_log into a generic prefedined LogObject

Initialize LogObject pointer arrays to NULL before using them. Use
const accessors to get to LogObject::m_name. Sprinkle more const
through the logging API.

Use the standard ConfigProcessor pattern to reload LogConfig. This
removes part of the need to keep a global array of inactive LogObjects.
Using newDeleter() in LogObjectManager::unmanage_api_object(),
removes the rest of this need. Now the whole Log::inactive_object
array can be removed.

Using the ConfigProcessor reload pattern also fixes a memory leak,
since previously the old LogConfig object was never freed.

Fix LogObjectManager::transfer_objects(). Previously, matching
objects would be assigned to the new manager, but also left on the
old manager, leaving it unclear who should have ownership of which
log object.

Move a bunch of inline LogObjectManager methods to the .cc file
where they are mre easily accessible. None of these are called often
enough to justify being inline.

The global Log::error_log is now managed my the log object manager
just like other predefined log objects. This lets us remove code
in a number of places that were treating is specially. It also fixes
a log collation bug in LogConfig::init(). This function attempts
to figure out which log space threshold to use. If all the logs are
being collated, then it used the orphan space value. The problem
is that the error log was never checked (and it is never collated),
so enabling log collation and making a poor choice of space thresholds
causes error logging to stop. Like the rest of the bugs here, the
fix is to stop treating the error log specially, and bring it into
the log object manager.


Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/538eba5c
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/538eba5c
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/538eba5c

Branch: refs/heads/master
Commit: 538eba5c078f6b14cbd3b6e6887da669fd7205c9
Parents: 3020176
Author: James Peach <jpeach@apache.org>
Authored: Wed Oct 2 11:12:00 2013 -0700
Committer: James Peach <jpeach@apache.org>
Committed: Thu Oct 31 08:16:28 2013 -0700

----------------------------------------------------------------------
 proxy/logging/Log.cc           | 111 +++++++---------
 proxy/logging/Log.h            |   9 +-
 proxy/logging/LogBuffer.cc     |   2 +-
 proxy/logging/LogBuffer.h      |   2 +-
 proxy/logging/LogConfig.cc     |  69 +++++-----
 proxy/logging/LogConfig.h      |   4 +-
 proxy/logging/LogFile.cc       |   3 +-
 proxy/logging/LogFile.h        |   8 +-
 proxy/logging/LogHost.cc       |   2 +-
 proxy/logging/LogHost.h        |   2 +-
 proxy/logging/LogObject.cc     | 245 ++++++++++++++++++++++++++++--------
 proxy/logging/LogObject.h      |  82 ++----------
 proxy/logging/LogPredefined.cc |  12 +-
 proxy/logging/LogPredefined.h  |   4 +
 14 files changed, 311 insertions(+), 244 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/Log.cc
----------------------------------------------------------------------
diff --git a/proxy/logging/Log.cc b/proxy/logging/Log.cc
index a43b0a0..9e642f6 100644
--- a/proxy/logging/Log.cc
+++ b/proxy/logging/Log.cc
@@ -62,18 +62,12 @@
 #define PERIODIC_TASKS_INTERVAL 5 // TODO: Maybe this should be done as a config option
 
 // Log global objects
-inkcoreapi TextLogObject *Log::error_log = NULL;
-LogConfig *Log::config = NULL;
+inkcoreapi LogObject *Log::error_log = NULL;
 LogFieldList Log::global_field_list;
 LogFormat *Log::global_scrap_format = NULL;
 LogObject *Log::global_scrap_object = NULL;
 Log::LoggingMode Log::logging_mode = LOG_MODE_NONE;
 
-// Inactive objects
-LogObject **Log::inactive_objects;
-size_t Log::numInactiveObjects;
-size_t Log::maxInactiveObjects;
-
 // Flush thread stuff
 EventNotify *Log::preproc_notify;
 EventNotify *Log::flush_notify;
@@ -102,54 +96,52 @@ RecRawStatBlock *log_rsb;
   This routine is invoked when the current LogConfig object says it needs
   to be changed (as the result of a manager callback).
   -------------------------------------------------------------------------*/
+
+LogConfig *Log::config = NULL;
+static unsigned log_configid = 0;
+
 void
 Log::change_configuration()
 {
+  LogConfig * prev = Log::config;
+  LogConfig * new_config = NULL;
+
   Debug("log-config", "Changing configuration ...");
 
-  LogConfig *new_config = NEW(new LogConfig);
+  new_config = NEW(new LogConfig);
   ink_assert(new_config != NULL);
   new_config->read_configuration_variables();
 
   // grab the _APImutex so we can transfer the api objects to
   // the new config
   //
-  ink_mutex_acquire(Log::config->log_object_manager._APImutex);
+  ink_mutex_acquire(prev->log_object_manager._APImutex);
   Debug("log-api-mutex", "Log::change_configuration acquired api mutex");
 
   new_config->init(Log::config);
 
-  // Swap in the new config object
-  //
+  // Make the new LogConfig active.
   ink_atomic_swap(&Log::config, new_config);
 
-  // Force new buffers for inactive objects
-  //
-  for (size_t i = 0; i < numInactiveObjects; i++) {
-    inactive_objects[i]->force_new_buffer();
-  }
+  // XXX There is a race condition with API objects. If TSTextLogObjectCreate()
+  // is called before the Log::config swap, then it will be blocked on the lock
+  // on the *old* LogConfig and register it's LogObject with that manager. If
+  // this happens, then the new TextLogObject will be immediately lost. Traffic
+  // Server would crash the next time the plugin referenced the freed object.
 
-  ink_mutex_release(Log::config->log_object_manager._APImutex);
+  ink_mutex_release(prev->log_object_manager._APImutex);
   Debug("log-api-mutex", "Log::change_configuration released api mutex");
 
-  Debug("log-config", "... new configuration in place");
-}
+  // Register the new config in the config processor; the old one will now be scheduled for
a
+  // future deletion. We don't need to do anything magical with refcounts, since the
+  // configProcessor will keep a reference count, and drop it when the deletion is scheduled.
+  configProcessor.set(log_configid, new_config);
 
-void
-Log::add_to_inactive(LogObject * object)
-{
-  if (Log::numInactiveObjects == Log::maxInactiveObjects) {
-    Log::maxInactiveObjects += LOG_OBJECT_ARRAY_DELTA;
-    LogObject **new_objects = new LogObject *[Log::maxInactiveObjects];
-
-    for (size_t i = 0; i < Log::numInactiveObjects; i++) {
-      new_objects[i] = Log::inactive_objects[i];
-    }
-    delete[]Log::inactive_objects;
-    Log::inactive_objects = new_objects;
-  }
+  // If we replaced the logging configuration, flush any log
+  // objects that weren't transferred to the new config ...
+  prev->log_object_manager.flush_all_objects();
 
-  Log::inactive_objects[Log::numInactiveObjects++] = object;
+  Debug("log-config", "... new configuration in place");
 }
 
 /*-------------------------------------------------------------------------
@@ -211,25 +203,7 @@ struct PeriodicWakeup : Continuation
 void
 Log::periodic_tasks(long time_now)
 {
-  // delete inactive objects
-  //
-  // we don't care if we miss an object that may be added to the set of
-  // inactive objects just after we have read numInactiveObjects and found
-  // it to be zero; we will get a chance to delete it next time
-  //
-
   Debug("log-api-mutex", "entering Log::periodic_tasks");
-  if (numInactiveObjects) {
-    ink_mutex_acquire(Log::config->log_object_manager._APImutex);
-    Debug("log-api-mutex", "Log::periodic_tasks acquired api mutex");
-    Debug("log-periodic", "Deleting inactive_objects");
-    for (size_t i = 0; i < numInactiveObjects; i++) {
-      delete inactive_objects[i];
-    }
-    numInactiveObjects = 0;
-    ink_mutex_release(Log::config->log_object_manager._APImutex);
-    Debug("log-api-mutex", "Log::periodic_tasks released api mutex");
-  }
 
   if (logging_mode_changed || Log::config->reconfiguration_needed) {
     Debug("log-config", "Performing reconfiguration, init status = %d", init_status);
@@ -258,12 +232,11 @@ Log::periodic_tasks(long time_now)
     if (config->space_is_short() || time_now % config->space_used_frequency == 0) {
       Log::config->update_space_used();
     }
+
     // See if there are any buffers that have expired
     //
     Log::config->log_object_manager.check_buffer_expiration(time_now);
-    if (error_log) {
-      error_log->check_buffer_expiration(time_now);
-    }
+
     // Check if we received a request to roll, and roll if so, otherwise
     // give objects a chance to roll if they need to
     //
@@ -910,10 +883,6 @@ Log::handle_logging_mode_change(const char */* name ATS_UNUSED */, RecDataT
/* d
 void
 Log::init(int flags)
 {
-  maxInactiveObjects = LOG_OBJECT_ARRAY_DELTA;
-  numInactiveObjects = 0;
-  inactive_objects = new LogObject*[maxInactiveObjects];
-
   collation_preproc_threads = 1;
   collation_accept_file_descriptor = NO_FD;
 
@@ -922,10 +891,11 @@ Log::init(int flags)
   config_flags = flags;
 
   // create the configuration object
-  //
-  config = NEW (new LogConfig);
+  config = NEW(new LogConfig());
   ink_assert (config != NULL);
 
+  log_configid = configProcessor.set(log_configid, config);
+
   // set the logging_mode and read config variables if needed
   //
   if (config_flags & LOGCAT) {
@@ -1215,7 +1185,6 @@ Log::va_error(const char *format, va_list ap)
 void *
 Log::preproc_thread_main(void *args)
 {
-  size_t buffers_preproced;
   int idx = *(int *)args;
 
   Debug("log-preproc", "log preproc thread is alive ...");
@@ -1223,16 +1192,20 @@ Log::preproc_thread_main(void *args)
   Log::preproc_notify[idx].lock();
 
   while (true) {
-    buffers_preproced = config->log_object_manager.preproc_buffers(idx);
+    size_t buffers_preproced;
+    LogConfig * current = (LogConfig *)configProcessor.get(log_configid);
 
-    if (error_log)
-      buffers_preproced += error_log->preproc_buffers(idx);
+    if (current) {
+      buffers_preproced = current->log_object_manager.preproc_buffers(idx);
+    }
 
     // config->increment_space_used(bytes_to_disk);
     // TODO: the bytes_to_disk should be set to Log
 
-    Debug("log-preproc","%zu buffers preprocessed this round",
-          buffers_preproced);
+    Debug("log-preproc","%zu buffers preprocessed from LogConfig %p (refcount=%d) this round",
+          buffers_preproced, current, current->m_refcount);
+
+    configProcessor.release(log_configid, current);
 
     // wait for more work; a spurious wake-up is ok since we'll just
     // check the queue and find there is nothing to do, then wait
@@ -1299,7 +1272,7 @@ Log::flush_thread_main(void * /* args ATS_UNUSED */)
       logfile->check_fd();
       if (!logfile->is_open()) {
         Warning("File:%s was closed, have dropped (%d) bytes.",
-                logfile->m_name, total_bytes);
+                logfile->get_name(), total_bytes);
 
         RecIncrRawStat(log_rsb, mutex->thread_holding,
                        log_stat_bytes_lost_before_written_to_disk_stat,
@@ -1313,7 +1286,7 @@ Log::flush_thread_main(void * /* args ATS_UNUSED */)
       while (total_bytes - bytes_written) {
         if (Log::config->logging_space_exhausted) {
           Debug("log", "logging space exhausted, failed to write file:%s, have dropped (%d)
bytes.",
-                  logfile->m_name, (total_bytes - bytes_written));
+                  logfile->get_name(), (total_bytes - bytes_written));
 
           RecIncrRawStat(log_rsb, mutex->thread_holding,
                          log_stat_bytes_lost_before_written_to_disk_stat,
@@ -1325,7 +1298,7 @@ Log::flush_thread_main(void * /* args ATS_UNUSED */)
                       total_bytes - bytes_written);
         if (len < 0) {
           Error("Failed to write log to %s: [tried %d, wrote %d, %s]",
-                logfile->m_name, total_bytes - bytes_written,
+                logfile->get_name(), total_bytes - bytes_written,
                 bytes_written, strerror(errno));
 
           RecIncrRawStat(log_rsb, mutex->thread_holding,

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/Log.h
----------------------------------------------------------------------
diff --git a/proxy/logging/Log.h b/proxy/logging/Log.h
index 883567f..a646275 100644
--- a/proxy/logging/Log.h
+++ b/proxy/logging/Log.h
@@ -404,7 +404,7 @@ public:
   inkcoreapi static int error(const char *format, ...) TS_PRINTFLIKE(1, 2);
 
   // public data members
-  inkcoreapi static TextLogObject *error_log;
+  inkcoreapi static LogObject *error_log;
   static LogConfig *config;
   static LogFieldList global_field_list;
 //    static LogBufferList global_buffer_full_list; vl: not used
@@ -414,12 +414,6 @@ public:
   static LogObject *global_scrap_object;
   static LoggingMode logging_mode;
 
-  // keep track of inactive objects
-  static LogObject **inactive_objects;
-  static size_t numInactiveObjects;
-  static size_t maxInactiveObjects;
-  static void add_to_inactive(LogObject * obj);
-
   // logging thread stuff
   static EventNotify *preproc_notify;
   static void *preproc_thread_main(void *args);
@@ -442,6 +436,7 @@ public:
 
   Log();                        // shut up stupid DEC C++ compiler
 
+  friend void RegressionTest_LogObjectManager_Transfer(RegressionTest *, int, int *);
 private:
 
   static void periodic_tasks(long time_now);

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/LogBuffer.cc
----------------------------------------------------------------------
diff --git a/proxy/logging/LogBuffer.cc b/proxy/logging/LogBuffer.cc
index d110bd2..e16963f 100644
--- a/proxy/logging/LogBuffer.cc
+++ b/proxy/logging/LogBuffer.cc
@@ -330,7 +330,7 @@ LogBuffer::LB_ResultCode LogBuffer::checkin_write(size_t write_offset)
 
 
 unsigned
-LogBuffer::add_header_str(char *str, char *buf_ptr, unsigned buf_len)
+LogBuffer::add_header_str(const char *str, char *buf_ptr, unsigned buf_len)
 {
   unsigned len = 0;
   // This was ambiguous - should it be the real strlen or the 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/LogBuffer.h
----------------------------------------------------------------------
diff --git a/proxy/logging/LogBuffer.h b/proxy/logging/LogBuffer.h
index 08ff92e..e12fa25 100644
--- a/proxy/logging/LogBuffer.h
+++ b/proxy/logging/LogBuffer.h
@@ -224,7 +224,7 @@ private:
 
   // private functions
   size_t _add_buffer_header();
-  unsigned add_header_str(char *str, char *buf_ptr, unsigned buf_len);
+  unsigned add_header_str(const char *str, char *buf_ptr, unsigned buf_len);
 
   // -- member functions that are not allowed --
   LogBuffer();

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/LogConfig.cc
----------------------------------------------------------------------
diff --git a/proxy/logging/LogConfig.cc b/proxy/logging/LogConfig.cc
index 4ba398c..54080f0 100644
--- a/proxy/logging/LogConfig.cc
+++ b/proxy/logging/LogConfig.cc
@@ -651,55 +651,47 @@ LogConfig::setup_collation(LogConfig * prev_config)
 void
 LogConfig::init(LogConfig * prev_config)
 {
+  LogObject * errlog = NULL;
+
   ink_assert(!initialized);
 
   setup_collation(prev_config);
 
   update_space_used();
 
-  // setup the error log before the rest of the log objects since
-  // we don't do filename conflict checking for it
-  //
-  TextLogObject *old_elog = Log::error_log;
-  TextLogObject *new_elog = 0;
-
-  // swap new error log for old error log unless
-  // -there was no error log and we don't want one
-  // -there was an error log, and the new one is identical
-  //  (the logging directory did not change)
-  //
-  if (!((!old_elog && !Log::error_logging_enabled()) ||
-        (old_elog && Log::error_logging_enabled() &&
-         (prev_config ? !strcmp(prev_config->logfile_dir, logfile_dir) : 0)))) {
-    if (Log::error_logging_enabled()) {
-      new_elog = NEW(new TextLogObject("error.log", logfile_dir, true, NULL,
-                                       rolling_enabled, collation_preproc_threads,
-                                       rolling_interval_sec, rolling_offset_hr,
-                                       rolling_size_mb));
-      if (new_elog->do_filesystem_checks() < 0) {
-        const char *msg = "The log file %s did not pass filesystem checks. " "No output will
be produced for this log";
-        Error(msg, new_elog->get_full_filename());
-        LogUtils::manager_alarm(LogUtils::LOG_ALARM_ERROR, msg, new_elog->get_full_filename());
-        delete new_elog;
-        new_elog = 0;
-      }
-    }
-    ink_atomic_swap(&Log::error_log, new_elog);
-    if (old_elog) {
-      old_elog->force_new_buffer();
-      Log::add_to_inactive(old_elog);
-    }
-  }
   // create log objects
   //
   if (Log::transaction_logging_enabled()) {
     setup_log_objects();
   }
-  // transfer objects from previous configuration
-  //
+
+  // ----------------------------------------------------------------------
+  // Construct a new error log object candidate.
+  if (Log::error_logging_enabled()) {
+    PreDefinedFormatInfo * info;
+
+    Debug("log", "creating predefined error log object");
+    info = MakePredefinedErrorLog(this);
+    errlog = this->create_predefined_object(info, 0, NULL);
+    errlog->set_fmt_timestamps();
+    delete info;
+  } else {
+    Log::error_log = NULL;
+  }
+
   if (prev_config) {
+    // Transfer objects from previous configuration.
     transfer_objects(prev_config);
+
+    // After transferring objects, we are going to keep either the new error log or the old
one. Figure out
+    // which one we are keeping and make that the global ...
+    if (Log::error_log) {
+      errlog = this->log_object_manager.find_by_format_name(Log::error_log->m_format->name());
+    }
   }
+
+  ink_atomic_swap(&Log::error_log, errlog);
+
   // determine if we should use the orphan log space value or not
   // we use it if all objects are collation clients, or if some are and
   // the specified space for collation is larger than that for local files
@@ -867,8 +859,11 @@ LogConfig::create_predefined_object(const PreDefinedFormatInfo * pdi,
size_t num
   }
 
   // give object to object manager
-  //
-  log_object_manager.manage_object(obj);
+  if (log_object_manager.manage_object(obj) != LogObjectManager::NO_FILENAME_CONFLICTS) {
+    delete obj;
+    return NULL;
+  }
+
   return obj;
 }
 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/LogConfig.h
----------------------------------------------------------------------
diff --git a/proxy/logging/LogConfig.h b/proxy/logging/LogConfig.h
index 8f7a714..78fe25f 100644
--- a/proxy/logging/LogConfig.h
+++ b/proxy/logging/LogConfig.h
@@ -27,8 +27,8 @@
 #define LOG_CONFIG_H
 
 #include "libts.h"
-
 #include "P_RecProcess.h"
+#include "ProxyConfig.h"
 
 /* Instead of enumerating the stats in DynamicStats.h, each module needs
    to enumerate its stats separately and register them with librecords
@@ -105,7 +105,7 @@ struct PreDefinedFormatInfo;
         for this new variable if it is exposed in the GUI
   -------------------------------------------------------------------------*/
 
-class LogConfig
+class LogConfig : public ConfigInfo
 {
 
 public:

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/LogFile.cc
----------------------------------------------------------------------
diff --git a/proxy/logging/LogFile.cc b/proxy/logging/LogFile.cc
index 501cf29..a276794 100644
--- a/proxy/logging/LogFile.cc
+++ b/proxy/logging/LogFile.cc
@@ -162,7 +162,7 @@ bool LogFile::exists(const char *pathname)
   -------------------------------------------------------------------------*/
 
 void
-LogFile::change_name(char *new_name)
+LogFile::change_name(const char *new_name)
 {
   ats_free(m_name);
   m_name = ats_strdup(new_name);
@@ -794,6 +794,7 @@ LogFile::check_fd()
   stat_check_count++;
 
   int err = open_file();
+  // XXX if open_file() returns, LOG_FILE_FILESYSTEM_CHECKS_FAILED, raise a more informative
alarm ...
   if (err != LOG_FILE_NO_ERROR && err != LOG_FILE_NO_PIPE_READERS) {
     if (!failure_last_call) {
       LogUtils::manager_alarm(LogUtils::LOG_ALARM_ERROR, "Traffic Server could not open logfile
%s.", m_name);

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/LogFile.h
----------------------------------------------------------------------
diff --git a/proxy/logging/LogFile.h b/proxy/logging/LogFile.h
index fa33f6d..1995ee7 100644
--- a/proxy/logging/LogFile.h
+++ b/proxy/logging/LogFile.h
@@ -75,7 +75,7 @@ private:
   void _build_name(const char *filename);
 
 public:
- MetaInfo(char *filename)
+ MetaInfo(const char *filename)
    : _flags(0)
   {
     _build_name(filename);
@@ -144,10 +144,10 @@ public:
 
   int roll(long interval_start, long interval_end);
 
-  char *get_name() const { return m_name; }
+  const char *get_name() const { return m_name; }
 
   void change_header(const char *header);
-  void change_name(char *new_name);
+  void change_name(const char *new_name);
 
   LogFileFormat get_format() const { return m_file_format; }
   const char *get_format_name() const {
@@ -175,7 +175,9 @@ public:
 
 public:
   LogFileFormat m_file_format;
+private:
   char *m_name;
+public:
   char *m_header;
   uint64_t m_signature;           // signature of log object stored
   MetaInfo *m_meta_info;

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/LogHost.cc
----------------------------------------------------------------------
diff --git a/proxy/logging/LogHost.cc b/proxy/logging/LogHost.cc
index a448340..c764f77 100644
--- a/proxy/logging/LogHost.cc
+++ b/proxy/logging/LogHost.cc
@@ -51,7 +51,7 @@
   LogHost
   -------------------------------------------------------------------------*/
 
-LogHost::LogHost(char *object_filename, uint64_t object_signature)
+LogHost::LogHost(const char *object_filename, uint64_t object_signature)
   : m_object_filename(ats_strdup(object_filename)),
     m_object_signature(object_signature),
     m_port(0),

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/LogHost.h
----------------------------------------------------------------------
diff --git a/proxy/logging/LogHost.h b/proxy/logging/LogHost.h
index 5aa32e5..cc7cc6d 100644
--- a/proxy/logging/LogHost.h
+++ b/proxy/logging/LogHost.h
@@ -39,7 +39,7 @@ class LogHost
   friend class LogCollationClientSM;
 
 public:
-  LogHost(char *object_filename, uint64_t object_signature);
+  LogHost(const char *object_filename, uint64_t object_signature);
   LogHost(const LogHost &);
   ~LogHost();
 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/LogObject.cc
----------------------------------------------------------------------
diff --git a/proxy/logging/LogObject.cc b/proxy/logging/LogObject.cc
index eb773b9..5109469 100644
--- a/proxy/logging/LogObject.cc
+++ b/proxy/logging/LogObject.cc
@@ -37,7 +37,7 @@
 #include "LogConfig.h"
 #include "LogAccess.h"
 #include "Log.h"
-#include "LogObject.h"
+#include "ts/TestBox.h"
 
 size_t
 LogBufferManager::preproc_buffers(LogBufferSink *sink) {
@@ -75,7 +75,7 @@ LogBufferManager::preproc_buffers(LogBufferSink *sink) {
   LogObject
   -------------------------------------------------------------------------*/
 
-LogObject::LogObject(LogFormat *format, const char *log_dir,
+LogObject::LogObject(const LogFormat *format, const char *log_dir,
                      const char *basename, LogFileFormat file_format,
                      const char *header, int rolling_enabled,
                      int flush_threads, int rolling_interval_sec,
@@ -551,7 +551,7 @@ LogObject::log(LogAccess * lad, const char *text_entry)
   // likewise, send data to a remote client even if local space is exhausted
   // (if there is a remote client, m_logFile will be NULL
   if (Log::config->logging_space_exhausted && !writes_to_pipe() && m_logFile)
{
-    Debug("log", "logging space exhausted, can't write to:%s, drop this entry", m_logFile->m_name);
+    Debug("log", "logging space exhausted, can't write to:%s, drop this entry", m_logFile->get_name());
     return Log::FULL;
   }
   // this verification must be done here in order to avoid 'dead' LogBuffers
@@ -884,6 +884,33 @@ TextLogObject::va_write(const char *format, va_list ap)
 /*-------------------------------------------------------------------------
   LogObjectManager
   -------------------------------------------------------------------------*/
+
+LogObjectManager::LogObjectManager()
+   : _numObjects(0), _maxObjects(LOG_OBJECT_ARRAY_DELTA), _numAPIobjects(0), _maxAPIobjects(LOG_OBJECT_ARRAY_DELTA)
+{
+  _objects = new LogObject *[_maxObjects];
+  _APIobjects = new LogObject *[_maxAPIobjects];
+  _APImutex = NEW(new ink_mutex);
+  ink_mutex_init(_APImutex, "_APImutex");
+
+  memset(_objects, 0, sizeof(LogObject *) * _maxObjects);
+  memset(_APIobjects, 0, sizeof(LogObject *) * _maxAPIobjects);
+}
+
+LogObjectManager::~LogObjectManager()
+{
+  for (unsigned i = 0; i < _maxObjects; i++) {
+    delete _objects[i];
+  }
+
+  for (unsigned i = 0; i < _maxAPIobjects; i++) {
+    delete _APIobjects[i];
+  }
+
+  delete[] _objects;
+  delete[] _APIobjects;
+  delete _APImutex;
+}
 void
 LogObjectManager::_add_object(LogObject * object)
 {
@@ -891,9 +918,8 @@ LogObjectManager::_add_object(LogObject * object)
     _maxObjects += LOG_OBJECT_ARRAY_DELTA;
     LogObject **_new_objects = new LogObject *[_maxObjects];
 
-    for (size_t i = 0; i < _numObjects; i++) {
-      _new_objects[i] = _objects[i];
-    }
+    memset(_new_objects, 0, sizeof(LogObject *) * _maxObjects);
+    memcpy(_new_objects, _objects, sizeof(LogObject *) * _numObjects);
     delete[]_objects;
     _objects = _new_objects;
   }
@@ -911,9 +937,8 @@ LogObjectManager::_add_api_object(LogObject * object)
     _maxAPIobjects += LOG_OBJECT_ARRAY_DELTA;
     LogObject **_new_objects = new LogObject *[_maxAPIobjects];
 
-    for (size_t i = 0; i < _numAPIobjects; i++) {
-      _new_objects[i] = _APIobjects[i];
-    }
+    memset(_new_objects, 0, sizeof(LogObject *) * _maxAPIobjects);
+    memcpy(_new_objects, _APIobjects, sizeof(LogObject *) * _numAPIobjects);
     delete[]_APIobjects;
     _APIobjects = _new_objects;
   }
@@ -988,12 +1013,12 @@ LogObjectManager::_solve_filename_conflicts(LogObject * log_object,
int maxConfl
 {
   int retVal = NO_FILENAME_CONFLICTS;
 
-  char *filename = log_object->get_full_filename();
+  const char *filename = log_object->get_full_filename();
 
   if (access(filename, F_OK)) {
     if (errno != ENOENT) {
       const char *msg = "Cannot access log file %s: %s";
-      char *se = strerror(errno);
+      const char *se = strerror(errno);
 
       Error(msg, filename, se);
       LogUtils::manager_alarm(LogUtils::LOG_ALARM_ERROR, msg, filename, se);
@@ -1086,7 +1111,7 @@ LogObjectManager::_solve_filename_conflicts(LogObject * log_object,
int maxConfl
 
 
 bool
-LogObjectManager::_has_internal_filename_conflict(char *filename, LogObject ** objects, int
numObjects)
+LogObjectManager::_has_internal_filename_conflict(const char *filename, LogObject ** objects,
int numObjects)
 {
   for (int i = 0; i < numObjects; i++) {
     LogObject *obj = objects[i];
@@ -1109,7 +1134,7 @@ int
 LogObjectManager::_solve_internal_filename_conflicts(LogObject *log_object, int maxConflicts,
int fileNum)
 {
   int retVal = NO_FILENAME_CONFLICTS;
-  char *filename = log_object->get_full_filename();
+  const char *filename = log_object->get_full_filename();
 
   if (_has_internal_filename_conflict(filename, _objects, _numObjects) ||
       _has_internal_filename_conflict(filename, _APIobjects, _numAPIobjects)) {
@@ -1152,25 +1177,34 @@ LogObjectManager::check_buffer_expiration(long time_now)
   size_t i;
 
   for (i = 0; i < _numObjects; i++) {
-    _objects[i]->check_buffer_expiration(time_now);
+    if (_objects[i]) {
+      _objects[i]->check_buffer_expiration(time_now);
+    }
   }
   for (i = 0; i < _numAPIobjects; i++) {
-    _APIobjects[i]->check_buffer_expiration(time_now);
+    if (_APIobjects[i]) {
+      _APIobjects[i]->check_buffer_expiration(time_now);
+    }
   }
 }
 
-size_t LogObjectManager::preproc_buffers(int idx)
+size_t
+LogObjectManager::preproc_buffers(int idx)
 {
-  size_t i;
   size_t buffers_preproced = 0;
 
-  for (i = 0; i < _numObjects; i++) {
-    buffers_preproced += _objects[i]->preproc_buffers(idx);
+  for (unsigned i = 0; i < _numObjects; i++) {
+    if (_objects[i]) {
+      buffers_preproced += _objects[i]->preproc_buffers(idx);
+    }
   }
 
-  for (i = 0; i < _numAPIobjects; i++) {
+  for (unsigned i = 0; i < _numAPIobjects; i++) {
+    if (_APIobjects[i]) {
       buffers_preproced += _APIobjects[i]->preproc_buffers(idx);
+    }
   }
+
   return buffers_preproced;
 }
 
@@ -1183,8 +1217,9 @@ LogObjectManager::unmanage_api_object(LogObject * logObject)
   for (size_t i = 0; i < _numAPIobjects; i++) {
     if (logObject == _APIobjects[i]) {
 
-      Log::add_to_inactive(logObject);
+      // Force a buffer flush, then schedule this LogObject to be deleted on the eventProcessor.
       logObject->force_new_buffer();
+      new_Deleter(logObject, HRTIME_SECONDS(60));
 
       for (size_t j = i + 1; j < _numAPIobjects; j++) {
         _APIobjects[j - 1] = _APIobjects[j];
@@ -1227,65 +1262,63 @@ LogObjectManager::open_local_pipes()
 void
 LogObjectManager::transfer_objects(LogObjectManager & old_mgr)
 {
-  LogObject *old_obj, *obj;
-  size_t i;
-  size_t num_kept_objects = 0;
+  unsigned num_kept_objects = 0;
 
   if (is_debug_tag_set("log-config-transfer")) {
     Debug("log-config-transfer", "TRANSFER OBJECTS: list of old objects");
-    for (i = 0; i < old_mgr._numObjects; i++) {
+    for (unsigned i = 0; i < old_mgr._numObjects; i++) {
       Debug("log-config-transfer", "%s", old_mgr._objects[i]->get_original_filename());
     }
 
     Debug("log-config-transfer", "TRANSFER OBJECTS : list of new objects");
-    for (i = 0; i < _numObjects; i++) {
+    for (unsigned i = 0; i < _numObjects; i++) {
       Debug("log-config-transfer", "%s", _objects[i]->get_original_filename());
     }
   }
 
-  for (i = 0; i < old_mgr._numAPIobjects; i++) {
+  // Transfer the API objects to the new manager.
+  for (unsigned i = 0; i < old_mgr._numAPIobjects; i++) {
     _add_api_object(old_mgr._APIobjects[i]);
   }
 
-  LogObject **old_objects = old_mgr._objects;
+  // And nuke them from the old manager ...
+  memset(old_mgr._APIobjects, 0, sizeof(LogObject *) * old_mgr._numAPIobjects);
+  old_mgr._numAPIobjects = 0;
 
-  for (i = 0; i < old_mgr._numObjects; i++) {
-    old_obj = old_objects[i];
+  for (unsigned i = 0; i < old_mgr._numObjects; ++i) {
+    LogObject * old_obj = old_mgr._objects[i];
+    LogObject * new_obj;
 
     Debug("log-config-transfer", "examining existing object %s", old_obj->get_base_filename());
 
-    // see if any of the new objects is just a copy of an old one,
-    // if so, keep the old one and delete the new one
-    //
-    size_t j = _numObjects;
-
+    // See if any of the new objects is just a copy of an old one. If so, transfer the
+    // old one to the new manager and delete the new one.
     if (num_kept_objects < _numObjects) {
-      for (j = 0; j < _numObjects; j++) {
-        obj = _objects[j];
+      for (unsigned j = 0; j < _numObjects; j++) {
+        new_obj = _objects[j];
 
         Debug("log-config-transfer",
-              "comparing existing object %s to new object %s", old_obj->get_base_filename(),
obj->get_base_filename());
+              "comparing existing object %s to new object %s", old_obj->get_base_filename(),
new_obj->get_base_filename());
 
-        if (*obj == *old_obj) {
+        if (*new_obj == *old_obj) {
           Debug("log-config-transfer", "keeping existing object %s", old_obj->get_base_filename());
 
-          _objects[j] = old_obj;
-          delete obj;
+          this->_objects[j] = old_obj;
+          old_mgr._objects[i] = NULL;
+
+          delete new_obj;
           ++num_kept_objects;
           break;
         }
       }
     }
-    // if old object is not in the new list, move it to list of
-    // inactive objects
-    //
-    if (j == _numObjects) {
-      Debug("log-config-transfer", "moving existing object %s to inactive list", old_obj->get_base_filename());
-
-      Log::add_to_inactive(old_obj);
-    }
   }
 
+  // At this point, the old manager has a sparse object array (ie. some NULL entries). This
is unfortunate, but
+  // we don't want to modify the old manager in a non-atomic way because another thread might
be flushing the
+  // log objects as we do this. We know the manager will be destroyed soon after log object
transfer, so this
+  // intermediate state should not last very long.
+
   if (is_debug_tag_set("log-config-transfer")) {
     Debug("log-config-transfer", "Log Object List after transfer:");
     display();
@@ -1293,6 +1326,56 @@ LogObjectManager::transfer_objects(LogObjectManager & old_mgr)
 }
 
 int
+LogObjectManager::roll_files(long time_now)
+{
+  int num_rolled = 0;
+  for (size_t i=0; i < _numObjects; i++) {
+    if (_objects[i]) {
+      num_rolled += _objects[i]->roll_files(time_now);
+    }
+  }
+  for (size_t i=0; i < _numAPIobjects; i++) {
+    if (_APIobjects[i]) {
+      num_rolled += _APIobjects[i]->roll_files(time_now);
+    }
+  }
+  return num_rolled;
+}
+
+void
+LogObjectManager::display(FILE * str)
+{
+  for (size_t i = 0; i < _numObjects; i++) {
+    if (_objects[i]) {
+      _objects[i]->display(str);
+    }
+  }
+}
+
+LogObject *
+LogObjectManager::find_by_format_name(const char *name) const
+{
+  for (unsigned i = 0; i < _numObjects; ++i) {
+    if (_objects[i] && _objects[i]->m_format->name_id() == LogFormat::id_from_name(name))
{
+      return _objects[i];
+    }
+  }
+  return NULL;
+}
+
+size_t
+LogObjectManager::get_num_collation_clients() const
+{
+  size_t coll_clients = 0;
+  for (unsigned i = 0; i < _numObjects; ++i) {
+    if (_objects[i] && _objects[i]->is_collation_client()) {
+      ++coll_clients;
+    }
+  }
+  return coll_clients;
+}
+
+int
 LogObjectManager::log(LogAccess * lad)
 {
   int ret = Log::SKIP;
@@ -1335,3 +1418,67 @@ LogObjectManager::log(LogAccess * lad)
 
   return ret;
 }
+
+void
+LogObjectManager::flush_all_objects()
+{
+  for (unsigned i = 0; i < this->_numObjects; ++i) {
+    if (this->_objects[i]) {
+      this->_objects[i]->force_new_buffer();
+    }
+  }
+
+  for (unsigned i = 0; i < this->_numAPIobjects; ++i) {
+    if (this->_APIobjects[i]) {
+      this->_APIobjects[i]->force_new_buffer();
+    }
+  }
+}
+
+#if TS_HAS_TESTS
+
+static LogObject *
+MakeTestLogObject(const char * name)
+{
+  const char * tmpdir = getenv("TMPDIR");
+  LogFormat format("testfmt", NULL);
+
+  if (!tmpdir) {
+    tmpdir = "/tmp";
+  }
+
+  return NEW(new LogObject(&format, tmpdir, name,
+                 LOG_FILE_ASCII /* file_format */, name /* header */,
+                 1 /* rolling_enabled */, 1 /* flush_threads */));
+}
+
+REGRESSION_TEST(LogObjectManager_Transfer)(RegressionTest * t, int /* atype ATS_UNUSED */,
int * pstatus)
+{
+  TestBox box(t, pstatus);
+
+  // There used to be a lot of confusion around whether LogObjects were owned by ome or more
LogObjectManager
+  // objects, or handed off to static storage in the Log class. This test just verifies that
this is no longer
+  // the case.
+  {
+    LogObjectManager mgr1;
+    LogObjectManager mgr2;
+
+    mgr1.manage_object(MakeTestLogObject("object1"));
+    mgr1.manage_object(MakeTestLogObject("object2"));
+    mgr1.manage_object(MakeTestLogObject("object3"));
+    mgr1.manage_object(MakeTestLogObject("object4"));
+
+    mgr2.transfer_objects(mgr1);
+
+    rprintf(t, "mgr1 has %d objects, mgr2 has %d objects\n",
+        (int)mgr1.get_num_objects(), (int)mgr2.get_num_objects());
+
+    rprintf(t, "running Log::periodoc_tasks()\n");
+    Log::periodic_tasks(ink_get_hrtime() / HRTIME_SECOND);
+    rprintf(t, "Log::periodoc_tasks() done\n");
+  }
+
+  box = REGRESSION_TEST_PASSED;
+}
+
+#endif

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/LogObject.h
----------------------------------------------------------------------
diff --git a/proxy/logging/LogObject.h b/proxy/logging/LogObject.h
index d8dab73..04a1e36 100644
--- a/proxy/logging/LogObject.h
+++ b/proxy/logging/LogObject.h
@@ -148,9 +148,9 @@ public:
   void displayAsXML(FILE * fd = stdout, bool extended = false);
   static uint64_t compute_signature(LogFormat * format, char *filename, unsigned int flags);
 
-  char *get_original_filename() const { return m_filename; }
-  char *get_full_filename() const { return (m_alt_filename ? m_alt_filename : m_filename);
}
-  char *get_base_filename() const { return m_basename; }
+  const char *get_original_filename() const { return m_filename; }
+  const char *get_full_filename() const { return (m_alt_filename ? m_alt_filename : m_filename);
}
+  const char *get_base_filename() const { return m_basename; }
 
   off_t get_file_size_bytes();
 
@@ -325,7 +325,7 @@ public:
 private:
 
   int _manage_object(LogObject * log_object, bool is_api_object, int maxConflicts);
-  static bool _has_internal_filename_conflict(char *filename, LogObject ** objects, int numObjects);
+  static bool _has_internal_filename_conflict(const char *filename, LogObject ** objects,
int numObjects);
   int _solve_filename_conflicts(LogObject * log_obj, int maxConflicts);
   int _solve_internal_filename_conflicts(LogObject * log_obj, int maxConflicts, int fileNum
= 0);
   void _add_object(LogObject * object);
@@ -333,28 +333,8 @@ private:
   int _roll_files(long time_now, bool roll_only_if_needed);
 
 public:
- LogObjectManager()
-   : _numObjects(0), _maxObjects(LOG_OBJECT_ARRAY_DELTA), _numAPIobjects(0), _maxAPIobjects(LOG_OBJECT_ARRAY_DELTA)
-  {
-    _objects = new LogObject *[_maxObjects];
-    _APIobjects = new LogObject *[_maxAPIobjects];
-    _APImutex = NEW(new ink_mutex);
-    ink_mutex_init(_APImutex, "_APImutex");
-  }
-
-  ~LogObjectManager() {
-    for (unsigned int i = 0; i < _maxObjects; i++) {
-      delete _objects[i];
-    }
-    delete[] _objects;
-
-    for (unsigned int i = 0; i < _maxAPIobjects; i++) {
-      delete _APIobjects[i];
-    }
-    delete[] _APIobjects;
-
-    delete _APImutex;
-  }
+  LogObjectManager();
+  ~LogObjectManager();
 
   // we don't define a destructor because the objects that the
   // LogObjectManager manages are either passed along to another
@@ -371,6 +351,9 @@ public:
   // return success
   bool unmanage_api_object(LogObject * logObject);
 
+  // Flush the buffers on all the managed log objects.
+  void flush_all_objects();
+
   LogObject *get_object_with_signature(uint64_t signature);
   void check_buffer_expiration(long time_now);
   size_t get_num_objects() const { return _numObjects; }
@@ -380,59 +363,16 @@ public:
   int log(LogAccess * lad);
   void display(FILE * str = stdout);
   void add_filter_to_all(LogFilter * filter);
-  LogObject *find_by_format_name(const char *name);
+  LogObject *find_by_format_name(const char *name) const ;
   size_t preproc_buffers(int idx);
   void open_local_pipes();
   void transfer_objects(LogObjectManager & mgr);
 
   bool has_api_objects() const  { return (_numAPIobjects > 0); }
 
-  size_t get_num_collation_clients();
-};
-
-inline int LogObjectManager::roll_files(long time_now)
-{
-    int num_rolled = 0;
-    for (size_t i=0; i < _numObjects; i++) {
-      num_rolled += _objects[i]->roll_files(time_now);
-    }
-    for (size_t i=0; i < _numAPIobjects; i++) {
-      num_rolled += _APIobjects[i]->roll_files(time_now);
-    }
-    return num_rolled;
+  size_t get_num_collation_clients() const;
 };
 
-inline void
-LogObjectManager::display(FILE * str)
-{
-  for (size_t i = 0; i < _numObjects; i++) {
-    _objects[i]->display(str);
-  }
-}
-
-inline LogObject *
-LogObjectManager::find_by_format_name(const char *name)
-{
-  for (size_t i = 0; i < _numObjects; i++) {
-    if (_objects[i]->m_format->name_id() == LogFormat::id_from_name(name)) {
-      return _objects[i];
-    }
-  }
-  return NULL;
-}
-
-inline size_t
-LogObjectManager::get_num_collation_clients()
-{
-  size_t coll_clients = 0;
-  for (size_t i = 0; i < _numObjects; i++) {
-    if (_objects[i]->is_collation_client()) {
-      ++coll_clients;
-    }
-  }
-  return coll_clients;
-}
-
 inline bool
 LogObject::operator==(LogObject & old)
 {

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/LogPredefined.cc
----------------------------------------------------------------------
diff --git a/proxy/logging/LogPredefined.cc b/proxy/logging/LogPredefined.cc
index 79e6d52..f91d5a0 100644
--- a/proxy/logging/LogPredefined.cc
+++ b/proxy/logging/LogPredefined.cc
@@ -41,6 +41,16 @@ const char * const PreDefinedFormatInfo::extended2 =
   "%<chi> - %<caun> [%<cqtn>] \"%<cqtx>\" %<pssc> %<pscl>
"
   "%<sssc> %<sscl> %<cqbl> %<pqbl> %<cqhl> %<pshl> %<pqhl>
%<sshl> %<tts> %<phr> %<cfsc> %<pfsc> %<crc>";
 
+PreDefinedFormatInfo *
+MakePredefinedErrorLog(LogConfig * config)
+{
+  LogFormat * fmt;
+
+  fmt = MakeTextLogFormat("error");
+  config->global_format_list.add(fmt, false);
+  return NEW(new PreDefinedFormatInfo(fmt, "error.log", true /* always ascii */, NULL /*
no log header */));
+}
+
 PreDefinedFormatList::PreDefinedFormatList()
 {
 }
@@ -57,7 +67,7 @@ PreDefinedFormatList::~PreDefinedFormatList()
 void
 PreDefinedFormatList::init(LogConfig * config)
 {
-  LogFormat *fmt;
+  LogFormat * fmt;
 
   fmt = NEW(new LogFormat("squid", PreDefinedFormatInfo::squid));
   config->global_format_list.add(fmt, false);

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/538eba5c/proxy/logging/LogPredefined.h
----------------------------------------------------------------------
diff --git a/proxy/logging/LogPredefined.h b/proxy/logging/LogPredefined.h
index 59c1fa3..b56ce06 100644
--- a/proxy/logging/LogPredefined.h
+++ b/proxy/logging/LogPredefined.h
@@ -62,4 +62,8 @@ struct PreDefinedFormatList
   PreDefinedFormatInfoList formats;
 };
 
+// Return a PreDefinedFormatInfo structure for the ASCII error log.
+PreDefinedFormatInfo *
+MakePredefinedErrorLog(LogConfig * config);
+
 #endif /* LOG_PREDEFINED_H */


Mime
View raw message