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:           |        Hours:                             
      Billable:  1        |   Totalhours:                             
      Internal:  0        |  
--------------------------+-------------------------------------------------
Changes (by jinmei):

  * owner:  jinmei => zzchen_pku


Comment:

 Replying to [comment:9 zzchen_pku]:
 > Patch has been updated.
 > Exception raised by ShouldRollover() will be handled by handleError()
 and logging errors will be reported to stderr.
 > Appreciate your opinions.

 Sorry for the very long delay.  I've finally found time for reviewing this
 stuff.

 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.

 Specific comments:

  - NSFileLogHandler.handleError()
    - please describe the intent of this (overridden) method.
    - the log error doesn't seem to entirely correct: doesn't it cover
 other cases than "update"?
  - NSFileLogHandler.shouldRollover(): a minor issue, but the following
 line seems to make the close() call above it unnecessary.
 {{{
             self.stream = None
 }}}
  - 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.  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).
  - 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.
  - 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?

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