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 stephen):

 I've been interested in this problem, so I'd like to add my two-
 pennyworth.

 > In the branch currently, we open() in the constructor and not on every
 log message. The open() would be more costly as it would involve path
 operations and access checks.
 Since all log messages generated by a process should be synchronised - and
 not those from a single logger - we don't need to open in every
 constructor. We only need to open the file once in the process.

 >> Not looking into it closely, but we should probably move this stuff to
 !LoggerImpl.
 I concur.  The logger is written using a pimpl idiom, and most of the
 logger code is in !LoggerImpl.

 >> On a related note, isn't there a better primitive than the very low
 level system call? From a quick look boost also has a similar (in addition
 to interprocess_mutex) API: file_lock.
 > After using interprocess_mutex and reverting it, I side with fcntl() for
 this case. Its use here is very straightforward and any UNIX programmer
 can follow it. The behaviour is well defined (multiple locks are OK, and
 when process terminates, the locks are removed automatically).
 Looking at the boost::file_lock documentation, I strongly suspect that
 this is a layer on top of fcntl.

 Some other general points:

 * Regarding the lock file, is there any requirement that it be created
 when the process starts?  Can't we use an existing file - for example, the
 configuration database file?

 * I think the lock code should be made more general, perhaps separated out
 into a separate class.  This has several advantages:

 a) Removes the details of lock implementation from the logger class (which
 should be focused on logging and nothing else).
 b) If the unlock operation is put into the class destructor, locks could
 be declared as scope limited and automatically freed if an exception is
 generated in the locked section of code and not caught.
 c) Easier to test.
 d) Easier to use locks for other purposes as well.

 Regarding the last point, file locks only lock a range of bytes in the
 file.  There is no reason why we can't use multiple ranges within the same
 file to implement multiple locks.

 Finally, one specific point with regards to the code:

 * Even single line "if" or "else" clauses should be enclosed in braces.

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