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

Comment (by jinmei):

 Replying to [comment:10 jelte]:
 > Yes it compiles for me now. Go! :)

 Thanks!  Merge done, and I'm closing the ticket, but I'll make
 follow comments on the open points (that will be carried over to #932)
 before that:

 > 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

 Ah, yes, you're right.  This NULL check doesn't make sense.  In the
 revised code that we'd make in #932, the entire function would look
 like this:

 {{{
 try {
    return (createNameObject(self->rrset->getName()));
 } catch (exception_type) { ... necessary exception handling and return
 NULL }
 }}}

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

 "Simply don't check" is obviously incorrect, and the use of nothrow
 is also incorrect (my fault), and, even if they were correct, handling
 the error cases in inconsistent ways wouldn't be nice anyway.

 And yes, fixing these is (part of) the intent of #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)

 Yeah, that's a good question.  Actually I believe it's a very unlikely
 event to get exceptions not derived from std::exception.  But (in most
 of the cases) since we cannot 100% control where exceptions or which
 exceptions could be thrown (if the underlying code uses external
 libraries including the standard lib), and since this is a very
 special module boundary (our internal C++ code and python
 interpreter), I thought we should expect anything.

 And, the reason why I separated "catch (std::exception)" and "catch
 (...)" was because that way we could provide a bit more detailed
 information via what() in the former.  In the latter case, I'm afraid
 we cannot say anything more than "unexpected" because we cannot assume
 it has what() or even the type of exception.  Maybe we could say
 something like "got an unexpected exception outside of libdns++" (as
 we know it shouldn't be from libdns++).

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

 This is a consistency issue.  In #932 we should fix our policy and use
 it consistently.

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