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

 * owner:  muks => jinmei


Comment:

 Replying to [comment:61 jinmei]:
 > 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).

 How does this look:
 {{{
 XYZ.    [func]*         muks
         The logger now uses a lockfile named `logger_lockfile' that is
         created in the local state directory to mutually separate
         individual logging operations from various processes. This is
         done so that log messages from different processes don't mix
         together in the middle of lines. The `logger_lockfile` is created
         with file permission mode 0660. BIND 10's local state directory
         should be writable and perhaps have g+s mode bit so that the
         `logger_lockfile` can be opened by a group of processes.
 }}}

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

 Yes CLEANFILES can be used instead. Both are equivalent (whereas clean-
 local allows commands to be run for cleanup, CLEANFILES only deletes the
 files). I've updated the file. The '-' in front of 'rm' instructs make to
 ignore error exit status of rm and continue making.

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

 In both cases, I've retained B10_FROM_BUILD and override it with
 B10_LOCKFILE_DIR_FROM_BUILD, so that at least if B10_FROM_BUILD is set,
 it'll use the corresponding dir for creating lockfiles.

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

 That code has been removed now (it was necessary in a temporary step that
 never was committed).

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

 Same explanation as above.

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

 It is used in the unit tests to test the lock implementation. It's best if
 tryLock() is in the place it is, than in the unit tests as it would depend
 upon the lock implementation and uses similar code (the do_lock() method).

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

 A comment has been added.

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

 Ticket #2015 has been created for it.

 > '''log/tests/logger_example.cc'''
 >
 > What are these setInterprocessSync calls testing?  The same as
 > logger_manager trick?  Please make some comments about the intent.

 A comment has been added.

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

 Even that is not enough. The logger is called/initialized by the tests in
 different ways. We can investigate this later, but setting the env
 variable in the Makefiles is necessary now for tests to pass.

 > '''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"
 > }}}

 This has been updated, as have some other header files.


 > - The declaration and definition of the destructor seem to be able to
 >   be skipped here.

 As InterprocessSync has an implementation, I have not added one to
 InterprocessSyncNull as it'll be empty too.

 > '''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"));
 > }}}

 That came out of habit. I have removed these.

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

 The unlinks are used to cleanup (remove the test lockfiles). Without
 these, distcheck would complain about stale files that exist after make
 clean. They can be removed by a clean target, but in this case it's better
 if the test cleans up. Is a fixture absolutely necessary here?

 > '''interprocess_sync_null_unittest.cc'''
 >
 > - What's the intent of repeating lock variants 5 times?

 It is just an arbitrary number. Any number of times lock() and unlock()
 are called, the null implementation should always return true.

 This has changed to "just 4" calls in a row now due to code changes. :)

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

 Eek. This has been fixed.

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

 Now the destructor checks the lock status before unlocking. I have updated
 the class comment about maintaining this status variable, and also updated
 tests towards it.

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