From: Stephen Morris Date: Thu, 19 May 2016 13:38:19 +0000 (+0100) Subject: [3164] Add changes suggested by review X-Git-Tag: trac4106_update_base~14^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4b7e67a38f6fb54d3cd06452245933805ad011b3;p=thirdparty%2Fkea.git [3164] Add changes suggested by review Change the connection timeout parameter from an "int" to an "unsigned int". Update the checks to allow for lexical_cast not throwing an exception when converting a string representing a negative number to an unsigned int. --- diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index daae437afb..0489ff1528 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -463,6 +463,7 @@ be followed by a comma and another object definition. "Dhcp4": { "lease-database": { "connect-timeout" : timeout-in-seconds, ... }, ... } The default value of five seconds should be more than adequate for local connections. +If a timeout is given though, it should be an integer greater than zero. Finally, the credentials of the account under which the server will access the database should be set: diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index d5a50397b1..5eec3e79da 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -462,6 +462,7 @@ be followed by a comma and another object definition. "Dhcp6": { "lease-database": { "connect-timeout" : timeout-in-seconds, ... }, ... } The default value of five seconds should be more than adequate for local connections. +If a timeout is given though, it should be an integer greater than zero. Finally, the credentials of the account under which the server will access the database should be set: diff --git a/src/lib/dhcpsrv/mysql_connection.cc b/src/lib/dhcpsrv/mysql_connection.cc index b7163e798f..e26c2332db 100755 --- a/src/lib/dhcpsrv/mysql_connection.cc +++ b/src/lib/dhcpsrv/mysql_connection.cc @@ -15,6 +15,7 @@ #include #include #include +#include using namespace isc; using namespace isc::dhcp; @@ -73,7 +74,7 @@ MySqlConnection::openDatabase() { isc_throw(NoDatabaseName, "must specify a name for the database"); } - int connect_timeout = MYSQL_DEFAULT_CONNECTION_TIMEOUT; + unsigned int connect_timeout = MYSQL_DEFAULT_CONNECTION_TIMEOUT; string stimeout; try { stimeout = getParameter("connect-timeout"); @@ -84,18 +85,29 @@ MySqlConnection::openDatabase() { if (stimeout.size() > 0) { // Timeout was given, so try to convert it to an integer. + try { - connect_timeout = boost::lexical_cast(stimeout); + connect_timeout = boost::lexical_cast(stimeout); } catch (...) { - // Timeout given but invalid. - isc_throw(DbInvalidTimeout, "database connection timeout (" << stimeout << - ") must be numeric"); + // Timeout given but could not be converted to an unsigned int. Set + // the connection timeout to an invalid value to trigger throwing + // of an exception. + connect_timeout = 0; } - // ... and check the range. - if (connect_timeout <= 0) { - isc_throw(DbInvalidTimeout, "database connection timeout (" << connect_timeout << - ") must be an integer greater than 0"); + // The timeout is only valid if greater than zero, as depending on the + // database, a zero timeout might signify someting like "wait + // indefinitely". + // + // The check below also rejects a value greater than the maximum + // integer value. The lexical_cast operation used to obtain a numeric + // value from a string can get confused if trying to convert a negative + // integer to an unsigned int: instead of throwing an exception, it may + // produce a large positive value. + if ((connect_timeout == 0) || + (connect_timeout > numeric_limits::max())) { + isc_throw(DbInvalidTimeout, "database connection timeout (" << + stimeout << ") must be an integer greater than 0"); } } diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc index 40546916f9..08405fbc4c 100644 --- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc +++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc @@ -1128,7 +1128,7 @@ PgSqlLeaseMgr::openDatabase() { isc_throw(NoDatabaseName, "must specify a name for the database"); } - int connect_timeout = PGSQL_DEFAULT_CONNECTION_TIMEOUT; + unsigned int connect_timeout = PGSQL_DEFAULT_CONNECTION_TIMEOUT; string stimeout; try { stimeout = dbconn_.getParameter("connect-timeout"); @@ -1139,18 +1139,29 @@ PgSqlLeaseMgr::openDatabase() { if (stimeout.size() > 0) { // Timeout was given, so try to convert it to an integer. + try { - connect_timeout = boost::lexical_cast(stimeout); + connect_timeout = boost::lexical_cast(stimeout); } catch (...) { - // Timeout given but invalid. - isc_throw(DbInvalidTimeout, "database connection timeout (" << stimeout << - ") must be numeric"); + // Timeout given but could not be converted to an unsigned int. Set + // the connection timeout to an invalid value to trigger throwing + // of an exception. + connect_timeout = 0; } - // ... and check the range. - if (connect_timeout <= 0) { - isc_throw(DbInvalidTimeout, "database connection timeout (" << connect_timeout << - ") must be an integer greater than 0"); + // The timeout is only valid if greater than zero, as depending on the + // database, a zero timeout might signify someting like "wait + // indefinitely". + // + // The check below also rejects a value greater than the maximum + // integer value. The lexical_cast operation used to obtain a numeric + // value from a string can get confused if trying to convert a negative + // integer to an unsigned int: instead of throwing an exception, it may + // produce a large positive value. + if ((connect_timeout == 0) || + (connect_timeout > numeric_limits::max())) { + isc_throw(DbInvalidTimeout, "database connection timeout (" << + stimeout << ") must be an integer greater than 0"); } }