Commit 7f644c25 authored by Marcin Siodelski's avatar Marcin Siodelski
Browse files

[2976] Changes as a result of code review.

parent a81a5238
......@@ -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
......
......@@ -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_;
};
......
......@@ -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
......
......@@ -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());
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment