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:8 vorner]:
 > > 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.
 >
 > Ah, okay, google told me that Linux doubles the specified buffer size
 > by setsockopt().  So, increasing it to 4 (or perhaps 5) should be
 > enough.  I've changed it to 10 to be a bit conservative (commit
 > 8fda3f0).  As commented in that commit, if it still makes it fail we
 > should take a more reliable approach, but for now I prefer the brevity.

 Yes, it passes now.

 > > 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?
 >
 > Actually I was not sure if this naming is a good one.  So I have no
 > problem with changing it.  I've changed it to "Receiver" (not a strong
 > preference over "Recipient", but the former is a bit shorter and would
 > look more consistent compared to "Forwarder", in that they share the
 > suffix of "er").

 This one looks OK.

 > That said, I don't deny that it could be improved in a future
 > version.  For now I'd move forward with this style of operation and
 > see how practical it is.

 OK, agreed.

 > >  * Should the local includes be done first, before other headers, to
 ensure they don't use something from the system headers included above?
 >
 > Hmm, I don't know.  Is that a common practice?  I have no problem with
 > reordering them that way if it's more common, but in that case I think
 > we should rather make it in our project-wide guideline.  For now I've
 > not make a change on this point.

 I met several projects doing so, and I use it on unconscious level
 already. We probably don't have it in guidelines, but IMO it makes some
 sense, as it makes sure the headers are stand-alone. Should I bring it to
 -dev?

 > >  * Should this use `UNIX_MATH_PATH` instead?
 > > {{{#!c++
 > > (sizeof(impl.sock_un_.sun_path) - 1 < unix_file.length())
 > > }}}
 >
 > You mean UNIX_MAX_PATH?  I thought it was not portable.

 I'm not sure about portability. I found it in my man page.

 > Maybe we should at least clarify the point?  e.g.
 > {{{#!c++
 >     // the copy should be safe due to the above check, but we'd be
 rather
 >     // paranoid about making it 100% sure even if the check has a bug
 (with
 >     // triggering the assertion in the worse case)
 >     strncpy(impl.sock_un_.sun_path, unix_file.c_str(),
 >             sizeof(impl.sock_un_.sun_path));
 >     assert(impl.sock_un_.sun_path[sizeof(impl.sock_un_.sun_path) - 1] ==
 '\0');
 > }}}

 Yes, maybe.

 > >  * I don't see why the „rest“ of the buffer with the path should be
 zeroed.
 >
 > At least my man page states strncpy ensures that:
 > {{{
 >      The strncpy() function copies at most n characters from s2 into s1.
 If s2
 >      is less than n characters long, the remainder of s1 is filled with
 `\0'
 >      characters.  Otherwise, s1 is not terminated.
 > }}}
 >
 > Isn't it part of standard?

 Ah, I didn't know about that one. It is indeed in my man page as well.

 > >  * Where does the magic number „2“ come from here?
 > > {{{#!c++
 > > impl.sock_un_len_ = 2 + unix_file.length();
 > > }}}
 >
 > Ah, okay, it should have (better) been sizeof(uint16_t).  Changed.
 > (in case it's still not clear, it's for the "2-byte header length").

 Actually, I meant this 2:
 {{{#!c++
     assert(impl.sock_un_.sun_path[sizeof(impl.sock_un_.sun_path) - 1] ==
 '\0');
     impl.sock_un_len_ = 2 + unix_file.length();
 #ifdef HAVE_SA_LEN
 }}}

 > >  * 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.
 >
 > I'm not sure what's wrong about this point.  addrinfo_list_ should be
 > a list of the head pointer of the addrinfo chain.  So this shouldn't
 > cause duplicate free:
 > {{{#!c++
 >         vector<struct addrinfo*>::const_iterator it;
 >         for (it = addrinfo_list_.begin(); it != addrinfo_list_.end();
 ++it) {
 >             freeaddrinfo(*it);
 >         }
 > }}}
 > (if that's what you are talking about).  Or am I missing something or
 > misunderstanding you?

 No, sorry, I misread the code. I thought the vector contained all the
 nodes.

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