From 6975d0af5881c9d140e2a4ceb6a602d2f246f148 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Mon, 25 Feb 2019 12:35:44 +0100 Subject: [PATCH 1/9] [#487,!242] Adjusted OptionalValue to Triplet and renamed to Optional. --- src/bin/dhcp4/tests/dhcp4_client.cc | 14 +- src/bin/dhcp4/tests/dhcp4_client.h | 4 +- src/bin/dhcp4/tests/dora_unittest.cc | 6 +- src/lib/dhcp/iface_mgr.cc | 14 +- src/lib/dhcp/iface_mgr.h | 6 +- src/lib/dhcpsrv/mysql_host_data_source.cc | 10 +- src/lib/dhcpsrv/parsers/option_data_parser.cc | 56 +++--- src/lib/dhcpsrv/parsers/option_data_parser.h | 10 +- src/lib/util/optional_value.h | 184 +++++++----------- src/lib/util/tests/optional_value_unittest.cc | 139 ++++--------- 10 files changed, 170 insertions(+), 273 deletions(-) diff --git a/src/bin/dhcp4/tests/dhcp4_client.cc b/src/bin/dhcp4/tests/dhcp4_client.cc index 19565308a4..d02e82df73 100644 --- a/src/bin/dhcp4/tests/dhcp4_client.cc +++ b/src/bin/dhcp4/tests/dhcp4_client.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -42,7 +42,7 @@ Dhcp4Client::Configuration::reset() { Dhcp4Client::Dhcp4Client(const Dhcp4Client::State& state) : config_(), - ciaddr_(IOAddress("0.0.0.0")), + ciaddr_(), curr_transid_(0), dest_addr_("255.255.255.255"), hwaddr_(generateHWAddr()), @@ -60,7 +60,7 @@ Dhcp4Client::Dhcp4Client(const Dhcp4Client::State& state) : Dhcp4Client::Dhcp4Client(boost::shared_ptr srv, const Dhcp4Client::State& state) : config_(), - ciaddr_(IOAddress("0.0.0.0")), + ciaddr_(), curr_transid_(0), dest_addr_("255.255.255.255"), fqdn_(), @@ -259,8 +259,8 @@ Dhcp4Client::doDiscover(const boost::shared_ptr& requested_addr) { addRequestedAddress(*requested_addr); } // Override the default ciaddr if specified by a test. - if (ciaddr_.isSpecified()) { - context_.query_->setCiaddr(ciaddr_.get()); + if (!ciaddr_.unspecified()) { + context_.query_->setCiaddr(ciaddr_); } appendExtraOptions(); appendClasses(); @@ -364,8 +364,8 @@ Dhcp4Client::doRequest() { context_.query_ = createMsg(DHCPREQUEST); // Override the default ciaddr if specified by a test. - if (ciaddr_.isSpecified()) { - context_.query_->setCiaddr(ciaddr_.get()); + if (!ciaddr_.unspecified()) { + context_.query_->setCiaddr(ciaddr_); } else if ((state_ == SELECTING) || (state_ == INIT_REBOOT)) { context_.query_->setCiaddr(IOAddress("0.0.0.0")); } else { diff --git a/src/bin/dhcp4/tests/dhcp4_client.h b/src/bin/dhcp4/tests/dhcp4_client.h index d86491f1fe..5201176bcf 100644 --- a/src/bin/dhcp4/tests/dhcp4_client.h +++ b/src/bin/dhcp4/tests/dhcp4_client.h @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -371,7 +371,7 @@ public: /// If this value is "unspecified" the default values will be used /// by the client. If this value is specified, it will override ciaddr /// in the client's messages. - isc::util::OptionalValue ciaddr_; + isc::util::Optional ciaddr_; /// @brief Adds extra option (an option the client will always send) /// diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc index 7ae1424c97..e74f473dd0 100644 --- a/src/bin/dhcp4/tests/dora_unittest.cc +++ b/src/bin/dhcp4/tests/dora_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -1040,7 +1040,7 @@ TEST_F(DORATest, ciaddr) { // Configure DHCP server. configure(DORA_CONFIGS[0], *client.getServer()); // Force ciaddr of Discover message to be non-zero. - client.ciaddr_.specify(IOAddress("10.0.0.50")); + client.ciaddr_ = IOAddress("10.0.0.50"); // Obtain a lease from the server using the 4-way exchange. ASSERT_NO_THROW(client.doDiscover(boost::shared_ptr< IOAddress>(new IOAddress("10.0.0.50")))); @@ -1076,7 +1076,7 @@ TEST_F(DORATest, ciaddr) { // Replace the address held by the client. The client will request // the assignment of this address but the server has a different // address for this client. - client.ciaddr_.specify(IOAddress("192.168.0.30")); + client.ciaddr_ = IOAddress("192.168.0.30"); ASSERT_NO_THROW(client.doRequest()); // The client is sending invalid ciaddr so the server should send a NAK. resp = client.getContext().response_; diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index a4dc8ad636..50c041e20e 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -213,7 +213,7 @@ void Iface::addUnicast(const isc::asiolink::IOAddress& addr) { << " already defined on the " << name_ << " interface."); } } - unicasts_.push_back(OptionalValue(addr, true)); + unicasts_.push_back(Optional(addr)); } bool @@ -244,7 +244,7 @@ Iface::hasAddress(const isc::asiolink::IOAddress& address) const { void Iface::addAddress(const isc::asiolink::IOAddress& addr) { - addrs_.push_back(Address(addr, OptionalValueState(true))); + addrs_.push_back(Address(addr)); } void @@ -252,7 +252,7 @@ Iface::setActive(const IOAddress& address, const bool active) { for (AddressCollection::iterator addr_it = addrs_.begin(); addr_it != addrs_.end(); ++addr_it) { if (address == addr_it->get()) { - addr_it->specify(OptionalValueState(active)); + addr_it->unspecified(!active); return; } } @@ -264,7 +264,7 @@ void Iface::setActive(const bool active) { for (AddressCollection::iterator addr_it = addrs_.begin(); addr_it != addrs_.end(); ++addr_it) { - addr_it->specify(OptionalValueState(active)); + addr_it->unspecified(!active); } } @@ -272,7 +272,7 @@ unsigned int Iface::countActive4() const { uint16_t count = 0; BOOST_FOREACH(Address addr, addrs_) { - if (addr.get().isV4() && addr.isSpecified()) { + if (!addr.unspecified() && addr.get().isV4()) { ++count; } } @@ -526,7 +526,7 @@ IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast, BOOST_FOREACH(Iface::Address addr, iface->getAddresses()) { // Skip non-IPv4 addresses and those that weren't selected.. - if (!addr.get().isV4() || !addr.isSpecified()) { + if (addr.unspecified() || !addr.get().isV4()) { continue; } diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index e771ce2764..229bb5fd0a 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -118,7 +118,7 @@ public: static const unsigned int MAX_MAC_LEN = 20; /// @brief Address type. - typedef util::OptionalValue Address; + typedef util::Optional Address; /// Type that defines list of addresses typedef std::list
AddressCollection; @@ -227,7 +227,7 @@ public: /// @brief Returns all addresses available on an interface. /// - /// The returned addresses are encapsulated in the @c util::OptionalValue + /// The returned addresses are encapsulated in the @c util::Optional /// class to be able to selectively flag some of the addresses as active /// (when optional value is specified) or inactive (when optional value /// is specified). If the address is marked as active, the diff --git a/src/lib/dhcpsrv/mysql_host_data_source.cc b/src/lib/dhcpsrv/mysql_host_data_source.cc index 75e2d17e18..8bb59e099e 100644 --- a/src/lib/dhcpsrv/mysql_host_data_source.cc +++ b/src/lib/dhcpsrv/mysql_host_data_source.cc @@ -1758,7 +1758,7 @@ public: std::vector createBindForSend(const OptionDescriptor& opt_desc, const std::string& opt_space, - const OptionalValue& subnet_id, + const Optional& subnet_id, const HostID& host_id) { // Hold pointer to the option to make sure it remains valid until @@ -1850,7 +1850,7 @@ public: bind_[7].length = &client_class_len_; // dhcp4_subnet_id: INT UNSIGNED NULL - if (subnet_id.isSpecified()) { + if (!subnet_id.unspecified()) { subnet_id_ = subnet_id; bind_[8].buffer_type = MYSQL_TYPE_LONG; bind_[8].buffer = reinterpret_cast(subnet_id_); @@ -2029,7 +2029,7 @@ public: void addOption(const MySqlHostDataSourceImpl::StatementIndex& stindex, const OptionDescriptor& opt_desc, const std::string& opt_space, - const OptionalValue& subnet_id, + const Optional& subnet_id, const HostID& host_id); /// @brief Inserts multiple options into the database. @@ -2624,7 +2624,7 @@ void MySqlHostDataSourceImpl::addOption(const StatementIndex& stindex, const OptionDescriptor& opt_desc, const std::string& opt_space, - const OptionalValue& subnet_id, + const Optional& subnet_id, const HostID& id) { std::vector bind = host_option_exchange_->createBindForSend(opt_desc, opt_space, @@ -2652,7 +2652,7 @@ MySqlHostDataSourceImpl::addOptions(const StatementIndex& stindex, if (options && !options->empty()) { for (OptionContainer::const_iterator opt = options->begin(); opt != options->end(); ++opt) { - addOption(stindex, *opt, *space, OptionalValue(), + addOption(stindex, *opt, *space, Optional(), host_id); } } diff --git a/src/lib/dhcpsrv/parsers/option_data_parser.cc b/src/lib/dhcpsrv/parsers/option_data_parser.cc index 97c34e0970..f447a9f757 100644 --- a/src/lib/dhcpsrv/parsers/option_data_parser.cc +++ b/src/lib/dhcpsrv/parsers/option_data_parser.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -47,7 +47,7 @@ OptionDataParser::parse(isc::data::ConstElementPtr single_option) { return (opt); } -OptionalValue +Optional OptionDataParser::extractCode(ConstElementPtr parent) const { uint32_t code; try { @@ -56,7 +56,7 @@ OptionDataParser::extractCode(ConstElementPtr parent) const { } catch (const std::exception&) { // The code parameter was not found. Return an unspecified // value. - return (OptionalValue()); + return (Optional()); } if (code == 0) { @@ -81,17 +81,17 @@ OptionDataParser::extractCode(ConstElementPtr parent) const { } - return (OptionalValue(code, OptionalValueState(true))); + return (Optional(code)); } -OptionalValue +Optional OptionDataParser::extractName(ConstElementPtr parent) const { std::string name; try { name = getString(parent, "name"); } catch (...) { - return (OptionalValue()); + return (Optional()); } if (name.find(" ") != std::string::npos) { @@ -100,7 +100,7 @@ OptionDataParser::extractName(ConstElementPtr parent) const { << getPosition("name", parent) << ")"); } - return (OptionalValue(name, OptionalValueState(true))); + return (Optional(name)); } std::string @@ -117,17 +117,17 @@ OptionDataParser::extractData(ConstElementPtr parent) const { return (data); } -OptionalValue +Optional OptionDataParser::extractCSVFormat(ConstElementPtr parent) const { bool csv_format = true; try { csv_format = getBoolean(parent, "csv-format"); } catch (...) { - return (OptionalValue(csv_format)); + return (Optional()); } - return (OptionalValue(csv_format, OptionalValueState(true))); + return (Optional(csv_format)); } std::string @@ -166,17 +166,17 @@ OptionDataParser::extractSpace(ConstElementPtr parent) const { return (space); } -OptionalValue +Optional OptionDataParser::extractPersistent(ConstElementPtr parent) const { bool persist = false; try { persist = getBoolean(parent, "always-send"); } catch (...) { - return (OptionalValue(persist)); + return (Optional()); } - return (OptionalValue(persist, OptionalValueState(true))); + return (Optional(persist)); } template @@ -231,16 +231,16 @@ OptionDataParser::createOption(ConstElementPtr option_data) { const Option::Universe universe = address_family_ == AF_INET ? Option::V4 : Option::V6; - OptionalValue code_param = extractCode(option_data); - OptionalValue name_param = extractName(option_data); - OptionalValue csv_format_param = extractCSVFormat(option_data); - OptionalValue persist_param = extractPersistent(option_data); + Optional code_param = extractCode(option_data); + Optional name_param = extractName(option_data); + Optional csv_format_param = extractCSVFormat(option_data); + Optional persist_param = extractPersistent(option_data); std::string data_param = extractData(option_data); std::string space_param = extractSpace(option_data); ConstElementPtr user_context = option_data->get("user-context"); // Require that option code or option name is specified. - if (!code_param.isSpecified() && !name_param.isSpecified()) { + if (code_param.unspecified() && name_param.unspecified()) { isc_throw(DhcpConfigError, "option data configuration requires one of" " 'code' or 'name' parameters to be specified" << " (" << option_data->getPosition() << ")"); @@ -248,16 +248,16 @@ OptionDataParser::createOption(ConstElementPtr option_data) { // Try to find a corresponding option definition using option code or // option name. - OptionDefinitionPtr def = code_param.isSpecified() ? - findOptionDefinition(space_param, code_param) : - findOptionDefinition(space_param, name_param); + OptionDefinitionPtr def = code_param.unspecified() ? + findOptionDefinition(space_param, name_param) : + findOptionDefinition(space_param, code_param); // If there is no definition, the user must not explicitly enable the // use of csv-format. if (!def) { // If explicitly requested that the CSV format is to be used, // the option definition is a must. - if (csv_format_param.isSpecified() && csv_format_param) { + if (!csv_format_param.unspecified() && csv_format_param) { isc_throw(DhcpConfigError, "definition for the option '" << space_param << "." << name_param << "' having code '" << code_param @@ -267,7 +267,7 @@ OptionDataParser::createOption(ConstElementPtr option_data) { // If there is no option definition and the option code is not specified // we have no means to find the option code. - } else if (name_param.isSpecified() && !code_param.isSpecified()) { + } else if (!name_param.unspecified() && code_param.unspecified()) { isc_throw(DhcpConfigError, "definition for the option '" << space_param << "." << name_param << "' does not exist (" @@ -282,7 +282,7 @@ OptionDataParser::createOption(ConstElementPtr option_data) { // If the definition is available and csv-format hasn't been explicitly // disabled, we will parse the data as comma separated values. - if (def && (!csv_format_param.isSpecified() || csv_format_param)) { + if (def && (csv_format_param.unspecified() || csv_format_param)) { // If the option data is specified as a string of comma // separated values then we need to split this string into // individual values - each value will be used to initialize @@ -325,11 +325,11 @@ OptionDataParser::createOption(ConstElementPtr option_data) { binary)); desc.option_ = option; - desc.persistent_ = persist_param.isSpecified() && persist_param; + desc.persistent_ = !persist_param.unspecified() && persist_param; } else { // Option name is specified it should match the name in the definition. - if (name_param.isSpecified() && (def->getName() != name_param.get())) { + if (!name_param.unspecified() && (def->getName() != name_param.get())) { isc_throw(DhcpConfigError, "specified option name '" << name_param << "' does not match the " << "option definition: '" << space_param @@ -341,12 +341,12 @@ OptionDataParser::createOption(ConstElementPtr option_data) { // Option definition has been found so let's use it to create // an instance of our option. try { - bool use_csv = !csv_format_param.isSpecified() || csv_format_param; + bool use_csv = csv_format_param.unspecified() || csv_format_param; OptionPtr option = use_csv ? def->optionFactory(universe, def->getCode(), data_tokens) : def->optionFactory(universe, def->getCode(), binary); desc.option_ = option; - desc.persistent_ = persist_param.isSpecified() && persist_param; + desc.persistent_ = !persist_param.unspecified() && persist_param; if (use_csv) { desc.formatted_value_ = data_param; } diff --git a/src/lib/dhcpsrv/parsers/option_data_parser.h b/src/lib/dhcpsrv/parsers/option_data_parser.h index 06770dd219..b19406bbef 100644 --- a/src/lib/dhcpsrv/parsers/option_data_parser.h +++ b/src/lib/dhcpsrv/parsers/option_data_parser.h @@ -1,4 +1,4 @@ -// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -107,7 +107,7 @@ private: /// /// @return Option code, possibly unspecified. /// @throw DhcpConfigError if option code is invalid. - util::OptionalValue + util::Optional extractCode(data::ConstElementPtr parent) const; /// @brief Retrieves parsed option name as an optional value. @@ -116,13 +116,13 @@ private: /// /// @return Option name, possibly unspecified. /// @throw DhcpConfigError if option name is invalid. - util::OptionalValue + util::Optional extractName(data::ConstElementPtr parent) const; /// @brief Retrieves csv-format parameter as an optional value. /// /// @return Value of the csv-format parameter, possibly unspecified. - util::OptionalValue extractCSVFormat(data::ConstElementPtr parent) const; + util::Optional extractCSVFormat(data::ConstElementPtr parent) const; /// @brief Retrieves option data as a string. /// @@ -145,7 +145,7 @@ private: /// @brief Retrieves persistent/always-send parameter as an optional value. /// /// @return Value of the persistent parameter, possibly unspecified. - util::OptionalValue extractPersistent(data::ConstElementPtr parent) const; + util::Optional extractPersistent(data::ConstElementPtr parent) const; /// @brief Address family: @c AF_INET or @c AF_INET6. uint16_t address_family_; diff --git a/src/lib/util/optional_value.h b/src/lib/util/optional_value.h index 5ff5bf0de5..bda8272a03 100644 --- a/src/lib/util/optional_value.h +++ b/src/lib/util/optional_value.h @@ -1,170 +1,124 @@ -// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/. -#ifndef OPTIONAL_VALUE_H -#define OPTIONAL_VALUE_H +#ifndef OPTIONAL_H +#define OPTIONAL_H #include +#include namespace isc { namespace util { -/// @brief Indicate if an @c OptionalValue is is specified or not. +/// @brief A template representing an optional value. /// -/// This is a simple wrapper class which holds a boolean value to indicate -/// if the @c OptionalValue is specified or not. By using this class in the -/// @c OptionalValue class constructor we avoid the ambiguity when the -/// @c OptionalValue encapsulates a bool type. -struct OptionalValueState { - - /// @brief Constructor. - /// - /// @param specified A boolean value to be assigned. - OptionalValueState(const bool specified) - : specified_(specified) { - } - /// @brief A bool value encapsulated by this structure. - bool specified_; -}; - -/// @brief Simple class representing an optional value. -/// -/// This template class encapsulates a value of any type. An additional flag -/// held by this class indicates if the value is "specified" or "unspecified". -/// For example, a configuration parser for DHCP server may use this class -/// to represent the value of the configuration parameter which may appear -/// in the configuration file, but is not mandatory. The value of the -/// @c OptionalValue may be initialized to "unspecified" initially. When the -/// configuration parser finds that the appropriate parameter exists in the -/// configuration file, the default value can be overridden and the value may -/// be marked as "specified". If the parameter is not found, the value remains -/// "unspecified" and the appropriate actions may be taken, e.g. the default -/// value may be used. +/// This template class encapsulates an optional value. The default implementation +/// encapsulates numeric values, but additional specializations are defined +/// as neccessary to support other types od data. /// -/// This is a generic class and may be used in all cases when there is a need -/// for the additional information to be carried along with the value. -/// Alternative approach is to use a pointer which is only initialized if the -/// actual value needs to be specified, but this may not be feasible in all -/// cases. +/// This class includes a boolean flag which indicates if the encapsulated +/// value is specified or unspecified. For example, a configuration parser +/// for the DHCP server may use this class to represent a value of the +/// configuration parameter which may appear in the configuration file, but +/// is not mandatory. The value of the @c Optional may be initialized to +/// "unspecified" initially. When the configuration parser finds that the +/// particular parameter exists in the configuration file, the default value +/// can be overriden and the value may be marked as "specified". If the +/// parameter is not found, the value remains "unspecified" and the appropriate +/// actions may be taken, e.g. the default value may be used. /// /// @tparam Type of the encapsulated value. template -class OptionalValue { +class Optional { public: - /// @brief Default constructor. + /// @brief Assigns a new value value and marks it "specified". /// - /// Note that the type @c T must have a default constructor to use this - /// constructor. - OptionalValue() - : value_(T()), specified_(false) { + /// @param other new actual value. + Optional& operator=(T other) { + default_ = other; + unspecified_ = false; + return (*this); } - /// @brief Constructor - /// - /// Creates optional value. The value defaults to "unspecified". - /// - /// @param value Default explicit value. - /// @param state Specifies bool which determines if the value is initially - /// specified or not (default is false). - explicit OptionalValue(const T& value, const OptionalValueState& state = - OptionalValueState(false)) - : value_(value), specified_(state.specified_) { - } - - /// @brief Retrieves the actual value. - T get() const { - return (value_); - } - - /// @brief Sets the actual value. + /// @brief Type cast operator. /// - /// @param value New value. - void set(const T& value) { - value_ = value; - } - - /// @brief Sets the new value and marks it specified. + /// This operator converts the optional value to the actual value being + /// encapsulated. /// - /// @param value New actual value. - void specify(const T& value) { - set(value); - specify(OptionalValueState(true)); + /// @return Encapsulated value. + operator T() const { + return (default_); } - /// @brief Sets the value to "specified" or "unspecified". + /// @brief Default constructor. /// - /// It does not alter the actual value. It only marks it "specified" or - /// "unspecified". - /// @param state determines if a value is specified or not - void specify(const OptionalValueState& state) { - specified_ = state.specified_; + /// Sets the encapsulated value to 0. + Optional() + : default_(T(0)), unspecified_(true) { } - /// @brief Checks if the value is specified or unspecified. + /// @brief Constructor /// - /// @return true if the value is specified, false otherwise. - bool isSpecified() const { - return (specified_); - } - - /// @brief Specifies a new value value and marks it "specified". + /// Creates optional value and marks it as "specified". /// - /// @param value New actual value. - void operator=(const T& value) { - specify(value); + /// @param value value to be assigned. + explicit Optional(T value) + : default_(value), unspecified_(false) { } - /// @brief Equality operator. - /// - /// @param value Actual value to compare to. - /// - /// @return true if the value is specified and equals the argument. - bool operator==(const T& value) const { - return (specified_ && (value_ == value)); + /// @brief Retrieves the actual value. + T get() const { + return (default_); } - /// @brief Inequality operator. + /// @brief Modifies the flag that indicates whether the value is specified + /// or unspecified. /// - /// @param value Actual value to compare to. - /// - /// @return true if the value is unspecified or unequal. - bool operator!=(const T& value) const { - return (!operator==(value)); + /// @param unspecified new value of the flag. If it is @c true, the + /// value is marked as unspecified, otherwise it is marked as specified. + void unspecified(bool unspecified) { + unspecified_ = unspecified; } - /// @brief Type cast operator. - /// - /// This operator converts the optional value to the actual value being - /// encapsulated. + /// @brief Checks if the value has been specified or unspecified. /// - /// @return Encapsulated value. - operator T() const { - return (value_); + /// @return true if the value hasn't been specified, false otherwise. + bool unspecified() const { + return (unspecified_); } -private: - T value_; ///< Encapsulated value. - bool specified_; ///< Flag which indicates if the value is specified. +protected: + T default_; ///< Encapsulated value. + bool unspecified_; ///< Flag which indicates if the value is specified. }; +/// @brief Specialization of the default @c Optional constructor for +/// strings. +/// +/// It calls default string object constructor. +template<> +inline Optional::Optional() + : default_(), unspecified_(true) { +} + /// @brief Inserts an optional value to a stream. /// /// This function overloads the global operator<< to behave as in -/// @c ostream::operator<< but applied to @c OptionalValue objects. +/// @c ostream::operator<< but applied to @c Optional objects. /// /// @param os A @c std::ostream object to which the value is inserted. -/// @param optional_value An @c OptionalValue object to be inserted into +/// @param optional_value An @c Optional object to be inserted into /// a stream. -/// @tparam Type of the value encapsulated by the @c OptionalValue object. +/// @tparam Type of the value encapsulated by the @c Optional object. /// /// @return A reference to the stream after insertion. template std::ostream& -operator<<(std::ostream& os, const OptionalValue& optional_value) { +operator<<(std::ostream& os, const Optional& optional_value) { os << optional_value.get(); return (os); } diff --git a/src/lib/util/tests/optional_value_unittest.cc b/src/lib/util/tests/optional_value_unittest.cc index 903cd05147..fe9d39e021 100644 --- a/src/lib/util/tests/optional_value_unittest.cc +++ b/src/lib/util/tests/optional_value_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2015-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -12,122 +12,65 @@ namespace { using namespace isc::util; -// This test checks that the constructor sets the values passed as arguments. -TEST(OptionalValueTest, constructor) { - // Do not specify the second parameter. The default should be that - // the value is "unspecified". - OptionalValue value1(10); +// This test checks that the constructors work correctly. +TEST(OptionalTest, constructor) { + // Explicitly set a value via constructor. The value becomes + // specified. + Optional value1(10); EXPECT_EQ(10, value1.get()); - EXPECT_FALSE(value1.isSpecified()); + EXPECT_FALSE(value1.unspecified()); - // Use the non-default value for second parameter. - OptionalValue value2(2, true); - EXPECT_EQ(2, value2.get()); - EXPECT_TRUE(value2.isSpecified()); + // Do not set a value in a constructor. The value should be + // unspecified. + Optional value2; + EXPECT_EQ(0, value2.get()); + EXPECT_TRUE(value2.unspecified()); } -// This test checks that the OptionalValue::set and OptionalValue::specify -// set the values as expected. -TEST(OptionalValueTest, set) { - OptionalValue value(10); - ASSERT_EQ(10, value.get()); - ASSERT_FALSE(value.isSpecified()); - - // Set new value. This should not change the "specified" flag. - value.set(100); - ASSERT_EQ(100, value.get()); - ASSERT_FALSE(value.isSpecified()); - - // Mark value "specified". The value itself should not change. - value.specify(OptionalValueState(true)); - ASSERT_EQ(100, value.get()); - ASSERT_TRUE(value.isSpecified()); - - // Once it is "specified", set the new value. It should remain specified. - value.set(5); - ASSERT_EQ(5, value.get()); - ASSERT_TRUE(value.isSpecified()); - - // Mark it "unspecified". The value should remain the same. - value.specify(OptionalValueState(false)); - ASSERT_EQ(5, value.get()); - ASSERT_FALSE(value.isSpecified()); -} +TEST(OptionalTest, constructorString) { + Optional value1("foo"); + EXPECT_EQ("foo", value1.get()); -// This test checks that the OptionalValue::specify functions may be used -// to set the new value and to mark value specified. -TEST(OptionalValueTest, specifyValue) { - OptionalValue value(10); - ASSERT_EQ(10, value.get()); - ASSERT_FALSE(value.isSpecified()); - - // Set the new value and mark it "specified". - value.specify(123); - ASSERT_EQ(123, value.get()); - ASSERT_TRUE(value.isSpecified()); - - // Specify another value. The value should be still "specified". - value.specify(1000); - ASSERT_EQ(1000, value.get()); - ASSERT_TRUE(value.isSpecified()); + Optional value2; + EXPECT_TRUE(value2.get().empty()); } // This test checks if the assignment operator assigning an actual // value to the optional value works as expected. -TEST(OptionalValueTest, assignValue) { - OptionalValue value(10); - ASSERT_EQ(10, value.get()); - ASSERT_FALSE(value.isSpecified()); +TEST(OptionalTest, assignValue) { + Optional value(10); + EXPECT_EQ(10, value.get()); + EXPECT_FALSE(value.unspecified()); - // Set the new value and mark it "specified". + // Assign a new value. value = 111; - ASSERT_EQ(111, value.get()); - ASSERT_TRUE(value.isSpecified()); + EXPECT_EQ(111, value.get()); + EXPECT_FALSE(value.unspecified()); - // Specify another value. The value should be still "specified". + // Assign another value. value = 1000; - ASSERT_EQ(1000, value.get()); - ASSERT_TRUE(value.isSpecified()); + EXPECT_EQ(1000, value.get()); + EXPECT_FALSE(value.unspecified()); } -// This test checks if the equality and inequality operators work -// correctly for the optional value. -TEST(OptionalValueTest, equalityOperators) { - OptionalValue value(10); - ASSERT_EQ(10, value.get()); - ASSERT_FALSE(value.isSpecified()); - - EXPECT_FALSE(value == 10); - EXPECT_TRUE(value != 10); - EXPECT_FALSE(value == 123); - EXPECT_TRUE(value != 123); - - value.specify(OptionalValueState(true)); - - EXPECT_TRUE(value == 10); - EXPECT_FALSE(value != 10); - EXPECT_FALSE(value == 123); - EXPECT_TRUE(value != 123); - - value = 123; - EXPECT_TRUE(value == 123); - EXPECT_FALSE(value != 123); - EXPECT_FALSE(value == 10); - EXPECT_TRUE(value != 10); - - value.specify(OptionalValueState(false)); - - EXPECT_FALSE(value == 123); - EXPECT_TRUE(value != 123); - EXPECT_FALSE(value == 10); - EXPECT_TRUE(value != 10); +// This test checks that it is possible to modify the flag that indicates +// if the value is specified or unspecified. +TEST(OptionalTest, modifyUnspecified) { + Optional value; + EXPECT_TRUE(value.unspecified()); + + value.unspecified(false); + EXPECT_FALSE(value.unspecified()); + + value.unspecified(true); + EXPECT_TRUE(value.unspecified()); } // This test checks if the type case operator returns correct value. -TEST(OptionalValueTest, typeCastOperator) { - OptionalValue value(-10, true); +TEST(OptionalTest, typeCastOperator) { + Optional value(-10); ASSERT_EQ(-10, value.get()); - ASSERT_TRUE(value.isSpecified()); + ASSERT_FALSE(value.unspecified()); int actual = value; EXPECT_EQ(-10, actual); -- GitLab From f51d01aeca4f685a3330ee89d277e83f4c0a17e4 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Mon, 25 Feb 2019 13:05:47 +0100 Subject: [PATCH 2/9] [#487,!242] Moved optional_value.h to optional.h --- src/bin/dhcp4/tests/dhcp4_client.h | 2 +- src/lib/dhcp/iface_mgr.h | 2 +- src/lib/dhcpsrv/cfg_subnets6.h | 4 ++-- src/lib/dhcpsrv/mysql_host_data_source.cc | 2 +- src/lib/dhcpsrv/parsers/dhcp_parsers.h | 4 ++-- src/lib/dhcpsrv/parsers/option_data_parser.h | 2 +- src/lib/util/Makefile.am | 4 ++-- src/lib/util/{optional_value.h => optional.h} | 0 src/lib/util/tests/Makefile.am | 2 +- .../{optional_value_unittest.cc => optional_unittest.cc} | 2 +- 10 files changed, 12 insertions(+), 12 deletions(-) rename src/lib/util/{optional_value.h => optional.h} (100%) rename src/lib/util/tests/{optional_value_unittest.cc => optional_unittest.cc} (98%) diff --git a/src/bin/dhcp4/tests/dhcp4_client.h b/src/bin/dhcp4/tests/dhcp4_client.h index 5201176bcf..e4e939cbbb 100644 --- a/src/bin/dhcp4/tests/dhcp4_client.h +++ b/src/bin/dhcp4/tests/dhcp4_client.h @@ -12,7 +12,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index 229bb5fd0a..1247b4fc21 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -16,7 +16,7 @@ #include #include #include -#include +#include #include #include diff --git a/src/lib/dhcpsrv/cfg_subnets6.h b/src/lib/dhcpsrv/cfg_subnets6.h index 18775ac4f7..3899fd2837 100644 --- a/src/lib/dhcpsrv/cfg_subnets6.h +++ b/src/lib/dhcpsrv/cfg_subnets6.h @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -14,7 +14,7 @@ #include #include #include -#include +#include #include #include diff --git a/src/lib/dhcpsrv/mysql_host_data_source.cc b/src/lib/dhcpsrv/mysql_host_data_source.cc index 8bb59e099e..c53ee40422 100644 --- a/src/lib/dhcpsrv/mysql_host_data_source.cc +++ b/src/lib/dhcpsrv/mysql_host_data_source.cc @@ -15,7 +15,7 @@ #include #include #include -#include +#include #include #include diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.h b/src/lib/dhcpsrv/parsers/dhcp_parsers.h index 92e1937cf3..8258bbe5e4 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_parsers.h +++ b/src/lib/dhcpsrv/parsers/dhcp_parsers.h @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -21,7 +21,7 @@ #include #include #include -#include +#include #include diff --git a/src/lib/dhcpsrv/parsers/option_data_parser.h b/src/lib/dhcpsrv/parsers/option_data_parser.h index b19406bbef..75d8191e3b 100644 --- a/src/lib/dhcpsrv/parsers/option_data_parser.h +++ b/src/lib/dhcpsrv/parsers/option_data_parser.h @@ -12,7 +12,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/src/lib/util/Makefile.am b/src/lib/util/Makefile.am index e045e81de9..c5ad6cad75 100644 --- a/src/lib/util/Makefile.am +++ b/src/lib/util/Makefile.am @@ -16,7 +16,7 @@ libkea_util_la_SOURCES += hash.h libkea_util_la_SOURCES += labeled_value.h labeled_value.cc libkea_util_la_SOURCES += memory_segment.h libkea_util_la_SOURCES += memory_segment_local.h memory_segment_local.cc -libkea_util_la_SOURCES += optional_value.h +libkea_util_la_SOURCES += optional.h libkea_util_la_SOURCES += pid_file.h pid_file.cc libkea_util_la_SOURCES += pointer_util.h libkea_util_la_SOURCES += process_spawn.h process_spawn.cc @@ -58,7 +58,7 @@ libkea_util_include_HEADERS = \ labeled_value.h \ memory_segment.h \ memory_segment_local.h \ - optional_value.h \ + optional.h \ pid_file.h \ pointer_util.h \ process_spawn.h \ diff --git a/src/lib/util/optional_value.h b/src/lib/util/optional.h similarity index 100% rename from src/lib/util/optional_value.h rename to src/lib/util/optional.h diff --git a/src/lib/util/tests/Makefile.am b/src/lib/util/tests/Makefile.am index 71d28c495f..b43f2f15a0 100644 --- a/src/lib/util/tests/Makefile.am +++ b/src/lib/util/tests/Makefile.am @@ -44,7 +44,7 @@ run_unittests_SOURCES += labeled_value_unittest.cc run_unittests_SOURCES += memory_segment_local_unittest.cc run_unittests_SOURCES += memory_segment_common_unittest.h run_unittests_SOURCES += memory_segment_common_unittest.cc -run_unittests_SOURCES += optional_value_unittest.cc +run_unittests_SOURCES += optional_unittest.cc run_unittests_SOURCES += pid_file_unittest.cc run_unittests_SOURCES += process_spawn_unittest.cc run_unittests_SOURCES += qid_gen_unittest.cc diff --git a/src/lib/util/tests/optional_value_unittest.cc b/src/lib/util/tests/optional_unittest.cc similarity index 98% rename from src/lib/util/tests/optional_value_unittest.cc rename to src/lib/util/tests/optional_unittest.cc index fe9d39e021..c39d49ddb7 100644 --- a/src/lib/util/tests/optional_value_unittest.cc +++ b/src/lib/util/tests/optional_unittest.cc @@ -5,7 +5,7 @@ // file, You can obtain one at http://mozilla.org/MPL/2.0/. #include -#include +#include #include namespace { -- GitLab From 69681285d0414389db46a7490f2554147bfc8779 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Mon, 25 Feb 2019 14:20:29 +0100 Subject: [PATCH 3/9] [#487,!242] Triplet now derives from Optional. --- src/lib/dhcpsrv/triplet.h | 44 +++++++++------------------------------ 1 file changed, 10 insertions(+), 34 deletions(-) diff --git a/src/lib/dhcpsrv/triplet.h b/src/lib/dhcpsrv/triplet.h index cb7ac09b49..02222f8060 100644 --- a/src/lib/dhcpsrv/triplet.h +++ b/src/lib/dhcpsrv/triplet.h @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -8,6 +8,7 @@ #define TRIPLET_H #include +#include namespace isc { namespace dhcp { @@ -33,9 +34,11 @@ namespace dhcp { /// it can be assigned to a plain integer or integer assigned to a Triplet. /// See TripletTest.operator test for details on an easy Triplet usage. template -class Triplet { +class Triplet : public util::Optional { public: + using util::Optional::get; + /// @brief Base type to Triplet conversion. /// /// Typically: uint32_t to Triplet assignment. It is very convenient @@ -44,27 +47,18 @@ public: /// @param other A number to be assigned as min, max and default value. Triplet& operator=(T other) { min_ = other; - default_ = other; + util::Optional::default_ = other; max_ = other; // The value is now specified because we just assigned one. - unspecified_ = false; + util::Optional::unspecified_ = false; return (*this); } - /// @brief Triplet to base type conversion - /// - /// Typically: Triplet to uint32_t assignment. It is very convenient - /// to be able to simply write uint32_t z = x; (where x is a Triplet) - operator T() const { - return (default_); - } - /// @brief Constructor without parameters. /// /// Marks value in @c Triplet unspecified. Triplet() - : min_(0), default_(0), max_(0), - unspecified_(true) { + : util::Optional(), min_(0), max_(0) { } /// @brief Sets a fixed value. @@ -74,16 +68,14 @@ public: /// /// @param value A number to be assigned as min, max and default value. Triplet(T value) - : min_(value), default_(value), max_(value), - unspecified_(false) { + : util::Optional(value), min_(value), max_(value) { } /// @brief Sets the default value and thresholds /// /// @throw BadValue if min <= def <= max rule is violated Triplet(T min, T def, T max) - : min_(min), default_(def), max_(max), - unspecified_(false) { + : util::Optional(def), min_(min), max_(max) { if ( (min_ > def) || (def > max_) ) { isc_throw(BadValue, "Invalid triplet values."); } @@ -92,9 +84,6 @@ public: /// @brief Returns a minimum allowed value T getMin() const { return (min_);} - /// @brief Returns the default value - T get() const { return (default_); } - /// @brief Returns value with a hint /// /// DHCP protocol treats any values sent by a client as hints. @@ -122,26 +111,13 @@ public: /// @brief Returns a maximum allowed value T getMax() const { return (max_); } - /// @brief Check if the value has been specified. - /// - /// @return true if the value hasn't been specified, or false otherwise. - bool unspecified() const { - return (unspecified_); - } - private: /// @brief the minimum value T min_; - /// @brief the default value - T default_; - /// @brief the maximum value T max_; - - /// @brief Indicates whether the value is unspecified. - bool unspecified_; }; -- GitLab From c3ec4cb50a24b0981fb17f969b1e5c3df6f318af Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 26 Feb 2019 12:45:14 +0100 Subject: [PATCH 4/9] [#487,!242] Use Optionals in the Subnet and Shared networks. --- src/bin/dhcp4/json_config_parser.cc | 4 +- src/bin/dhcp4/tests/config_parser_unittest.cc | 40 +++++----- src/bin/dhcp6/json_config_parser.cc | 4 +- src/bin/dhcp6/tests/config_parser_unittest.cc | 16 ++-- src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc | 2 +- src/lib/dhcpsrv/cfg_subnets6.cc | 4 +- src/lib/dhcpsrv/network.cc | 1 + src/lib/dhcpsrv/network.h | 55 +++++++------- src/lib/dhcpsrv/subnet.cc | 19 ++--- src/lib/dhcpsrv/subnet.h | 21 +++--- .../tests/shared_network_parser_unittest.cc | 8 +- .../shared_networks_list_parser_unittest.cc | 6 +- src/lib/dhcpsrv/tests/subnet_unittest.cc | 16 ++-- src/lib/util/optional.h | 49 +++++++++++-- src/lib/util/tests/optional_unittest.cc | 73 ++++++++++++++++++- 15 files changed, 215 insertions(+), 103 deletions(-) diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index 0fe2094b74..549e0957bd 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/src/bin/dhcp4/json_config_parser.cc @@ -210,14 +210,14 @@ public: continue; } - if (iface != (*subnet)->getIface()) { + if ((*subnet)->getIface() != iface) { isc_throw(DhcpConfigError, "Subnet " << (*subnet)->toText() << " has specified interface " << (*subnet)->getIface() << ", but earlier subnet in the same shared-network" << " or the shared-network itself used " << iface); } - if (authoritative != (*subnet)->getAuthoritative()) { + if ((*subnet)->getAuthoritative() != authoritative) { isc_throw(DhcpConfigError, "Subnet " << (*subnet)->toText() << " has different authoritative setting " << (*subnet)->getAuthoritative() diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index 489d8eec35..3de157ee31 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -1248,9 +1248,9 @@ TEST_F(Dhcp4ParserTest, nextServerGlobal) { Subnet4Ptr subnet = CfgMgr::instance().getStagingCfg()-> getCfgSubnets4()->selectSubnet(IOAddress("192.0.2.200")); ASSERT_TRUE(subnet); - EXPECT_EQ("1.2.3.4", subnet->getSiaddr().toText()); - EXPECT_EQ("foo", subnet->getSname()); - EXPECT_EQ("bar", subnet->getFilename()); + EXPECT_EQ("1.2.3.4", subnet->getSiaddr().get().toText()); + EXPECT_EQ("foo", subnet->getSname().get()); + EXPECT_EQ("bar", subnet->getFilename().get()); } // Checks if the next-server and other fixed BOOTP fields defined as @@ -1283,9 +1283,9 @@ TEST_F(Dhcp4ParserTest, nextServerSubnet) { Subnet4Ptr subnet = CfgMgr::instance().getStagingCfg()-> getCfgSubnets4()->selectSubnet(IOAddress("192.0.2.200")); ASSERT_TRUE(subnet); - EXPECT_EQ("1.2.3.4", subnet->getSiaddr().toText()); - EXPECT_EQ("foo", subnet->getSname()); - EXPECT_EQ("bar", subnet->getFilename()); + EXPECT_EQ("1.2.3.4", subnet->getSiaddr().get().toText()); + EXPECT_EQ("foo", subnet->getSname().get()); + EXPECT_EQ("bar", subnet->getFilename().get()); } // Test checks several negative scenarios for next-server configuration: bogus @@ -1431,9 +1431,9 @@ TEST_F(Dhcp4ParserTest, nextServerOverride) { Subnet4Ptr subnet = CfgMgr::instance().getStagingCfg()-> getCfgSubnets4()->selectSubnet(IOAddress("192.0.2.200")); ASSERT_TRUE(subnet); - EXPECT_EQ("1.2.3.4", subnet->getSiaddr().toText()); - EXPECT_EQ("some-name.example.org", subnet->getSname()); - EXPECT_EQ("bootfile.efi", subnet->getFilename()); + EXPECT_EQ("1.2.3.4", subnet->getSiaddr().get().toText()); + EXPECT_EQ("some-name.example.org", subnet->getSname().get()); + EXPECT_EQ("bootfile.efi", subnet->getFilename().get()); } // Check whether it is possible to configure echo-client-id @@ -5902,12 +5902,12 @@ TEST_F(Dhcp4ParserTest, sharedNetworksDerive) { ASSERT_TRUE(s); // These are values derived from shared network scope: - EXPECT_EQ("eth0", s->getIface()); + EXPECT_EQ("eth0", s->getIface().get()); EXPECT_FALSE(s->getMatchClientId()); EXPECT_TRUE(s->getAuthoritative()); EXPECT_EQ(IOAddress("1.2.3.4"), s->getSiaddr()); - EXPECT_EQ("foo", s->getSname()); - EXPECT_EQ("bar", s->getFilename()); + EXPECT_EQ("foo", s->getSname().get()); + EXPECT_EQ("bar", s->getFilename().get()); EXPECT_TRUE(s->hasRelayAddress(IOAddress("5.6.7.8"))); EXPECT_EQ(Network::HR_OUT_OF_POOL, s->getHostReservationMode()); @@ -5918,12 +5918,12 @@ TEST_F(Dhcp4ParserTest, sharedNetworksDerive) { s = checkSubnet(*subs, "192.0.2.0/24", 100, 200, 400); // These are values derived from shared network scope: - EXPECT_EQ("eth0", s->getIface()); + EXPECT_EQ("eth0", s->getIface().get()); EXPECT_TRUE(s->getMatchClientId()); EXPECT_TRUE(s->getAuthoritative()); - EXPECT_EQ(IOAddress("11.22.33.44"), s->getSiaddr()); - EXPECT_EQ("some-name.example.org", s->getSname()); - EXPECT_EQ("bootfile.efi", s->getFilename()); + EXPECT_EQ(IOAddress("11.22.33.44"), s->getSiaddr().get()); + EXPECT_EQ("some-name.example.org", s->getSname().get()); + EXPECT_EQ("bootfile.efi", s->getFilename().get()); EXPECT_TRUE(s->hasRelayAddress(IOAddress("55.66.77.88"))); EXPECT_EQ(Network::HR_DISABLED, s->getHostReservationMode()); @@ -5938,7 +5938,7 @@ TEST_F(Dhcp4ParserTest, sharedNetworksDerive) { // This subnet should derive its renew-timer from global scope. // All other parameters should have default values. s = checkSubnet(*subs, "192.0.3.0/24", 1, 2, 4); - EXPECT_EQ("", s->getIface()); + EXPECT_EQ("", s->getIface().get()); EXPECT_TRUE(s->getMatchClientId()); EXPECT_FALSE(s->getAuthoritative()); EXPECT_EQ(IOAddress("0.0.0.0"), s->getSiaddr()); @@ -6000,7 +6000,7 @@ TEST_F(Dhcp4ParserTest, sharedNetworksDeriveClientClass) { SharedNetwork4Ptr net = nets->at(0); ASSERT_TRUE(net); - EXPECT_EQ("alpha", net->getClientClass()); + EXPECT_EQ("alpha", net->getClientClass().get()); // The first shared network has two subnets. const Subnet4Collection * subs = net->getAllSubnets(); @@ -6011,12 +6011,12 @@ TEST_F(Dhcp4ParserTest, sharedNetworksDeriveClientClass) { // shared-network level. Subnet4Ptr s = checkSubnet(*subs, "192.0.1.0/24", 1, 2, 4); ASSERT_TRUE(s); - EXPECT_EQ("alpha", s->getClientClass()); + EXPECT_EQ("alpha", s->getClientClass().get()); // For the second subnet, the values are overridden on subnet level. // The value should not be inherited. s = checkSubnet(*subs, "192.0.2.0/24", 1, 2, 4); - EXPECT_EQ("beta", s->getClientClass()); // beta defined on subnet level + EXPECT_EQ("beta", s->getClientClass().get()); // beta defined on subnet level // Ok, now check the second shared network. It doesn't have anything defined // on shared-network or subnet level, so everything should have default diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc index 26d4831061..fff19d006c 100644 --- a/src/bin/dhcp6/json_config_parser.cc +++ b/src/bin/dhcp6/json_config_parser.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -298,7 +298,7 @@ public: continue; } - if (iface != (*subnet)->getIface()) { + if ((*subnet)->getIface() != iface) { isc_throw(DhcpConfigError, "Subnet " << (*subnet)->toText() << " has specified interface " << (*subnet)->getIface() << ", but earlier subnet in the same shared-network" diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index f2f96d9c47..ebb285e8c2 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -1396,7 +1396,7 @@ TEST_F(Dhcp6ParserTest, subnetInterface) { Subnet6Ptr subnet = CfgMgr::instance().getStagingCfg()->getCfgSubnets6()-> selectSubnet(IOAddress("2001:db8:1::5"), classify_); ASSERT_TRUE(subnet); - EXPECT_EQ(valid_iface_, subnet->getIface()); + EXPECT_EQ(valid_iface_, subnet->getIface().get()); } // This test checks if invalid interface name will be rejected in @@ -6304,14 +6304,14 @@ TEST_F(Dhcp6ParserTest, sharedNetworksDeriveInterfaces) { // subnet6 level. Subnet6Ptr s = checkSubnet(*subs, "2001:db1::/48", 900, 1800, 3600, 7200); ASSERT_TRUE(s); - EXPECT_EQ("eth0", s->getIface()); + EXPECT_EQ("eth0", s->getIface().get()); // For the second subnet, the renew-timer should be 100, because it // was specified explicitly. Other parameters a derived // from global scope to shared-network level and later again to // subnet6 level. checkSubnet(*subs, "2001:db2::/48", 900, 1800, 3600, 7200); - EXPECT_EQ("eth0", s->getIface()); + EXPECT_EQ("eth0", s->getIface().get()); // Ok, now check the second shared subnet. net = nets->at(1); @@ -6323,7 +6323,7 @@ TEST_F(Dhcp6ParserTest, sharedNetworksDeriveInterfaces) { // This subnet should derive its renew-timer from global scope. s = checkSubnet(*subs, "2001:db3::/48", 900, 1800, 3600, 7200); - EXPECT_EQ("", s->getIface()); + EXPECT_EQ("", s->getIface().get()); } @@ -6403,7 +6403,7 @@ TEST_F(Dhcp6ParserTest, sharedNetworksDeriveClientClass) { // Let's check the first one. SharedNetwork6Ptr net = nets->at(0); ASSERT_TRUE(net); - EXPECT_EQ("alpha", net->getClientClass()); + EXPECT_EQ("alpha", net->getClientClass().get()); const Subnet6Collection * subs = net->getAllSubnets(); ASSERT_TRUE(subs); @@ -6413,13 +6413,13 @@ TEST_F(Dhcp6ParserTest, sharedNetworksDeriveClientClass) { // shared-network level. Subnet6Ptr s = checkSubnet(*subs, "2001:db1::/48", 900, 1800, 3600, 7200); ASSERT_TRUE(s); - EXPECT_EQ("alpha", s->getClientClass()); + EXPECT_EQ("alpha", s->getClientClass().get()); // For the second subnet, the values are overridden on subnet level. // The value should not be inherited. s = checkSubnet(*subs, "2001:db2::/48", 900, 1800, 3600, 7200); ASSERT_TRUE(s); - EXPECT_EQ("beta", s->getClientClass()); // beta defined on subnet level + EXPECT_EQ("beta", s->getClientClass().get()); // beta defined on subnet level // Ok, now check the second shared network. It doesn't have // anything defined on shared-network or subnet level, so diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc index 7d9db8891a..5eaffb98f3 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc @@ -732,7 +732,7 @@ public: MySqlBinding::condCreateString(subnet->getIface()), MySqlBinding::createInteger(static_cast(subnet->getMatchClientId())), MySqlBinding::createTimestamp(subnet->getModificationTime()), - MySqlBinding::condCreateInteger(subnet->getSiaddr().toUint32()), + MySqlBinding::condCreateInteger(subnet->getSiaddr().get().toUint32()), createBinding(subnet->getT2()), createInputRelayBinding(subnet), createBinding(subnet->getT1()), diff --git a/src/lib/dhcpsrv/cfg_subnets6.cc b/src/lib/dhcpsrv/cfg_subnets6.cc index ed8592d713..0370f8fd68 100644 --- a/src/lib/dhcpsrv/cfg_subnets6.cc +++ b/src/lib/dhcpsrv/cfg_subnets6.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -188,7 +188,7 @@ CfgSubnets6::selectSubnet(const std::string& iface_name, // If interface name matches with the one specified for the subnet // and the client is not rejected based on the classification, // return the subnet. - if ((iface_name == (*subnet)->getIface()) && + if (((*subnet)->getIface() == iface_name) && (*subnet)->clientSupported(client_classes)) { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE, diff --git a/src/lib/dhcpsrv/network.cc b/src/lib/dhcpsrv/network.cc index 2320122926..af2e1262f1 100644 --- a/src/lib/dhcpsrv/network.cc +++ b/src/lib/dhcpsrv/network.cc @@ -14,6 +14,7 @@ using namespace isc::asiolink; using namespace isc::data; +using namespace isc::util; namespace isc { namespace dhcp { diff --git a/src/lib/dhcpsrv/network.h b/src/lib/dhcpsrv/network.h index a1fb3abceb..4095214d99 100644 --- a/src/lib/dhcpsrv/network.h +++ b/src/lib/dhcpsrv/network.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -135,7 +136,7 @@ public: /// other resources to a client. /// /// @param iface_name Interface name. - void setIface(const std::string& iface_name) { + void setIface(const util::Optional& iface_name) { iface_name_ = iface_name; } @@ -143,7 +144,7 @@ public: /// selected. /// /// @return Interface name as text. - std::string getIface() const { + util::Optional getIface() const { return (iface_name_); }; @@ -230,7 +231,7 @@ public: void requireClientClass(const isc::dhcp::ClientClass& class_name); /// @brief Returns classes which are required to be evaluated - const isc::dhcp::ClientClasses& getRequiredClasses() const; + const ClientClasses& getRequiredClasses() const; /// @brief returns the client class /// @@ -238,7 +239,7 @@ public: /// returned it is valid. /// /// @return client class @ref client_class_ - const isc::dhcp::ClientClass& getClientClass() const { + const util::Optional& getClientClass() const { return (client_class_); } @@ -286,7 +287,7 @@ public: /// performance reasons. /// /// @return whether in-pool host reservations are allowed. - HRMode + util::Optional getHostReservationMode() const { return (host_reservation_mode_); } @@ -296,7 +297,7 @@ public: /// See @ref getHostReservationMode for details. /// /// @param mode mode to be set - void setHostReservationMode(HRMode mode) { + void setHostReservationMode(const util::Optional& mode) { host_reservation_mode_ = mode; } @@ -312,38 +313,38 @@ public: } /// @brief Returns whether or not T1/T2 calculation is enabled. - bool getCalculateTeeTimes() const { + util::Optional getCalculateTeeTimes() const { return (calculate_tee_times_); } /// @brief Sets whether or not T1/T2 calculation is enabled. /// /// @param calculate_tee_times new value of enabled/disabled. - void setCalculateTeeTimes(const bool& calculate_tee_times) { + void setCalculateTeeTimes(const util::Optional& calculate_tee_times) { calculate_tee_times_ = calculate_tee_times; } /// @brief Returns percentage to use when calculating the T1 (renew timer). - double getT1Percent() const { + util::Optional getT1Percent() const { return (t1_percent_); } /// @brief Sets new precentage for calculating T1 (renew timer). /// /// @param t1_percent New percentage to use. - void setT1Percent(const double& t1_percent) { + void setT1Percent(const util::Optional& t1_percent) { t1_percent_ = t1_percent; } /// @brief Returns percentage to use when calculating the T2 (rebind timer). - double getT2Percent() const { + util::Optional getT2Percent() const { return (t2_percent_); } /// @brief Sets new precentage for calculating T2 (rebind timer). /// /// @param t2_percent New percentage to use. - void setT2Percent(const double& t2_percent) { + void setT2Percent(const util::Optional& t2_percent) { t2_percent_ = t2_percent; } @@ -355,7 +356,7 @@ public: protected: /// @brief Holds interface name for which this network is selected. - std::string iface_name_; + util::Optional iface_name_; /// @brief Relay information /// @@ -367,7 +368,7 @@ protected: /// If defined, only clients belonging to that class will be allowed to use /// this particular network. The default value for this is an empty string, /// which means that any client is allowed, regardless of its class. - ClientClass client_class_; + util::Optional client_class_; /// @brief Required classes /// @@ -387,19 +388,19 @@ protected: /// @brief Specifies host reservation mode /// /// See @ref HRMode type for details. - HRMode host_reservation_mode_; + util::Optional host_reservation_mode_; /// @brief Pointer to the option data configuration for this subnet. CfgOptionPtr cfg_option_; /// @brief Enables calculation of T1 and T2 timers - bool calculate_tee_times_; + util::Optional calculate_tee_times_; /// @brief Percentage of the lease lifetime to use when calculating T1 timer - double t1_percent_; + util::Optional t1_percent_; /// @brief Percentage of the lease lifetime to use when calculating T2 timer - double t2_percent_; + util::Optional t2_percent_; }; /// @brief Pointer to the @ref Network object. @@ -421,7 +422,7 @@ public: /// be used to identify the client's lease. /// /// @return true if client identifiers should be used, false otherwise. - bool getMatchClientId() const { + util::Optional getMatchClientId() const { return (match_client_id_); } @@ -430,7 +431,7 @@ public: /// /// @param match If this value is true, the client identifiers are not /// used for lease lookup. - void setMatchClientId(const bool match) { + void setMatchClientId(const util::Optional& match) { match_client_id_ = match; } @@ -439,7 +440,7 @@ public: /// /// @return true if requests for unknown IP addresses should be rejected, /// false otherwise. - bool getAuthoritative() const { + util::Optional getAuthoritative() const { return (authoritative_); } @@ -448,7 +449,7 @@ public: /// /// @param authoritative If this value is true, the requests for unknown IP /// addresses will be rejected with DHCPNAK messages - void setAuthoritative(const bool authoritative) { + void setAuthoritative(const util::Optional& authoritative) { authoritative_ = authoritative; } @@ -467,10 +468,10 @@ private: /// @brief Should server use client identifiers for client lease /// lookup. - bool match_client_id_; + util::Optional match_client_id_; /// @brief Should requests for unknown IP addresses be rejected. - bool authoritative_; + util::Optional authoritative_; }; /// @brief Specialization of the @ref Network object for DHCPv6 case. @@ -514,7 +515,7 @@ public: /// is supported or unsupported for the subnet. /// /// @return true if the Rapid Commit option is supported, false otherwise. - bool getRapidCommit() const { + util::Optional getRapidCommit() const { return (rapid_commit_); } @@ -522,7 +523,7 @@ public: /// /// @param rapid_commit A boolean value indicating that the Rapid Commit /// option support is enabled (if true), or disabled (if false). - void setRapidCommit(const bool rapid_commit) { + void setRapidCommit(const util::Optional& rapid_commit) { rapid_commit_ = rapid_commit; }; @@ -544,7 +545,7 @@ private: /// /// It's default value is false, which indicates that the Rapid /// Commit is disabled for the subnet. - bool rapid_commit_; + util::Optional rapid_commit_; }; } // end of namespace isc::dhcp diff --git a/src/lib/dhcpsrv/subnet.cc b/src/lib/dhcpsrv/subnet.cc index eb41681dae..78f9b9de49 100644 --- a/src/lib/dhcpsrv/subnet.cc +++ b/src/lib/dhcpsrv/subnet.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -18,6 +18,7 @@ using namespace isc::asiolink; using namespace isc::data; using namespace isc::dhcp; +using namespace isc::util; namespace { @@ -309,30 +310,30 @@ Subnet4::clientSupported(const isc::dhcp::ClientClasses& client_classes) const { return (Network4::clientSupported(client_classes)); } -void Subnet4::setSiaddr(const isc::asiolink::IOAddress& siaddr) { - if (!siaddr.isV4()) { +void Subnet4::setSiaddr(const Optional& siaddr) { + if (!siaddr.get().isV4()) { isc_throw(BadValue, "Can't set siaddr to non-IPv4 address " << siaddr); } siaddr_ = siaddr; } -isc::asiolink::IOAddress Subnet4::getSiaddr() const { +Optional Subnet4::getSiaddr() const { return (siaddr_); } -void Subnet4::setSname(const std::string& sname) { +void Subnet4::setSname(const Optional& sname) { sname_ = sname; } -const std::string& Subnet4::getSname() const { +const Optional& Subnet4::getSname() const { return (sname_); } -void Subnet4::setFilename(const std::string& filename) { +void Subnet4::setFilename(const Optional& filename) { filename_ = filename; } -const std::string& Subnet4::getFilename() const { +const Optional& Subnet4::getFilename() const { return (filename_); } @@ -717,7 +718,7 @@ Subnet4::toElement() const { isc::data::merge(map, d4o6.toElement()); // Set next-server - map->set("next-server", Element::create(getSiaddr().toText())); + map->set("next-server", Element::create(getSiaddr().get().toText())); // Set server-hostname map->set("server-hostname", Element::create(getSname())); diff --git a/src/lib/dhcpsrv/subnet.h b/src/lib/dhcpsrv/subnet.h index bd28c7ebfb..0e720ede0b 100644 --- a/src/lib/dhcpsrv/subnet.h +++ b/src/lib/dhcpsrv/subnet.h @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -536,32 +537,32 @@ public: /// Will be used for siaddr field (the next server) that typically is used /// as TFTP server. If not specified, the default value of 0.0.0.0 is /// used. - void setSiaddr(const isc::asiolink::IOAddress& siaddr); + void setSiaddr(const util::Optional& siaddr); /// @brief Returns siaddr for this subnet /// /// @return siaddr value - isc::asiolink::IOAddress getSiaddr() const; + util::Optional getSiaddr() const; /// @brief Sets server hostname for the Subnet4 /// /// Will be used for server hostname field (may be empty if not defined) - void setSname(const std::string& sname); + void setSname(const util::Optional& sname); /// @brief Returns server hostname for this subnet /// /// @return server hostname value - const std::string& getSname() const; + const util::Optional& getSname() const; /// @brief Sets boot file name for the Subnet4 /// /// Will be used for boot file name (may be empty if not defined) - void setFilename(const std::string& filename); + void setFilename(const util::Optional& filename); /// @brief Returns boot file name for this subnet /// /// @return boot file name value - const std::string& getFilename() const; + const util::Optional& getFilename() const; /// @brief Returns DHCP4o6 configuration parameters. /// @@ -608,13 +609,13 @@ private: virtual void checkType(Lease::Type type) const; /// @brief siaddr value for this subnet - isc::asiolink::IOAddress siaddr_; + util::Optional siaddr_; /// @brief server hostname for this subnet - std::string sname_; + util::Optional sname_; /// @brief boot file name for this subnet - std::string filename_; + util::Optional filename_; /// @brief All the information related to DHCP4o6 Cfg4o6 dhcp4o6_; diff --git a/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc b/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc index d0599a5f35..df82864006 100644 --- a/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc @@ -214,7 +214,7 @@ TEST_F(SharedNetwork4ParserTest, parse) { // Check basic parameters. EXPECT_EQ("bird", network->getName()); - EXPECT_EQ("eth1", network->getIface()); + EXPECT_EQ("eth1", network->getIface().get()); // Check user context. ConstElementPtr context = network->getContext(); @@ -275,7 +275,7 @@ TEST_F(SharedNetwork4ParserTest, clientClassMatchClientIdAuthoritative) { network = parser.parse(config_element); ASSERT_TRUE(network); - EXPECT_EQ("alpha", network->getClientClass()); + EXPECT_EQ("alpha", network->getClientClass().get()); EXPECT_FALSE(network->getMatchClientId()); @@ -449,7 +449,7 @@ TEST_F(SharedNetwork6ParserTest, parse) { // Check basic parameters. EXPECT_EQ("bird", network->getName()); - EXPECT_EQ("eth1", network->getIface()); + EXPECT_EQ("eth1", network->getIface().get()); // Check user context. ConstElementPtr context = network->getContext(); @@ -494,7 +494,7 @@ TEST_F(SharedNetwork6ParserTest, clientClass) { network = parser.parse(config_element); ASSERT_TRUE(network); - EXPECT_EQ("alpha", network->getClientClass()); + EXPECT_EQ("alpha", network->getClientClass().get()); } // This test verifies that it's possible to specify require-client-classes diff --git a/src/lib/dhcpsrv/tests/shared_networks_list_parser_unittest.cc b/src/lib/dhcpsrv/tests/shared_networks_list_parser_unittest.cc index 53289c1e9a..ebe329308f 100644 --- a/src/lib/dhcpsrv/tests/shared_networks_list_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/shared_networks_list_parser_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -43,13 +43,13 @@ TEST(SharedNetworkListParserTest, parse) { SharedNetwork4Ptr network1 = cfg->getByName("bird"); ASSERT_TRUE(network1); EXPECT_EQ("bird", network1->getName()); - EXPECT_EQ("eth0", network1->getIface()); + EXPECT_EQ("eth0", network1->getIface().get()); EXPECT_FALSE(network1->getContext()); SharedNetwork4Ptr network2 = cfg->getByName("monkey"); ASSERT_TRUE(network2); EXPECT_EQ("monkey", network2->getName()); - EXPECT_EQ("eth1", network2->getIface()); + EXPECT_EQ("eth1", network2->getIface().get()); ASSERT_TRUE(network2->getContext()); EXPECT_EQ(1, network2->getContext()->size()); EXPECT_TRUE(network2->getContext()->get("comment")); diff --git a/src/lib/dhcpsrv/tests/subnet_unittest.cc b/src/lib/dhcpsrv/tests/subnet_unittest.cc index e69c6fc961..54a2af4d26 100644 --- a/src/lib/dhcpsrv/tests/subnet_unittest.cc +++ b/src/lib/dhcpsrv/tests/subnet_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -122,13 +122,13 @@ TEST(Subnet4Test, siaddr) { Subnet4 subnet(IOAddress("192.0.2.1"), 24, 1000, 2000, 3000); // Check if the default is 0.0.0.0 - EXPECT_EQ("0.0.0.0", subnet.getSiaddr().toText()); + EXPECT_EQ("0.0.0.0", subnet.getSiaddr().get().toText()); // Check that we can set it up EXPECT_NO_THROW(subnet.setSiaddr(IOAddress("1.2.3.4"))); // Check that we can get it back - EXPECT_EQ("1.2.3.4", subnet.getSiaddr().toText()); + EXPECT_EQ("1.2.3.4", subnet.getSiaddr().get().toText()); // Check that only v4 addresses are supported EXPECT_THROW(subnet.setSiaddr(IOAddress("2001:db8::1")), @@ -146,7 +146,7 @@ TEST(Subnet4Test, serverHostname) { EXPECT_NO_THROW(subnet.setSname("foobar")); // Check that we can get it back - EXPECT_EQ("foobar", subnet.getSname()); + EXPECT_EQ("foobar", subnet.getSname().get()); } // Checks whether boot-file-name field can be set and retrieved correctly. @@ -160,7 +160,7 @@ TEST(Subnet4Test, bootFileName) { EXPECT_NO_THROW(subnet.setFilename("foobar")); // Check that we can get it back - EXPECT_EQ("foobar", subnet.getFilename()); + EXPECT_EQ("foobar", subnet.getFilename().get()); } // Checks if the match-client-id flag can be set and retrieved. @@ -448,7 +448,7 @@ TEST(Subnet4Test, clientClass) { // Let's allow only clients belonging to "bar" class. subnet->allowClientClass("bar"); - EXPECT_EQ("bar", subnet->getClientClass()); + EXPECT_EQ("bar", subnet->getClientClass().get()); EXPECT_FALSE(subnet->clientSupported(no_class)); EXPECT_FALSE(subnet->clientSupported(foo_class)); @@ -1032,7 +1032,7 @@ TEST(Subnet6Test, clientClass) { // Let's allow only clients belonging to "bar" class. subnet->allowClientClass("bar"); - EXPECT_EQ("bar", subnet->getClientClass()); + EXPECT_EQ("bar", subnet->getClientClass().get()); EXPECT_FALSE(subnet->clientSupported(no_class)); EXPECT_FALSE(subnet->clientSupported(foo_class)); @@ -1480,7 +1480,7 @@ TEST(Subnet6Test, iface) { EXPECT_TRUE(subnet.getIface().empty()); subnet.setIface("en1"); - EXPECT_EQ("en1", subnet.getIface()); + EXPECT_EQ("en1", subnet.getIface().get()); } // This trivial test checks if the interface-id option can be set and diff --git a/src/lib/util/optional.h b/src/lib/util/optional.h index bda8272a03..626c5c3b48 100644 --- a/src/lib/util/optional.h +++ b/src/lib/util/optional.h @@ -7,6 +7,7 @@ #ifndef OPTIONAL_H #define OPTIONAL_H +#include #include #include @@ -37,8 +38,11 @@ public: /// @brief Assigns a new value value and marks it "specified". /// + /// @tparam A Type of the value to be assigned. Typically this is @c T, but + /// may also be a type that can be cast to @c T. /// @param other new actual value. - Optional& operator=(T other) { + template + Optional& operator=(A other) { default_ = other; unspecified_ = false; return (*this); @@ -54,23 +58,40 @@ public: return (default_); } + /// @brief Equality operator. + /// + /// @param other value to be compared. + bool operator==(const T& other) const { + return (default_ == other); + } + + /// @brief Inequality operator. + /// + /// @param other value to be compared. + bool operator!=(const T& other) const { + return (default_ != other); + } + /// @brief Default constructor. /// - /// Sets the encapsulated value to 0. + /// Sets the encapsulated value to 0 and marks it as "unspecified". Optional() : default_(T(0)), unspecified_(true) { } /// @brief Constructor /// - /// Creates optional value and marks it as "specified". + /// Sets an explicit value and marks it as "specified". /// + /// @tparam A Type of the value to be assigned. Typically this is @c T, but + /// may also be a type that can be cast to @c T. /// @param value value to be assigned. - explicit Optional(T value) + template + Optional(A value) : default_(value), unspecified_(false) { } - /// @brief Retrieves the actual value. + /// @brief Retrieves the encapsulated value. T get() const { return (default_); } @@ -91,6 +112,16 @@ public: return (unspecified_); } + /// @brief Checks if the encapsulated value is empty. + /// + /// This method can be overloaded in the template specializations that + /// are dedicated to strings, vectors etc. + /// + /// @throw isc::InvalidOperation. + bool empty() const { + isc_throw(isc::InvalidOperation, "call to empty() not supported"); + } + protected: T default_; ///< Encapsulated value. bool unspecified_; ///< Flag which indicates if the value is specified. @@ -105,6 +136,14 @@ inline Optional::Optional() : default_(), unspecified_(true) { } +/// @brief Specialization of the @c Optional::empty method for strings. +/// +/// @return true if the value is empty, false otherwise. +template<> +inline bool Optional::empty() const { + return (default_.empty()); +} + /// @brief Inserts an optional value to a stream. /// /// This function overloads the global operator<< to behave as in diff --git a/src/lib/util/tests/optional_unittest.cc b/src/lib/util/tests/optional_unittest.cc index c39d49ddb7..ebfc2007ca 100644 --- a/src/lib/util/tests/optional_unittest.cc +++ b/src/lib/util/tests/optional_unittest.cc @@ -27,12 +27,16 @@ TEST(OptionalTest, constructor) { EXPECT_TRUE(value2.unspecified()); } +// This test checks if the constructors for a string value +// work correctly. TEST(OptionalTest, constructorString) { Optional value1("foo"); EXPECT_EQ("foo", value1.get()); + EXPECT_FALSE(value1.unspecified()); Optional value2; EXPECT_TRUE(value2.get().empty()); + EXPECT_TRUE(value2.unspecified()); } // This test checks if the assignment operator assigning an actual @@ -53,6 +57,22 @@ TEST(OptionalTest, assignValue) { EXPECT_FALSE(value.unspecified()); } +// This test checks if the assignment operator assigning an actual +// string value to the optional value works as expected. +TEST(OptionalTest, assignStringValue) { + Optional value("foo"); + EXPECT_EQ("foo", value.get()); + EXPECT_FALSE(value.unspecified()); + + value = "bar"; + EXPECT_EQ("bar", value.get()); + EXPECT_FALSE(value.unspecified()); + + value = "foobar"; + EXPECT_EQ("foobar", value.get()); + EXPECT_FALSE(value.unspecified()); +} + // This test checks that it is possible to modify the flag that indicates // if the value is specified or unspecified. TEST(OptionalTest, modifyUnspecified) { @@ -69,11 +89,60 @@ TEST(OptionalTest, modifyUnspecified) { // This test checks if the type case operator returns correct value. TEST(OptionalTest, typeCastOperator) { Optional value(-10); - ASSERT_EQ(-10, value.get()); - ASSERT_FALSE(value.unspecified()); + EXPECT_EQ(-10, value.get()); + EXPECT_FALSE(value.unspecified()); int actual = value; EXPECT_EQ(-10, actual); } +// This test checks if the type case operator returns correct string +// value. +TEST(OptionalTest, stringCastOperator) { + Optional value("xyz"); + EXPECT_EQ("xyz", value.get()); + EXPECT_FALSE(value.unspecified()); + + std::string actual = value; + EXPECT_EQ("xyz", actual); +} + +// This test checks that the equality operators work as expected. +TEST(OptionalTest, equality) { + int exp_value = 1234; + Optional value(1234); + EXPECT_TRUE(value == exp_value); + EXPECT_FALSE(value != exp_value); +} + +// This test checks that the equality operators for strings work as +// expected. +TEST(OptionalTest, stringEquality) { + const char* exp_value = "foo"; + Optional value("foo"); + EXPECT_TRUE(value == exp_value); + EXPECT_FALSE(value != exp_value); +} + +// This test checks that an exception is thrown when calling an empty() +// method on non-string optional value. +TEST(OptionalTest, empty) { + Optional value(10); + EXPECT_THROW(value.empty(), isc::InvalidOperation); +} + +// This test checks that no exception is thrown when calling an empty() +// method on string optional value and that it returns an expected +// boolean value. +TEST(OptionalTest, stringEmpty) { + Optional value("foo"); + bool is_empty = true; + ASSERT_NO_THROW(is_empty = value.empty()); + EXPECT_FALSE(is_empty); + + value = ""; + ASSERT_NO_THROW(is_empty = value.empty()); + EXPECT_TRUE(is_empty); +} + } // end of anonymous namespace -- GitLab From 8ceec1b306ba68aaa30eb515573d28d34f377328 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 26 Feb 2019 14:27:57 +0100 Subject: [PATCH 5/9] [#487,!242] Simple optional parameters are unit tested in subnet and netw. --- src/lib/dhcpsrv/network.h | 10 +- src/lib/dhcpsrv/subnet.cc | 3 +- .../dhcpsrv/tests/shared_network_unittest.cc | 78 ++++++++++++++- src/lib/dhcpsrv/tests/subnet_unittest.cc | 95 +++++++++++++++++++ src/lib/util/optional.h | 5 +- src/lib/util/tests/optional_unittest.cc | 4 + 6 files changed, 185 insertions(+), 10 deletions(-) diff --git a/src/lib/dhcpsrv/network.h b/src/lib/dhcpsrv/network.h index 4095214d99..804684a9be 100644 --- a/src/lib/dhcpsrv/network.h +++ b/src/lib/dhcpsrv/network.h @@ -118,9 +118,9 @@ public: /// @brief Constructor. Network() - : iface_name_(), client_class_(""), t1_(), t2_(), valid_(), - host_reservation_mode_(HR_ALL), cfg_option_(new CfgOption()), - calculate_tee_times_(false), t1_percent_(0.0), t2_percent_(0.0) { + : iface_name_(), client_class_(), t1_(), t2_(), valid_(), + host_reservation_mode_(HR_ALL, true), cfg_option_(new CfgOption()), + calculate_tee_times_(), t1_percent_(), t2_percent_() { } /// @brief Virtual destructor. @@ -415,7 +415,7 @@ public: /// @brief Constructor. Network4() - : Network(), match_client_id_(true), authoritative_(false) { + : Network(), match_client_id_(true, true), authoritative_() { } /// @brief Returns the flag indicating if the client identifiers should @@ -480,7 +480,7 @@ public: /// @brief Constructor. Network6() - : Network(), preferred_(0), interface_id_(), rapid_commit_(false) { + : Network(), preferred_(), interface_id_(), rapid_commit_() { } /// @brief Returns preferred lifetime (in seconds) diff --git a/src/lib/dhcpsrv/subnet.cc b/src/lib/dhcpsrv/subnet.cc index 78f9b9de49..c14c2b3553 100644 --- a/src/lib/dhcpsrv/subnet.cc +++ b/src/lib/dhcpsrv/subnet.cc @@ -247,8 +247,7 @@ Subnet4::Subnet4(const isc::asiolink::IOAddress& prefix, uint8_t length, const Triplet& t2, const Triplet& valid_lifetime, const SubnetID id) - : Subnet(prefix, length, id), Network4(), - siaddr_(IOAddress("0.0.0.0")) { + : Subnet(prefix, length, id), Network4(), siaddr_() { if (!prefix.isV4()) { isc_throw(BadValue, "Non IPv4 prefix " << prefix.toText() << " specified in subnet4"); diff --git a/src/lib/dhcpsrv/tests/shared_network_unittest.cc b/src/lib/dhcpsrv/tests/shared_network_unittest.cc index cbbc01e51c..7060d7fd02 100644 --- a/src/lib/dhcpsrv/tests/shared_network_unittest.cc +++ b/src/lib/dhcpsrv/tests/shared_network_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -22,6 +22,44 @@ using namespace isc::dhcp; namespace { +// This test verifies the default values set for the shared +// networks and verifies that the optional values are unspecified. +TEST(SharedNetwork4Test, defaults) { + SharedNetwork4Ptr network(new SharedNetwork4("frog")); + EXPECT_TRUE(network->getIface().unspecified()); + EXPECT_TRUE(network->getIface().empty()); + + EXPECT_TRUE(network->getClientClass().unspecified()); + EXPECT_TRUE(network->getClientClass().empty()); + + EXPECT_TRUE(network->getValid().unspecified()); + EXPECT_EQ(0, network->getValid().get()); + + EXPECT_TRUE(network->getT1().unspecified()); + EXPECT_EQ(0, network->getT1().get()); + + EXPECT_TRUE(network->getT2().unspecified()); + EXPECT_EQ(0, network->getT2().get()); + + EXPECT_TRUE(network->getHostReservationMode().unspecified()); + EXPECT_EQ(Network::HR_ALL, network->getHostReservationMode().get()); + + EXPECT_TRUE(network->getCalculateTeeTimes().unspecified()); + EXPECT_FALSE(network->getCalculateTeeTimes().get()); + + EXPECT_TRUE(network->getT1Percent().unspecified()); + EXPECT_EQ(0.0, network->getT1Percent().get()); + + EXPECT_TRUE(network->getT2Percent().unspecified()); + EXPECT_EQ(0.0, network->getT2Percent().get()); + + EXPECT_TRUE(network->getMatchClientId().unspecified()); + EXPECT_TRUE(network->getMatchClientId().get()); + + EXPECT_TRUE(network->getAuthoritative().unspecified()); + EXPECT_FALSE(network->getAuthoritative().get()); +} + // This test verifies that shared network can be given a name and that // this name can be retrieved. TEST(SharedNetwork4Test, getName) { @@ -426,6 +464,44 @@ TEST(SharedNetwork4Test, delAll) { ASSERT_EQ(0, network->getAllSubnets()->size()); } +// This test verifies the default values set for the shared +// networks and verifies that the optional values are unspecified. +TEST(SharedNetwork6Test, defaults) { + SharedNetwork6Ptr network(new SharedNetwork6("frog")); + EXPECT_TRUE(network->getIface().unspecified()); + EXPECT_TRUE(network->getIface().empty()); + + EXPECT_TRUE(network->getClientClass().unspecified()); + EXPECT_TRUE(network->getClientClass().empty()); + + EXPECT_TRUE(network->getValid().unspecified()); + EXPECT_EQ(0, network->getValid().get()); + + EXPECT_TRUE(network->getT1().unspecified()); + EXPECT_EQ(0, network->getT1().get()); + + EXPECT_TRUE(network->getT2().unspecified()); + EXPECT_EQ(0, network->getT2().get()); + + EXPECT_TRUE(network->getHostReservationMode().unspecified()); + EXPECT_EQ(Network::HR_ALL, network->getHostReservationMode().get()); + + EXPECT_TRUE(network->getCalculateTeeTimes().unspecified()); + EXPECT_FALSE(network->getCalculateTeeTimes().get()); + + EXPECT_TRUE(network->getT1Percent().unspecified()); + EXPECT_EQ(0.0, network->getT1Percent().get()); + + EXPECT_TRUE(network->getT2Percent().unspecified()); + EXPECT_EQ(0.0, network->getT2Percent().get()); + + EXPECT_TRUE(network->getPreferred().unspecified()); + EXPECT_EQ(0, network->getPreferred().get()); + + EXPECT_TRUE(network->getRapidCommit().unspecified()); + EXPECT_FALSE(network->getRapidCommit().get()); +} + // This test verifies that shared network can be given a name and that // this name can be retrieved. TEST(SharedNetwork6Test, getName) { diff --git a/src/lib/dhcpsrv/tests/subnet_unittest.cc b/src/lib/dhcpsrv/tests/subnet_unittest.cc index 54a2af4d26..6937d21446 100644 --- a/src/lib/dhcpsrv/tests/subnet_unittest.cc +++ b/src/lib/dhcpsrv/tests/subnet_unittest.cc @@ -43,6 +43,57 @@ TEST(Subnet4Test, constructor) { BadValue); // IPv6 addresses are not allowed in Subnet4 } +// This test verifies the default values set for the subnets and verifies +// that the optional values are unspecified. +TEST(Subnet4Test, defaults) { + Triplet t1; + Triplet t2; + Triplet valid_lft; + Subnet4 subnet(IOAddress("192.0.2.0"), 24, t1, t2, valid_lft); + + EXPECT_TRUE(subnet.getIface().unspecified()); + EXPECT_TRUE(subnet.getIface().empty()); + + EXPECT_TRUE(subnet.getClientClass().unspecified()); + EXPECT_TRUE(subnet.getClientClass().empty()); + + EXPECT_TRUE(subnet.getValid().unspecified()); + EXPECT_EQ(0, subnet.getValid().get()); + + EXPECT_TRUE(subnet.getT1().unspecified()); + EXPECT_EQ(0, subnet.getT1().get()); + + EXPECT_TRUE(subnet.getT2().unspecified()); + EXPECT_EQ(0, subnet.getT2().get()); + + EXPECT_TRUE(subnet.getHostReservationMode().unspecified()); + EXPECT_EQ(Network::HR_ALL, subnet.getHostReservationMode().get()); + + EXPECT_TRUE(subnet.getCalculateTeeTimes().unspecified()); + EXPECT_FALSE(subnet.getCalculateTeeTimes().get()); + + EXPECT_TRUE(subnet.getT1Percent().unspecified()); + EXPECT_EQ(0.0, subnet.getT1Percent().get()); + + EXPECT_TRUE(subnet.getT2Percent().unspecified()); + EXPECT_EQ(0.0, subnet.getT2Percent().get()); + + EXPECT_TRUE(subnet.getMatchClientId().unspecified()); + EXPECT_TRUE(subnet.getMatchClientId().get()); + + EXPECT_TRUE(subnet.getAuthoritative().unspecified()); + EXPECT_FALSE(subnet.getAuthoritative().get()); + + EXPECT_TRUE(subnet.getSiaddr().unspecified()); + EXPECT_TRUE(subnet.getSiaddr().get().isV4Zero()); + + EXPECT_TRUE(subnet.getSname().unspecified()); + EXPECT_TRUE(subnet.getSname().empty()); + + EXPECT_TRUE(subnet.getFilename().unspecified()); + EXPECT_TRUE(subnet.getFilename().empty()); +} + // Checks that the subnet id can be either autogenerated or set to an // arbitrary value through the constructor. TEST(Subnet4Test, subnetID) { @@ -688,6 +739,50 @@ TEST(Subnet6Test, constructor) { BadValue); // IPv4 addresses are not allowed in Subnet6 } +// This test verifies the default values set for the shared +// networks and verifies that the optional values are unspecified. +TEST(SharedNetwork6Test, defaults) { + Triplet t1; + Triplet t2; + Triplet preferred_lft; + Triplet valid_lft; + Subnet6 subnet(IOAddress("2001:db8:1::"), 64, t1, t2, preferred_lft, + valid_lft); + + EXPECT_TRUE(subnet.getIface().unspecified()); + EXPECT_TRUE(subnet.getIface().empty()); + + EXPECT_TRUE(subnet.getClientClass().unspecified()); + EXPECT_TRUE(subnet.getClientClass().empty()); + + EXPECT_TRUE(subnet.getValid().unspecified()); + EXPECT_EQ(0, subnet.getValid().get()); + + EXPECT_TRUE(subnet.getT1().unspecified()); + EXPECT_EQ(0, subnet.getT1().get()); + + EXPECT_TRUE(subnet.getT2().unspecified()); + EXPECT_EQ(0, subnet.getT2().get()); + + EXPECT_TRUE(subnet.getHostReservationMode().unspecified()); + EXPECT_EQ(Network::HR_ALL, subnet.getHostReservationMode().get()); + + EXPECT_TRUE(subnet.getCalculateTeeTimes().unspecified()); + EXPECT_FALSE(subnet.getCalculateTeeTimes().get()); + + EXPECT_TRUE(subnet.getT1Percent().unspecified()); + EXPECT_EQ(0.0, subnet.getT1Percent().get()); + + EXPECT_TRUE(subnet.getT2Percent().unspecified()); + EXPECT_EQ(0.0, subnet.getT2Percent().get()); + + EXPECT_TRUE(subnet.getPreferred().unspecified()); + EXPECT_EQ(0, subnet.getPreferred().get()); + + EXPECT_TRUE(subnet.getRapidCommit().unspecified()); + EXPECT_FALSE(subnet.getRapidCommit().get()); +} + // Checks that the subnet id can be either autogenerated or set to an // arbitrary value through the constructor. TEST(Subnet6Test, subnetID) { diff --git a/src/lib/util/optional.h b/src/lib/util/optional.h index 626c5c3b48..3ecb75dadd 100644 --- a/src/lib/util/optional.h +++ b/src/lib/util/optional.h @@ -86,9 +86,10 @@ public: /// @tparam A Type of the value to be assigned. Typically this is @c T, but /// may also be a type that can be cast to @c T. /// @param value value to be assigned. + /// @param unspecified initial state. Default is "unspecified". template - Optional(A value) - : default_(value), unspecified_(false) { + Optional(A value, const bool unspecified = false) + : default_(value), unspecified_(unspecified) { } /// @brief Retrieves the encapsulated value. diff --git a/src/lib/util/tests/optional_unittest.cc b/src/lib/util/tests/optional_unittest.cc index ebfc2007ca..d76de69d69 100644 --- a/src/lib/util/tests/optional_unittest.cc +++ b/src/lib/util/tests/optional_unittest.cc @@ -25,6 +25,10 @@ TEST(OptionalTest, constructor) { Optional value2; EXPECT_EQ(0, value2.get()); EXPECT_TRUE(value2.unspecified()); + + Optional value3(true, true); + EXPECT_TRUE(value3.get()); + EXPECT_TRUE(value3.unspecified()); } // This test checks if the constructors for a string value -- GitLab From 0b4bb9aad843b89cd09cfdbb5684f3e3f8c567c9 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Wed, 27 Feb 2019 08:06:21 +0100 Subject: [PATCH 6/9] [#487,!242] Set defaults for 4o6 config in subnet. --- src/bin/dhcp4/tests/config_parser_unittest.cc | 12 ++++++------ src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc | 8 ++++---- src/lib/dhcpsrv/cfg_4o6.cc | 6 +++--- src/lib/dhcpsrv/cfg_4o6.h | 13 +++++++------ src/lib/dhcpsrv/tests/subnet_unittest.cc | 9 +++++++++ 5 files changed, 29 insertions(+), 19 deletions(-) diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index 3de157ee31..f77955c1da 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -5221,8 +5221,8 @@ TEST_F(Dhcp4ParserTest, 4o6subnet) { Cfg4o6& dhcp4o6 = subnet->get4o6(); EXPECT_TRUE(dhcp4o6.enabled()); - EXPECT_EQ(IOAddress("2001:db8::123"), dhcp4o6.getSubnet4o6().first); - EXPECT_EQ(45, dhcp4o6.getSubnet4o6().second); + EXPECT_EQ(IOAddress("2001:db8::123"), dhcp4o6.getSubnet4o6().get().first); + EXPECT_EQ(45, dhcp4o6.getSubnet4o6().get().second); } // Checks if the DHCPv4 is able to parse the configuration with 4o6 subnet @@ -5318,7 +5318,7 @@ TEST_F(Dhcp4ParserTest, 4o6iface) { Cfg4o6& dhcp4o6 = subnet->get4o6(); EXPECT_TRUE(dhcp4o6.enabled()); - EXPECT_EQ("ethX", dhcp4o6.getIface4o6()); + EXPECT_EQ("ethX", dhcp4o6.getIface4o6().get()); } // Checks if the DHCPv4 is able to parse the configuration with both 4o6 network @@ -5356,9 +5356,9 @@ TEST_F(Dhcp4ParserTest, 4o6subnetIface) { // ... and that subnet has 4o6 network interface specified. Cfg4o6& dhcp4o6 = subnet->get4o6(); EXPECT_TRUE(dhcp4o6.enabled()); - EXPECT_EQ(IOAddress("2001:db8::543"), dhcp4o6.getSubnet4o6().first); - EXPECT_EQ(21, dhcp4o6.getSubnet4o6().second); - EXPECT_EQ("ethX", dhcp4o6.getIface4o6()); + EXPECT_EQ(IOAddress("2001:db8::543"), dhcp4o6.getSubnet4o6().get().first); + EXPECT_EQ(21, dhcp4o6.getSubnet4o6().get().second); + EXPECT_EQ("ethX", dhcp4o6.getIface4o6().get()); } // Checks if the DHCPv4 is able to parse the configuration with 4o6 network diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc index 5eaffb98f3..0332a9ba7d 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc @@ -676,11 +676,11 @@ public: // Convert DHCPv4o6 subnet to text. std::string dhcp4o6_subnet; - if (!subnet->get4o6().getSubnet4o6().first.isV6Zero() || - (subnet->get4o6().getSubnet4o6().second != 128u)) { + if (!subnet->get4o6().getSubnet4o6().get().first.isV6Zero() || + (subnet->get4o6().getSubnet4o6().get().second != 128u)) { std::ostringstream s; - s << subnet->get4o6().getSubnet4o6().first << "/" - << static_cast(subnet->get4o6().getSubnet4o6().second); + s << subnet->get4o6().getSubnet4o6().get().first << "/" + << static_cast(subnet->get4o6().getSubnet4o6().get().second); dhcp4o6_subnet = s.str(); } diff --git a/src/lib/dhcpsrv/cfg_4o6.cc b/src/lib/dhcpsrv/cfg_4o6.cc index d55b441aa6..c37d773479 100644 --- a/src/lib/dhcpsrv/cfg_4o6.cc +++ b/src/lib/dhcpsrv/cfg_4o6.cc @@ -24,10 +24,10 @@ Cfg4o6::toElement() const { // Set 4o6-interface result->set("4o6-interface", Element::create(iface4o6_)); // Set 4o6-subnet - if (!subnet4o6_.first.isV6Zero() || (subnet4o6_.second != 128u)) { + if (!subnet4o6_.get().first.isV6Zero() || (subnet4o6_.get().second != 128u)) { std::ostringstream oss; - oss << subnet4o6_.first << "/" - << static_cast(subnet4o6_.second); + oss << subnet4o6_.get().first << "/" + << static_cast(subnet4o6_.get().second); result->set("4o6-subnet", Element::create(oss.str())); } else { result->set("4o6-subnet", Element::create(std::string())); diff --git a/src/lib/dhcpsrv/cfg_4o6.h b/src/lib/dhcpsrv/cfg_4o6.h index e231e775e6..22d7664a70 100644 --- a/src/lib/dhcpsrv/cfg_4o6.h +++ b/src/lib/dhcpsrv/cfg_4o6.h @@ -1,4 +1,4 @@ -// Copyright (C) 2015-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2015-2019 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -9,6 +9,7 @@ #include #include +#include #include namespace isc { @@ -24,7 +25,7 @@ struct Cfg4o6 : public isc::data::CfgToElement { /// /// Initializes fields to their default value. Cfg4o6() - :enabled_(false), subnet4o6_(std::make_pair(asiolink::IOAddress("::"), 128u)) { + :enabled_(false), subnet4o6_(std::make_pair(asiolink::IOAddress("::"), 128u), true) { } /// @brief Returns whether the DHCP4o6 is enabled or not. @@ -41,7 +42,7 @@ struct Cfg4o6 : public isc::data::CfgToElement { /// @brief Returns the DHCP4o6 interface. /// @return value of the 4o6-interface parameter. - std::string getIface4o6() const { + util::Optional getIface4o6() const { return (iface4o6_); } @@ -54,7 +55,7 @@ struct Cfg4o6 : public isc::data::CfgToElement { /// @brief Returns prefix/len for the IPv6 subnet. /// @return prefix/length pair - std::pair getSubnet4o6() const { + util::Optional > getSubnet4o6() const { return (subnet4o6_); } @@ -90,10 +91,10 @@ private: bool enabled_; /// Specifies the network interface used as v4 subnet selector. - std::string iface4o6_; + util::Optional iface4o6_; /// Specifies the IPv6 subnet used for v4 subnet selection. - std::pair subnet4o6_; + util::Optional > subnet4o6_; /// Specifies the v6 interface-id used for v4 subnet selection. OptionPtr interface_id_; diff --git a/src/lib/dhcpsrv/tests/subnet_unittest.cc b/src/lib/dhcpsrv/tests/subnet_unittest.cc index 6937d21446..a957f3b077 100644 --- a/src/lib/dhcpsrv/tests/subnet_unittest.cc +++ b/src/lib/dhcpsrv/tests/subnet_unittest.cc @@ -92,6 +92,15 @@ TEST(Subnet4Test, defaults) { EXPECT_TRUE(subnet.getFilename().unspecified()); EXPECT_TRUE(subnet.getFilename().empty()); + + EXPECT_FALSE(subnet.get4o6().enabled()); + + EXPECT_TRUE(subnet.get4o6().getIface4o6().unspecified()); + EXPECT_TRUE(subnet.get4o6().getIface4o6().empty()); + + EXPECT_TRUE(subnet.get4o6().getSubnet4o6().unspecified()); + EXPECT_TRUE(subnet.get4o6().getSubnet4o6().get().first.isV6Zero()); + EXPECT_EQ(128, subnet.get4o6().getSubnet4o6().get().second); } // Checks that the subnet id can be either autogenerated or set to an -- GitLab From e46d2260879ce668db15fa8dcbcdd9358f581975 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Wed, 27 Feb 2019 10:38:03 +0100 Subject: [PATCH 7/9] [#487,!242] Adjust CQL and PgSQL to OptionalValue changes. --- src/lib/dhcpsrv/cql_host_data_source.cc | 24 +++++++++++------------ src/lib/dhcpsrv/pgsql_host_data_source.cc | 8 ++++---- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/lib/dhcpsrv/cql_host_data_source.cc b/src/lib/dhcpsrv/cql_host_data_source.cc index e31071b86e..dc0fd37ef8 100644 --- a/src/lib/dhcpsrv/cql_host_data_source.cc +++ b/src/lib/dhcpsrv/cql_host_data_source.cc @@ -29,7 +29,7 @@ #include #include #include -#include +#include #include #include // for uint64_t @@ -174,7 +174,7 @@ public: /// @param option_space option space /// @param option_descriptor structure used to hold option information void prepareExchange(const HostPtr& host, - const OptionalValue& subnet_id, + const Optional& subnet_id, const IPv6Resrv* const reservation, const std::string& option_space, const OptionDescriptor& option_descriptor); @@ -193,7 +193,7 @@ public: /// @param statement_tag tag of the statement being executed /// @param data array being filled with data from to the Host object void createBindForMutation(const HostPtr& host, - const OptionalValue& subnet_id, + const Optional& subnet_id, const IPv6Resrv* const reservation, const std::string& option_space, const OptionDescriptor& option_descriptor, @@ -214,7 +214,7 @@ public: /// @param statement_tag tag of the statement being executed /// @param data array being filled with data from to the Host object void createBindForDelete(const HostPtr& host, - const OptionalValue& subnet_id, + const Optional& subnet_id, const IPv6Resrv* const reservation, const std::string& option_space, const OptionDescriptor& option_descriptor, @@ -940,7 +940,7 @@ CqlHostExchange::createBindForSelect(AnyArray& data, StatementTag /* not used */ void CqlHostExchange::prepareExchange(const HostPtr& host, - const OptionalValue& subnet_id, + const Optional& subnet_id, const IPv6Resrv* const reservation, const std::string& option_space, const OptionDescriptor& option_descriptor) { @@ -1100,7 +1100,7 @@ CqlHostExchange::prepareExchange(const HostPtr& host, option_client_class_.clear(); // option_subnet_id: int - if (subnet_id.isSpecified()) { + if (!subnet_id.unspecified()) { option_subnet_id_ = subnet_id; } else { option_subnet_id_ = 0; @@ -1131,7 +1131,7 @@ CqlHostExchange::prepareExchange(const HostPtr& host, void CqlHostExchange::createBindForMutation(const HostPtr& host, - const OptionalValue& subnet_id, + const Optional& subnet_id, const IPv6Resrv* const reservation, const std::string& option_space, const OptionDescriptor& option_descriptor, @@ -1187,7 +1187,7 @@ CqlHostExchange::createBindForMutation(const HostPtr& host, void CqlHostExchange::createBindForDelete(const HostPtr& host, - const OptionalValue& subnet_id, + const Optional& subnet_id, const IPv6Resrv* const reservation, const std::string& option_space, const OptionDescriptor& option_descriptor, @@ -1659,7 +1659,7 @@ protected: /// information for the current denormalized table entry's option virtual bool insertOrDeleteHost(bool insert, const HostPtr& host, - const OptionalValue& subnet_id = OptionalValue(), + const Optional& subnet_id = Optional(), const IPv6Resrv* const reservation = NULL, const std::string& option_space = NULL_OPTION_SPACE, const OptionDescriptor& option_descriptor = OptionDescriptor(false)); @@ -2073,14 +2073,14 @@ CqlHostDataSourceImpl::insertOrDeleteHostWithOptions(bool insert, } option_found = true; /// @todo: Assign actual value to subnet id. - result = insertOrDeleteHost(insert, host, OptionalValue(), reservation, + result = insertOrDeleteHost(insert, host, Optional(), reservation, space, option); } } } if (result && !option_found) { // @todo: Assign actual value to subnet id. - result = insertOrDeleteHost(insert, host, OptionalValue(), reservation); + result = insertOrDeleteHost(insert, host, Optional(), reservation); } return (result); @@ -2177,7 +2177,7 @@ CqlHostDataSourceImpl::getHostCollection(StatementTag statement_tag, bool CqlHostDataSourceImpl::insertOrDeleteHost(bool insert, const HostPtr& host, - const OptionalValue& subnet_id, + const Optional& subnet_id, const IPv6Resrv* const reservation, const std::string& option_space, const OptionDescriptor& option_descriptor) { diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.cc b/src/lib/dhcpsrv/pgsql_host_data_source.cc index d6a9d22924..88e3070ea2 100644 --- a/src/lib/dhcpsrv/pgsql_host_data_source.cc +++ b/src/lib/dhcpsrv/pgsql_host_data_source.cc @@ -15,7 +15,7 @@ #include #include #include -#include +#include #include #include @@ -1360,7 +1360,7 @@ public: void addOption(const PgSqlHostDataSourceImpl::StatementIndex& stindex, const OptionDescriptor& opt_desc, const std::string& opt_space, - const OptionalValue& subnet_id, + const Optional& subnet_id, const HostID& host_id); /// @brief Inserts multiple options into the database. @@ -1942,7 +1942,7 @@ void PgSqlHostDataSourceImpl::addOption(const StatementIndex& stindex, const OptionDescriptor& opt_desc, const std::string& opt_space, - const OptionalValue&, + const Optional&, const HostID& id) { PsqlBindArrayPtr bind_array; bind_array = host_option_exchange_->createBindForSend(opt_desc, opt_space, @@ -1969,7 +1969,7 @@ PgSqlHostDataSourceImpl::addOptions(const StatementIndex& stindex, if (options && !options->empty()) { for (OptionContainer::const_iterator opt = options->begin(); opt != options->end(); ++opt) { - addOption(stindex, *opt, *space, OptionalValue(), + addOption(stindex, *opt, *space, Optional(), host_id); } } -- GitLab From 4d1dae1addd573e479f01bc10e89caa5296dfcb9 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Thu, 28 Feb 2019 11:08:26 +0100 Subject: [PATCH 8/9] [#487,!242] Added a note to the Optonal template about required specializations. --- src/lib/util/optional.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/lib/util/optional.h b/src/lib/util/optional.h index 3ecb75dadd..d75787a74b 100644 --- a/src/lib/util/optional.h +++ b/src/lib/util/optional.h @@ -75,6 +75,19 @@ public: /// @brief Default constructor. /// /// Sets the encapsulated value to 0 and marks it as "unspecified". + /// + /// The caller must ensure that the constructor of the class @c T + /// creates a valid object when invoked with 0 as an argument. + /// For example, a @c std::string(0) compiles but will crash at + /// runtime as 0 is not a valid pointer for the + /// @c std::string(const char*) constructor. Therefore, the + /// specialization of the @c Optional template for @c std::string + /// is provided below. It uses @c std::string default constructor. + /// + /// For any other type used with this template which doesn't come + /// with an appropriate constructor, the caller must create a + /// template specialization similar to the one provided for + /// @c std::string below. Optional() : default_(T(0)), unspecified_(true) { } -- GitLab From 1c58e0ce9b9fd6fc8864dbfb2335bc5841c78ff3 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Thu, 28 Feb 2019 12:48:17 +0100 Subject: [PATCH 9/9] [#487,!242] Addressed the review comments in the optional_unittest.cc --- src/lib/util/tests/optional_unittest.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/lib/util/tests/optional_unittest.cc b/src/lib/util/tests/optional_unittest.cc index d76de69d69..7aad5c9fe4 100644 --- a/src/lib/util/tests/optional_unittest.cc +++ b/src/lib/util/tests/optional_unittest.cc @@ -26,6 +26,7 @@ TEST(OptionalTest, constructor) { EXPECT_EQ(0, value2.get()); EXPECT_TRUE(value2.unspecified()); + // Use the non-default value for second parameter. Optional value3(true, true); EXPECT_TRUE(value3.get()); EXPECT_TRUE(value3.unspecified()); @@ -46,9 +47,9 @@ TEST(OptionalTest, constructorString) { // This test checks if the assignment operator assigning an actual // value to the optional value works as expected. TEST(OptionalTest, assignValue) { - Optional value(10); + Optional value(10, true); EXPECT_EQ(10, value.get()); - EXPECT_FALSE(value.unspecified()); + EXPECT_TRUE(value.unspecified()); // Assign a new value. value = 111; -- GitLab