[5020] Interface parser migrated to SimpleParser

src/bin/dhcp4/json_config_parser.cc

@@ -420,10 +420,9 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id,
         (config_id.compare("dhcp4o6-port") == 0) )  {
         parser = new Uint32Parser(config_id,
                                   globalContext()->uint32_values_);
-    } else if (config_id.compare("interfaces-config") == 0) {
-        parser = new IfacesConfigParser4();
     } else if (config_id.compare("subnet4") == 0) {
         parser = new Subnets4ListConfigParser(config_id);
+    // interface-config has been migrated to SimpleParser already.
     // option-data and option-def have been converted to SimpleParser already.
     } else if ((config_id.compare("next-server") == 0)) {
         parser  = new StringParser(config_id,
@@ -583,6 +582,11 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
         const std::map<std::string, ConstElementPtr>& values_map =
                                                         mutable_cfg->mapValue();
         BOOST_FOREACH(config_pair, values_map) {
+            // In principle we could have the following code structured as a series
+            // of long if else if clauses. That would give a marginal performance
+            // boost, but would make the code less readable. We had serious issues
+            // with the parser code debuggability, so I decided to keep it as a
+            // series of independent ifs.
 
             if (config_pair.first == "option-def") {
                 // This is converted to SimpleParser and is handled already above.
@@ -596,6 +600,14 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
                 continue;
             }
 
+            if (config_pair.first == "interfaces-config") {
+                IfacesConfigParser parser(AF_INET);
+                CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
+                parser.parse(cfg_iface, config_pair.second);
+                continue;
+            }
+
+            // Legacy DhcpConfigParser stuff below
             ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first,
                                                            config_pair.second));
             LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PARSER_CREATED)
@@ -604,11 +616,6 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
                 subnet_parser = parser;
             } else if (config_pair.first == "lease-database") {
                 leases_parser = parser;
-            } else if (config_pair.first == "interfaces-config") {
-                // The interface parser is independent from any other
-                // parser and can be run here before any other parsers.
-                iface_parser = parser;
-                parser->build(config_pair.second);
             } else if (config_pair.first == "hooks-libraries") {
                 // Executing commit will alter currently-loaded hooks
                 // libraries.  Check if the supplied libraries are valid,

src/bin/dhcp6/json_config_parser.cc

@@ -701,12 +701,11 @@ DhcpConfigParser* createGlobal6DhcpConfigParser(const std::string& config_id,
         (config_id.compare("dhcp4o6-port") == 0) )  {
         parser = new Uint32Parser(config_id,
                                  globalContext()->uint32_values_);
-    } else if (config_id.compare("interfaces-config") == 0) {
-        parser = new IfacesConfigParser6();
     } else if (config_id.compare("subnet6") == 0) {
         parser = new Subnets6ListConfigParser(config_id);
     // option-data and option-def are no longer needed here. They're now
-    //  converted to SimpleParser and are handled in configureDhcp6Server
+    // converted to SimpleParser and are handled in configureDhcp6Server.
+    // interfaces-config has been converted to SimpleParser.
     }  else if (config_id.compare("version") == 0) {
         parser  = new StringParser(config_id,
                                    globalContext()->string_values_);
@@ -857,6 +856,11 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
         }
 
         BOOST_FOREACH(config_pair, values_map) {
+            // In principle we could have the following code structured as a series
+            // of long if else if clauses. That would give a marginal performance
+            // boost, but would make the code less readable. We had serious issues
+            // with the parser code debuggability, so I decided to keep it as a
+            // series of independent ifs.
 
             if (config_pair.first == "option-def") {
                 // This is converted to SimpleParser and is handled already above.
@@ -870,6 +874,13 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
                 continue;
             }
 
+            if (config_pair.first == "interfaces-config") {
+                IfacesConfigParser parser(AF_INET6);
+                CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
+                parser.parse(cfg_iface, config_pair.second);
+                continue;
+            }
+
             ParserPtr parser(createGlobal6DhcpConfigParser(config_pair.first,
                                                            config_pair.second));
             LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL, DHCP6_PARSER_CREATED)

src/lib/dhcpsrv/parsers/ifaces_config_parser.cc

@@ -18,82 +18,52 @@ using namespace isc::data;
 namespace isc {
 namespace dhcp {
 
-InterfaceListConfigParser::InterfaceListConfigParser(const uint16_t protocol)
-    : protocol_(protocol) {
-}
-
 void
-InterfaceListConfigParser::build(ConstElementPtr value) {
-    CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
-
-    BOOST_FOREACH(ConstElementPtr iface, value->listValue()) {
+IfacesConfigParser::parseInterfacesList(const CfgIfacePtr& cfg_iface,
+                                           ConstElementPtr ifaces_list) {
+    BOOST_FOREACH(ConstElementPtr iface, ifaces_list->listValue()) {
         std::string iface_name = iface->stringValue();
         try {
             cfg_iface->use(protocol_, iface_name);
 
         } catch (const std::exception& ex) {
             isc_throw(DhcpConfigError, "Failed to select interface: "
-                      << ex.what() << " (" << value->getPosition() << ")");
+                      << ex.what() << " (" << iface->getPosition() << ")");
         }
     }
 }
 
-void
-InterfaceListConfigParser::commit() {
-    // Nothing to do.
-}
-
 IfacesConfigParser::IfacesConfigParser(const uint16_t protocol)
     : protocol_(protocol) {
 }
 
 void
-IfacesConfigParser::build(isc::data::ConstElementPtr ifaces_config) {
-    BOOST_FOREACH(ConfigPair element, ifaces_config->mapValue()) {
-        try {
-            if (element.first == "interfaces") {
-                InterfaceListConfigParser parser(protocol_);
-                parser.build(element.second);
-
-            } 
-
-        } catch (const std::exception& ex) {
-            // Append line number where the error occurred.
-            isc_throw(DhcpConfigError, ex.what() << " ("
-                      << element.second->getPosition() << ")");
-        }
-    }
-}
-
-bool
-IfacesConfigParser::isGenericParameter(const std::string& parameter) const {
-    // Currently, the "interfaces" is the only common parameter for
-    // DHCPv4 and DHCPv6.
-    return (parameter == "interfaces");
-}
-
-IfacesConfigParser4::IfacesConfigParser4()
-    : IfacesConfigParser(AF_INET) {
-}
-
-void
-IfacesConfigParser4::build(isc::data::ConstElementPtr ifaces_config) {
-    IfacesConfigParser::build(ifaces_config);
+IfacesConfigParser::parse(const CfgIfacePtr& cfg,
+                          const isc::data::ConstElementPtr& ifaces_config) {
 
     // Get the pointer to the interface configuration.
-    CfgIfacePtr cfg = CfgMgr::instance().getStagingCfg()->getCfgIface();
     bool socket_type_specified = false;
     BOOST_FOREACH(ConfigPair element, ifaces_config->mapValue()) {
         try {
-            if (element.first == "dhcp-socket-type") {
-                cfg->useSocketType(AF_INET, element.second->stringValue());
-                socket_type_specified = true;
+            if (element.first == "interfaces") {
+                parseInterfacesList(cfg, element.second);
+                continue;
+
+            }
 
-            } else if (!isGenericParameter(element.first)) {
-                isc_throw(DhcpConfigError, "usupported parameter '"
-                          << element.first << "'");
+            if (element.first == "dhcp-socket-type") {
+                if (protocol_ == AF_INET) {
+                    cfg->useSocketType(AF_INET, element.second->stringValue());
+                    socket_type_specified = true;
+                    continue;
+                } else {
+                    isc_throw(DhcpConfigError,
+                              "dhcp-socket-type is not supported in DHCPv6");
+                }
             }
 
+            isc_throw(DhcpConfigError, "usupported parameter '"
+                      << element.first << "'");
         } catch (const std::exception& ex) {
             // Append line number where the error occurred.
             isc_throw(DhcpConfigError, ex.what() << " ("
@@ -109,28 +79,5 @@ IfacesConfigParser4::build(isc::data::ConstElementPtr ifaces_config) {
     }
 }
 
-IfacesConfigParser6::IfacesConfigParser6()
-    : IfacesConfigParser(AF_INET6) {
-}
-
-void
-IfacesConfigParser6::build(isc::data::ConstElementPtr ifaces_config) {
-    IfacesConfigParser::build(ifaces_config);
-
-    BOOST_FOREACH(ConfigPair element, ifaces_config->mapValue()) {
-        try {
-            if (!isGenericParameter(element.first)) {
-                isc_throw(DhcpConfigError, "usupported parameter '"
-                          << element.first << "'");
-            }
-
-        } catch (const std::exception& ex) {
-            // Append line number where the error occurred.
-            isc_throw(DhcpConfigError, ex.what() << " ("
-                      << element.second->getPosition() << ")");
-        }
-    }
-}
-
 } // end of namespace isc::dhcp
 } // end of namespace isc

src/lib/dhcpsrv/parsers/ifaces_config_parser.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2016 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,65 +8,22 @@
 #define IFACES_CONFIG_PARSER_H
 
 #include <cc/data.h>
-#include <dhcpsrv/parsers/dhcp_config_parser.h>
+#include <cc/simple_parser.h>
+#include <dhcpsrv/cfg_iface.h>
 #include <dhcpsrv/parsers/dhcp_parsers.h>
 
 namespace isc {
 namespace dhcp {
 
-/// @brief Parser for interface list definition.
-///
-/// This parser handles Dhcp4/interfaces-config/interfaces and
-/// Dhcp6/interfaces-config/interfaces entries.
-/// It contains a list of network interfaces that the server listens on.
-/// In particular, it can contain an "*" that designates all interfaces.
-class InterfaceListConfigParser : public DhcpConfigParser {
-public:
-
-    /// @brief Constructor
-    ///
-    /// @param protocol AF_INET for DHCPv4 and AF_INET6 for DHCPv6.
-    InterfaceListConfigParser(const uint16_t protocol);
-
-    /// @brief Parses a list of interface names.
-    ///
-    /// This method parses a list of interface/address tuples in a text
-    /// format. The tuples specify the IP addresses and corresponding
-    /// interface names on which the server should listen to the DHCP
-    /// messages. The address is optional in each tuple and, if not
-    /// specified, the interface name (without slash character) should
-    /// be present.
-    ///
-    /// @param value pointer to the content of parsed values
-    ///
-    /// @throw DhcpConfigError if the interface names and/or addresses
-    /// are invalid.
-    virtual void build(isc::data::ConstElementPtr value);
-
-    /// @brief Does nothing.
-    virtual void commit();
-
-private:
-
-    /// @brief AF_INET for DHCPv4 and AF_INET6 for DHCPv6.
-    uint16_t protocol_;
-
-};
-
-
 /// @brief Parser for the configuration of interfaces.
 ///
 /// This parser parses the "interfaces-config" parameter which holds the
 /// full configuration of the DHCP server with respect to the use of
 /// interfaces, sockets and alike.
 ///
-/// This parser uses the @c InterfaceListConfigParser to parse the
-/// list of interfaces on which the server should listen. It handles
-/// remaining parameters internally.
-///
-/// This parser is used as a base for the DHCPv4 and DHCPv6 specific
-/// parsers and should not be used directly.
-class IfacesConfigParser : public DhcpConfigParser {
+/// This parser is used in both DHCPv4 and DHCPv6. Derived parsers
+/// are not needed.
+class IfacesConfigParser : public isc::data::SimpleParser {
 public:
 
     /// @brief Constructor
@@ -74,81 +31,30 @@ public:
     /// @param protocol AF_INET for DHCPv4 and AF_INET6 for DHCPv6.
     IfacesConfigParser(const uint16_t protocol);
 
-    /// @brief Parses generic parameters in "interfaces-config".
+    /// @brief Parses content of the "interfaces-config".
     ///
-    /// The generic parameters in the "interfaces-config" map are
-    /// the ones that are common for DHCPv4 and DHCPv6.
+    /// @param config parsed structures will be stored here
+    /// @param values pointer to the content of parsed values
     ///
-    /// @param ifaces_config A data element holding configuration of
-    /// interfaces.
-    virtual void build(isc::data::ConstElementPtr ifaces_config);
-
-    /// @brief Commit, unused.
-    virtual void commit() { }
+    /// @throw DhcpConfigError if the interface names and/or addresses
+    /// are invalid.
+    void parse(const CfgIfacePtr& config, const isc::data::ConstElementPtr& values);
 
-    /// @brief Checks if the specified parameter is a common parameter
-    /// for DHCPv4 and DHCPv6 interface configuration.
-    ///
-    /// This method is invoked by the derived classes to check if the
-    /// particular parameter is supported.
+private:
+    /// @brief parses interfaces-list structure
     ///
-    /// @param parameter A name of the parameter.
+    /// This method goes through all the interfaces-specified in
+    /// 'interfaces-list' and enabled them in the specified configuration
+    /// structure
     ///
-    /// @return true if the specified parameter is a common parameter
-    /// for DHCPv4 and DHCPv6 server.
-    bool isGenericParameter(const std::string& parameter) const;
-
-private:
+    /// @param cfg_iface parsed interfaces will be specified here
+    /// @param ifaces_list interfaces-list to be parsed
+    /// @throw DhcpConfigError if the interface names are invalid.
+    void parseInterfacesList(const CfgIfacePtr& cfg_iface,
+                             isc::data::ConstElementPtr ifaces_list);
 
     /// @brief AF_INET for DHCPv4 and AF_INET6 for DHCPv6.
     int protocol_;
-
-};
-
-
-/// @brief Parser for the "interfaces-config" parameter of the DHCPv4 server.
-class IfacesConfigParser4 : public IfacesConfigParser {
-public:
-
-    /// @brief Constructor.
-    ///
-    /// Sets the protocol to AF_INET.
-    IfacesConfigParser4();
-
-    /// @brief Parses DHCPv4 specific parameters.
-    ///
-    /// Internally it invokes the @c InterfaceConfigParser::build to parse
-    /// generic parameters. In addition, it parses the following parameters:
-    /// - dhcp-socket-type
-    ///
-    /// @param ifaces_config A data element holding configuration of
-    /// interfaces.
-    ///
-    /// @throw DhcpConfigError if unsupported parameters is specified.
-    virtual void build(isc::data::ConstElementPtr ifaces_config);
-
-};
-
-/// @brief Parser for the "interfaces-config" parameter of the DHCPv4 server.
-class IfacesConfigParser6 : public IfacesConfigParser {
-public:
-
-    /// @brief Constructor.
-    ///
-    /// Sets the protocol to AF_INET6.
-    IfacesConfigParser6();
-
-    /// @brief Parses DHCPv6 specific parameters.
-    ///
-    /// Internally it invokes the @c InterfaceConfigParser::build to parse
-    /// generic parameters. Currently it doesn't parse any other parameters.
-    ///
-    /// @param ifaces_config A data element holding configuration of
-    /// interfaces.
-    ///
-    /// @throw DhcpConfigError if unsupported parameters is specified.
-    virtual void build(isc::data::ConstElementPtr ifaces_config);
-
 };
 
 }

src/lib/dhcpsrv/tests/ifaces_config_parser_unittest.cc

@@ -56,8 +56,9 @@ TEST_F(IfacesConfigParserTest, interfaces) {
     ElementPtr config_element = Element::fromJSON(config);
 
     // Parse the configuration.
-    IfacesConfigParser4 parser;
-    ASSERT_NO_THROW(parser.build(config_element));
+    IfacesConfigParser parser(AF_INET);
+    CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
+    ASSERT_NO_THROW(parser.parse(cfg_iface, config_element));
 
     // Open sockets according to the parsed configuration.
     SrvConfigPtr cfg = CfgMgr::instance().getStagingCfg();
@@ -77,7 +78,8 @@ TEST_F(IfacesConfigParserTest, interfaces) {
     config = "{ \"interfaces\": [ \"eth0\", \"*\" ] }";
     config_element = Element::fromJSON(config);
 
-    ASSERT_NO_THROW(parser.build(config_element));
+    cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
+    ASSERT_NO_THROW(parser.parse(cfg_iface, config_element));
 
     cfg = CfgMgr::instance().getStagingCfg();
     ASSERT_NO_THROW(cfg->getCfgIface()->openSockets(AF_INET, 10000));
@@ -100,8 +102,9 @@ TEST_F(IfacesConfigParserTest, socketTypeRaw) {
     ElementPtr config_element = Element::fromJSON(config);
 
     // Parse the configuration.
-    IfacesConfigParser4 parser;
-    ASSERT_NO_THROW(parser.build(config_element));
+    IfacesConfigParser parser(AF_INET);
+    CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
+    ASSERT_NO_THROW(parser.parse(cfg_iface, config_element));
 
     // Compare the resulting configuration with a reference
     // configuration using the raw socket.
@@ -125,8 +128,9 @@ TEST_F(IfacesConfigParserTest, socketTypeDatagram) {
     ElementPtr config_element = Element::fromJSON(config);
 
     // Parse the configuration.
-    IfacesConfigParser4 parser;
-    ASSERT_NO_THROW(parser.build(config_element));
+    IfacesConfigParser parser(AF_INET);
+    CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
+    ASSERT_NO_THROW(parser.parse(cfg_iface, config_element));
 
     // Compare the resulting configuration with a reference
     // configuration using the raw socket.
@@ -139,18 +143,19 @@ TEST_F(IfacesConfigParserTest, socketTypeDatagram) {
 // Test that the configuration rejects the invalid socket type.
 TEST_F(IfacesConfigParserTest, socketTypeInvalid) {
     // For DHCPv4 we only accept the raw socket or datagram socket.
-    IfacesConfigParser4 parser4;
+    IfacesConfigParser parser4(AF_INET);
+    CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
     std::string config = "{ \"interfaces\": [ ],"
         "\"dhcp-socket-type\": \"default\" }";
     ElementPtr config_element = Element::fromJSON(config);
-    ASSERT_THROW(parser4.build(config_element), DhcpConfigError);
+    ASSERT_THROW(parser4.parse(cfg_iface, config_element), DhcpConfigError);
 
     // For DHCPv6 we don't accept any socket type.
-    IfacesConfigParser6 parser6;
+    IfacesConfigParser parser6(AF_INET6);
     config = "{ \"interfaces\": [ ],"
         " \"dhcp-socket-type\": \"udp\" }";
     config_element = Element::fromJSON(config);
-    ASSERT_THROW(parser6.build(config_element), DhcpConfigError);
+    ASSERT_THROW(parser6.parse(cfg_iface, config_element), DhcpConfigError);
 }
 
 } // end of anonymous namespace