BIND 10 trac3082, updated. 33edd2ed3d657798c02a75f236c087527f8137ad [3082] Addressed comments from the second review.
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Aug 16 07:33:17 UTC 2013
The branch, trac3082 has been updated
via 33edd2ed3d657798c02a75f236c087527f8137ad (commit)
from da34264f7277725b59764e73be597ee7cc513a90 (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 33edd2ed3d657798c02a75f236c087527f8137ad
Author: Marcin Siodelski <marcin at isc.org>
Date: Fri Aug 16 09:32:49 2013 +0200
[3082] Addressed comments from the second review.
Also, removed line wraps in toText function and added validation of the
flags field when calling unpack.
-----------------------------------------------------------------------
Summary of changes:
src/lib/dhcp/option4_client_fqdn.cc | 56 +++++++++++---------
src/lib/dhcp/option4_client_fqdn.h | 17 +++++-
src/lib/dhcp/tests/option4_client_fqdn_unittest.cc | 18 ++-----
3 files changed, 51 insertions(+), 40 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/dhcp/option4_client_fqdn.cc b/src/lib/dhcp/option4_client_fqdn.cc
index 4e28a5d..7aa0e81 100644
--- a/src/lib/dhcp/option4_client_fqdn.cc
+++ b/src/lib/dhcp/option4_client_fqdn.cc
@@ -37,7 +37,7 @@ class Option4ClientFqdnImpl {
public:
/// Holds flags carried by the option.
uint8_t flags_;
- // Holds RCODE1 and RCODE2 values.
+ /// Holds RCODE1 and RCODE2 values.
Option4ClientFqdn::Rcode rcode1_;
Option4ClientFqdn::Rcode rcode2_;
/// Holds the pointer to a domain name carried in the option.
@@ -48,11 +48,12 @@ public:
/// @brief Constructor, from domain name.
///
/// @param flags A value of the flags option field.
+ /// @param rcode An object representing the RCODE1 and RCODE2 values.
/// @param domain_name A domain name carried by the option given in the
/// textual format.
- /// @param domain_name_type A value which indicates whether domain-name
- /// is partial of fully qualified.
- Option4ClientFqdnImpl(const uint8_t flag,
+ /// @param name_type A value which indicates whether domain-name is partial
+ /// or fully qualified.
+ Option4ClientFqdnImpl(const uint8_t flags,
const Option4ClientFqdn::Rcode& rcode,
const std::string& domain_name,
const Option4ClientFqdn::DomainNameType name_type);
@@ -95,7 +96,7 @@ public:
/// to false when validating flags in the received message. This is because
/// server should ignore MBZ bits in received messages.
/// @throw InvalidOption6FqdnFlags if flags are invalid.
- static void checkFlags(const uint8_t flags);
+ static void checkFlags(const uint8_t flags, const bool check_mbz);
/// @brief Parse the Option provided in the wire format.
///
@@ -123,18 +124,19 @@ public:
};
Option4ClientFqdnImpl::
-Option4ClientFqdnImpl(const uint8_t flag,
+Option4ClientFqdnImpl(const uint8_t flags,
const Option4ClientFqdn::Rcode& rcode,
const std::string& domain_name,
const Option4ClientFqdn::DomainNameType name_type)
- : flags_(flag),
+ : flags_(flags),
rcode1_(rcode),
rcode2_(rcode),
domain_name_(),
domain_name_type_(name_type) {
- // Check if flags are correct.
- checkFlags(flags_);
+ // Check if flags are correct. Also, check that MBZ bits are not set. If
+ // they are, throw exception.
+ checkFlags(flags_, true);
// Set domain name. It may throw an exception if domain name has wrong
// format.
setDomainName(domain_name, name_type);
@@ -145,8 +147,10 @@ Option4ClientFqdnImpl::Option4ClientFqdnImpl(OptionBufferConstIter first,
: rcode1_(Option4ClientFqdn::RCODE_CLIENT()),
rcode2_(Option4ClientFqdn::RCODE_CLIENT()) {
parseWireData(first, last);
- // Verify that flags value was correct.
- checkFlags(flags_);
+ // Verify that flags value was correct. This constructor is used to parse
+ // incoming packet, so don't check MBZ bits. They are ignored because we
+ // don't want to discard the whole option because MBZ bits are set.
+ checkFlags(flags_, false);
}
Option4ClientFqdnImpl::
@@ -216,9 +220,9 @@ setDomainName(const std::string& domain_name,
}
void
-Option4ClientFqdnImpl::checkFlags(const uint8_t flags) {
+Option4ClientFqdnImpl::checkFlags(const uint8_t flags, const bool check_mbz) {
// The Must Be Zero (MBZ) bits must not be set.
- if ((flags & ~Option4ClientFqdn::FLAG_MASK) != 0) {
+ if (check_mbz && ((flags & ~Option4ClientFqdn::FLAG_MASK) != 0)) {
isc_throw(InvalidOption4FqdnFlags,
"invalid DHCPv4 Client FQDN Option flags: 0x"
<< std::hex << static_cast<int>(flags) << std::dec);
@@ -383,8 +387,9 @@ Option4ClientFqdn::setFlag(const uint8_t flag, const bool set_flag) {
new_flag &= ~flag;
}
- // Check new flags. If they are valid, apply them.
- Option4ClientFqdnImpl::checkFlags(new_flag);
+ // Check new flags. If they are valid, apply them. Also, check that MBZ
+ // bits are not set.
+ Option4ClientFqdnImpl::checkFlags(new_flag, true);
impl_->flags_ = new_flag;
}
@@ -465,22 +470,25 @@ Option4ClientFqdn::unpack(OptionBufferConstIter first,
OptionBufferConstIter last) {
setData(first, last);
impl_->parseWireData(first, last);
+ // Check that the flags in the received option are valid. Ignore MBZ bits,
+ // because we don't want to discard the whole option because of MBZ bits
+ // being set.
+ impl_->checkFlags(impl_->flags_, false);
}
std::string
Option4ClientFqdn::toText(int indent) {
std::ostringstream stream;
std::string in(indent, ' '); // base indentation
- std::string in_add(2, ' '); // second-level indentation is 2 spaces long
- stream << in << "type=" << type_ << "(CLIENT_FQDN)" << std::endl
- << in << "flags:" << std::endl
- << in << in_add << "N=" << (getFlag(FLAG_N) ? "1" : "0") << std::endl
- << in << in_add << "E=" << (getFlag(FLAG_E) ? "1" : "0") << std::endl
- << in << in_add << "O=" << (getFlag(FLAG_O) ? "1" : "0") << std::endl
- << in << in_add << "S=" << (getFlag(FLAG_S) ? "1" : "0") << std::endl
- << in << "domain-name='" << getDomainName() << "' ("
+ stream << in << "type=" << type_ << " (CLIENT_FQDN), "
+ << "flags: ("
+ << "N=" << (getFlag(FLAG_N) ? "1" : "0") << ", "
+ << "E=" << (getFlag(FLAG_E) ? "1" : "0") << ", "
+ << "O=" << (getFlag(FLAG_O) ? "1" : "0") << ", "
+ << "S=" << (getFlag(FLAG_S) ? "1" : "0") << "), "
+ << "domain-name='" << getDomainName() << "' ("
<< (getDomainNameType() == PARTIAL ? "partial" : "full")
- << ")" << std::endl;
+ << ")";
return (stream.str());
}
diff --git a/src/lib/dhcp/option4_client_fqdn.h b/src/lib/dhcp/option4_client_fqdn.h
index 3c89117..fc0fb66 100644
--- a/src/lib/dhcp/option4_client_fqdn.h
+++ b/src/lib/dhcp/option4_client_fqdn.h
@@ -167,13 +167,26 @@ public:
/// This constructor is used to create an instance of the option which will
/// be included in outgoing messages.
///
+ /// Note that the RCODE values are encapsulated by the Rcode object (not a
+ /// simple uint8_t value). This helps to prevent a caller from confusing the
+ /// flags value with rcode value (both are uint8_t values). For example:
+ /// if caller swaps the two, it will be detected in the compilation time.
+ /// Also, this API encourages the caller to use two predefined functions:
+ /// @c RCODE_SERVER and @c RCODE_CLIENT to set the value of RCODE. These
+ /// functions generate objects which represent the only valid values to be
+ /// be passed to the constructor (255 and 0 respectively). Other
+ /// values should not be used. However, it is still possible that the other
+ /// entity (client or server) sends the option with invalid value. Although,
+ /// the RCODE values are ignored, there should be a way to represent such
+ /// invalid RCODE value. The Rcode class is capable of representing it.
+ ///
/// @param flags a combination of flags to be stored in flags field.
/// @param rcode @c Rcode object representing a value for RCODE1 and RCODE2
/// fields of the option. Both fields are assigned the same value
/// encapsulated by the parameter.
/// @param domain_name a name to be stored in the domain-name field.
- /// @param partial_domain_name indicates if the domain name is partial
- /// (if true) or full (false).
+ /// @param domain_name_type indicates if the domain name is partial
+ /// or full.
/// @throw InvalidOption4FqdnFlags if value of the flags field is wrong.
/// @throw InvalidOption4FqdnDomainName if the domain-name is invalid.
explicit Option4ClientFqdn(const uint8_t flags,
diff --git a/src/lib/dhcp/tests/option4_client_fqdn_unittest.cc b/src/lib/dhcp/tests/option4_client_fqdn_unittest.cc
index 1412f90..6de08ab 100644
--- a/src/lib/dhcp/tests/option4_client_fqdn_unittest.cc
+++ b/src/lib/dhcp/tests/option4_client_fqdn_unittest.cc
@@ -866,13 +866,8 @@ TEST(Option4ClientFqdnTest, toText) {
// The base indentation of the option will be set to 2. It should appear
// as follows.
std::string ref_string =
- " type=81(CLIENT_FQDN)\n"
- " flags:\n"
- " N=1\n"
- " E=1\n"
- " O=1\n"
- " S=0\n"
- " domain-name='myhost.example.com.' (full)\n";
+ " type=81 (CLIENT_FQDN), flags: (N=1, E=1, O=1, S=0), "
+ "domain-name='myhost.example.com.' (full)";
const int indent = 2;
EXPECT_EQ(ref_string, option->toText(indent));
@@ -888,13 +883,8 @@ TEST(Option4ClientFqdnTest, toText) {
Option4ClientFqdn::PARTIAL))
);
ref_string =
- "type=81(CLIENT_FQDN)\n"
- "flags:\n"
- " N=0\n"
- " E=0\n"
- " O=0\n"
- " S=0\n"
- "domain-name='myhost' (partial)\n";
+ "type=81 (CLIENT_FQDN), flags: (N=0, E=0, O=0, S=0), "
+ "domain-name='myhost' (partial)";
EXPECT_EQ(ref_string, option->toText());
}
More information about the bind10-changes
mailing list