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

 * owner:  stephen => jinmei


Comment:

 > I've made a couple of additional commits for further style cleanups. I
 believe they are non controversial.
 No problems - they were some things I missed.

 > Using exceptions is probably a good idea, but maybe we'd avoid using
 isc_throw and isc Exception classes. isc::Exception relies on <string>,
 ...
 I had thought that but had used isc::Exception for consistency - all code
 in BIND 10 uses the same base exception type and same way of throwing
 exceptions.  The other thought was that although the failure is returned
 via a status code to the environment, we might in future want to log the
 reason for the failure via a call to syslog(), in which case a string
 would be useful.

 > ...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).
 In fact there is no need even to define !SocketCreatorError; we could just
 throw (and catch) an "int".

 I don't mind either way - it depends if you think the arguments above
 outweigh the arguments for changing them.

 > 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.
 The code that was replaced was similar to your patch, i.e.
 {{{
 A* a;
 B* b = static_cast<B*>(static_cast<void*>(a));
 }}}
 In other words, going from a pointer to one type to a pointer to an
 incompatible type via a void* pointer.  I understand the point about
 sockaddr as an opaque type (so it is an "allowable" incompatible cast),
 but the same method could be used for an "invalid" incompatible cast, e.g.
 to end up having sockaddr* variable pointing to an int.  So I changed the
 code to use reinterpret_cast on the grounds that if it walks like a duck
 and it quacks like a duck...  Also, it is shorter and simpler to
 understand.

 An alternative that avoids a cast is a union:
 {{{
 union {
     sockaddr     addr;
     sockaddr_in  addr_in;
     sockaddr_in6 addr_in6;
 };
 }}}
 This would emphasise that the different structs are alternative ways of
 representing the same data.  For the time being I have left the code as it
 is (with reinterpret_cast) but am happy to change it.

 > 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 ... but I wonder we'd even make them even more
 reader-friendly by introducing more descriptive constant variables like
 this
 Possible, but the REAME description of the protocol explicitly talks about
 the individual characters sent back.  To make it easier to relate that
 description to the code I would suggest that we leave the literal text as
 is.

 > We could avoid having a mutable variable, although in this case it may
 be a matter of taste
 For historical reasons I prefer functions to have one entry point and one
 exit point.  But in this case the code is so trivial it doesn't really
 matter so I've adopted your suggestion.

 > and while looking at it, I wonder whether this one is better than
 hardcoding 0 and 1:
 {{{
 run(stdin, stdout); // Read commands from stdin, output to stdout
 }}}
 stdin/stdout are FILE* variables, but I have altered it to use
 STDIN_FILENO and STDOUT_FILENO.

 > 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:
 I did this for the suggested getFamily() (renamed to getSocketType() to
 avoid confusion with the address family) and getErrorCode(). (Incidentally
 I used a mutable type (c.f. your comments on main.cc) to avoid compiler
 warnings in getSocketType(), as protocolError() does not return.  Although
 not strictly necessary for getErrorCode() - the compiler recognises that
 isc_throw will not return - it was used to emphasise the similarity of the
 code structure to that of getSocketType().)

 The problem with the suggested getSockAddr() function is that the sockaddr
 pointer is a pointer to another data structure.  Either that would have to
 be allocated statically within getSockAddr() or allocated dynamically with
 the caller responsbile for deallocating it.  Either seemed to make the
 code more complicated so I left it as is.

 > 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.
 As I understand the fs_send code, it is done via the CMSG macros which
 "are used to create and access control messages (also called ancillary
 data) that are not a part of the socket payload" (cmsg(3) man page).
 output_fd appears to be the socket used for communication.  I have updated
 the header though.

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