Commit da34264f authored by Marcin Siodelski's avatar Marcin Siodelski
Browse files

[3082] Address review comments.

parent cf24637d
......@@ -23,40 +23,102 @@
namespace isc {
namespace dhcp {
/// @brief Implements the logic for the Option6ClientFqdn class.
///
/// The purpose of the class is to separate the implementation details
/// of the Option4ClientFqdn class from the interface. This implementation
/// uses libdns classes to process FQDNs. At some point it may be
/// desired to split libdhcp++ from libdns. In such case the
/// implementation of this class may be changed. The declaration of the
/// Option6ClientFqdn class holds the pointer to implementation, so
/// the transition to a different implementation would not affect the
/// header file.
class Option4ClientFqdnImpl {
public:
/// Holds flags carried by the option.
uint8_t flags_;
// Holds RCODE1 and RCODE2 values.
Option4ClientFqdn::Rcode rcode1_;
Option4ClientFqdn::Rcode rcode2_;
/// Holds the pointer to a domain name carried in the option.
boost::shared_ptr<isc::dns::Name> domain_name_;
/// Indicates whether domain name is partial or fully qualified.
Option4ClientFqdn::DomainNameType domain_name_type_;
/// @brief Constructor, from domain name.
///
/// @param flags A value of the flags option field.
/// @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,
const Option4ClientFqdn::Rcode& rcode,
const std::string& domain_name,
const Option4ClientFqdn::DomainNameType name_type);
/// @brief Constructor, from wire data.
///
/// @param first An iterator pointing to the begining of the option data
/// in the wire format.
/// @param last An iterator poiting to the end of the option data in the
/// wire format.
Option4ClientFqdnImpl(OptionBufferConstIter first,
OptionBufferConstIter last);
/// @brief Copy constructor.
///
/// @param source An object being copied.
Option4ClientFqdnImpl(const Option4ClientFqdnImpl& source);
/// @brief Assignment operator.
///
/// @param source An object which is being assigned.
Option4ClientFqdnImpl& operator=(const Option4ClientFqdnImpl& source);
/// @brief Set a new domain name for the option.
///
/// @param domain_name A new domain name to be assigned.
/// @param name_type A value which indicates whether the domain-name is
/// partial or fully qualified.
void setDomainName(const std::string& domain_name,
const Option4ClientFqdn::DomainNameType name_type);
/// @brief Check if flags are valid.
///
/// In particular, this function checks if the N and S bits are not
/// set to 1 in the same time.
///
/// @param flags A value carried by the flags field of the option.
/// @param check_mbz A boolean value which indicates if this function should
/// check if the MBZ bits are set (if true). This parameter should be set
/// 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);
/// @brief Parse the Option provided in the wire format.
///
/// @param first An iterator pointing to the begining of the option data
/// in the wire format.
/// @param last An iterator poiting to the end of the option data in the
/// wire format.
void parseWireData(OptionBufferConstIter first,
OptionBufferConstIter last);
/// @brief Parse domain-name encoded in the canonical format.
///
void parseCanonicalDomainName(OptionBufferConstIter first,
OptionBufferConstIter last);
void
parseASCIIDomainName(OptionBufferConstIter first,
OptionBufferConstIter last);
/// @brief Parse domain-name emcoded in the deprecated ASCII format.
///
/// @param first An iterator pointing to the begining of the option data
/// where domain-name is stored.
/// @param last An iterator poiting to the end of the option data where
/// domain-name is stored.
void parseASCIIDomainName(OptionBufferConstIter first,
OptionBufferConstIter last);
};
......@@ -100,8 +162,16 @@ Option4ClientFqdnImpl(const Option4ClientFqdnImpl& source)
}
Option4ClientFqdnImpl&
// This assignment operator handles assignment to self, it copies all
// required values.
// cppcheck-suppress operatorEqToSelf
Option4ClientFqdnImpl::operator=(const Option4ClientFqdnImpl& source) {
domain_name_.reset(new isc::dns::Name(*source.domain_name_));
if (source.domain_name_) {
domain_name_.reset(new isc::dns::Name(*source.domain_name_));
} else {
domain_name_.reset();
}
// Assignment is exception safe.
flags_ = source.flags_;
......@@ -121,7 +191,7 @@ setDomainName(const std::string& domain_name,
std::string name = isc::util::str::trim(domain_name);
if (name.empty()) {
if (name_type == Option4ClientFqdn::FULL) {
isc_throw(InvalidOption4ClientFqdnDomainName,
isc_throw(InvalidOption4FqdnDomainName,
"fully qualified domain-name must not be empty"
<< " when setting new domain-name for DHCPv4 Client"
<< " FQDN Option");
......@@ -136,7 +206,7 @@ setDomainName(const std::string& domain_name,
domain_name_type_ = name_type;
} catch (const Exception& ex) {
isc_throw(InvalidOption4ClientFqdnDomainName,
isc_throw(InvalidOption4FqdnDomainName,
"invalid domain-name value '"
<< domain_name << "' when setting new domain-name for"
<< " DHCPv4 Client FQDN Option");
......@@ -149,7 +219,7 @@ void
Option4ClientFqdnImpl::checkFlags(const uint8_t flags) {
// The Must Be Zero (MBZ) bits must not be set.
if ((flags & ~Option4ClientFqdn::FLAG_MASK) != 0) {
isc_throw(InvalidOption4ClientFqdnFlags,
isc_throw(InvalidOption4FqdnFlags,
"invalid DHCPv4 Client FQDN Option flags: 0x"
<< std::hex << static_cast<int>(flags) << std::dec);
}
......@@ -158,7 +228,7 @@ Option4ClientFqdnImpl::checkFlags(const uint8_t flags) {
// MUST be 0. Checking it here.
if ((flags & (Option4ClientFqdn::FLAG_N | Option4ClientFqdn::FLAG_S))
== (Option4ClientFqdn::FLAG_N | Option4ClientFqdn::FLAG_S)) {
isc_throw(InvalidOption4ClientFqdnFlags,
isc_throw(InvalidOption4FqdnFlags,
"both N and S flag of the DHCPv4 Client FQDN Option are set."
<< " According to RFC 4702, if the N bit is 1 the S bit"
<< " MUST be 0");
......@@ -192,7 +262,7 @@ Option4ClientFqdnImpl::parseWireData(OptionBufferConstIter first,
}
} catch (const Exception& ex) {
isc_throw(InvalidOption4ClientFqdnDomainName,
isc_throw(InvalidOption4FqdnDomainName,
"failed to parse the domain-name in DHCPv4 Client FQDN"
<< " Option: " << ex.what());
}
......@@ -270,6 +340,9 @@ Option4ClientFqdn::Option4ClientFqdn(const Option4ClientFqdn& source)
}
Option4ClientFqdn&
// This assignment operator handles assignment to self, it uses copy
// constructor of Option4ClientFqdnImpl to copy all required values.
// cppcheck-suppress operatorEqToSelf
Option4ClientFqdn::operator=(const Option4ClientFqdn& source) {
Option4ClientFqdnImpl* old_impl = impl_;
impl_ = new Option4ClientFqdnImpl(*source.impl_);
......@@ -278,16 +351,11 @@ Option4ClientFqdn::operator=(const Option4ClientFqdn& source) {
}
bool
Option4ClientFqdn::getFlag(const Flag flag) const {
// Caller should query for one of the: E, N, S or O flags. However, there
// are valid enumerator values which should not be accepted by this function.
// For example a value of 0x3 is valid (because it belongs to the range between the
// lowest and highest enumerator). The value 0x3 represents two flags:
// S and O and would cause ambiguity. Therefore, we selectively check
// that the flag is equal to one of the explicit enumerator values. If
// not, throw an exception.
Option4ClientFqdn::getFlag(const uint8_t flag) const {
// Caller should query for one of the: E, N, S or O flags. Any other value
/// is invalid and results in the exception.
if (flag != FLAG_S && flag != FLAG_O && flag != FLAG_N && flag != FLAG_E) {
isc_throw(InvalidOption4ClientFqdnFlags, "invalid DHCPv4 Client FQDN"
isc_throw(InvalidOption4FqdnFlags, "invalid DHCPv4 Client FQDN"
<< " Option flag specified, expected E, N, S or O");
}
......@@ -295,11 +363,12 @@ Option4ClientFqdn::getFlag(const Flag flag) const {
}
void
Option4ClientFqdn::setFlag(const Flag flag, const bool set_flag) {
// Check that flag is in range between 0x1 and 0x7. Note that this
// allows to set or clear multiple flags concurrently.
Option4ClientFqdn::setFlag(const uint8_t flag, const bool set_flag) {
// Check that flag is in range between 0x1 and 0x7. Although it is
// discouraged this check doesn't preclude the caller from setting
// multiple flags concurrently.
if (((flag & ~FLAG_MASK) != 0) || (flag == 0)) {
isc_throw(InvalidOption4ClientFqdnFlags, "invalid DHCPv4 Client FQDN"
isc_throw(InvalidOption4FqdnFlags, "invalid DHCPv4 Client FQDN"
<< " Option flag " << std::hex
<< static_cast<int>(flag) << std::dec
<< "is being set. Expected combination of E, N, S and O");
......
......@@ -25,16 +25,16 @@ namespace dhcp {
/// @brief Exception thrown when invalid flags have been specified for
/// DHCPv4 Client FQDN %Option.
class InvalidOption4ClientFqdnFlags : public Exception {
class InvalidOption4FqdnFlags : public Exception {
public:
InvalidOption4ClientFqdnFlags(const char* file, size_t line, const char* what) :
InvalidOption4FqdnFlags(const char* file, size_t line, const char* what) :
isc::Exception(file, line, what) {}
};
/// @brief Exception thrown when invalid domain name is specified.
class InvalidOption4ClientFqdnDomainName : public Exception {
class InvalidOption4FqdnDomainName : public Exception {
public:
InvalidOption4ClientFqdnDomainName(const char* file, size_t line,
InvalidOption4FqdnDomainName(const char* file, size_t line,
const char* what) :
isc::Exception(file, line, what) {}
};
......@@ -65,9 +65,9 @@ class Option4ClientFqdnImpl;
/// where:
/// - N flag specifies whether server should (0) or should not (1) perform DNS
/// Update,
/// - E flag specifies encoding of the Domain Name field. If this flag is set to 1
/// it indicates canonical wire format without compression. 0 indicates the deprecated
/// ASCII format.
/// - E flag specifies encoding of the Domain Name field. If this flag is set
/// to 1 it indicates canonical wire format without compression. 0 indicates
/// the deprecated ASCII format.
/// - O flag is set by the server to indicate that it has overridden client's
/// preference set with the S bit.
/// - S flag specifies whether server should (1) or should not (0) perform
......@@ -82,13 +82,14 @@ class Option4ClientFqdnImpl;
/// at the end of their wire representation (or lack of dot at the end, in
/// case of ASCII encoding). It is also accepted to create an instance of
/// this option which has empty domain-name. Clients use empty domain-names
/// to indicate that server should generate complete fully qualified domain-name.
/// to indicate that server should generate complete fully qualified
/// domain-name.
///
/// @warning: The RFC4702 section 2.3.1 states that the clients and servers
/// should use character sets specified in RFC952, section 2.1 for ASCII-encoded
/// domain-names. This class doesn't detect the character set violation for
/// ASCII-encoded domain-name. It could be implemented in the future but it is not
/// important now for two reasons:
/// ASCII-encoded domain-name. It could be implemented in the future but it is
/// not important now for two reasons:
/// - ASCII encoding is deprecated
/// - clients SHOULD obey restrictions but if they don't, server may still
/// process the option
......@@ -112,14 +113,17 @@ class Option4ClientFqdnImpl;
class Option4ClientFqdn : public Option {
public:
/// @brief Enumeration holding different flags used in the Client
/// FQDN %Option.
enum Flag {
FLAG_S = 0x01,
FLAG_O = 0x02,
FLAG_E = 0x04,
FLAG_N = 0x08
};
///
/// @name A set of constants used to identify and set bits in the flags field
//@{
static const uint8_t FLAG_S = 0x01; ///< Bit S
static const uint8_t FLAG_O = 0x02; ///< Bit O
static const uint8_t FLAG_E = 0x04; ///< Bit E
static const uint8_t FLAG_N = 0x08; ///< Bit N
//@}
/// @brief Mask which zeroes MBZ flag bits.
static const uint8_t FLAG_MASK = 0xF;
/// @brief Represents the value of one of the RCODE1 or RCODE2 fields.
///
......@@ -149,10 +153,8 @@ public:
FULL
};
/// @brief Mask which zeroes MBZ flag bits.
static const uint8_t FLAG_MASK = 0xF;
/// @brief The size of the fixed fields within DHCPv4 Client Fqdn %Option.
/// @brief The size in bytes of the fixed fields within DHCPv4 Client Fqdn
/// %Option.
///
/// The fixed fields are:
/// - Flags
......@@ -162,17 +164,19 @@ public:
/// @brief Constructor, creates option instance using flags and domain name.
///
/// This constructor is used to create instance of the option which will be
/// included in outgoing messages.
/// This constructor is used to create an instance of the option which will
/// be included in outgoing messages.
///
/// @param flag a combination of flags to be stored in flags field.
/// @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.
/// 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).
explicit Option4ClientFqdn(const uint8_t flag,
/// @throw InvalidOption4FqdnFlags if value of the flags field is wrong.
/// @throw InvalidOption4FqdnDomainName if the domain-name is invalid.
explicit Option4ClientFqdn(const uint8_t flags,
const Rcode& rcode,
const std::string& domain_name,
const DomainNameType domain_name_type = FULL);
......@@ -182,11 +186,12 @@ public:
/// This constructor creates an instance of the option with empty
/// domain-name. This domain-name is marked partial.
///
/// @param flag a combination of flags to be stored in flags field.
/// @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. Both fields are assigned the same value encapsulated by this
/// parameter.
Option4ClientFqdn(const uint8_t flag, const Rcode& rcode);
/// @throw InvalidOption4FqdnFlags if value of the flags field is invalid.
Option4ClientFqdn(const uint8_t flags, const Rcode& rcode);
/// @brief Constructor, creates an option instance from part of the buffer.
///
......@@ -198,6 +203,10 @@ public:
///
/// @param first the lower bound of the buffer to create option from.
/// @param last the upper bound of the buffer to create option from.
/// @throw InvalidOption4FqdnFlags if value of the flags field is invalid.
/// @throw InvalidOption4FqdnDomainName if the domain-name carried by the
/// option is invalid.
/// @throw OutOfRange if the option is truncated.
explicit Option4ClientFqdn(OptionBufferConstIter first,
OptionBufferConstIter last);
......@@ -213,18 +222,26 @@ public:
/// @brief Checks if the specified flag of the DHCPv4 Client FQDN %Option
/// is set.
///
/// @param flag an enum value specifying the flag to be checked.
/// @param flag A value specifying a bit within flags field to be checked.
/// It must be one of the following @c FLAG_S, @c FLAG_E, @c FLAG_O,
/// @c FLAG_N.
///
/// @return true if the bit of the specified flag is set, false otherwise.
bool getFlag(const Flag flag) const;
/// @return true if the bit of the specified flags bit is set, false
/// otherwise.
/// @throw InvalidOption4ClientFlags if specified flag which value is to be
/// returned is invalid (is not one of the FLAG_S, FLAG_N, FLAG_O).
bool getFlag(const uint8_t flag) const;
/// @brief Modifies the value of the specified DHCPv4 Client Fqdn %Option
/// flag.
///
/// @param flag an enum value specifying the flag to be modified.
/// @param flag A value specifying a bit within flags field to be set. It
/// must be one of the following @c FLAG_S, @c FLAG_E, @c FLAG_O, @c FLAG_N.
/// @param set a boolean value which indicates whether flag should be
/// set (true), or cleared (false).
void setFlag(const Flag flag, const bool set);
/// @throw InvalidOption4ClientFlags if specified flag which value is to be
/// set is invalid (is not one of the FLAG_S, FLAG_N, FLAG_O).
void setFlag(const uint8_t flag, const bool set);
/// @brief Sets the flag field value to 0.
void resetFlags();
......@@ -256,6 +273,8 @@ public:
/// @param domain_name domain name field value in the text format.
/// @param domain_name_type type of the domain name: partial or fully
/// qualified.
/// @throw InvalidOption4FqdnDomainName if the specified domain-name is
/// invalid.
void setDomainName(const std::string& domain_name,
const DomainNameType domain_name_type);
......
......@@ -170,7 +170,7 @@ OptionDataTypeUtil::writeBinary(const std::string& hex_str,
bool
OptionDataTypeUtil::readBool(const std::vector<uint8_t>& buf) {
if (buf.size() < 1) {
if (buf.empty()) {
isc_throw(BadDataTypeCast, "unable to read the buffer as boolean"
<< " value. Invalid buffer size " << buf.size());
}
......
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