BIND 10 #805: Client side code to request new sockets (C++)

[BIND 10 Development]

#805: Client side code to request new sockets (C++)
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20120110
                   Priority:         |            Resolution:
  blocker                            |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  Unclassified                       |  Estimated Difficulty:  5
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
  Socket creator                     |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 More comments on asiodns changes.  I've done for this part.

 I made some suggested changes directly to the branch.

 '''misc small comments on asiodns'''

 - logger.{h,cc}: copyright year should be 2012 as they are new files?

 - tcp_server.cc: isn't it better to use LOG_DEBUG instead of
   logger.debug to avoid unnecessary overhead?  Same for udp_server.

 - tcp_server.cc: is accesptor_.reset() exception free?  If not it's
   safer to include it in the try block.  Same for udp_server.

 - tcp_server.cc: I thought we generally place 'catch' at the same line
   as the closing brace for 'try':
 {{{#!c++
     } catch (const std::exception& exception) {
         // Whatever the thing throws, it is something from ASIO and we
 convert
         // it
         isc_throw(IOError, exception.what());
     }
 }}}
   maybe a matter of taste except for consistency, though.  Same for
   udp_server.cc

 - udp_server.cc: I don't see the need for the temporary variable
   'proto' and simplify the code (just like the TCP counterpart)
 {{{#!c++
 +        try {
 +            socket_->assign(af == AF_INET6 ? udp::v6() : udp::v4(), fd);
 +        }

 }}}

 - I have some suggestions on the doxygen description of
   dns_service.h.  directly committed.

 '''dns_server_unittest.cc'''

 - why do we use `SetUp()` and `commonSetup()`?  Using constructors
   seem to be more natural, and by doing so we could constify some
   member variable(s) (such as server_address_).
 - freeaddrinfo should be removed here:
 {{{
 +        if (error != 0) {
 +            freeaddrinfo(res);
 }}}
 - I'd personally want to make it more exception safe by defining
   'checker_', 'lookup_' etc as scoped pointers.  See also the
   discussion about FD leak below.  But this branch isn't responsible
   for it and we are running out of time, so I'm okay with deferring it
   to a later cleanup (maybe we should create a ticket and get it done
   in the next sprint).
 - FdInit::SetUp() could leak created sockets on exception.  While in
   this case we could say it doesn't matter much because the test
   process would immediately die on the exception anyway, I'd personally
   like to make our code as clean as possible, even for tests.  If we
   leave such loose code I'm afraid it would degrade the entire product
   by giving the impression that we generally code it carelessly.  I'm
   also afraid that such misdemeanor bugs would introduce a larger
   amount of noise if and when we apply static analysis tools to tests,
   thereby obscuring other, more serious bugs.  Finally, leaks in tests
   may introduce other unexpected side effects such as exceeding the
   allowable limit of open files, making some subsequent tests fail.
   Although it would not be a real concern in this particular case, I'd
   rather try to make all code safe than selectively being loose on
   cases where "it should be safe".  I understand brevity is important,
   and it could be possible that sometimes brevity may be more
   important than correctness for tests, so if we needed
   complicated/tricky workaround to make the code "safer", maybe it
   could be justified to ignore the error case (and in that case we'd
   add a false-positive filter or something for static analysis tools).
   But in this specific case I believe making the code safer doesn't
   require a large trick.  We can simply introduce and use a wrapper
   container like the `ScopedSocket` structure I used in
   socketsession.cc.

   That said, considering we are running out of time again, I'm okay
   with deferring this, too.  I thought I proposed making ScopedSocket
   a semi-public utility, so we might defer the cleanup until that
   point.

 - it makes me nervous to define non plain-old static object like this:
 {{{#!c++
 oasio::io_service DNSServerTest::service;
 }}}
   If this really needs to be (class) static, I'd define it as a
   pointer and new it only when it's NULL in the constructor.
 {{{#!c++
         static asio::io_service* service;
         DNSServerTest() {
             if (service == NULL) {
                 service = new asio::io_service;
             }
         }
 }}}

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