BIND 10 #2976: Implement DnsClient packet construction

[BIND 10 Development]

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

 * owner:  tmark => marcin


Comment:

 Review Comments:

 GENERAL Comment:

 I started to ask you why you didn't derive D2UpdateMessage from
 Dns::Message but having looked into Dns:Message I see that it is geared
 specifically to handle
 DNS Query and Answer messages; and it would have to be modified to handle
 DNS
 Update sections.

 For an initial implementation, using composition rather than inheritance
 is
 perhaps alright but I am somewhat concerned about mapping DNS Update
 sections
 to DNS Query/Answer sections.  It may work now, but what if one of those
 sections takes on new, incompatible behavior?

 I think the real issue is that Dns::Message is not generic enough.  It
 needs to
 be more of base class that handles an arbitrary number and type of
 section,
 and let specialized derivations for Query, Answer, Update ... handle the
 specifics.

 The other direction, would be to extend Dns::Message to explicitly support
 the DNS Update sections.

 I think this issue well need to be revisted.

 ----------------------------------------


 d2_update_message.h:


 * Line length, some lines (76, 115, 279, 284...) are longer than 80 chars

 * Line 112,  i/wheteher/whether/

 * I would recommend renaming D2UpdateMessage::validate to
 validateResponse, to
 make it even clearer that it is only used to validate RESPONSE.

 * missing briefs for D2UpdateMessage attributes message_ and zone_

 ----------------------------------

 d2_update_message.cc:


 * Line length again some lines longer than 80

 * D2UpdateMessage(const bool parse = false);
 I think it would be better to pass in dns:Message::PARSE or RENDER, rather
 than a boolean value called parse.  Even clearer would to do define a
 "direction" enum like Inbound and Outbound...

 * line 134 "This class implements exposes the getZone() ..." it
 'implements' or
 'exposes' not both ;)

 * line 144 uses an assert() to assure question is not null.  It should
 test
 for it and throw an exception instead.

 * line 146 and 150 - Extraneous blank lines!

 * line 199, exception text has too many "should"s

 -----------------------------------

 d2_zone.h:

 * Line length again  - some lines longer than 80


 --------------------------------------

 d2_zone_unittests.cc:

 *  Missing doxygen throughout! Tsk Tsk


 --------------------------------------

 d2_update_message_unittests.cc:

 * No @brief for D2UpdateMessageTest
 * Tests should have @briefs not just comments

 * line #64,  why not declare the buffer as "char
 name_data[name_length+no_zero_byte]", then it's always going to be large
 enough

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