BIND 10 #1522: Implementatio of isc::server_common::SocketRequestor

[BIND 10 Development]

#1522: Implementatio of isc::server_common::SocketRequestor
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  UnAssigned
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120110
  blocker                            |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  10
Feature Depending on Ticket:         |
  Socket creator                     |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:3 jelte]:

 I've taken an initial look at the branch.  I don't think we can
 complete the entire review process (I've not even reviewed all of the
 branch) and I'll be off for a while for holidays, so I've dumped what
 I've found so far without picking up the ticket.

 - First, it didn't compile.  I've fixed it myself and pushed the fix.
 - Second, tests didn't pass for me.  I've also fixed it myself and
   pushed the fix.

 '''socket_request.cc'''

 - 'static' isn't necessary for the constant strings:
 {{{#!c++
 static const std::string CREATOR_SOCKET_OK("1");
 }}}
  (and to be pedantic this use of static is officially deprecated in
  C++)

 - in general, defining non POT object this way is a source of static
   initialization fiasco.  sometimes it can happen to be safe depending
   on how exactly it's used, but I'd generally rather avoid it in the
   first place than making it very sure it's safe.  In this case we
   could either define it as char* and compare them using strcmp() or
   define it via a proxy function:
 {{{#!c++
 const std::string& CREATOR_SOCKET_OK() {
     static const std::string str("1");
     return (str);
 }
 }}}

 - The test failure was because the original code didn't take into
   account the case where sockaddr variants have a "length" member (in
   this case sun_len).  I also suspect there may be some kernel
   implementation that rejects connect()/bind() if this member isn't
   set, so it would be safer if we do this:
 {{{#!c++
 #include <config.h>
 ...
 #ifdef HAVE_SA_LEN
     sock_pass_addr.sun_len = <length>;
 #endif
 }}}

 - I suspect there's a off-by-one bug here:
 {{{#!c++
         if (path.size() > sizeof(sock_pass_addr.sun_path)) {
             isc_throw(SocketError, "Unable to open domain socket " << path
 <<
                                    ": path too long");
         }

         strcpy(sock_pass_addr.sun_path, path.c_str());
 }}}
   Consider the case path.size() == sizeof(sun_path) and where the
   terminating \0 will be written.  (If my guess is correct, tell
   vorner that this proves why it was prudent to add an assert() in the
   `SocketSessionForwarder` constructor:-)

 - If the test doesn't cover the boundary case (I've not checked it),
   I'd add it.

 - createFdShareSocket (maybe some others also) is not exception safe:
   If it throws after creating the socket it will leak.  I'd use
   something like `ScopedSocket` defined n lib/util/io/socketsession.cc
   (latest master).  Maybe we should move it to sockaddr_util.h (and
   rename the file) so that it can be shared by other libraries?

 - I'd avoid C-style cast:
 {{{#!c++
         if (connect(sock_pass_fd,
                     (struct sockaddr *)&sock_pass_addr,
                     len) == -1) {
 }}}
   (btw this code also breaks the style guideline in terms of the
   position of *).  I defined and used a conversion template in
   lib/util/io/sockaddr_util.h.  It's still a kind of fraud for the
   compiler, but IMO still better than C-style cast or
   reinterpret_cast, if only to limit the use of these beasts to cases
   where there is absolutely no other alternative.

 - getSocketFd: In my understanding passed_sock_fd == 0 is a valid
   case.

 '''socket_requester_test.cc'''

 - in create(), the code around strncpy() made me nervous, too,
   although in this case it doesn't seem to have a bug.

 '''dns_server_unittest.cc'''

 This is not for this branch (I guess it's a result of importing #805),
 but the tests didn't pass for me, too.  I needed to add this:
 {{{#!c++
          struct sockaddr_in addr;
 +        memset(&addr, 0, sizeof(addr));
          addr.sin_family = AF_INET;
 }}}

 While looking at it I noticed some other things for this file:
 - some of the above comments apply to this implementation, too:
   exception safety, consideration for sin_len, non plain-old static
   object, unnecessary namescope-level static.
 - I would personally use getaddrinfo to create a sockaddr() to avoid
   this kind of portability issue.
 - I would use IPv6 for testing for future generation software like
   BIND 10 (we could also test IPv4 if we want).
 - technically, passing protocol 0 to socket for AF_INET isn't good:
 {{{#!c++
         int result(socket(AF_INET, type, 0));
 }}}
   If and when DCCP or SCTP is more common this can be ambiguous.

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