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

[BIND 10 Development]

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

 * owner:  jelte => vorner


Comment:

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

 ack, updated

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

 aesthetics :) But now that I see it again, it went over the official line
 length anyway, so inlined it.

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

 oops, that shouldn't have been in there. Removed the comment.

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

 Only if we want to run memory checkers on our tests. As I said, they serve
 very similar use-cases, and can probably be made into one function (or
 perhaps initTest can call cleanup if necessary then set it to the new
 value), but from an API point-of-view, calling something called init to
 clean up seems weird :) (and I did not want to mess with initTest() at
 this point because of followup work in 805 and/or other branches related
 to the socket creator).

 >  * The status messages from boss are 2 bytes long, either "0\n" or
 "1\n", you seem to take only the one byte.

 ah, doh, changed.

 >  * This should be „so far“?
 > {{{#!c++
 > // Clears the messages the client sent to far on the fake msgq
 > }}}

 yep

 >  * What use is clearMsgQueue() at the beginning of a test (there are
 multiple such tests).

 When initializing a ModuleCCSession, there is already some communication
 to the cc session (this is also the reason the SocketRequestorTest
 initializer adds an empty success response before starting the fake cc
 session). We don't need to see what's in it here, so it just empties the
 queue for easier access to the later messages we do want to check.

 >  * The test `testBadSocketReleaseAnswers` seem to have two parts. Should
 they be commented what failure they test?

 added

 >  * With the mini-fork-client thing, when you accept, the
 `client_address` is unused. Passing NULL should be OK there.

 ack, null'ed

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

 Well, in a way, reusing it and seeing that the next one does not fail is a
 way of testing this. I couldn't really come up with any better way to test
 it :)

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