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

[BIND 10 Development]

#1522: Implementatio of isc::server_common::SocketRequestor
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jelte
  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 jelte):

 Addressing comments one at a time, in the order they came in

 Replying to [comment:5 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.
 >

 thanks

 > '''socket_request.cc'''
 >
 > - 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);
 > }
 > }}}
 >

 went for that suggestion, we can reuse these in wrappers (just need to add
 them in the header, but I have not done that).

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

 ok added

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

 heh

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

 Yeah, fixed the check, and added a test

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

 Hmm, looking at that new code I think there is more of it that is intended
 to be used here (or if not intended, at least useful).

 For now I've went for the path of least resistance, added a close() in the
 existing throws. This will still leak if there is any other exception in
 that part of the code (or if a new throw is added). But given the seeming
 intent of the classes in that socketsession, this functionality should
 probably be moved there as well (i.e. why not let SocketSessionReceiver
 create and connect to a socket, instead of passing it an opened one? or
 perhaps provide both options if there is a use-case for the existing one)

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

 Same as above; for now I left this in here, as is, with a TODO message (oh
 the pain, committing code with TODOs).

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

 oh yes it is, and i actually have seen it (due to an error elsewhere, but
 still)

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

 actually it did, the same off-by-one as in the code itself (though we are
 creating the original length here it is less dangerous, but that piece
 could in theory change.

 > '''dns_server_unittest.cc'''
 >

 skipping these to be handled in 805

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