BIND 10 #1452: add UDP socket passing to fd_X handling

[BIND 10 Development]

#1452: add UDP socket passing to fd_X handling
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jinmei
                       Type:  task   |                Status:  reviewing
                   Priority:  major  |             Milestone:
                  Component:  DDNS   |  Sprint-20111220
                   Keywords:         |            Resolution:
            Defect Severity:  N/A    |             Sensitive:  0
Feature Depending on Ticket:  DDNS   |           Sub-Project:  DNS
        Add Hours to Ticket:  0      |  Estimated Difficulty:  5
                  Internal?:  0      |           Total Hours:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:5 jinmei]:
 > Unfortunately the diff is pretty large.  Basically, however, (I hope)
 > it only does some straightforward things.  But the resulting code was
 > this big due to the need for considering various corner cases and
 > providing detailed tests.  My suggestion for review is to first read
 > the overview doxygen documentation in util/io/socketsssion.h (in the
 > `SocketSessionUtility` page), then look at the ForwardTest::pushAndPop
 > test (in util/tests/socketsession_unittest.cc), which checks basic
 > expected behaviors, along with the corresponding implementation.

 It's quite OK to keep in mind, even when large code-wise.

 But there are some problems with the branch.

 First, the library now uses exceptions, so it must link to it. I pushed a
 makefile fix, so it now compiles for me.

 The tests fail for me:
 {{{
 [ RUN      ] ForwardTest.pushTooFast
 socketsession_unittest.cc:685: Failure
 Expected: multiPush(forwarder_, *getSockAddr("192.0.2.1", "53").first,
 large_text_.c_str(), large_text_.length()) throws an exception of type
 SocketSessionError.
   Actual: it throws nothing.
 [  FAILED  ] ForwardTest.pushTooFast (0 ms)
 }}}

 My guess for this is, linux takes the buffer size as some kind of hint
 only and makes its own mind about the proper size and 3 messages fit
 without a problem. If I increased it to 3000, the test passed, I didn't
 test when it started to pass and I'm not sure if it might be dependant on
 some system parameters (amount of memory, size of memory management pages,
 etc).

 Also, is the „receptor“ name a good one? It reminds me with the little
 parts of cells that detect which chemicals are around and what temperature
 it is. Shouldn't it be a „recipient“, for the one who receives?

 I'm not sure about how the blocking reads are used and such. For one, if
 there comes three messages to be forwarded at once and they are all
 handled by auth before a single context switch happens, the last one would
 get dropped, even if the system would be completely capable of handling
 the load. Also, I heard some system (linux is one of them) can cause a
 spurious wakeup on a select for a socket that has no data to read. I'm not
 sure if it can be with a local socket, or if it was a result of some
 packet that came for the connection but was corrupt, or something. Anyway,
 it is probably good enough for now, if we're considering to do it
 differently with the receptionist (I'd very like to have auth, xfrin,
 xfrout and ddns blocking then, using only single connection to the
 receptionist).

 Also, there are several mostly minor issues:
  * Should the SA_TYPE in template be upper case? Don't we use it for
 constants and enum values? This is type, so it should be CamelCase:
 {{{#!c++
 template <typename SA_TYPE>
 }}}
  * Should we use cerrno instead of errno.h? The same applies for stdint.h
 and string.h.
  * Should the local includes be done first, before other headers, to
 ensure they don't use something from the system headers included above?
  * Should this use `UNIX_MATH_PATH` instead?
 {{{#!c++
 (sizeof(impl.sock_un_.sun_path) - 1 < unix_file.length())
 }}}
  * If you already check the size, why do you use the (more complicated)
 strncpy instead of strcpy? Is there a trick I'm forgetting?
 {{{#!c++
     strncpy(impl.sock_un_.sun_path, unix_file.c_str(),
             sizeof(impl.sock_un_.sun_path));
 }}}
  * I don't see why the „rest“ of the buffer with the path should be
 zeroed. The strncpy will zero only one byte after the end of string, and
 this might be far after it. I don't see anything zeroing it anywhere, so
 it might contain quite anything what was on the stack:
 {{{#!c++
 assert(impl.sock_un_.sun_path[sizeof(impl.sock_un_.sun_path) - 1] ==
 '\0');
 }}}
  * Where does the magic number „2“ come from here?
 {{{#!c++
 impl.sock_un_len_ = 2 + unix_file.length();
 }}}
  * Should the right side be impl.sock_un_len_? Nobody anywhere assigned to
 sock_un_len_ and I don't see it anywhere.
 {{{#!c++
 #ifdef HAVE_SA_LEN
     impl.sock_un_.sun_len = sock_un_len_;
 #endif
 }}}
  * As described in the man page (The freeaddrinfo() function frees the
 memory that was allocated for the dynamically allocated linked list res.),
 the freeaddrinfo should be called only once on the whole result ‒ it frees
 the whole list, not on each of the nodes there. I guess there was no
 problem here, as it produces only single-element lists here, but it's
 needlessly complicated and, in principle, wrong, as it may create double-
 free errors.

 With regards

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