BIND 10 #301: Review result for auth

[BIND 10 Development]

#301: Review result for auth
------------------------------+---------------------------------------------
      Reporter:  zhanglikun   |        Owner:  jinmei  
          Type:  enhancement  |       Status:  assigned
      Priority:  major        |    Milestone:          
     Component:  b10-auth     |   Resolution:          
      Keywords:               |    Sensitive:  0       
Estimatedhours:  0.0          |        Hours:  0       
      Billable:  1            |   Totalhours:  0       
      Internal:  0            |  
------------------------------+---------------------------------------------

Comment(by jinmei):

 Replying to [ticket:301 zhanglikun]:
 > The following comments was inputted by stephen for ticket #289, so
 seperated it to one single ticket, maybe jinmei is the best person for
 accepting this ticket.
 >
 Actually, for auth_srv I suspect Evan is the best person as the original
 author of the class, but I took this ticket as a whole.

 I've created a branch "trac301", and I believe I addressed most of the
 comments.  The changeset is r2969.  I'm giving the ticket back to Stephen.

 > src/bin/auth/auth_srv.cc
 > With very similar names (AuthSrv and AuthSrvImpl), it might be clearer
 to put the declaration and implementation of AuthSrvImpl into separate
 files.
 >
 I'm not sure if I understand this, but since AuthSrvImpl is the
 implementation class for AuthSrv using the pimpl idiom, we cannot separate
 AuthSrvImpl into a different .cc file.  I noted it's for pimpl in
 auth_src.h.

 > The AuthSrv setXxxx and getXxxx functions are sufficiently trivial that
 consideration could be given to defining them inline in the header file.
 >
 It could, but for impl we cannot.  In any case these are not expected to
 be performance sensitive operations, so I think it's okay.

 > Check with Jerry Chen about recent (9 August 2010) changes to this file
 (in processAxfrQuery) concerning problems with AXFRs failing because of
 connection problems.
 >
 I don't understand what this comment means.  Please clarify.

 > src/bin/auth/auth_srv.h
 > A header describing the purpose of the class would be useful.
 >
 Done.

 > Remark: all methods (not just ones changed by this update) should have a
 doxygen header.
 >
 Done.

 > setXfrinSession: looks as if the last line of the header comment is
 missing.
 >
 Fixed.

 > src/bin/auth/asio_link.cc
 > The asio_link.h file contains the class definitions for the wrapper
 around ASIO. However, the file asio_link.cc contains additional classes
 (such as TCPClient) that know about the application. I suggest that
 asio_link.cc contain only those classes and functions relating to the
 wrapper interface and that the other classes be moved to a separate file.
 This will make it easier for asio_link to be used elsewhere.
 >
 Yes, I've noted that.  We'll eventually refactor the code so that the
 wrapper implementation is independent from specific applications.  Evan
 seems to do something along with this line in #327.  In any case I think
 it's beyond the scope of this ticket.

 > Header documentation for the helper classes (e.g. what is their purpose
 and the purpose of their methods) should be provided.
 >
 Done.

 I've not touched TCPServer and UDPServer this time if you intended to
 include these because there seems to be an attempt of heavily refactoring
 these classes (#327).  That refactoring ticket would be a better place to
 improve the description.

 One more point: I've renamed AuthSrv::configSession() to
 AuthSrv::getConfigSession() based on our coding guideline.

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