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

 Replying to [comment:14 hanfeng]:

 First off, don't "fix" the points before tests, please.  By giving early
 feedback, I expected you to add test cases to reproduce the issues I
 pointed out (or confirm they are not an issue), then fix them, and then
 confirm they are fixed using the tests.

 > processNotify
 >           o at least according to the doc Element::createFromString()
 could return NULL. If that's the case it should be handled accordingly.
 (see Trac #250)
 >           after checking the code I found it will raise exception when
 meet error

 Yes, Jelte clarified that in Trac #250.  But the more important point is
 that we should be aware of return values of any method we use or the
 possibility of exception being thrown *at the time we wrote the code*.  Of
 course we sometimes overlook such possibilities and we don't have to be
 100% correct at that point (and that's actually one of the reasons why we
 need code review), but when I saw the initial code it made me nervous
 because so many error cases seemed to be just ignored or very roughly
 handled (e.g. by using a catch-all catch, which is itself bad, and even it
 doesn't help if the error is reported by a return value).

 So, while you can count on reviewers pointing out overlooked failure mode,
 please be a bit more careful about such cases *before asking for review*.

 >           o shouldn't it handle the case where "rcode" indicates an
 error?
 >           the parseAnswer function is quite strange, it raise exception,
 then I don't understand the rcode value is used for what:(

 I'm not sure what you mean here, but at least according to the function
 description the rcode can be non 0, indicating an error, in which case
 parseAnswer() returns an error message.  We should handle that case
 explicitly (or if we are confident it's impossible in this context, we
 should assert() that).

 But again, don't fix it *now*.  Add tests first, please:-)

 >           o when the error happens, return SERVFAIL, rather than
 ignoring it.
 >           according to RFC 1996, the master only care three things, one
 is no reply received, and the master will send notify later, the second is
 with NOTIMP, it's same with master received NOERROR reply, master willn't
 send the notify to the slave again. So if error happens, we just ignore
 it, when wait for next try

 Ah, okay.  Could you leave code comments on this?  I think this is a non
 trivial behavior.

 > For asio_link wrap,
 > 1 if we wrap the asio lib, all the other part in BIND10 using asio
 should through the asio wrapper, maybe this should be put into another
 ticket.

 Yes, that's the intent.  I could have tried that level of overhaul
 refactoring, but didn't do so because we've already done too many things
 in this branch.

 > 2 For class !ServerSet, it's functionality can be replace by using
 auto_ptr which from STD, and also can make sure if exception occured, the
 memory will be freed during the construction call.

 Ah, yes, good suggestion.  I've modified the code so that it uses
 boost::shared_ptr instead of the helper class (and confirmed that it
 didn't break anything trhough tests).  We could use auto_ptr in this case
 as you said, but I simply followed our common practice with shared_ptr
 since auto_ptr has so many pitfalls to be used safely.  We've already
 heavily relied on shared_ptr's and the usage in this case isn't leaked via
 a public header file, so I think the additional dependency (on boost) is
 acceptable.

 Besides, we'll eventually be free from creating UDP/TCPServer classes
 within the asio_link module once we generalize it.  That job will be
 shifted to the user module.  So this part will likely be revised anyway.

 Are other part of the patch okay, or will more comments come?  (I normally
 expect this size of patch would have tens of comments including overlooked
 bugs:-)

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