BIND 10 #267: python log shouldn't die with OSError

[BIND 10 Development]

#267: python log shouldn't die with OSError
--------------------------+-------------------------------------------------
      Reporter:  jinmei   |        Owner:  zzchen_pku                 
          Type:  defect   |       Status:  reviewing                  
      Priority:  major    |    Milestone:  06. 4th Incremental Release
     Component:  logging  |   Resolution:                             
      Keywords:           |    Sensitive:  0                          
Estimatedhours:  0.0      |        Hours:                             
      Billable:  1        |   Totalhours:                             
      Internal:  0        |  
--------------------------+-------------------------------------------------
Changes (by stephen):

  * owner:  stephen => zzchen_pku


Comment:

 Branch created at: r2616
 Code reviewed:  r2932

 All things raised by Jinmei have been addressed, apart from those listed
 below.  There are also a couple of additional things that I have added.


 '''src/lib/python/isc/log/log.py'''

 ''!__init!__'' and ''shouldRollover()''
 Are we absolutely sure that "filename" contains a directory component at
 this point?  If given as just a bare filename (e.g. "log.txt"), then
 os.path.split() will return the empty string as its first element and the
 subsequent call to os.makedirs() will fail.

 Remark: "dir" (used to hold the result of os.path.split()) is the name of
 a built-in Python function. Its use as a variable is OK, but it is
 highlighted when I look at the file with an editor that does syntax
 colouring.


 >> - regarding creating directories to the log file: my previous comment
 wasn't sufficiently addressed: Please
 >> describe the intent of this behavior as comments (including why you
 extended the original behavior of python
 >> logging this way).

 I don't think that the comments have quite the level of detail that Jinmei
 wanted.  I think the essence of the question is why create the
 directories?  Most other applications will fail if the directory does not
 exist.


 >> - NSFileLogHandler.update_config(): maybe I was confused in the
 previous comment. If we create a missing
 >> directory (or directories) to the log file on initialization, and if we
 want to be consistent (I do), it looks like we
 >> should create necessary directories here; otherwise the underlying
 logging framework won't create them on writing logs.

 Remark: I think that the location of the directory creation is OK - when
 the handler is initialized and whenever there is a possibility that the
 logging system will write to a different file.  Changes made via a call to
 update_config() will be picked up in the next call to shouldRollover().
 (Incidentally, a comment to that effect in update_config() would be
 useful.)


 '''src/lib/python/isc/log/tests/log_test.py'''
 ''General''
 File should include the standard header comments.

 ''Class !TestRotateFileHandler''
 To check the last point above (about the change to update_config() being
 picked up at the next call to shouldRollover()), a test should be added
 for the interaction of the two methods - update the configuration, log a
 record, and make sure that the log file has rolled.

 test_handle_Error(): this is a no-op.  I suggest that it could log a
 message, then read the log file and check that the logged message is as
 expected.

 ''Class !TestSysLogHandler''
 test_emit(): This only checks that the emit() call does not fail - it does
 not check that the record has been sent to the syslog stream.

 ''Class !TestLogger''
 test_logging_init(): Suggestion: This method has lines of the form

 {{{
 ret = ABC in AYZ
 self.assertTrue(ret)
 }}}

 ... which might be better expressed (and give more information on an
 error) as:

 {{{self.assertIn(ABC, XYZ, "<Reason why the assertion failed>")}}}

 However, the code as it stands does work so there is no urgent need to
 refactor.

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