BIND 10 #1454: Pass UPDATE packets from b10-auth to DDNS module

[BIND 10 Development]

#1454: Pass UPDATE packets from b10-auth to DDNS module
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jinmei
                       Type:  task   |                Status:  reviewing
                   Priority:  minor  |             Milestone:
                  Component:  DDNS   |  Sprint-20120124
                   Keywords:         |            Resolution:
            Defect Severity:  N/A    |             Sensitive:  0
Feature Depending on Ticket:  DDNS   |           Sub-Project:  DNS
        Add Hours to Ticket:  0      |  Estimated Difficulty:  3
                  Internal?:  0      |           Total Hours:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:12 jinmei]:
 > That said, the comment is now actually not very accurate because the
 > code in the while loop now catches one exception.  So I'd suggest
 > revise the comment to something like:

 Yes, you're right, it got outdated, so I applied your version.
 >
 > > If it was for check_command only, shouldn't we rather catch it inside
 the function and return a negative/error answer instead of falling through
 the cc session code?
 >
 > check_command() is in a different module so it's not that trivial
 > whether we can safely modify it for the convenience of ddns.

 No, I meant catching them inside the command_handler and config_handler,
 so the exception would be converted to negative answer for the caller. If
 the check_command itself threw, assuming because it lost the connection to
 the CC, we really should terminate.

 > > >   Also, what if self.accept() or self.handle_incoming() raises an
 > > >   exception?
 > >
 > > Well, accept should not raise EINTR, since the connection is already
 waiting there at the time, and others would be fatal. If these would be
 caught, it would be in the place where the above comment is. Similar for
 handle_incoming. Or do you have an idea what to do with such an exception?
 >
 > accept() involves the creation of SocketSessionReceiver, which could
 > fail and result in an exception.  I also think some error from the
 > accept system call such as ECONNABORTED or EMFILE could be handled
 > more gracefully because it's only for a single forwarder.  In general,
 > I rather think it's okay to catch all BIND10-internal exceptions from
 > accept() and handle_session(), (and maybe also check_command()), but
 > as commented above it's currently not easily distinguished.

 Hmm, right, there might be some that would be recoverable. But I don't
 really think these would happen in practice enough to get worried about
 them (eg. ECONNABORTED could happen, but auth does not give up connecting
 now).

 > So, as a compromise it may be okay for now to let it die in that case,
 > but I'd at least explicitly log that event within the loop before
 > doing so.

 There's bunch of log-exception calls in the main function (down in the
 file, outside of the class). I think these could be enough.

 > - This comment doesn't seem to be addressed (even if only to reject
 >   it): I'd move logging after close() so that even if logging raises
 >   an exception it at least completes the cleanup:

 I probably moved a different line previously. Anyway, this is not so
 critical with python, because it would lose the reference and it'd close
 it automatically.

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