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
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Note: I have made 3 commits; the first is the name and namespace change,
 the second is the rest of the comments, and the third is a proposal to get
 rid of the logbuffer singleton (we can still decide not to include that
 one), but see below :)

 Replying to [comment:18 jinmei]:
 > Replying to [comment:17 jelte]:
 >
 > '''About singleton'''
 >
 > > Come to think of it, flushing the logbuffer should to in processEnd().
 If we want to get rid of the singleton storage, what we *could* do is get
 all known loggers in processInit(), and do getAppender("buffer") on them,
 storing them in a vector of `SharedAppenderPtrs`, and flush those in
 processEnd() (instead of using the singleton like my last commit does).
 >
 > In that case the vector would have to be a singleton, so it wouldn't
 > really be a "solution".  We could still have processInit() return the
 > buffer to the caller, (the caller of) which would then pass it
 > `processEnd()`, but that doesn't really seem to be clean anyway.  At
 > this point I guess I have to give up "making it right" within this
 > ticket.  So, let's keep the singleton buffer as is.
 >

 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.

 > Personally, I'd still like to clean up the singleton/global stuff in

 <snip>

 I have no problems with that approach, but:

 > But that's obviously beyond the scope of this ticket, even if it makes
 > sense.  And probably something we only dream but not actually work on.
 >

 yeah...

 >
 > > > '''log_buffer.h'''
 >
 > By mentioning _impl.xx, I didn't mean the pimpl idiom, but clarifying
 that
 > the `LogBuffer` class etc are part of internal helpers when we use
 > log4cplus as the underlying log library.  e.g, so they won't be
 > accidentally used by normal applications.  In that sense, my specific
 > suggestion is to rename the files, and introduce a separate namespace
 > like "detail", "internal", or "lgo4cplus_helper" under isc::log for
 > these.
 >

 ah, like that, ok, done

 > > > '''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.

 > > > - `LogBuffer::flush`: what if it throws before clear()?
 > > >
 > {{{#!cpp
 >     LoggerEventPtrList stored_copy;
 >     stored_.swap(stored_copy);
 >
 >     LoggerEventPtrList::const_iterator it;
 >     for (it = stored_copy.begin(); it != stored_copy.end(); ++it) {
 >         log4cplus::Logger logger =
 >             log4cplus::Logger::getInstance((*it)->getLoggerName());
 >
 >         logger.log((*it)->getLogLevel(), (*it)->getMessage());
 >     }
 >     flushed_ = true;
 > }}}
 >
 > This requires a bit more additional code but I don't see any "trouble"
 > in it.  See also the next topic.
 >

 ok, using this as well.

 > > > '''log_buffer_unittest.cc'''
 >
 > Okay, but while trying to understand it, I found one minor and subtle
 > issue: if we simply do flush here:
 > {{{#!cpp
 >     ~LogBufferTest() {
 >         buffer1.flush();
 >         buffer2.flush();
 >     }
 > }}}
 > it would result in extending the vector while flush() iterating
 > through that exact vector (because the logger still has the
 > `BufferAppender`, which shares the same buffer), leading to a messy
 > effect (crash in my case).  If flush() works on a swapped vector, we
 > could avoid such disruption.
 >

 that's done now :)

 > > > - Related to the point of directly relying on log4cplus API, I
 wonder
 > > >   whether the original intent is to exclude tests directly depending
 > > >   on log4cplus from the unittests.  I think direct unit tests are a
 > > >   good practice anyway, but maybe you want to ask Stephen about the
 > > >   original plan.
 > >
 > > I guess this other point ties in more to whether we should forcibly
 hide the type as deep as possible. (but in that case the tests would still
 need to create them somehow)
 >
 > I think in this case the issue is to separate things (rather than
 > "hide") specific to a particular library, especially if it could be
 > switched to something else (like the case for botan vs openssl).  If
 > these are cleanly separated, it will be easier to build/test
 > everything for a particular set of environment.  But for this ticket I
 > don't necessarily insist that.
 >

 Ok

 > > > '''buffer_manager_impl.cc'''
 >
 > - processSpecification: It now seems to be back to the original
 >   version, so it's probably better to make it a full copy of it.  At
 >   least I guess we should remove this comment:
 > {{{#!cpp
 >     // If this is an 'empty' specification, just set the root logger
 > }}}
 >

 doh, of course

 > > > '''cfgmgr.py'''
 > >
 > > Added comments (we need to trigger the process() call to flush the
 buffers in case there is no explicit config)
 >
 > Okay, and if I understand it correctly, this means cfgmgr will use the
 > default of log library, not the default given in logging.spec, right?
 > Shouldn't it be the latter?  Even though they are likely to be the
 > same, they can be different. (Although, technically this doesn't seem
 > to be an issue of this branch, but already exists in the current
 > implementation).
 >

 yes, and yes

 > '''log/README'''
 >
 > - I'd reorder some sentences, like this:
 > {{{
 > ... Note that if this option is used,
 > the program SHOULD call one of the LoggerManager's process() calls (if
 > you are using the built-in logging configuration handling in
 > ModuleCCSession, this is automatically handled.)  If the program exits
 > before this is done, all log messages are dumped in a shortened format
 > to stdout (so that no messages get lost).
 > }}}

 done

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