Opened 7 years ago

Closed 7 years ago

#2976 closed enhancement (complete)

Implement DnsClient packet construction

Reported by: tmark Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20130703
Component: ~dhcp-ddns(obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by tmark)

Implement the packet construction interface for the DnsClient?.
which culminates in a toWire format. It must also deconstruct response packets. This should be based on libdns++.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by tmark

  • Description modified (diff)

comment:2 Changed 7 years ago by tmark

  • Component changed from dhcp to dhcp-ddns

comment:3 Changed 7 years ago by marcin

  • Owner set to marcin
  • Status changed from new to assigned

comment:4 follow-up: Changed 7 years ago by jreed

Probably doesn't matter but ... did commit 16156f814b54dffdc0304016aa41b0746c613d3a have an unrelated change (according to commit message)?

comment:5 in reply to: ↑ 4 Changed 7 years ago by marcin

Replying to jreed:

Probably doesn't matter but ... did commit 16156f814b54dffdc0304016aa41b0746c613d3a have an unrelated change (according to commit message)?

Yes, you can say so. I was writing doxygen documentation and I realized that behavior of one of the functions being documented was incorrect. I fixed it as soon as I realized it and committed along with documentation. I could have split it to two commits. Nevertheless, the change is intended.

comment:6 Changed 7 years ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from assigned to reviewing

Proposed ChangeLog entry:

6XX.	[func]		marcin
	b10-dhcp-ddns: Implemented DNS Update message construction.
	(Trac #2796, git abcd)

Please review.

comment:7 Changed 7 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:8 follow-up: Changed 7 years ago by tmark

  • Owner changed from tmark to marcin

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

comment:9 in reply to: ↑ 8 Changed 7 years ago by marcin

  • Owner changed from marcin to tmark

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

comment:10 Changed 7 years ago by tmark

  • Owner changed from tmark to marcin

Changes acceptable. Go ahead and merge.

comment:11 Changed 7 years ago by marcin

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged with commit 0d6a9ba0f5d489d5387ef8f898fcf7d7ad61907c.

Note: See TracTickets for help on using tickets.