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      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jelte


Comment:

 Hello

 I'd have a different set of comments:
  * The comment with `CREATOR_SOCKET_UNAVAILABLE` seems misleading. In the
 case of this status, the fd is not sent.
  * When closing the unix domain sockets:
 {{{#!c++
 std::map<std::string, int>::iterator it;
 for (it = fd_share_sockets_.begin(); it != fd_share_sockets_.end(); ++it )
 }}}
    Is there a reason to declare the it outside the cycle? This would leave
 it in scope even after the end of it (not that there would be much after
 the end).
  * This comment change is strange:
 {{{#!diff
      /// \param session the CC session that'll be used to talk to the
 -    ///     socket creator.
 -    /// \throw InvalidOperation when it is called more than once.
 +    ///                socket creator.
 +    /// \throw InvalidOperation when it is called more than once,
 +    ///                         when socket_path is empty
      static void init(config::ModuleCCSession& session);
 }}}
    Where is the socket_path it talks about?
  * The cleanup, is it really needed? Isn't initTest(NULL) enough? I know
 it doesn't release the memory, but is it a problem in tests?
  * The status messages from boss are 2 bytes long, either "0\n" or "1\n",
 you seem to take only the one byte.
  * This should be „so far“?
 {{{#!c++
 // Clears the messages the client sent to far on the fake msgq
 }}}
  * What use is clearMsgQueue() at the beginning of a test (there are
 multiple such tests).
  * The test `testBadSocketReleaseAnswers` seem to have two parts. Should
 they be commented what failure they test?
  * With the mini-fork-client thing, when you accept, the `client_address`
 is unused. Passing NULL should be OK there.
  * When you send a nonsense instead of send_fd (eg. when it is -2), can
 you be sure the socket will be in a consistent state? And that it'll eat
 the one wrong byte somehow? Because you seem to use the socket after that.

 Thanks

 With regards

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