From: Marcin Siodelski Date: Tue, 13 Aug 2013 18:02:12 +0000 (+0200) Subject: [3036] Address review comments. X-Git-Tag: bind10-1.2.0beta1-release~257^2~9 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3edad954ddf775772818ff87221bde52350bebc5;p=thirdparty%2Fkea.git [3036] Address review comments. --- diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 56addba2aa..77b1c7db0b 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -457,9 +457,7 @@ bool Dhcpv6Srv::run() { .arg(e.what()); } - // Although we don't support sending the NameChangeRequests to - // bind10-d2 module, we already call sendNameChangeRequets() here - // to empty the queue. Otherwise, the queue would bloat. + // Send NameChangeRequests to the b10-dhcp_ddns module. sendNameChangeRequests(); } } @@ -1097,6 +1095,7 @@ Dhcpv6Srv::createRemovalNameChangeRequest(const Lease6Ptr& lease) { // DHCID can be computed from it. This may throw an exception if hostname // has invalid format. Again, this should be only possible in case of // manual intervention in the database. + // The DHCID computation is further in this function. std::vector hostname_wire; try { OptionDataTypeUtil::writeFqdn(lease->hostname_, hostname_wire); @@ -1133,7 +1132,7 @@ void Dhcpv6Srv::sendNameChangeRequests() { while (!name_change_reqs_.empty()) { // @todo Once next NameChangeRequest is picked from the queue - // we should send it to the bind10-d2 module. Currently we + // we should send it to the bind10-dhcp_ddns module. Currently we // just drop it. name_change_reqs_.pop(); } diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index d6a11512a7..4c7b33da63 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -334,7 +334,8 @@ protected: /// /// @param question A message received from the client. /// @param [out] answer A server's response where FQDN option will be added. - /// @param fqdn A DHCPv6 Client FQDN %Option to be added. + /// @param fqdn A DHCPv6 Client FQDN %Option to be added to the server's + /// response to a client. void appendClientFqdn(const Pkt6Ptr& question, Pkt6Ptr& answer, const Option6ClientFqdnPtr& fqdn); diff --git a/src/lib/dhcp/option6_client_fqdn.cc b/src/lib/dhcp/option6_client_fqdn.cc index 5fcb81d6e7..512bf7ab74 100644 --- a/src/lib/dhcp/option6_client_fqdn.cc +++ b/src/lib/dhcp/option6_client_fqdn.cc @@ -23,43 +23,97 @@ 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 Option6ClientFqdn class from the interface. This implementation +/// uses b10-libdns classes to process FQDNs. At some point it may be +/// desired to split b10-libdhcp++ from b10-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 Option6ClientFqdnImpl { public: + /// Holds flags carried by the option. uint8_t flags_; + /// Holds the pointer to a domain name carried in the option. boost::shared_ptr domain_name_; + /// Indicates whether domain name is partial or fully qualified. Option6ClientFqdn::DomainNameType domain_name_type_; - Option6ClientFqdnImpl(const uint8_t flag, + /// @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. + Option6ClientFqdnImpl(const uint8_t flags, const std::string& domain_name, const Option6ClientFqdn::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. Option6ClientFqdnImpl(OptionBufferConstIter first, OptionBufferConstIter last); + /// @brief Copy constructor. + /// + /// @param source An object being copied. Option6ClientFqdnImpl(const Option6ClientFqdnImpl& source); + /// @brief Assignment operator. + /// + /// @param source An object which is being assigned. Option6ClientFqdnImpl& operator=(const Option6ClientFqdnImpl& 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 Option6ClientFqdn::DomainNameType name_type); - static void checkFlags(const uint8_t flags); - + /// @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 InvalidFqdnOptionFlags if flags are invalid. + static void checkFlags(const uint8_t flags, const bool check_mbz); + + /// @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); }; Option6ClientFqdnImpl:: -Option6ClientFqdnImpl(const uint8_t flag, +Option6ClientFqdnImpl(const uint8_t flags, const std::string& domain_name, const Option6ClientFqdn::DomainNameType name_type) - : flags_(flag), + : flags_(flags), domain_name_(), domain_name_type_(name_type) { - // Check if flags are correct. - checkFlags(flags_); + // Check if flags are correct. Also check if MBZ bits are set. + checkFlags(flags_, true); // Set domain name. It may throw an exception if domain name has wrong // format. setDomainName(domain_name, name_type); @@ -68,8 +122,9 @@ Option6ClientFqdnImpl(const uint8_t flag, Option6ClientFqdnImpl::Option6ClientFqdnImpl(OptionBufferConstIter first, OptionBufferConstIter last) { parseWireData(first, last); - // Verify that flags value was correct. - checkFlags(flags_); + // Verify that flags value was correct. Do not check if MBZ bits are + // set because we should ignore those bits in received message. + checkFlags(flags_, false); } Option6ClientFqdnImpl:: @@ -126,9 +181,9 @@ setDomainName(const std::string& domain_name, } void -Option6ClientFqdnImpl::checkFlags(const uint8_t flags) { +Option6ClientFqdnImpl::checkFlags(const uint8_t flags, const bool check_mbz) { // The Must Be Zero (MBZ) bits must not be set. - if ((flags & ~Option6ClientFqdn::FLAG_MASK) != 0) { + if (check_mbz && ((flags & ~Option6ClientFqdn::FLAG_MASK) != 0)) { isc_throw(InvalidFqdnOptionFlags, "invalid DHCPv6 Client FQDN Option flags: 0x" << std::hex << static_cast(flags) << std::dec); @@ -153,7 +208,9 @@ Option6ClientFqdnImpl::parseWireData(OptionBufferConstIter first, // The domain-name may be empty. if (std::distance(first, last) < Option6ClientFqdn::FLAG_FIELD_LEN) { isc_throw(OutOfRange, "DHCPv6 Client FQDN Option (" - << D6O_CLIENT_FQDN << ") is truncated"); + << D6O_CLIENT_FQDN << ") is truncated. Minimal option" + << " size is " << Option6ClientFqdn::FLAG_FIELD_LEN + << ", got option with size " << std::distance(first, last)); } // Parse flags @@ -259,7 +316,7 @@ Option6ClientFqdn::setFlag(const Flag flag, const bool set_flag) { } // Check new flags. If they are valid, apply them. - Option6ClientFqdnImpl::checkFlags(new_flag); + Option6ClientFqdnImpl::checkFlags(new_flag, true); impl_->flags_ = new_flag; } diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 52a0f810fc..f655cf30e6 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above