BIND 10 #183: UNIX domain socket for msgq

[BIND 10 Development]

#183: UNIX domain socket for msgq
-----------------------------+----------------------------------------------
 Reporter:  larissas         |        Owner:  jinmei                     
     Type:  defect           |       Status:  reviewing                  
 Priority:  major            |    Milestone:  04. 2nd Incremental Release
Component:  message-library  |   Resolution:                             
 Keywords:                   |    Sensitive:  0                          
-----------------------------+----------------------------------------------

Comment(by jinmei):

 A meta level comment: don't we have to worry about portability of UNIX
 domain sockets?  I actually don't remember why we chose to change the
 transport from TCP to UNIX domain (to which I don't necessarily object),
 but that won't work for Windows (do it?), and there may be other UNIX-like
 systems that cause portability problems with UNIX domain sockets.

 Specific comments:

  - about r1900
   - why add the 'pass' to the end of some tests?
   - test_open_socket_default()
    I think if you delete BIND10_MSGQ_SOCKET_FILE from os.environ you
 should recover it within the test.  Otherwise this test case overrides a
 global state of the test process and may affect other test results.

  - about r1901 (bind10.py.in): is this necessary?  {{{__init__}}} did the
 same thing, and self.msgq_socket_file doesn't seem to be modified after
 that.
 {{{
     def startup(self):
         if self.msgq_socket_file is not None:
              c_channel_env["BIND10_MSGQ_SOCKET_FILE"] =
 self.msgq_socket_file
 }}}
   Also, the assumption of the self.msgq_socket_file type is inconsistent
 with {{{__init__}}}:
 {{{
         if self.msgq_socket_file is not None:
             os.environ['BIND10_MSGQ_SOCKET_FILE'] =
 str(self.msgq_socket_file)
 }}}
   That is, in {{{__init__}}} self.msgq_socket_file is assumed to be
 (possibly) non string.

  - msgq.py.in.setup_listener
   Why are the following lines commented out?  Clearly we don't need
 bind('127.0.0.1'), and I also don't think we need SO_REUSEADDR (it's only
 necessary for dealing with TCP's TIME_WAIT periods after closing a socket
 at the server side).  So I basically we can simply purge these lines.  If
 there's a reason to keep them as comments, please also explain that as
 comments.

 {{{
         #self.listen_socket.setsockopt(socket.SOL_SOCKET,
 socket.SO_REUSEADDR, 1)
         #self.listen_socket.bind(("127.0.0.1", self.c_channel_port))
 }}}

  - lib/cc/session.cc:SocketSession::establish()
   shouldn't we need something equvalent to this?
 {{{
 #ifdef HAVE_SIN_LEN
     sin.sin_len = sizeof(struct sockaddr_in);
 #endif
 }}}
   BSDs have sun_len member in sockaddr_un{}.

  - lib/cc/session.cc:SocketSession::establish()
   I don't think it's a good practice to blindly null-terminate sun_path:
 {{{
     sun.sun_family = AF_UNIX;
     strncpy(sun.sun_path, socket_file, sizeof(sun.sun_path) - 1);
     sun.sun_path[sizeof(sun.sun_path) - 1] = '\0';
 }}}
    IMO if socket_file is too long we should explicitly fail (for example,
 the auto-truncated file may exist, then something surprising will happen).
 In any case please write a test to cover this case.

  - {{{lib/python/isc/cc/session.py.in:Session.__init__}}}
   (not specific to this patch, but) shouldn't we close the socket in these
 cases?
 {{{
             if not env:
                 raise ProtocolError("Could not get local name")
             self._lname = msg["lname"]
             if not self._lname:
                 raise ProtocolError("Could not get local name")
         except socket.error as se:
                 raise SessionError(se)
 }}}
   If so, please also write a test case to cover the bug (maybe as a
 separate ticket).

  - about r1916
   Do we have a case that we should use the default parameter of
 "socket_file"?  In general, I think we should avoid the use of default
 parameters unless there's a strong reason for that (because such a
 implicit default behavior often misleads the code reader, while it's
 sometimes convenient).

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