From: Tomek Mrugalski Date: Mon, 23 Jul 2012 14:35:54 +0000 (+0200) Subject: [1708] DHCPv{4,6} start-up fixes: X-Git-Tag: trac2351_base~65^2~34^2~5 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=e1d0d73d043c58305c1affbee3abe52cdee92289;p=thirdparty%2Fkea.git [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 4fb0a771a4..a86b010792 100644 --- 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 472f46f467..1087cc7fb6 100644 --- a/src/bin/dhcp4/main.cc +++ b/src/bin/dhcp4/main.cc @@ -17,6 +17,7 @@ #include #include #include +#include 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(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 065c80c2ad..935bba645a 100644 --- 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 d28d9b7d21..9b43f5374f 100644 --- 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 96e2dd14dd..aebee90699 100644 --- a/src/bin/dhcp6/main.cc +++ b/src/bin/dhcp6/main.cc @@ -17,6 +17,7 @@ #include #include #include +#include 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(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 123b10599d..399c370742 100644 --- 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)