Commit 6199d48f authored by JINMEI Tatuya's avatar JINMEI Tatuya
Browse files

- applied review feedback

- added comments about thread-safe considerations (based on review comments)
- some more code cleanups


git-svn-id: svn://bind10.isc.org/svn/bind10/branches/jinmei-dnsrrparams@473 e5f2f494-b856-4b98-b285-d166d9295462
parent 8e33df2e
......@@ -169,29 +169,20 @@ RRParamRegistry::add(const string& typecode_string, uint16_t typecode,
// Rollback logic on failure is complicated. If adding the new type or
// class fails, we should revert to the original state, cleaning up
// intermediate state. But we need to make sure that we don't remove
// existing data. addType()/AddClass() will simply ignore an attempt to
// existing data. addType()/addClass() will simply ignore an attempt to
// add the same data, so the cleanup should be performed only when we add
// something new but we fail in other part of the process.
bool will_add_type = false;
bool type_added = false;
bool will_add_class = false;
bool class_added = false;
if (impl_->code2typemap.find(typecode) == impl_->code2typemap.end()) {
will_add_type = true;
}
if (impl_->code2classmap.find(classcode) == impl_->code2classmap.end()) {
will_add_class = true;
}
try {
type_added = addType(typecode_string, typecode);
class_added = addClass(classcode_string, classcode);
} catch (...) {
if (will_add_type && type_added) {
if (type_added) {
removeType(typecode);
}
if (will_add_class && class_added) {
if (class_added) {
removeClass(classcode);
}
throw;
......@@ -222,7 +213,7 @@ caseStringEqual(const string& s1, const string& s2, size_t n)
/// Code logic for RRTypes and RRClasses is mostly common except (C++) type and
/// member names. So we define type-independent templates to describe the
/// common logic and let concrete classes to avoid code duplicates.
/// common logic and let concrete classes use it to avoid code duplicates.
/// The following summarize template parameters used in the set of template
/// functions:
/// PT: parameter type, either RRTypeParam or RRClassParam
......@@ -326,7 +317,7 @@ codeToText(uint16_t code, MC& codemap)
bool
RRParamRegistry::addType(const string& type_string, uint16_t code)
{
return (addParam<RRTypeParam, CodeRRTypeMap, StrRRTypeMap, RRTypeExist>
return (addParam<RRTypeParam, CodeRRTypeMap, StrRRTypeMap, RRTypeExists>
(type_string, code, impl_->code2typemap, impl_->str2typemap));
}
......@@ -353,7 +344,7 @@ RRParamRegistry::codeToTypeText(uint16_t code) const
bool
RRParamRegistry::addClass(const string& class_string, uint16_t code)
{
return (addParam<RRClassParam, CodeRRClassMap, StrRRClassMap, RRClassExist>
return (addParam<RRClassParam, CodeRRClassMap, StrRRClassMap, RRClassExists>
(class_string, code, impl_->code2classmap, impl_->str2classmap));
}
......@@ -375,7 +366,8 @@ RRParamRegistry::textToClassCode(const string& class_string) const
string
RRParamRegistry::codeToClassText(uint16_t code) const
{
return (codeToText<RRClassParam, CodeRRClassMap>(code, impl_->code2classmap));
return (codeToText<RRClassParam, CodeRRClassMap>(code,
impl_->code2classmap));
}
}
}
......@@ -30,22 +30,22 @@ namespace dns {
struct RRParamRegistryImpl;
///
/// \brief A standard DNS module exception that is thrown if a new RR class is
/// \brief A standard DNS module exception that is thrown if a new RR type is
/// being registered with a different type string.
///
class RRClassExist : public Exception {
class RRTypeExists : public Exception {
public:
RRClassExist(const char* file, size_t line, const char* what) :
RRTypeExists(const char* file, size_t line, const char* what) :
isc::dns::Exception(file, line, what) {}
};
///
/// \brief A standard DNS module exception that is thrown if a new RR type is
/// \brief A standard DNS module exception that is thrown if a new RR class is
/// being registered with a different type string.
///
class RRTypeExist : public Exception {
class RRClassExists : public Exception {
public:
RRTypeExist(const char* file, size_t line, const char* what) :
RRClassExists(const char* file, size_t line, const char* what) :
isc::dns::Exception(file, line, what) {}
};
......@@ -73,6 +73,17 @@ public:
/// constructor is intentionally private, and applications must get access to
/// the single instance via the \c getRegistry() static member function.
///
/// In the current implementation, access to the singleton \c RRParamRegistry
/// object is not thread safe.
/// The application should ensure that multiple threads don't race in the
/// first invocation of \c getRegistry(), and, if the registry needs to
/// be changed dynamically, read and write operations are performed
/// exclusively.
/// Since this class should be static in common usage this restriction would
/// be acceptable in practice.
/// In the future, we may extend the implementation so that multiple threads can
/// get access to the registry fully concurrently without any restriction.
///
/// Note: the implementation of this class is incomplete: we should at least
/// add RDATA related parameters. This will be done in a near future version,
/// at which point some of method signatures will be changed.
......@@ -88,6 +99,24 @@ private:
~RRParamRegistry();
//@}
public:
///
/// \brief Return the singleton instance of \c RRParamRegistry.
///
/// This method is a unified access point to the singleton instance of
/// the RR parameter registry (\c RRParamRegistry).
/// On first invocation it internally constructs an instance of the
/// \c RRParamRegistry class and returns a reference to it.
/// This is a static object inside this method and will remain valid
/// throughout the rest of the application lifetime.
/// On subsequent calls this method simply returns a reference to the
/// singleton object.
///
/// If resource allocation fails in the first invocation,
/// a corresponding standard exception will be thrown.
/// This method never fails otherwise. In particular, this method
/// doesn't throw an exception once the singleton instance is constructed.
///
/// \return A reference to the singleton instance of \c RRParamRegistry.
static RRParamRegistry& getRegistry();
///
......
......@@ -82,14 +82,14 @@ TEST_F(RRParamRegistryTest, addError)
EXPECT_THROW(RRParamRegistry::getRegistry().add(test_type_str,
test_type_code,
test_class_str, 1),
RRClassExist);
RRClassExists);
EXPECT_EQ("IN", RRClass(1).toText());
// Same for RRType
EXPECT_THROW(RRParamRegistry::getRegistry().add(test_type_str, 1,
test_class_str,
test_class_code),
RRTypeExist);
RRTypeExists);
EXPECT_EQ("A", RRType(1).toText());
}
}
......@@ -80,6 +80,11 @@ public:
/// before the initialization for \c default_type, we need help from
/// the proxy function.
///
/// In the current implementation, the initialization of the well-known
/// static objects is not thread safe. The same consideration as the
/// \c RRParamRegistry class applies. We may extend the implementation so
/// that the initialization is ensured to be thread safe in a future version.
///
/// Note to developers: since it's expected that some of these constant
/// \c RRType objects are frequently used in a performance sensitive path,
/// we define these proxy functions as inline. This makes sense only when
......
Supports Markdown
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