BIND 10 #2445: suppress initial log

[BIND 10 Development]

#2445: suppress initial log
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  defect        |  jinmei
            Priority:  medium        |                       Status:
           Component:  Boss of BIND  |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20121218
         Sub-Project:  Core          |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:20 jelte]:

 > > '''About singleton'''

 > perhaps not really that much more clean, but i was imagining it'd be
 stored in the !LoggerManagerImpl instance. In fact, I don't understand why
 everything in !LoggerManagerImpl was made static; it makes sense for a few
 of the things there, but most calls are made on an instance anyway.
 >
 > So in the third commit, (which we can leave out if this is too much of a
 change), I've applied this idea; each BufferAppender does keep its own
 logbuffer; !LoggerManagerImpl walks through all loggers and collects them
 in the (no longer static) processInit(), and flushes them all in the (also
 no longer static) processEnd(), and the singleton LogBuffer is removed.

 I think I like it.  At least it seems to be one step closer to the
 "right design".  And, to this end, I'd rather unify `LogBuffer`
 into `BufferAppender`.  In fact, methods of `BufferAppender` are now
 trivial wrappers to corresponding methods of `LogBuffer` (flush to
 flush, append to `LogBuffer::add()`), and the `LogBuffer` itself is
 basically a simple encapsulation of a vector.  So the revised
 `BufferAppender` would look like:

 {{{#!cpp
 class BufferAppender : public log4cplus::Appender {
 public:
     void flush(); // this does what LogBuffer::flush did
 protected:
     // this does what LogBuffer::add() did
     virtual void append(const log4cplus::spi::InternalLoggingEvent&
 event);
 private:
     void flushStdout();
     LoggerEventPtrList stored_;
     bool flushed_;
 };
 }}}

 And we won't need `getLogBuffer()`; A new
 `BufferAppender::getBufferSize()` would suffice (and, in any event,
 I'd like to avoid providing non-const method unless it's related to
 the core responsibility of the class because it can be abused to break
 class integrity unnecessarily).

 > > > > '''log_buffer.cc'''
 > >
 > > Since this method is expected to be called in a very special context
 > > (essentially after the program's lifetime), I'd be as conservative as
 > > possible: avoid using user-defined classes or other/std libraries as
 > > much as possible, e.g:
 > > {{{#!cpp
 > >     for (it = stored_.begin(); it != stored_.end(); ++it) {
 > >         printf("Severity=%d [%s]: %s\n", (*it)->getLogLevel(),
 > >                (*it)->getLoggerName().c_str(),
 (*it)->getMessage().c_str());
 > >     }
 > > }}}
 > >
 >
 > Right, at the very initial version it only printed message and logger
 name, and I didn't really like printing severity as a number, which is why
 i pulled in the manager again. But ok, using the above now.

 I've come up with an idea to improve it a bit: convert the integer
 levels to string versions at the time of `BufferAppender::append()`
 and store the result in the buffer.  In flushStdout() we sue the
 stored string.

 '''log_buffer_impl.cc'''

 - In flush(), you shouldn't need explicit clear() after swap:
 {{{#!cpp
     LoggerEventPtrList stored_copy;
     stored_.swap(stored_copy);
 ...
     stored_.clear();
 }}}

 '''logger_manager_impl.cc'''

 - in `flushBufferAppenders`, I suspect making a local copy is not
   (necessarily) needed.  In this case, there's no risk of disruption I
   mentioned in my previous comment, and if flushBufferAppenders()
   throws in the middle of it, remaining appenders in
   buffer_appender_store_ will be cleaned up in the manager's
   destructor (which will soon take place in practice).  But if you
   want to guarantee that buffer_appender_store_ will always be cleared
   within flushBufferAppenders(), I don't oppose to making a copy here,
   too.

-- 
Ticket URL: <http://bind10.isc.org/ticket/2445#comment:21>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development