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

Comment (by jinmei):

 Replying to [comment:11 vorner]:

 > > - run: it seems that the comment about the exception at the beginning
 > >   of the while loop is losing its sense.  It was originally for this
 > >   specific line:
 > > {{{#!python
 > >                     self._cc.check_command(True)
 > > }}}
 > >   and I guess it meant exceptions from check_command().  Now it's
 > >   unclear due to the distance between the comment and the line, and
 > >   even if we move the comment I'm not sure if it makes much sense
 > >   because we now have more code in the loop.
 >
 > I understood the comment to mean like if there's bug/problem parsing the
 packet, or something like that, we might want to drop the packet, log the
 exception, but continue handling the other packets that are coming in. In
 that sense, it is still valid.

 On re-reading it, the comment now doesn't look that awkward.  Since it
 took some time already I don't quite remember why I first saw it
 awkward (maybe "here" seemed to indicate a small fragment of code),
 but it doesn't seem to be a big issue to address.

 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:

 {{{
             # In this event loop we propage most of exceptions, which will
             # subsequently kill the b10-ddns process, but ideally it would
 be
             # better to catch any exceptions that b10-ddns can recover
 from.
             # We currently have no exception hierarchy to make such a
             # distinction easily, but once we do, we should catch and
 handle
             # non fatal exceptions here and continue the process.
 }}}

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

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

 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.

 I have a few more minor comments on the revised code:

 - handle_session: due to the change of the method name, it's probably
   better to pydoc too, e.g:
 {{{#!python
         Handle incoming session on the socket with given fileno.
 }}}

 - Likewise, test_incoming_called would better be renamed

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

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