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):

 Replying to [comment:10 stephen]:

 > > 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 don't mind either way - it depends if you think the arguments above
 outweigh the arguments for changing them.

 I'm personally okay with keeping isc::Exception with <string>.  Others
 (e.g. Michal) may have a different opinion, however, so my suggestion
 is to keep the current version for the purpose of this ticket and
 confirm the intent at bind10-dev.  Also, if we keep using it, I'd
 explicitly add dependency to libexception in the Makefile.am:
 {{{
 b10_sockcreator_LDADD +=
 $(top_builddir)/src/lib/exceptions/libexceptions.la
 }}}

 I suspect currently it happens to work because libutil_io internally
 adds this dependency.  But if the sock creator becomes a direct user
 of it, it's fragile to rely on the indirect dependency setup.

 > > I'd like to avoid using reinterpret_cast [...]
 > The code that was replaced was similar to your patch, i.e.
 {{{
 A* a;
 B* b = static_cast<B*>(static_cast<void*>(a));
 }}}

 Yes, I know.

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

 I agree the above nested static_cast is almost equally evil as
 reinterpret_cast in that you could abuse it (and the intent is often
 unclear - whether the developer was confident it was actually safe or
 just tried to deceive the compiler).  The convertSockAddr() templates
 are an attempt to at least make the intent clearer: they are supposed
 to be used for only converting between compatible sockaddr variants
 when it's known to be safe, although the template interface does not
 prevent abuse.  If we want we could even perform static checker to
 enforce to specify the templated type explicitly and must be some
 sockaddr(_xxx).

 In that sense, it won't be much different if we add detailed comments
 on why we use reinterpret_cast and why it's actually safe in this
 specific case.  In fact, I'd like to propose doing so wherever we use
 reinterpret_cast in our code (and it would be checked in review) -
 that would discourage an easy use of really dangerous reinterpret_cast
 in development.  And, by reducing the number of cases where
 reinterpret_cast is used, we can more easily check if we follow this
 convention via a simple grep and eye-check.  If grep produces 100
 matches that would be more difficult.  For this reason I'd still
 prefer avoiding reinterpret_cast in this case.

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

 Right, in the specific case of sockaddr variants, this is equally
 safe.  My concern with this approach is that it could convey a false
 sense of message that we could abuse union to deceive the compiler.
 Detailed comments about it may help discourage such attempts, though.

 Going back to the branch, I'd prefer avoiding reinterpret_cast
 somehow.  If you are okay with it and have a stronger preference on
 one of the alternatives, I'm okay with that if we can make it clearer
 why we do this, why it's safe in this specific case, and that the same
 workaround shouldn't easily be adopted just to deceive the compiler
 (especially if you use an alternative that is different from the
 existing convertSockAddr() framework that is dedicated for that kind
 of purpose); if you still think reinterpret_cast is the best among all
 approaches, I think we should discuss project-wide to reach consensus.

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

 Okay.

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

 Oops, I was confused.  Confirmed the revised code.

 > > 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: [...]

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

 Okay.

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

 Oops, you're right, I was confused on this, too.  I didn't know we can
 pass FD via the interprocess channel with the stdin/stdout interfaces.
 So the original description was actually correct, but the revised
 version is also okay.

 Finally, I made a few more trivial style cleanups.

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