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:  jelte
  stephen                            |                Status:  closed
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110712
                   Priority:  major  |            Resolution:  complete
                  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      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * status:  reviewing => closed
 * resolution:   => complete


Comment:

 Replying to [comment:11 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).
 >

 ohyeah :) done.

 > '''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.
 >

 ok, that looks like a decent solution

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

 hmm, yes. We could make a convention to always init logging before
 including specific libraries. But that's fragile, and in any case if a
 fundamental problem like this occurs we'd want the program to terminate
 anyway. Created ticket #1103, and replaced the try/catch block with a
 comment pointing to that trac ticket.

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

 warn is fine with me, changed

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

 I'll leave it to warn as well, for now

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

 the other two messages with _BAD_ in them as well, but also added it to
 the qr_not_set, and uncaught_error messages now.

 > BTW, not resending it probably makes sense as notifies are basically
 > just hints (so they don't need high reliability)..
 >

 true, and in some of the cases chances are high that the remote server did
 indeed get it right, it just mucked up its response (although in the case
 of mismatched query_name i wonder). More imporantly, if it fails
 reproducable, we probaly don't want to bother it with the same information
 again :)


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

 thanks

 > > > - _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).

 Oh I know it happens (i think nsd did it for quite some time), but i was
 wondering if we don't unintentionally drop the response before we even get
 to this part of the code.

 Since with one exception, these changes were cosmetic, I'm gonna assume
 it's okay and merge it, and closing the ticket. If not, you hereby have my
 permission to reopen it, and publicly flog me :)

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