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

 It generally looks good.  I have some miscellaneous comments, most of
 which are minor.

 '''ddns.py'''

 - not necessarily objecting, but is there any reason for using a
   different term than "TRACE_BASIC"?
 {{{#!python
 DBG_ACTIVITY = logger.DBGLVL_TRACE_BASIC
 }}}
   I think it's generally better to use the same terminology for
   overall consistency unless there's a reason for not doing so.
 - I'd say "messages (or requests)" instead of "packets":
 {{{#!python
         # List of the sessions where we get the packets
         self._socket_sessions = {}
 }}}
   as "packet" is not technically correct in the case of TCP.

 - "socket sessions" could be misleading because they are actually
   "session receivers" ("sessions" are things popped out from each
   receiver):
 {{{#!python
         self._socket_sessions = {}
 }}}
   I'd name it something like "_socksession_receivers".  Same comment
   applies to some other "session" variables used in the main code and
   test (I'd rename them to something like "receiver").

 - I think it's more helpful if we log the "from address"
 {{{#!python
         socket = self._listen_socket.accept()
         fileno = socket.fileno()
         logger.debug(DBG_ACTIVITY, DDNS_NEW_CONN, fileno)
 }}}
   instead of, or in addition to, the file descriptor.  I also don't
   see the need for the temporary "fileno" variable.

 - handle_request: I don't understand the first paragraph of pydoc:
 {{{
         This is called whenever a new DDNS request comes. Here the magic
         happens, the rest is either subroutines of the magic or the
 accounting
         around it that calls it from time to time.
 }}}
   what's the "rest"?  what's "accounting around it"?  What's it in
   "that calls it"?

 - handle_request: I'd remove eg. (or replace it with "i.e.") from the
   second paragraph of pydoc:
 {{{
         It is called with the request being session as received from
         SocketSessionReceiver, eg. tuple
         (socket, local_address, remote_address, data).
 }}}

 - handle_incoming: it seems to me "incoming" is a bit broad.  maybe
   something like "handle_session" or "handle_new_session" is better.
   or, since this should always be a DDNS request message in practice
   (except for some unexpected or buggy cases), we may name this
   "accept_request" (so it can be named differently from
   "handle_request").  Same sense of comment applies to
   "DDNS_INCOMING".

 - the temporary "request" variable doesn't seem to be necessary:
 {{{#!python
             request = session.pop()
             self.handle_request(request)
 }}}
   Or, we could separate these two and move the second line to an
   "else" block (which should be added)

 - I'd move logging after close() so that even if logging raises an
   exception it at least completes the cleanup:
 {{{#!python
             logger.warn(DDNS_DROP_CONN, fileno, se)
             del self._socket_sessions[fileno]
             socket.close()
 }}}

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

 - run: is there any reason we don't handle exceptions?
 {{{#!python
             # TODO: Catch select exceptions, like EINTR
 }}}
   Also, what if self.accept() or self.handle_incoming() raises an
   exception?

 '''ddns_messages.mes'''

 - If you didn't you may want to "normalize" the file using
   tools/reorder_message_file.py
 - DDNS_NEW_CONN: I'd clarify (in the detailed description) that it's a
   connection on a UNIX domain socket, and should basically be from a
   b10-auth instance.

 '''ddns_test.py'''

 - I'd name `FakeSession` `FakeSessionReceiver` as (in the underlying
   API) "session" means a different notion.

 - `TestDDNSServer`: self.cc_session should better be "private"?

 - test_accept: I'd add a comment about where the magic number of '3'
   comes from:
 {{{#!python
         # Now the new socket session receiver is stored in the dict
         self.assertEqual([3],
 list(self.ddns_server._socket_sessions.keys()))
 }}}
   e.g. "_listen_socket is FakeSocket(2), whose accept() will return
   another FakeSocket with a fake fileno of 3 (2 + 1)"

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