[1708] DHCPv{4,6} start-up fixes:

- better exception handling in dhcpv{4,6}Srv constructors
- port number is now parsed with boost::lexical_cast
- new tests for port numbers implemented
- ControlledDhcpv{4,6}Srv objects are now automatic
- runDhcpv4() method renamed to runCommand() to better prepare
  for upcoming 4/6 test merge.

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 4fb0a771a42127a6c634c49d0251510c95581d9a..a86b010792ab6bc53f8131e9bb0c7812522f9d6d 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -37,16 +37,21 @@ const std::string HARDCODED_SERVER_ID = "192.0.2.1";
 Dhcpv4Srv::Dhcpv4Srv(uint16_t port) {
     cout << "Initialization: opening sockets on port " << port << endl;
 
-    // first call to instance() will create IfaceMgr (it's a singleton)
-    // it may throw something if things go wrong
-    IfaceMgr::instance();
+    try {
+        // first call to instance() will create IfaceMgr (it's a singleton)
+        // it may throw something if things go wrong
+        IfaceMgr::instance();
 
-    /// @todo: instantiate LeaseMgr here once it is imlpemented.
-    IfaceMgr::instance().printIfaces();
+        /// @todo: instantiate LeaseMgr here once it is imlpemented.
 
-    IfaceMgr::instance().openSockets4(port);
+        IfaceMgr::instance().openSockets4(port);
 
-    setServerID();
+        setServerID();
+    } catch (const std::exception &e) {
+        cerr << "Error during DHCPv4 server startup: " << e.what() << endl;
+        shutdown_ = true;
+        return;
+    }
 
     shutdown_ = false;
 }

diff --git a/src/bin/dhcp4/main.cc b/src/bin/dhcp4/main.cc
index 472f46f4674b1baff10e2e400d14f42ebba5cb60..1087cc7fb63308ca46f685f91eca50bf56c6ed7d 100644 (file)
--- a/src/bin/dhcp4/main.cc
+++ b/src/bin/dhcp4/main.cc
@@ -17,6 +17,7 @@
 #include <log/dummylog.h>
 #include <log/logger_support.h>
 #include <dhcp4/ctrl_dhcp4_srv.h>
+#include <boost/lexical_cast.hpp>
 
 using namespace std;
 using namespace isc::dhcp;
@@ -64,8 +65,14 @@ main(int argc, char* argv[]) {
             stand_alone = true;
             break;
         case 'p':
-            port_number = strtol(optarg, NULL, 10);
-            if (port_number == 0) {
+            try {
+                port_number = boost::lexical_cast<int>(optarg);
+            } catch (const boost::bad_lexical_cast &) {
+                cerr << "Failed to parse port number: [" << optarg
+                     << "], 1-65535 allowed." << endl;
+                usage();
+            }
+            if (port_number <= 0 || port_number > 65535) {
                 cerr << "Failed to parse port number: [" << optarg
                      << "], 1-65535 allowed." << endl;
                 usage();
@@ -97,11 +104,11 @@ main(int argc, char* argv[]) {
         cout << "[b10-dhcp4] Initiating DHCPv4 server operation." << endl;
 
         /// @todo: pass verbose to the actul server once logging is implemented
-        ControlledDhcpv4Srv* server = new ControlledDhcpv4Srv(port_number);
+        ControlledDhcpv4Srv server(port_number);
 
         if (!stand_alone) {
             try {
-                server->establishSession();
+                server.establishSession();
             } catch (const std::exception& ex) {
                 cerr << "Failed to establish BIND10 session. "
                     "Running in stand-alone mode:" << ex.what() << endl;
@@ -112,10 +119,7 @@ main(int argc, char* argv[]) {
             cout << "Skipping connection to the BIND10 msgq." << endl;
         }
 
-        server->run();
-        delete server;
-        server = NULL;
-
+        server.run();
     } catch (const std::exception& ex) {
         cerr << "[b10-dhcp4] Server failed: " << ex.what() << endl;
         ret = EXIT_FAILURE;

diff --git a/src/bin/dhcp4/tests/dhcp4_test.py b/src/bin/dhcp4/tests/dhcp4_test.py
index 065c80c2ad7d6d6fd6d693d54238ecc054b3e9f4..935bba645aff7dad7c0c34fd4548cf6d39346a9a 100644 (file)
--- a/src/bin/dhcp4/tests/dhcp4_test.py
+++ b/src/bin/dhcp4/tests/dhcp4_test.py
@@ -34,7 +34,7 @@ class TestDhcpv4Daemon(unittest.TestCase):
     def tearDown(self):
         pass
 
-    def runDhcp4(self, params, wait=1):
+    def runCommand(self, params, wait=1):
         """
         This method runs dhcp4 and returns a touple: (returncode, stdout, stderr)
         """
@@ -127,14 +127,14 @@ class TestDhcpv4Daemon(unittest.TestCase):
         print("Note: Purpose of some of the tests is to check if DHCPv4 server can be started,")
         print("      not that is can bind sockets correctly. Please ignore binding errors.")
 
-        (returncode, output, error) = self.runDhcp4(["../b10-dhcp4", "-v"])
+        (returncode, output, error) = self.runCommand(["../b10-dhcp4", "-v"])
 
         self.assertEqual( str(output).count("[b10-dhcp4] Initiating DHCPv4 server operation."), 1)
 
     def test_portnumber_0(self):
         print("Check that specifying port number 0 is not allowed.")
 
-        (returncode, output, error) = self.runDhcp4(['../b10-dhcp4', '-p', '0'])
+        (returncode, output, error) = self.runCommand(['../b10-dhcp4', '-p', '0'])
 
         # When invalid port number is specified, return code must not be success
         self.assertTrue(returncode != 0)
@@ -145,7 +145,7 @@ class TestDhcpv4Daemon(unittest.TestCase):
     def test_portnumber_missing(self):
         print("Check that -p option requires a parameter.")
 
-        (returncode, output, error) = self.runDhcp4(['../b10-dhcp4', '-p'])
+        (returncode, output, error) = self.runCommand(['../b10-dhcp4', '-p'])
 
         # When invalid port number is specified, return code must not be success
         self.assertTrue(returncode != 0)
@@ -153,10 +153,32 @@ class TestDhcpv4Daemon(unittest.TestCase):
         # Check that there is an error message about invalid port number printed on stderr
         self.assertEqual( str(error).count("option requires an argument"), 1)
 
+    def test_portnumber_invalid1(self):
+        print("Check that -p option is check against bogus port number (999999).")
+
+        (returncode, output, error) = self.runCommand(['../b10-dhcp4', '-p','999999'])
+
+        # When invalid port number is specified, return code must not be success
+        self.assertTrue(returncode != 0)
+
+        # Check that there is an error message about invalid port number printed on stderr
+        self.assertEqual( str(error).count("Failed to parse port number"), 1)
+
+    def test_portnumber_invalid2(self):
+        print("Check that -p option is check against bogus port number (123garbage).")
+
+        (returncode, output, error) = self.runCommand(['../b10-dhcp4', '-p','123garbage'])
+
+        # When invalid port number is specified, return code must not be success
+        self.assertTrue(returncode != 0)
+
+        # Check that there is an error message about invalid port number printed on stderr
+        self.assertEqual( str(error).count("Failed to parse port number"), 1)
+
     def test_portnumber_nonroot(self):
         print("Check that specifying unprivileged port number will work.")
 
-        (returncode, output, error) = self.runDhcp4(['../b10-dhcp4', '-p', '10057'])
+        (returncode, output, error) = self.runCommand(['../b10-dhcp4', '-s', '-p', '10057'])
 
         # When invalid port number is specified, return code must not be success
         # TODO: Temporarily commented out as socket binding on systems that do not have
@@ -169,7 +191,7 @@ class TestDhcpv4Daemon(unittest.TestCase):
     def test_skip_msgq(self):
         print("Check that connection to BIND10 msgq can be disabled.")
 
-        (returncode, output, error) = self.runDhcp4(['../b10-dhcp4', '-s', '-p', '10057'])
+        (returncode, output, error) = self.runCommand(['../b10-dhcp4', '-s', '-p', '10057'])
 
         # When invalid port number is specified, return code must not be success
         # TODO: Temporarily commented out as socket binding on systems that do not have

diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index d28d9b7d214dfbb0740181672d53ce8065accfb4..9b43f5374f788bfb0ec55ff691cf6e463b307fbc 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -45,23 +45,24 @@ Dhcpv6Srv::Dhcpv6Srv(uint16_t port) {
     // first call to instance() will create IfaceMgr (it's a singleton)
     // it may throw something if things go wrong
     try {
-        IfaceMgr::instance();
-    } catch (const std::exception &e) {
-        cout << "Failed to instantiate InterfaceManager:" << e.what() << ". Aborting." << endl;
-        shutdown_ = true;
-    }
 
-    if (IfaceMgr::instance().countIfaces() == 0) {
-        cout << "Failed to detect any network interfaces. Aborting." << endl;
-        shutdown_ = true;
-    }
+        if (IfaceMgr::instance().countIfaces() == 0) {
+            cout << "Failed to detect any network interfaces. Aborting." << endl;
+            shutdown_ = true;
+            return;
+        }
 
-    // Now try to open IPv6 sockets on detected interfaces.
-    IfaceMgr::instance().openSockets6(port);
+        IfaceMgr::instance().openSockets6(port);
 
-    /// @todo: instantiate LeaseMgr here once it is imlpemented.
+        setServerID();
 
-    setServerID();
+        /// @todo: instantiate LeaseMgr here once it is imlpemented.
+
+    } catch (const std::exception &e) {
+        cerr << "Error during DHCPv4 server startup: " << e.what() << endl;
+        shutdown_ = true;
+        return;
+    }
 
     shutdown_ = false;
 }

diff --git a/src/bin/dhcp6/main.cc b/src/bin/dhcp6/main.cc
index 96e2dd14ddd41d298905c028792d006a50879679..aebee90699179ffadcdc08f53f4a173d22931b50 100644 (file)
--- a/src/bin/dhcp6/main.cc
+++ b/src/bin/dhcp6/main.cc
@@ -17,6 +17,7 @@
 #include <log/dummylog.h>
 #include <log/logger_support.h>
 #include <dhcp6/ctrl_dhcp6_srv.h>
+#include <boost/lexical_cast.hpp>
 
 using namespace std;
 using namespace isc::dhcp;
@@ -64,8 +65,14 @@ main(int argc, char* argv[]) {
             stand_alone = true;
             break;
         case 'p':
-            port_number = strtol(optarg, NULL, 10);
-            if (port_number == 0) {
+            try {
+                port_number = boost::lexical_cast<int>(optarg);
+            } catch (const boost::bad_lexical_cast &) {
+                cerr << "Failed to parse port number: [" << optarg
+                     << "], 1-65535 allowed." << endl;
+                usage();
+            }
+            if (port_number <= 0 || port_number > 65535) {
                 cerr << "Failed to parse port number: [" << optarg
                      << "], 1-65535 allowed." << endl;
                 usage();
@@ -97,11 +104,11 @@ main(int argc, char* argv[]) {
         cout << "b10-dhcp6: Initiating DHCPv6 server operation." << endl;
 
         /// @todo: pass verbose to the actual server once logging is implemented
-        ControlledDhcpv6Srv* server = new ControlledDhcpv6Srv(port_number);
+        ControlledDhcpv6Srv server(port_number);
 
         if (!stand_alone) {
             try {
-                server->establishSession();
+                server.establishSession();
             } catch (const std::exception& ex) {
                 cerr << "Failed to establish BIND10 session. "
                     "Running in stand-alone mode:" << ex.what() << endl;
@@ -112,9 +119,7 @@ main(int argc, char* argv[]) {
             cout << "Skipping connection to the BIND10 msgq." << endl;
         }
 
-        server->run();
-        delete server;
-        server = NULL;
+        server.run();
 
     } catch (const std::exception& ex) {
         cerr << "[b10-dhcp6] Server failed: " << ex.what() << endl;

diff --git a/src/bin/dhcp6/tests/dhcp6_test.py b/src/bin/dhcp6/tests/dhcp6_test.py
index 123b10599d3f1f32d0f060fe6189dbd324e6ee9b..399c3707421fd713fd7c9636abb3ade851ca6894 100644 (file)
--- a/src/bin/dhcp6/tests/dhcp6_test.py
+++ b/src/bin/dhcp6/tests/dhcp6_test.py
@@ -155,17 +155,38 @@ class TestDhcpv6Daemon(unittest.TestCase):
         # Check that there is an error message about invalid port number printed on stderr
         self.assertEqual( str(error).count("option requires an argument"), 1)
 
+    def test_portnumber_invalid1(self):
+        print("Check that -p option is check against bogus port number (999999).")
+
+        (returncode, output, error) = self.runCommand(['../b10-dhcp6', '-p','999999'])
+
+        # When invalid port number is specified, return code must not be success
+        self.assertTrue(returncode != 0)
+
+        # Check that there is an error message about invalid port number printed on stderr
+        self.assertEqual( str(error).count("Failed to parse port number"), 1)
+
+    def test_portnumber_invalid2(self):
+        print("Check that -p option is check against bogus port number (123garbage).")
+
+        (returncode, output, error) = self.runCommand(['../b10-dhcp6', '-p','123garbage'])
+
+        # When invalid port number is specified, return code must not be success
+        self.assertTrue(returncode != 0)
+
+        # Check that there is an error message about invalid port number printed on stderr
+        self.assertEqual( str(error).count("Failed to parse port number"), 1)
+
     def test_portnumber_nonroot(self):
         print("Check that specifying unprivileged port number will work.")
 
-        (returncode, output, error) = self.runCommand(['../b10-dhcp6', '-p', '10547'])
+        (returncode, output, error) = self.runCommand(['../b10-dhcp6', '-s', '-p', '10547'])
 
         # When invalid port number is specified, return code must not be success
         # TODO: Temporarily commented out as socket binding on systems that do not have
         #       interface detection implemented currently fails.
         # self.assertTrue(returncode == 0)
 
-        # Check that there is a message on stdout about opening proper port
         self.assertEqual( str(output).count("opening sockets on port 10547"), 1)
 
     def test_skip_msgq(self):
@@ -178,7 +199,6 @@ class TestDhcpv6Daemon(unittest.TestCase):
         #       interface detection implemented currently fails.
         # self.assertTrue(returncode == 0)
 
-        # Check that there is an error message about invalid port number printed on stderr
         self.assertEqual( str(output).count("Skipping connection to the BIND10 msgq."), 1)