BIND 10 trac2976, updated. 7f644c2532ecf0f3dab826a091fbe900dc5352f4 [2976] Changes as a result of code review.
BIND 10 source code commits
bind10-changes at lists.isc.org
Thu Jun 27 11:22:32 UTC 2013
The branch, trac2976 has been updated
via 7f644c2532ecf0f3dab826a091fbe900dc5352f4 (commit)
from a81a52389a3ce477db992da27cf09cb1adc0e49e (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 7f644c2532ecf0f3dab826a091fbe900dc5352f4
Author: Marcin Siodelski <marcin at isc.org>
Date: Thu Jun 27 13:22:18 2013 +0200
[2976] Changes as a result of code review.
-----------------------------------------------------------------------
Summary of changes:
src/bin/d2/d2_update_message.cc | 31 ++++++++++---------
src/bin/d2/d2_update_message.h | 37 ++++++++++++++++++-----
src/bin/d2/tests/d2_update_message_unittests.cc | 35 +++++++++++----------
src/bin/d2/tests/d2_zone_unittests.cc | 14 +++++++--
4 files changed, 77 insertions(+), 40 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/d2/d2_update_message.cc b/src/bin/d2/d2_update_message.cc
index 1ee6543..71fb9f3 100644
--- a/src/bin/d2/d2_update_message.cc
+++ b/src/bin/d2/d2_update_message.cc
@@ -23,11 +23,12 @@ namespace d2 {
using namespace isc::dns;
-D2UpdateMessage::D2UpdateMessage(const bool parse)
- : message_(parse ? dns::Message::PARSE : dns::Message::RENDER) {
+D2UpdateMessage::D2UpdateMessage(const Direction direction)
+ : message_(direction == INBOUND ?
+ dns::Message::PARSE : dns::Message::RENDER) {
// If this object is to create an outgoing message, we have to
// set the proper Opcode field and QR flag here.
- if (!parse) {
+ if (direction == OUTBOUND) {
message_.setOpcode(Opcode(Opcode::UPDATE_CODE));
message_.setHeaderFlag(dns::Message::HEADERFLAG_QR, false);
@@ -131,16 +132,18 @@ D2UpdateMessage::fromWire(isc::util::InputBuffer& buffer) {
// not be the message that we are interested in, but needs to be
// parsed so as we can check its ID, Opcode etc.
message_.fromWire(buffer);
- // This class implements exposes the getZone() function. This function
- // will return pointer to the D2Zone object if non-empty Zone
- // section exists in the received message. It will return NULL pointer
- // if it doesn't exist. The pointer is held in the D2UpdateMessage class
- // member. We need to update this pointer every time we parse the
- // message.
+ // This class exposes the getZone() function. This function will return
+ // pointer to the D2Zone object if non-empty Zone section exists in the
+ // received message. It will return NULL pointer if it doesn't exist.
+ // The pointer is held in the D2UpdateMessage class member. We need to
+ // update this pointer every time we parse the message.
if (getRRCount(D2UpdateMessage::SECTION_ZONE) > 0) {
// There is a Zone section in the received message. Replace
// Zone pointer with the new value.
QuestionPtr question = *message_.beginQuestion();
+ // If the Zone counter is greater than 0 (which we have checked)
+ // there must be a valid Question pointer stored in the message_
+ // object. If there isn't, it is a programming error.
assert(question);
zone_.reset(new D2Zone(question->getName(), question->getClass()));
@@ -155,7 +158,7 @@ D2UpdateMessage::fromWire(isc::util::InputBuffer& buffer) {
// or an error message can be printed. Other than that, we
// will check that there is at most one Zone record and QR flag
// is set.
- validate();
+ validateResponse();
}
dns::Message::Section
@@ -184,7 +187,7 @@ D2UpdateMessage::ddnsToDnsSection(const UpdateMsgSection section) {
}
void
-D2UpdateMessage::validate() const {
+D2UpdateMessage::validateResponse() const {
// Verify that we are dealing with the DNS Update message. According to
// RFC 2136, section 3.8 server will copy the Opcode from the query.
// If we are dealing with a different type of message, we may simply
@@ -198,9 +201,9 @@ D2UpdateMessage::validate() const {
// Received message should have QR flag set, which indicates that it is
// a RESPONSE.
if (getQRFlag() == REQUEST) {
- isc_throw(InvalidQRFlag, "received message should should have QR flag"
- << " set, to indicate that it is a RESPONSE message, the QR"
- << " flag is unset");
+ isc_throw(InvalidQRFlag, "received message should have QR flag set,"
+ " to indicate that it is a RESPONSE message; the QR"
+ << " flag in received message is unset");
}
// DNS server may copy a Zone record from the query message. Since query
// must comprise exactly one Zone record (RFC 2136, section 2.3), the
diff --git a/src/bin/d2/d2_update_message.h b/src/bin/d2/d2_update_message.h
index 44d0cd4..98e98a8 100644
--- a/src/bin/d2/d2_update_message.h
+++ b/src/bin/d2/d2_update_message.h
@@ -81,14 +81,23 @@ public:
class D2UpdateMessage {
public:
- /// Indicates whether DNS Update message is a REQUEST or RESPONSE.
+ /// @brief Indicates if the @c D2UpdateMessage object encapsulates Inbound
+ /// or Outbound message.
+ enum Direction {
+ INBOUND,
+ OUTBOUND
+ };
+
+ /// @brief Indicates whether DNS Update message is a REQUEST or RESPONSE.
enum QRFlag {
REQUEST,
RESPONSE
};
- /// Identifies sections in the DNS Update Message. Each message comprises
- /// message Header and may contain the following sections:
+ /// @brief Identifies sections in the DNS Update Message.
+ ///
+ /// Each message comprises message Header and may contain the following
+ /// sections:
/// - ZONE
/// - PREREQUISITE
/// - UPDATE
@@ -116,9 +125,8 @@ public:
/// message contents and @c D2UpdateMessage::toWire function to create
/// on-wire data.
///
- /// @param parse indicates if this is an incoming message (true) or outgoing
- /// message (false).
- D2UpdateMessage(const bool parse = false);
+ /// @param direction indicates if this is an inbound or outbound message.
+ D2UpdateMessage(const Direction direction = OUTBOUND);
///
/// @name Copy constructor and assignment operator
@@ -303,9 +311,22 @@ private:
/// @throw isc::d2::InvalidQRFlag if QR flag is not set to RESPONSE
/// @throw isc::d2::InvalidZone section, if Zone section comprises more
/// than one record.
- void validate() const;
-
+ void validateResponse() const;
+
+ /// @brief An object representing DNS Message which is used by the
+ /// implementation of @c D2UpdateMessage to perform low level.
+ ///
+ /// Declaration of this object pollutes the header with the details
+ /// of @c D2UpdateMessage implementation. It might be cleaner to use
+ /// Pimpl idiom to hide this object in an D2UpdateMessageImpl. However,
+ /// it would bring additional complications to the implementation
+ /// while the benefit would low - this header is not a part of any
+ /// common library. Therefore, if implementation is changed, modification of
+ /// private members of this class in the header has low impact.
dns::Message message_;
+
+ /// @brief Holds a pointer to the object, representing Zone in the DNS
+ /// Update.
D2ZonePtr zone_;
};
diff --git a/src/bin/d2/tests/d2_update_message_unittests.cc b/src/bin/d2/tests/d2_update_message_unittests.cc
index e83653e..28e01c7 100644
--- a/src/bin/d2/tests/d2_update_message_unittests.cc
+++ b/src/bin/d2/tests/d2_update_message_unittests.cc
@@ -33,13 +33,18 @@ using namespace isc::util;
namespace {
+// @brief Test fixture class for testing D2UpdateMessage object.
class D2UpdateMessageTest : public ::testing::Test {
public:
- D2UpdateMessageTest() {
- }
+ // @brief Constructor.
+ //
+ // Does nothing.
+ D2UpdateMessageTest() { }
- ~D2UpdateMessageTest() {
- };
+ // @brief Destructor.
+ //
+ // Does nothing.
+ ~D2UpdateMessageTest() { };
// @brief Return string representation of the name encoded in wire format.
//
@@ -58,18 +63,16 @@ public:
//
// @return string representation of the name.
std::string readNameFromWire(InputBuffer& buf, size_t name_length,
- bool no_zero_byte = false) {
- // 64 characters bytes should be sufficent for current tests.
- // It may be extended if required.
- char name_data[64];
+ const bool no_zero_byte = false) {
+ std::vector<uint8_t> name_data;
// Create another InputBuffer which holds only the name in the wire
// format.
- buf.readData(name_data, name_length);
+ buf.readVector(name_data, name_length);
if (no_zero_byte) {
++name_length;
- name_data[name_length-1] = 0;
+ name_data.push_back(0);
}
- InputBuffer name_buf(name_data, name_length);
+ InputBuffer name_buf(&name_data[0], name_length);
// Parse the name and return its textual representation.
Name name(name_buf);
return (name.toText());
@@ -201,7 +204,7 @@ TEST_F(D2UpdateMessageTest, fromWire) {
InputBuffer buf(bin_msg, sizeof(bin_msg));
// Create an object to be used to decode the message from the wire format.
- D2UpdateMessage msg(true);
+ D2UpdateMessage msg(D2UpdateMessage::INBOUND);
// Decode the message.
ASSERT_NO_THROW(msg.fromWire(buf));
@@ -288,7 +291,7 @@ TEST_F(D2UpdateMessageTest, fromWireInvalidOpcode) {
// The 'true' argument passed to the constructor turns the
// message into the parse mode in which the fromWire function
// can be used to decode the binary mesasage data.
- D2UpdateMessage msg(true);
+ D2UpdateMessage msg(D2UpdateMessage::INBOUND);
// When using invalid Opcode, the fromWire function should
// throw NotUpdateMessage exception.
EXPECT_THROW(msg.fromWire(buf), isc::d2::NotUpdateMessage);
@@ -312,7 +315,7 @@ TEST_F(D2UpdateMessageTest, fromWireInvalidQRFlag) {
// The 'true' argument passed to the constructor turns the
// message into the parse mode in which the fromWire function
// can be used to decode the binary mesasage data.
- D2UpdateMessage msg(true);
+ D2UpdateMessage msg(D2UpdateMessage::INBOUND);
// When using invalid QR flag, the fromWire function should
// throw InvalidQRFlag exception.
EXPECT_THROW(msg.fromWire(buf), isc::d2::InvalidQRFlag);
@@ -351,7 +354,7 @@ TEST_F(D2UpdateMessageTest, fromWireTooManyZones) {
// The 'true' argument passed to the constructor turns the
// message into the parse mode in which the fromWire function
// can be used to decode the binary mesasage data.
- D2UpdateMessage msg(true);
+ D2UpdateMessage msg(D2UpdateMessage::INBOUND);
// When parsing a message with more than one Zone record,
// exception should be thrown.
EXPECT_THROW(msg.fromWire(buf), isc::d2::InvalidZoneSection);
@@ -572,7 +575,7 @@ TEST_F(D2UpdateMessageTest, toWireInvalidQRFlag) {
// The 'true' argument passed to the constructor turns the
// message into the parse mode in which the fromWire function
// can be used to decode the binary mesasage data.
- D2UpdateMessage msg(true);
+ D2UpdateMessage msg(D2UpdateMessage::INBOUND);
ASSERT_NO_THROW(msg.fromWire(buf));
// The message is parsed. The QR Flag should now indicate that
diff --git a/src/bin/d2/tests/d2_zone_unittests.cc b/src/bin/d2/tests/d2_zone_unittests.cc
index 8a7e9d5..853cdbe 100644
--- a/src/bin/d2/tests/d2_zone_unittests.cc
+++ b/src/bin/d2/tests/d2_zone_unittests.cc
@@ -23,24 +23,34 @@ using namespace isc::dns;
namespace {
+// This test verifies that Zone object is created and its constructor sets
+// appropriate values for its members.
TEST(D2ZoneTest, constructor) {
+ // Create first object.
D2Zone zone1(Name("example.com"), RRClass::ANY());
EXPECT_EQ("example.com.", zone1.getName().toText());
EXPECT_EQ(RRClass::ANY().getCode(), zone1.getClass().getCode());
-
+ // Create another object to make sure that constructor doesn't assign
+ // fixed values, but they change when constructor's parameters change.
D2Zone zone2(Name("foo.example.com"), RRClass::IN());
EXPECT_EQ("foo.example.com.", zone2.getName().toText());
EXPECT_EQ(RRClass::IN().getCode(), zone2.getClass().getCode());
}
+// This test verifies that toText() function returns text representation of
+// of the zone in expected format.
TEST(D2ZoneTest, toText) {
+ // Create first object.
D2Zone zone1(Name("example.com"), RRClass::ANY());
EXPECT_EQ("example.com. ANY SOA\n", zone1.toText());
-
+ // Create another object with different parameters to make sure that the
+ // function's output changes accordingly.
D2Zone zone2(Name("foo.example.com"), RRClass::IN());
EXPECT_EQ("foo.example.com. IN SOA\n", zone2.toText());
}
+// This test verifies that the equality and inequality operators behave as
+// expected.
TEST(D2ZoneTest, compare) {
const Name a("a"), b("b");
const RRClass in(RRClass::IN()), any(RRClass::ANY());
More information about the bind10-changes
mailing list