BIND 10 #915: TSIG: (really) complete python library update

[BIND 10 Development]

#915: TSIG: (really) complete python library update
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110517
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  DNSPacket API                      |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  2.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei
 * milestone:  Sprint-20110531 => Sprint-20110517


Comment:

 general:

 didn't compile :/ I had to move a few headers up (see attached diff for
 what made it work for me), I did notice that in a few source files
 Python.h is explicitely included before anything else, that is also a
 possibility (would need to be added to the two files touched in my diff,
 where i just moved pydnspp_common.h to be the first, which also works
 becaurse the first thing that header does is include Python.h)

 except for this i think all my comments can be defered to the cleanup
 stage, but i wanted to mention them anyway:

 rrset_python.cc:

 was already 'bad'; we do a mostly superfluous if NULL check after some of
 the 'new' calls.  If we really think we should have this check we should
 at least set an exception string (this way it'll become a SystemError
 'error but no exception'), but we probably can just remove the check.  One
 instance i noticed here was line 201 since that changed, but there are
 more.  We also have places where we simply don't check, and places where
 we use nothrow, where we do correctly check (and the instance i found also
 correctly sets an exception).  Anyway as said, that's more of a cleanup
 thing (and one of the thing i guess you had in mind with #932).

 message_python.cc:

 do we really think we might get exceptions that are not derived from
 std::exception? If so, maybe we should reflect that a little bit more in
 the error message (just 'unexpected' seems a bit too general, since we
 already have an 'unexpected' message right before that, where we do
 include the what(), which we don't have now)

 (same for the message in catch(...) in pydnspp_towire.h, and
 tsig_python.cc (and more :))

 For comparison, TSIGKey_init() does not catch exception, just (...) (This
 might also be something to defer to (one of) the cleanup ticket(s) btw, if
 it isn't in there already).


 and one completely unrelated comment/annoyance :)
 the TSIG construction exception misses a space in tsig_250.cc:
     if (iss.bad() || iss.fail()) {
         isc_throw(InvalidRdataText, "Invalid TSIG text: parse error" <<
                   full_input);

 Anyway, I think this can be merged.

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