Re: [12/17] git commit: TS-2302: convert Log::error_log into a generic prefedined LogObject

[Igor Galić <i....@brainsware.org>]

----- Original Message -----
> 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.
> 
> 
[snip]
> Commit: 538eba5c078f6b14cbd3b6e6887da669fd7205c9
[snip]
> 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

Really? Do we really care about DEC's C++ Compiler?
  
> +  friend void RegressionTest_LogObjectManager_Transfer(RegressionTest *,
> int, int *);
>  private:
>  
>    static void periodic_tasks(long time_now);
> 

[snip]

> @@ -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

Who is `ome` ?

> +  // 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

[snip]

++ i
Igor Galić

Tel: +43 (0) 664 886 22 883
Mail: i.galic@brainsware.org
URL: http://brainsware.org/
GPG: 6880 4155 74BD FD7C B515  2EA5 4B1D 9E08 A097 C9AE