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

Comment(by zzchen_pku):

 > First, I've created a branch (branches/trac267) for the convenience of
 update/review cycles.  Please check the branch, and commit any subsequent
 changes to the branch instead of attachments.
 Thanks.
 > Specific comments:
 >
 >  - NSFileLogHandler.handleError()
 >    - please describe the intent of this (overridden) method.
 Done. Have added comments.
 >    - the log error doesn't seem to entirely correct: doesn't it cover
 other cases than "update"?
 Done.
 >  - NSFileLogHandler.shouldRollover(): a minor issue, but the following
 line seems to make the close() call above it unnecessary.
 > {{{
 >             self.stream = None
 > }}}
 Done.
 >  - 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).
 >  - 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.
 Have added comments.
 >  This behaviour should be tested (updating the config with a different
 non existent directory for the new log file, and confirming the new file
 is created).
 The case has been covered by test_log_message().
 >  - NSLogger.log_message(): please explain why you need to catch all
 exceptions here.  I'd first avoid a catch-all catch by limiting possible
 exceptions.  But if it's not feasible I'd explicitly explain why I catch
 all.
 logging.log(level, msg, *args, **kwargs) may throw TypeError and KeyError,
 NSLogger.log_message() will catch these two specific exceptions.
 >  - tests don't seem to be sufficient.  There are no assertions in the
 added test code, and the intent of the test is not very clear.
 >  - now that tests are added, did you check the coverage, i.e, the code
 you added to log.py is covered by tests?
 unittest has been modified.
 The code branch is trac267.

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