BIND 10 #2977: Implement DnsClient Packet I/O.

[BIND 10 Development]

#2977: Implement DnsClient Packet I/O.
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130717
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:  2975
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tmark


Comment:

 Replying to [comment:10 tmark]:
 > Review Comments:
 >
 > * I think launch test mode parameter should default to false.

 I just thought that it may be useful if you force to set it explicitly
 when creating new unit tests. That way, test implementer will have to use
 the function consciously. Otherwise, when implementing a new test, I could
 omit this parameter causing unexpected behaviour. The problem with this
 function is, that it is not obvious that this behaviour is unexpected
 until you realize that logging is not working properly, which is something
 people don't pay much attention to.

 >
 > * dns_client.cc  - Comment states we aren't implementing TCP because it
 would require a bunch of new tests:
 > {{{
 > "
 > // @todo At some point we may need to implement TCP. It should be
 straight
 > // forward but would require a bunch of new unit tests. That's why we
 > // currently disable TCP. Once implemented the check below should be
 > "
 > }}}

 I think you right. That sounds better. However, in fact the implementation
 should be mostly restricted to unit tests anyway. But, I did what you
 suggested.

 >
 > Actually, we aren't doing it yet because we are doing a phased
 implementation.  I think it would be better to say "TCP support will be
 provided in a future release."
 >
 > * is it typical in PIMPL for the impl class members to be public?

 I saw it is common practice in the BIND10 code. Since, this is
 implementation detail it is probably ok to do it.

 >
 > * should DNSClient::impl_ be a smart pointer?

 It could. I used raw pointer because it is used in a very simple way:
 create a pointer in constructor and destroy in the destructor. We don't
 expose this pointer anywhere else so there should be no risk involved
 here. On the other hand, smart pointers do the reference counting which
 may be considered unnecessary overhead in this particular case.

 >
 > * you declare the copy and assignment operators private because
 DNSClient is a singleton.  It is not a singleton.   It has a public
 constructor which allows me to make any number of instances.  The real
 reason to hide them, is that the default implementations would make a copy
 of the DNSClient::impl_ which is a pointer.
 > Either the reason we can't copy is wrong in the comment, or you need to
 make it a singleton.  I would prefer the former, I don't see a reason for
 it to be a singleton.

 Yes. Sorry the comment is a reminder from some early class development
 stage when I copied things over from the other file.

 >
 >
 > * dns_client_unittests.cc  line 120 is too long

 it was just two characters too long ;-) But ok, I fixed it .

 >
 > * dns_client_unittests.cc "allowed. This why we will"  change "why" to
 "way"
 >

 Fixed.

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