BIND 10 #764: Conversion of BIND10 Python library to use new logging interface

[BIND 10 Development]

#764: Conversion of BIND10 Python library to use new logging interface
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110712
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  logging                            |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  7.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:10 jelte]:

 The revised code basically looks okay.  There are a bit more minor
 things to be fixed or points you may want to discuss more (see below
 for specifics), but I'm okay with fixing them and merging at your
 discretion.

 > > - Also, you may want to update the following sentence due to the
 > >   introduction of -d:
 >
 > updated (made it 'current working directory (unless -d is used)')

 Ok, and I just noticed that "with extensions of .h and .cc." may have
 to be updated because of -p (not for the main subject of this branch,
 though).

 '''python/isc/config/Makefile.am'''

 > > - This is not specific to this branch, but I've noticed one
 > >   controversial point.  Due to this the message py will be installed
 > >   under pyexecdir (such as /usr/local/lib/python3.1/site-packages/):
 > >   I don't think it good.  First, to be in such a generic package
 [...]

 > So far this was a deliberate choice as a tradeoff for source vs. build
 directory trees and the way python resolves library components. If you can
 come up with a better solution I'm certainly fine with that. Otherwise we
 can consider changing the filenames to make it more clear.

 I've created a ticket for this (#1101).  This is beyond the scope of
 this ticket anyway.  Keep the current convention for now.

 > > - NOTIFY_OUT_IMPORT_DNS isn't defined:
 ...
 > > - And, I found a more fundamental problem here (not specific to this
 > >   branch though): when this log is triggered, it's quite unlikely that
 > >   the logging has been initialized.  I suspect there are other such
 > >   cases.
 >
 > yes this is something i have been starting to worry about also, not only
 that it hasn't been initialized (which, if so, should be easy to fix), but
 also that even if initialized, it'll also come before -v is read, not to
 mention general configuration. Since this is an error, it doesn't matter
 on severity-level right now, but it still something we need to think
 about.

 Create a new ticket?  I guess we should probably stop catching the
 exception in this case and let the invoking process handle the case
 more smartly.  For now, I'd rather suggesting commenting this out
 (with commenting why we do this) because, as I pointed out, if the
 exception happens and then the log is triggered it will result in
 another exception and the program will terminate despite the intent of
 catching it in the first place.

 > > - NOTIFY_OUT_RETRY_EXCEEDED: I'd use a higher level than info (if any)
 > >   for this event because it should be unusual.
 >
 > error?

 The best would be "notice", but IIRC our current framework doesn't
 have it.  Error is a bit too strong, so I'd choose "warn", but this
 may be a matter of preference in the end.  So I'd leave it to you.

 > > - NOTIFY_OUT_REPLY_QR_NOT_SET: I'd use lower level than error, because
 > >   it's not our error.  Same for other BAD_ cases.
 >
 > warn ok?

 To me, it's even lower than warn.  But I know opinions varied on
 jabber.  I'd leave it to you.  We may also have a project wide
 discussion on a guideline of log levels.

 > > - NOTIFY_OUT_REPLY_QR_NOT_SET: I'd describe what will happen for the
 > >   notify request in this case.  Specifically, whether it will be
 > >   resent.  Same for other BAD_ cases.
 >
 > added that we did get a response, so we don't resend the notify (which i
 think is the reasoning behind the behaviour)

 Only NOTIFY_OUT_REPLY_BAD_QUERY_NAME seems to be updated on this point.
 BTW, not resending it probably makes sense as notifies are basically
 just hints (so they don't need high reliability)..

 > > - NOTIFY_OUT_REPLY_QR_NOT_SET: I'd say "message" instead of "packet".
 > >   Same for other BAD_ cases.
 >
 > ok

 I noticed other places where "packet" would better be "message".  I
 updated the message as such.

 > > - _get_notify_reply(): not specific to this branch, but what if
 > >   tgt_addr != addr?
 >
 > hmm, yeah, i'd guess we should warn but continue (since this does tend
 to occur once in a while, though it's fixed in most server), i'm not
 entirely sure whether we actually get to this piece of code when it does
 (apart from unit tests)

 Unless it's not a connected socket, that could happen due to an
 intentional attack, unlucky coincidence, or a bit naive
 implementation of the remote server (e.g., not ensuring the source
 address of the response be the same as the destination of the
 request).

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