BIND 10 #1593: Cleanup of socket creator code

[BIND 10 Development]

#1593: Cleanup of socket creator code
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20120221
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  Core
            Defect Severity:  N/A    |  Estimated Difficulty:  4
Feature Depending on Ticket:         |           Total Hours:  0
  Socket creator                     |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Overall I like the cleanup made in the branch.

 '''general'''

 - I've made a couple of additional commits for further style
   cleanups.  I believe they are non controversial.

 - Using exceptions is probably a good idea, but maybe we'd avoid using
   isc_throw and isc Exception classes.  isc::Exception relies on
   <string>, which adds otherwise-unnecessary dependency on the C++
   standard library from the socket creator.  I guess we'd like to
   minimize dependency from the socket creator in general (for security
   reasons).  And, in this case, since we explicitly catch the
   exception and don't use the what() string anyway, it seems we can
   simply define SocketCreatorError (and its derived classes) as
   independent classes (not even derived from std::exception).

 - I'd like to avoid using reinterpret_cast unless there's absolutely
   no other alternative (and when we know what we are doing) such as
   for converting integer to a pointer so we can always be aware of
   where exactly we are using this beast.  I'd suggest extending the
   existing pointer conversion utility in lib/util/io and using it
   (attaching a patch).  In this case this conversion should actually
   be totally valid because we are using sockaddr* just as an opaque
   type.

 - adding comments to clarify the magic characters (these were a
   perfect example of 'read only code', which can be only understood by
   the original author) like this is good:
 {{{#!c++
                 writeMessage(output_fd, "S", 1);   // ... in the socket()
 call
 }}}
   but I wonder we'd even make them even more reader-friendly by
   introducing more descriptive constant variables like this:
 {{{#!c++
 const char* const SOCKET_FAILURE = "S"; // or this could be just char
 }}}

 '''main.cc'''

 We could avoid having a mutable variable, although in this case it may
 be a matter of taste:
 {{{#!c++
 main() {
     try {
         run(0, 1); // Read commands from stdin, output to stdout
     } catch (const SocketCreatorError& ec) {
         return (ec.getExitCode());
     }
     return (0);
 }
 }}}

 ..., and while looking at it, I wonder whether this one is better than
 hardcoding 0 and 1:
 {{{#!c++
         run(stdin, stdout); // Read commands from stdin, output to stdout
 }}}

 '''sockcreator.cc'''

 - suggestion: I wonder whether handleRequest() (btw I renamed it from
   handle_request to be more aligned with the coding guideline) if we
   changed it to something like this:
 {{{#!c++
     const int sock_type = getFamily(type[0]);
     const sockaddr* addr = getSockAddr(type[1]);
     ...
     } else {
         // Error.  Tell the client.
         writeMessage(output_fd, "E", 1);           // Error occurred...
         const char error_code = getErrorCode(result);
         writeMessage(output_fd, &error_code, sizeof(error_code));
 }}}
   and getXXX is responsible for the conversion and throwing the
   exception for the unexpected cases.  That way we won't be bothered
   with the mostly impossible cases of protocolError/IternalError in
   the main code logic.  But if you think it's too much or don't think
   it a good idea, I'm okay with keeping the current version.

 '''sockcreator.h'''

 - If you mean passing the FD through output_fd by "message/socket", I
   don't think it's correct.  FDs are passed through a separate UNIX
   domain socket.
 {{{#!c++
 /// \param output_fd File descriptor of the stream to which the output
 ///        (message/socket or error message) is written.
 }}}

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