BIND 10 #2976: Implement DnsClient packet construction

[BIND 10 Development]

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

 * owner:  marcin => tmark


Comment:

 Replying to [comment:8 tmark]:
 > 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.

 I spent a while thinking what the right direction should be. Extending the
 isc::dns::Message class could be cleaner but would be very involved.
 Mainly because, it explicitly calls sections as they are defined for DNS
 messages other than DNS Update: Answer, Question etc. Also, there are
 certain constraints that apply to DNS Updates, which do not apply to other
 DNS messages. For this reason, I thought that having a separate class
 would be easier and faster to implement. The idea was to have a clear
 separation of the interface from its implementation. Having this separated
 we can always replace an implementation with something independent from
 isc::dns::Message. I agree that relying on the Message class brings a risk
 that change in an underlying implementation affects D2 class. On the other
 hand, Message class is implemented based on RFCs and there is no room for
 changes there. I also can't really imagine that the Message class is
 modified in such a way that it becomes incompatible with D2UpdateMessage
 class because both message formats are defined in RFCs such, that they can
 be processed in the same way.

 >
 > ----------------------------------------
 >
 >
 > d2_update_message.h:
 >
 >
 > * Line length, some lines (76, 115, 279, 284...) are longer than 80
 chars

 I folded all long lines.

 >
 > * Line 112,  i/wheteher/whether/

 Corrected.

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

 Renamed as suggested

 >
 > * missing briefs for D2UpdateMessage attributes message_ and zone_

 Added doxygen comments. Private class members are not included in the
 doxygen documentation but some comments may be useful for people reading
 the header.

 >
 > ----------------------------------
 >
 > d2_update_message.cc:
 >
 >
 > * Line length again some lines longer than 80

 Folded.

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

 I changed the constructor as suggested.

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

 Corrected.

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

 I am not sure. Although it could throw an exception, an occurrence of a
 NULL pointer is a programming error. I like protecting the code against
 programming errors with asserts because it will output useful information
 where the particular error occurred.

 >
 > * line 146 and 150 - Extraneous blank lines!

 They are left intentionally to make the code more readable - separate if
 from else section.

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

 Removed extra shoulds.

 >
 > -----------------------------------
 >
 > d2_zone.h:
 >
 > * Line length again  - some lines longer than 80

 Folded.

 >
 >
 > --------------------------------------
 >
 > d2_zone_unittests.cc:
 >
 > *  Missing doxygen throughout! Tsk Tsk

 Added comments to tests.

 >
 >
 > --------------------------------------
 >
 > d2_update_message_unittests.cc:
 >
 > * No @brief for D2UpdateMessageTest
 > * Tests should have @briefs not just comments

 IMHO, they don't have to. Please note that doxygen documentation is not
 created for unit tests, because they belong to anonymous namespace. In
 other files, we used to add comments for test cases, but they were never
 in the form of doxygen.

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

 I the past, I saw warnings and errors generated by various compilers when
 using variadic arrays. Therefore, I try to avoid them. In order to make it
 a bit cleaner, I replaced the array with an STL vector.

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