BIND 10 #1704: log output mixed

[BIND 10 Development]

#1704: log output mixed
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  muks
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20120529
  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 muks):

 Some of the points have already been addressed in other comments. I'll
 just reply to the ones which haven't.

 Replying to [comment:36 jinmei]:
 > - As documented, we cannot do complicated things involving resource
 >   allocation in the `Logger` constructor.  In fact, this branch causes
 >   crash on my environment due to the static initialization fiasco.
 >   Not looking into it closely, but we should probably move this stuff
 >   to `LoggerImpl`.

 This is now done.

 > - I know it could be tricky, but I'd like to test this feature as much
 >   as possible.

 There is now a unit test for this feature, that forks another process and
 tests the lock while the log message is being output.

 > - I'd like to see documentation about the lock file, and the
 >   relationship with the magic environment variables (including when we
 >   need to define them).

 Please see my earlier reply on this (there's a question).

 > - Considering the bind10 process will often start as root and
 >   initially open the lock file, are others running as a non privileged
 >   user can get access to it?

 The branch now creates the lock file with mode 0660. This should be fine
 if the directory is bind10 group owned with g+w and the processes run as
 bind10 group.

 > - I'm simply not sure, so just asking: is it a good idea to repeat the
 >   "unable to use lock file" log when the process fails to open it?

 It is indeed a bad idea as it'll clutter up the output. This message is no
 longer repeated.

 > - Maybe we should allow the bind10 process to delete the lock file at
 >   the very end of its process?

 This is now implemented.

 > - The else block of `Logger::output` is (possibly) not exception
 >   safe.  Consider the case where the main outputRaw throws.

 The lock/unlock functionality is now inside an object which goes out of
 extent when it exits that block.

 > - If we want to log the event when locking/unlocking fails, I think
 >   it should be an ERROR, not a WARN.

 You are right on this too. A problem with unlocking (though unlikely)
 should be an ERROR definitely as it'd block all other log attempts.

 > - for log_test.py, may be a matter of taste, but since we'd need to
 >   run it from Makefile in practice (due to other setups), we could
 >   just set B10_FROM_SOURCE in the Makefile before running the test
 >   script.  That way we don't have to auto-generate it from .in.

 Done.

 ----

 > On the use of `file_lock`

 It seems to open/close the file each time (if we instantiate the object
 every time for raii) which will reduce performance. The raii part to work
 with exceptions is now implemented in a utility class which is fd based.

 > On having a lock file class

 There are two things to consider:

  * We just have one use case, and muxing several locks into a single lock
 file (maintaining range lists) will be over-implementing it. Even if we
 had more than one use-case, IMO we should have locks in separate files so
 that there's no global state.
  * Our use-case also ties in with the logger. We log if there's a problem
 locking/unlocking.

 The lock/unlock code is in a different utility class now.

 > On re-using the b10-config.db file for range locking instead of a
 separate lock file

 Applying an exclusive lock (even advisory) requires the file to be opened
 with write access flag. I think it's a bad approach to this. We don't need
 to mix functionality this way when a lock file is almost free.

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