BIND 10 #1704: log output mixed

[BIND 10 Development]

#1704: log output mixed
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20120612
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  logging                            |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  15
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:60 muks]:
 > Passes distcheck now. :)

 Okay, the substantial one was resolved.

 As usual with a certain amount of diff, however, I have some more
 comments.  See below.

 '''Other general things'''

 Please don't forget updating changelog with taking into account the
 review comment.  Please also remember documenting the magic env
 variables (which I think should go to a separate ticket at this stage).

 '''top Makefile'''

 Why are we using clean-local?  Can't we use CLEANFILEs?  What's the
 purpose of '-' before 'rm'?

 '''bind10_src.py'''

 - Should we still use B10_FROM_BUILD for the lock path?
 {{{#!python
     if "B10_FROM_BUILD" in os.environ:
         lpath = os.environ["B10_FROM_BUILD"]
 }}}
   See also the similar comment on interprocess_sync_file.cc.

 '''bind10_test.py'''

 - The intent of the trick in check_environment_unchanged isn't
   obvious.  Please comment.  (and does it really make sense?  at least
   the tests seem to pass without this)

 '''interprocess_sync_file.cc'''

 - Why are we still using B10_FROM_BUILD in InterprocessSyncFile::do_lock?
 {{{#!c++
         const char* const env = getenv("B10_FROM_BUILD");
         if (env != NULL) {
             lockfile_path = env;
         }
 }}}
  It doesn't seem to have any effect now (it doesn't cause any
  regression if we disable it.

 - tryLock doesn't seem to be necessary any more.  In general, if we
   don't use a particular feature and don't have an immediate plan to
   use it, I'd apply the yagni principle and clean it up.

 '''logger_manager.cc'''

 What's the purpose of turning off the lock?  Maybe the reason
 explained in 6b3132f71b397e4925b80ed1dc1e3cb834be1294?  It's not
 obvious from the code, so please add some more detailed comments about
 why.

 '''log/tests/Makefile.am'''

 Using AM_V_GEN seems to make sense now that we use the silent mode by
 default.  But in that case it's even better to use it for all similar
 cases for consistency.  Not an issue for this branch though.  Maybe
 create a new ticket?


 '''log/tests/logger_example.cc'''

 What are these setInterprocessSync calls testing?  The same as
 logger_manager trick?  Please make some comments about the intent.

 '''python tests'''

 It doesn't seem to be sustainable to require the
 B10_LOCKFILE_DIR_FROM_BUILD trick in all Makefile's.  I'd, e.g.,
 extend lib/python/isc/log/log.cc:resetUnitTestRootLogger() to set the
 environment variable.  But that's probably too much for this branch.
 Let's create a separate ticket and defer the task to it.

 '''interprocess_sync_null.h'''

 - Assuming this file could be used outside the source tree, I'd use
   `<util/interprocess_sync.h>` here:
 {{{#!cpp
 #include "util/interprocess_sync.h"
 }}}
 - The declaration and definition of the destructor seem to be able to
   be skipped here.

 '''interprocess_sync_file_unittest.cc'''

 - What's the purpose of the empty string at the beginning of unlink?
 {{{#!cpp
   EXPECT_EQ (0, unlink("" TEST_DATA_TOPBUILDDIR "/test_lockfile"));
 }}}
 - And what's the purpose of these unlinks?  Would something wrong
   happen without these?  If so, it's generally better to do that by
   introducing a fixture and doing the cleanup in the destructor or in
   tearDown() and/or making sure the test starts cleanly by doing the
   cleanup in the constructor or in setUp().

 '''interprocess_sync_null_unittest.cc'''

 - What's the intent of repeating lock variants 5 times?

 '''logger_lock_test.cc'''

 - This comment seems to be a copy-paste and not make sense in this
   context:
 {{{#!cpp
 /// \brief Test InitLogger
 ///
 /// A program used in testing the logger that initializes logging using
 /// initLogger(), then outputs several messages at different severities
 and
 /// debug levels.  An external script sets the environment variables and
 checks
 /// that they have the desired effect.
 }}}
 - Hmm, this output makes me notice we do unlock twice (once
   explicitly, then via the destructor).  This may be safe in the file
   lock implementation, but I'd personally prevent that from happening
   at the `InterprocessSyncLocker` level.  That's especially so as this
   implementation intends to force applications to use the
   `InterprocessSyncLocker` wrapper.  And, in any event, if we want to
   require derived classes of `InterprocessSync` that duplicate unlock
   be safe, I think that should be explicitly documented.

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