BIND 10 #805: Client side code to request new sockets (C++)

[BIND 10 Development]

#805: Client side code to request new sockets (C++)
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20120110
                   Priority:         |            Resolution:
  blocker                            |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  Unclassified                       |  Estimated Difficulty:  5
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
  Socket creator                     |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:23 vorner]:

 > > If my guess is correct, you are hit by another, unrelated bug that the
 > > bind10.special_component module unconditionally invokes the installed
 > > version of socket creator.  If that is the case please try the
 > > attached patch.
 >
 > Thanks, yes, you're right. I included the patch. I'll have to solve the
 installation problem of mine sometime.

 Okay, good to know that.  And I believe we can now remove the setting
 of "PATH" from run_bind10.sh.  (But maybe we want to defer it to a
 separate cleanup ticket to avoid unexpected regression due to that).

 > > As for how to test SO_REUSEADDR, if it's okay to actually get the
 > > socket the creator code we can test it using getsockopt.
 >
 > I added the test. I also added another option ‒ the ipv6 only for INET6
 sockets the same way, to keep compatibility. But I think we might want to
 rethink using this one, as the only „problem“ it solves is different
 default list of addresses for different systems.

 The test didn't pass on my environment (I suspect it doesn't pass for
 BSD variants in general).  I've fixed it.  See b37cfa5.

 As for IPV6_V6ONLY, we need it.  Maybe you are not aware of it (not
 surprisingly - it's quite subtle) it does prevent various kinds of
 real harm (but I wouldn't bother to discuss details now).

 > > - at least per API even close(2) could fail.  I believe we should
 > >   catch that case and kill the socket creator should that happen.
 > > - same for send_fd()
 >
 > I added the termination of the application. But I'm not sure if we
 really do want to do that, if it isn't better to leak the socket than to
 bring the whole bind10 down. Anyway, close shouldn't fail for the sockets
 in socket creator, so it probably doesn't matter.

 I've made slight modifications - see 61d3224.  Regarding whether
 aborting is the best option, we can discuss it.  Personally, I'd
 rather stop everything if the socket creator encounters an unexpected
 error.  I'd generally assume it's designed and implemented to be very
 simple and robust and practically never see such cases - if we really
 need to worry about cases where the creator actually has unexpected
 failure and triggers the entire system stop, I'm afraid that it
 indicates either the design or the implementation of the creator
 should be revisited.  Also, since the socket creator doesn't leave
 logs, ignoring failures doesn't seem to be good in general.

 > > - sockcreator.cc breaks many style guidelines.
 >
 > Well, the code there is 1.5 years old, maybe some of the guidelines are
 newer than that. If we want to clean it up, should it go to another
 ticket?

 Most of the guidelines that it breaks (in particular: camel typing for
 functions and using () for return) had existed even before you joined
 BIND10.

 It's easy to fix them, but at this stage it's probably better to
 complete the branch first.  So a separate cleanup ticket is okay.

 As we mention cleanups, I have a couple of more things to fix in that
 context (okay in the separate ticket):

 - I'd avoid using hardcoded magic numbers.
 - I'd really avoid the complicated macro used in the test.  That's
   largely my personal preference, but I encountered one practical
   issue in this branch due to the use of macro.  As noted above, some
   of the getsockopt tests failed for me.  But gtest indicated the line
   where the macro was used, not the exact point in the macro where it
   failed, e.g.
 {{{
 sockcreator_tests.cc:106: Failure
 Value of: on
   Actual: 4
 Expected: 1
 }}}
   And line 106 (at that time, in my local modified copy) was this one:
 {{{#!c++
     TEST_ANY_CREATE(SOCK_DGRAM, sockaddr_in, AF_INET, sin_family,
 INADDR_SET,
                     UDP_CHECK);
 }}}
   which took me some time to identify the specific test that failed.
   Eliminating the use of macro may not be the only solution to such
   problems, but taking into account many other known bad effects of
   macros, I'd say this incident is a good reason for cleaning it up.

 And, one last comment:

 - can this comment be now removed?
 {{{#!c++
                     // FIXME: Check the output and write a test for it
 }}}

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