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:

 Replying to [comment:21 jinmei]:
 > Replying to [comment:20 jelte]:
 >
 >
 > 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:
 >
 <snip>

 OK, good point :) Done. Renamed some files (again), to match the contents
 (i.e. log_buffer_impl -> buffer_appender_impl)

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

 ah, good idea :) Done too, added and changed some typedefs in an attempt
 to keep it readable; log level strings are now converted to string in
 append. Does result in a few more copies, but I do not think this is a
 problem (from the point of view of performance).

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

 removed

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

 I know, however, I indeed did it so that it was always cleared.


 Replying to [comment:23 jinmei]:
 > One more thing: in `LoggerManagerImpl::flushBufferAppenders()`,
 > I'd make sure dynamic_cast succeeds:
 >
 > {{{#!cpp
 >         internal::BufferAppender* app =
 >             dynamic_cast<internal::BufferAppender*>(it->get());
 >         assert(app != NULL); // <= add this line
 >         app->flush();
 > }}}

 done


 Replying to [comment:24 jinmei]:
 > Yet another thing: I guess we can simply remove these:
 > {{{#!cpp
 >     //LogBuffer buffer_appender1->getLogBuffer().
 >     //LogBuffer buffer_appender2->getLogBuffer().
 > }}}

 yeah removed (along with some other cleanups)

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