Commit f04d4835 authored by JINMEI Tatuya's avatar JINMEI Tatuya
Browse files

addressed another review comment:

 - allowed commit() to throw an exception (although discouraging it strongly), and catch and convert it in configureAuthServer()
 - added a test for that scenario
 - changed the type of FatalError to make sure it won't be caught in the middle of server program

git-svn-id: svn:// e5f2f494-b856-4b98-b285-d166d9295462
parent a4143077
......@@ -26,6 +26,7 @@
#include <auth/common.h>
using namespace boost;
using namespace std;
changeUser(const char* const username) {
......@@ -42,14 +43,14 @@ changeUser(const char* const username) {
if (runas_pw == NULL) {
isc_throw(FatalError, "Unknown user name or UID:" << username);
throw FatalError("Unknown user name or UID:" + string(username));
if (setgid(runas_pw->pw_gid) < 0) {
isc_throw(FatalError, "setgid() failed: " << strerror(errno));
throw FatalError("setgid() failed: " + string(strerror(errno)));
if (setuid(runas_pw->pw_uid) < 0) {
isc_throw(FatalError, "setuid() failed: " << strerror(errno));
throw FatalError("setuid() failed: " + string(strerror(errno)));
......@@ -17,12 +17,18 @@
#ifndef __COMMON_H
#define __COMMON_H 1
#include <exceptions/exceptions.h>
#include <stdexcept>
#include <string>
class FatalError : public isc::Exception {
/// An exception class that is thrown in an unrecoverable error condition.
/// This exception should not be caught except at the highest level of
/// the application only for terminating the program gracefully, and so
/// it cannot be a derived class of \c isc::Exception.
class FatalError : public std::runtime_error {
FatalError(const char* file, size_t line, const char* what) :
isc::Exception(file, line, what) {}
FatalError(const std::string& what) : std::runtime_error(what)
#endif // __COMMON_H
......@@ -30,6 +30,7 @@
#include <auth/auth_srv.h>
#include <auth/config.h>
#include <auth/common.h>
using namespace std;
using boost::shared_ptr;
......@@ -163,6 +164,16 @@ MemoryDatasourceConfig::build(ConstElementPtr config_value) {
/// A special parser for testing: it throws from commit() despite the
/// suggested convention of the class interface.
class ThrowerCommitConfig : public AuthConfigParser {
virtual void build(ConstElementPtr) {} // ignore param, do nothing
virtual void commit() {
throw 10;
// This is a generalized version of create function that can create
// an AuthConfigParser object for "internal" use.
......@@ -177,6 +188,14 @@ createAuthConfigParser(AuthSrv& server, const std::string& config_id,
return (new DatasourcesConfig(server));
} else if (internal && config_id == "datasources/memory") {
return (new MemoryDatasourceConfig(server));
} else if (config_id == "_commit_throw") {
// This is for testing purpose only and should not appear in the
// actual configuration syntax. While this could crash the caller
// as a result, the server implementation is expected to perform
// syntax level validation and should be safe in practice. In future,
// we may introduce dynamic registration of configuration parsers,
// and then this test can be done in a cleaner and safer way.
return (new ThrowerCommitConfig());
} else {
isc_throw(AuthConfigError, "Unknown configuration identifier: " <<
......@@ -220,7 +239,12 @@ configureAuthServer(AuthSrv& server, ConstElementPtr config_set) {
BOOST_FOREACH(ParserPtr parser, parsers) {
try {
BOOST_FOREACH(ParserPtr parser, parsers) {
} catch (...) {
throw FatalError("Unrecoverable error: "
"a configuration parser threw in commit");
......@@ -54,7 +54,9 @@ public:
/// value(s) to be applied to the server. This method may throw an exception
/// when it encounters an error.
/// The \c commit() method actually applies the new configuration value
/// to the server. It must not throw an exception.
/// to the server. It's basically not expected to throw an exception;
/// any configuration operations that can fail (such as ones involving
/// resource allocation) should be done in \c build().
/// When the destructor is called before \c commit(), the destructor is
/// supposed to make sure the state of the \c AuthSrv object is the same
......@@ -119,10 +121,15 @@ public:
/// Apply the prepared configuration value to the server.
/// This method must be exception free, and, as a consequence, it should
/// normally not involve resource allocation.
/// This method is expected to be exception free, and, as a consequence,
/// it should normally not involve resource allocation.
/// Typically it would simply perform exception free assignment or swap
/// operation on the value prepared in \c build().
/// In some cases, however, it may be very difficult to meet this
/// condition in a realistic way, while the failure case should really
/// be very rare. In such a case it may throw, and, if the parser is
/// called via \c configureAuthServer(), the caller will convert the
/// exception as a fatal error.
/// This method is expected to be called after \c build(), and only once.
/// The result is undefined otherwise.
......@@ -145,7 +152,12 @@ public:
/// a corresponding standard exception will be thrown.
/// Other exceptions may also be thrown, depending on the implementation of
/// the underlying derived class of \c AuthConfigError.
/// In any case the strong guarantee is provided as described above.
/// In any case the strong guarantee is provided as described above except
/// in the very rare cases where the \c commit() method of a parser throws
/// an exception. If that happens this function converts the exception
/// into a \c FatalError exception and rethrows it. This exception is
/// expected to be caught at the highest level of the application to terminate
/// the program gracefully.
/// \param server The \c AuthSrv object to be configured.
/// \param config_set A JSON style configuration to apply to \c server.
......@@ -26,6 +26,7 @@
#include <auth/auth_srv.h>
#include <auth/config.h>
#include <auth/common.h>
#include <testutils/mockups.h>
......@@ -103,6 +104,12 @@ TEST_F(AuthConfigTest, unknownConfigVar) {
TEST_F(AuthConfigTest, exceptionFromCommit) {
EXPECT_THROW(configureAuthServer(server, Element::fromJSON(
"{\"_commit_throw\": 10}")),
class MemoryDatasrcConfigTest : public AuthConfigTest {
MemoryDatasrcConfigTest() :
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment