BIND 10 #383: Generalize IOService constructor

BIND 10 Development do-not-reply at isc.org
Fri Oct 22 07:51:36 UTC 2010


#383: Generalize IOService constructor
------------------------------+---------------------------------------------
      Reporter:  jelte        |        Owner:  each     
          Type:  enhancement  |       Status:  reviewing
      Priority:  major        |    Milestone:           
     Component:  recurser     |   Resolution:           
      Keywords:               |    Sensitive:  0        
Estimatedhours:  0.0          |        Hours:  0        
      Billable:  1            |   Totalhours:  0        
      Internal:  0            |  
------------------------------+---------------------------------------------
Changes (by jelte):

  * owner:  jelte => each
  * status:  new => reviewing


Comment:

 Replying to [comment:3 each]:
 >
 > Review notes:
 > {{{
 > +    /// These are currently very specific to the authoritative server
 > +    /// implementation.
 > }}}
 >
 > I hadn't realized this comment was still there; it's not correct
 anymore.
 >

 Removed. While I was at it, I also added \param documentation in
 asio_link.h

 > Not sure why DNSService returns a native asio::io_service instead of an
 IOService, but it's fine with me either way.  (If it changes, though, it
 ought to be renamed getIOService(), both for clarity and for style
 compliance.  I assume IOService::get_io_service() was named the way it was
 because proper capitalization would be misleading about its return type;
 anyway, that's why I left it that way.)
 >
 > asiolink_unittest.cc:
 > {{{
 > +        if (dns_service_ != NULL) {
 > +            delete dns_service_;
 >          }
 > }}}
 >

 Hmm. At this moment I think the only reason this function is used is to
 get to the native one in the end, which is why right now I made it pass
 that through directly. I'm inclined to leave it that way for now. Should
 be a trivial change if anyone disagrees.

 > Stephen mentioned in his review of #327 that checking for null around a
 delete isn't necessary; I forgot to fix that.
 >

 doh, I knew that. Still some C habits shining through :)

 > main.cc:
 > {{{
 > +        io_service = new IOService();
 > }}}
 >
 > io_service could be allocated on the stack now.  The only reason it
 wasn't before was that it needed to be instantiated down inside the if
 statement.
 >

 Ack. Still needs to be global though, unfortunately.

 > recursor_unittest.cc:
 > Changes are fine, but will also need to be made in auth_unittest.cc; I
 don't see that in the diff.

 IOService isn't used directly in those tests, so there wasn't much to
 change...

 Review comments addressed in r3312

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


More information about the bind10-tickets mailing list