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

 Hello

 Replying to [comment:13 jinmei]:
 > I made some suggested changes directly to the branch.

 Thanks for them.

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

 Ah, it's a new year already, I didn't get used to it yet :-|.

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

 The reset itself would be exception safe (because the pointer is null, so
 it calls no delete and the only code inside would be assignment to a
 pointer).

 But thinking about it, who knows what the constructor of the socket might
 throw, so I included it inside the block.

 > - tcp_server.cc: I thought we generally place 'catch' at the same line
 >   as the closing brace for 'try':

 Yes, I just forget about it, I'm used to a different style.

 > - 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_).

 Changed, with the exception of SetUp of the FdInit, because that one uses
 the ASSERT_* macros and they contain return.

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

 As the destructor calls delete on those, it is exception safe. But the
 scoped pointers could make it shorter (no need for the deletes in the
 destructor). Anyway, I'd postpone it for some future cleanup ticket. Would
 you like to create it?

 > - FdInit::SetUp() could leak created sockets on exception.  While in
 >   [...]
 >
 >   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.

 I do agree that we should try to have the cleanest code possible.

 Is there a ticket for this proposal? So we could add a note about it
 there? Or should I put some try-catch(...) block around it, as a short-
 term workaround?

 > - 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.

 I didn't really create the variable, and I believe this usage should be
 safe, but I did something with it nevertheless. Instead of reusing the
 same service, the service is now member of the test object (so each test
 gets fresh one) and the static one is only a pointer to the active one.

 Thanks

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