BIND 10 #1539: The sending part of passing UPDATE packets

[BIND 10 Development]

#1539: The sending part of passing UPDATE packets
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120529
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  b10-auth                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:  DDNS   |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:11 vorner]:

 Thanks for the review.

 > {{{
 >   CXX    run_unittests-auth_messages.o
 > auth_srv_unittest.cc: In function ‘void<unnamed>::checkAddrPort(const
 sockaddr&, const std::string&, uint16_t)’:
 > auth_srv_unittest.cc:1431:57: error: ‘const struct sockaddr’ has no
 member named ‘sa_len’
 > make[6]: *** [run_unittests-auth_srv_unittest.o] Error 1
 > make[6]: *** Waiting for unfinished jobs....
 > make[6]: Leaving directory `/home/vorner/work/bind10/src/bin/auth/tests'
 > }}}

 Oops, my bad.  Fixed it.

 > Also, I don't think the place for the unix-domain socket should be
 configurable. Who cares about such internal thing? After all, nothing
 outside the system will use the socket file, it should be our job to place
 it correctly and find it. Would it be OK to just have a one temporary
 directory created at startup somewhere (`/tmp`) and put everything inside
 it? Something like the socket distribution code in boss does for the
 created sockets.

 Hmm, point taken.  But I think there are two things mixed (maybe
 because I wasn't clear enough):

 - the (default) path to the socket file and special hack based on
   environment variables are hardcoded in the source.
 - both auth and ddns do the same thing with lots of essentially
   duplicate code and possible inconsistency

 If we use the single common directory, the first issue is basically
 resolved (although I also see some disadvantages of using something
 like /tmp, so this one itself may be debatable).  But the second issue
 still exists at least about the file name.  Besides, regardless of
 this point, I think we should implement some form of notification for
 auth that ddns is running (or not) as commented in the ticket.  If we
 have ddns report it itself, we could also have it tell auth the socket
 file path.  Then the second problem will be resolved.

 Anyway, I understand it may not be the best idea to include the path
 in the DDNS configuration, so I revise the corresponding comment
 accordingly (634994e).  I believe it's enough for the purpose of this
 ticket.

 > The wrapper around the session forwarder seems to have bad
 encapsulation. It relies on external action when there's an exception ‒ if
 there's an exception, someone from outside needs to explicitly call close.
 Is it OK? If so, would it be worth commenting?

 Yeah, I was aware of that.  The main reason why I did it that way is
 because I expected it to be used for xfrout eventually, and we'd like
 to log the error case differently for xfrout and ddns.  But, on
 re-reading the code, I noticed it wouldn't be that bad if we use the
 same log message for the both case.  So I revised the code and really
 almost everything is now hidden in `SocketSessionForwarderHolder`.
 It's 434d67f.

 > Don't we have a copy of the `formatEndpoint` somewhere? Would it make
 sense to put it somewhere for general use?

 I was aware of the issue, and it didn't seem we already have anything
 like that for C++.  I was also aware that we'd eventually want to make
 it for wider use, but since the branch was getting big, I didn't go
 that further.  But now you explicitly asked (and other part of the
 branch doesn't seem to be very controversial), I've extended
 `IOEndpoint` so that it can be directly passed to logger `arg()`.
 As a bonus, we can test the conversion code directly, so I also added
 the tests.

 > The comment here should be updated to be true:
 > {{{#!diff
 > @@ -98,7 +102,8 @@ SrvTestBase::unsupportedRequest() {
 >          // set Opcode to 'i', which iterators over all possible codes
 except
 >          // the standard query and notify
 >          if (i == isc::dns::Opcode::QUERY().getCode() ||
 > -            i == isc::dns::Opcode::NOTIFY().getCode()) {
 > +            i == isc::dns::Opcode::NOTIFY().getCode() ||
 > +            i == isc::dns::Opcode::UPDATE().getCode()) {
 >              continue;
 >          }
 >          createDataFromFile("simplequery_fromWire.wire");
 > }}}

 Good catch, fixed.  And, it's less likely that we'll support other
 types at least in near future, I made the comment more generic so we
 don't have to revise it again for this issue.

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