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

[BIND 10 Development]

#221: Refactoring auth server and merge the axfr and notify logic into it
-------------------------+--------------------------------------------------
 Reporter:  hanfeng      |        Owner:  jinmei                                        
     Type:  enhancement  |       Status:  reviewing                                     
 Priority:  major        |    Milestone:  05. 3rd Incremental Release: Serious Secondary
Component:  b10-auth     |   Resolution:                                                
 Keywords:               |    Sensitive:  0                                             
-------------------------+--------------------------------------------------

Comment(by jinmei):

 Additional comments on the latest version of processNotify() and
 processAxfrQuery().

  - general
    - Style issues still remain.  I've directly fixed them and committed
 the fix to the branch.
    - please don't make notifyin_fromwire by naively copy other data files
 (btw the comment is wrong).  they are expected to be generated from a
 .spec file using lib/dns/tests/testdata/gen-wiredata.py.

  - processMessage
    - update the comment: "we only support normal queries", which is not
 true any more.

  - processAxfrQuery
    - when axfr_client_.connect() fails I suspect the auth server will
 receive an ASIO exception (and kill the server as a result).  It should be
 hidden within the !XfroutClient module
    - why returning SERVFAIL for AXFR/UDP?  does RFC say so?
    - parentheses after return: please make the style consistent.
    - I suspect it shouldn't return a response if the axfr request is
 successfully forwarded.
    - variable name "is_xfrin_connection_established_" is confusing because
 we use the term "xfrin" for the opposite direction.

  - processNotify
    - many of my previous comments still hold.
    - if the session cannot be established it should return an error
 message.
    - at least according to the doc Element::createFromString() could
 return NULL.  If that's the case it should be handled accordingly. (see
 Trac #250)
    - shouldn't it handle the case where "rcode" indicates an error?
    - why do we need these two catch clauses?  Although the error messages
 are slightly different, I don't see much benefit in this case to separate
 these two (note that we could show where the exception is triggered from
 isc::Exception).  On the other hand, since we cannot be sure what kind of
 underlying modules are necessary in the try clause, just catching these
 two specific exception classes may be too restricted.  If I were to
 implement it, I'd catch all (isc::)Exception objects (see also
 processNormalQuery).
    - when the error happens, return SERVFAIL, rather than ignoring it.

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