BIND 10 #221: review: Refactoring auth server and merge the axfr and notify logic into it

[BIND 10 Development]

#221: review: Refactoring auth server and merge the axfr and notify logic into it
------------------------------+---------------------------------------------
      Reporter:  hanfeng      |        Owner:  jinmei                     
          Type:  enhancement  |       Status:  reviewing                  
      Priority:  major        |    Milestone:  06. 4th Incremental Release
     Component:  b10-auth     |   Resolution:                             
      Keywords:               |    Sensitive:  0                          
Estimatedhours:               |        Hours:                             
      Billable:  1            |   Totalhours:                             
      Internal:  0            |  
------------------------------+---------------------------------------------
Changes (by each):

  * owner:  UnAssigned => jinmei


Comment:

 I don't see anything particularly wrong here, but I do have a few nits and
 some questions about design decisions, see below.

 It's generally okay, and I expect any changes you make in response to the
 comments below to be fairly insignificant.  I'll leave it to your judgment
 whether to go ahead and commit after you've made such changes, or to seek
 further review.

 Index: src/lib/xfr/xfrout_client.h

 Minor nit 1:  In some other abstract base classes (e.g. !AbstractDataSrc,
 AbstractRRset) you've put in a boilerplate comment explaining why the
 constructor is declared "protected".  I noticed that comment is missing
 here, and thought I'd mention it in case you consider it important enough
 to add.

 Minor nit 2:  Seeing that the methods in !XfroutClient had been changed
 from non-virtual to virtual confused me for a bit; I thought there must be
 another derived class somewhere that extended !XfroutClient them, and was
 wondering if I was missing part of the code.  I'd appreciate a comment
 explaining why they're virtual.

 Index: src/lib/xfr/xfrout_client.cc

 All okay.

 Index: src/lib/cc/session.h

 Same note about missing comment on the "protected" constructor
 declaration.

 Index: src/bin/auth/auth_srv.h

 Seems fine.

 Index: src/bin/auth/auth_srv.cc

 "if (is_axfr_connection_established_)" is incorrect English ;)

 More seriously, that flag only has to do with xfrout, not xfrin, so maybe
 it should have a more specific name?  "xfrout_connected" or something.

 I'm not clear on why xfrout_client needs to be declared by the caller and
 passed into the !AuthSrv constructor, as opposed to having it instantiated
 by the !AuthSrvImpl constructor and leaving the !AuthSrv constructor's
 signature unchanged.  I don't see any place where xfrout_client is
 accessed from outside !AuthSrv.

 Index: src/bin/auth/asio_link.h

 +namespace asio {
 +class io_service;
 +namespace ip {
 +class address;
 +}
 +}

 Does the above code serve a useful purpose?  I commented it out and found
 it still compiled fine, though maybe other compilers would have problems.
 If it's needed, please add a comment explaining the reason.

 There were some minor grammatical errors in the comment explaining the
 asio_link class (missing paragraph breaks, singular/plural
 disagreement)--I can just fix them in the branch.

 The comments about IOEndpoint say that it's a wrapper for both ip::address
 and for the tcp/udp endpoint classes.  I think the first one is a mistake
 and will remove it in the branch.

 Comment on IOEndpoint::getAddress() says the return value is "not a real
 object" but appears to mean that it is one.  I'm fixing the comment.

 The static IOSocket::getDummy* methods seem rather unusual; why can't
 there just be a derived class !DummySocket?  I'm not seeing why this needs
 to be in the base class API.

 +    /// This method blocks until the \c stop() method is called via some
 +    /// handler.

 It does some other stuff too, right? :)

 Index: src/bin/auth/asio_link.cc

 This code seems fine...

 +        asio_endpoint_placeholder_(
 +            new tcp::endpoint(ip::address::from_string(address.toText()),
 +                              port)),
 +        asio_endpoint_(*asio_endpoint_placeholder_)

 ...but I don't understand the point.  Why not have asio_endpoint_ be a
 real object instead of a reference, and construct it in the normal way,
 without using "new" and "delete"?  (See attached diff.)

 Index: src/bin/auth/main.cc

 All seems fine except for the point mentioned before--that I don't know
 why xfrout_client is declared here.

 Index: src/bin/xfrin/xfrin.py.in

 +            # The default RR class is IN.  We should fix this so that
 +            # the class is passed in the command arg (where we specify
 +            # the default)

 This comment appears to be outdated; it seems as if the rrclass ''is''
 passed in the command arguments now.

 Index: src/bin/auth/tests/asio_link_unittest.cc

 Two of the tests currently fail:

 [  FAILED  ] IOServiceTest.badPort
 [  FAILED  ] IOServiceTest.unavailableAddress

 So fix or disable them.

 Index: src/bin/auth/tests/auth_srv_unittest.cc
 Index: src/bin/xfrin/tests/xfrin_test.py

 These look fine.

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