[master] Merge branch 'trac3213'

configure.ac

@@ -212,6 +212,7 @@ AC_HELP_STRING([--disable-setproctitle-check],
 # OS dependent configuration
 SET_ENV_LIBRARY_PATH=no
 ENV_LIBRARY_PATH=LD_LIBRARY_PATH
+bind10_undefined_pthread_behavior=no
 
 case "$host" in
 *-solaris*)
@@ -223,13 +224,25 @@ case "$host" in
 	# Destroying locked mutexes, condition variables being waited
 	# on, etc. are undefined behavior on Solaris, so we set it as
 	# such here.
-	AC_DEFINE([HAS_UNDEFINED_PTHREAD_BEHAVIOR], [1], [Does this platform have some undefined pthreads behavior?])
+	bind10_undefined_pthread_behavior=yes
 	;;
 *-apple-darwin*)
 	# Starting with OSX 10.7 (Lion) we must choose which IPv6 API to use
 	# (RFC2292 or RFC3542).
 	CPPFLAGS="$CPPFLAGS -D__APPLE_USE_RFC_3542"
 
+	# In OS X 10.9 (and possibly any future versions?) pthread_cond_destroy
+	# doesn't work as documented, which makes some of unit tests fail.
+	# Testing a specific system and version is not a good practice, but
+	# identifying this behavior would be too heavy (running a program
+	# with multiple threads), so this is a compromise.  In general,
+	# it should be avoided to rely on 'osx_version' unless there's no
+	# viable alternative.
+	osx_version=`/usr/bin/sw_vers -productVersion`
+	if [ test $osx_version = "10.9" ]; then
+		bind10_undefined_pthread_behavior=yes
+	fi
+
 	# libtool doesn't work perfectly with Darwin: libtool embeds the
 	# final install path in dynamic libraries and our loadable python
 	# modules always refer to that path even if it's loaded within the
@@ -252,6 +265,9 @@ esac
 AM_CONDITIONAL(SET_ENV_LIBRARY_PATH, test $SET_ENV_LIBRARY_PATH = yes)
 AC_SUBST(SET_ENV_LIBRARY_PATH)
 AC_SUBST(ENV_LIBRARY_PATH)
+if [ test $bind10_undefined_pthread_behavior = "yes" ]; then
+   AC_DEFINE([HAS_UNDEFINED_PTHREAD_BEHAVIOR], [1], [Does this platform have some undefined pthreads behavior?])
+fi
 
 # Our experiments have shown Solaris 10 has broken support for the
 # IPV6_USE_MIN_MTU socket option for getsockopt(); it doesn't return the value

m4macros/ax_boost_for_bind10.m4

@@ -70,6 +70,10 @@ fi
 AC_CHECK_HEADERS([boost/shared_ptr.hpp boost/foreach.hpp boost/interprocess/sync/interprocess_upgradable_mutex.hpp boost/date_time/posix_time/posix_time_types.hpp boost/bind.hpp boost/function.hpp],,
   AC_MSG_ERROR([Missing required header files.]))
 
+# clang can cause false positives with -Werror without -Qunused-arguments.
+# it can be triggered if used with ccache.
+AC_CHECK_DECL([__clang__], [CLANG_CXXFLAGS="-Qunused-arguments"], [])
+
 # Detect whether Boost tries to use threads by default, and, if not,
 # make it sure explicitly.  In some systems the automatic detection
 # may depend on preceding header files, and if inconsistency happens
@@ -87,7 +91,7 @@ AC_TRY_COMPILE([
 # Boost offset_ptr is known to not compile on some platforms, depending on
 # boost version, its local configuration, and compiler.  Detect it.
 CXXFLAGS_SAVED="$CXXFLAGS"
-CXXFLAGS="$CXXFLAGS -Werror"
+CXXFLAGS="$CXXFLAGS $CLANG_CXXFLAGS -Werror"
 AC_MSG_CHECKING([Boost offset_ptr compiles])
 AC_TRY_COMPILE([
 #include <boost/interprocess/offset_ptr.hpp>
@@ -102,7 +106,7 @@ CXXFLAGS="$CXXFLAGS_SAVED"
 # FreeBSD ports
 if test "X$GXX" = "Xyes"; then
    CXXFLAGS_SAVED="$CXXFLAGS"
-   CXXFLAGS="$CXXFLAGS -Werror"
+   CXXFLAGS="$CXXFLAGS $CLANG_CXXFLAGS -Werror"
 
    AC_MSG_CHECKING([Boost numeric_cast compiles with -Werror])
    AC_TRY_COMPILE([
@@ -140,11 +144,9 @@ BOOST_MAPPED_FILE_CXXFLAG=
 CXXFLAGS_SAVED="$CXXFLAGS"
 try_flags="no"
 if test "X$GXX" = "Xyes"; then
-  CXXFLAGS="$CXXFLAGS -Wall -Wextra -Werror"
+  CXXFLAGS="$CXXFLAGS $CLANG_CXXFLAGS -Wall -Wextra -Werror"
   try_flags="$try_flags -Wno-error"
 fi
-# clang can cause false positives with -Werror without -Qunused-arguments
-AC_CHECK_DECL([__clang__], [CXXFLAGS="$CXXFLAGS -Qunused-arguments"], [])
 
 AC_MSG_CHECKING([Boost managed_mapped_file compiles])
 CXXFLAGS_SAVED2="$CXXFLAGS"
@@ -152,7 +154,7 @@ for flag in $try_flags; do
   if test "$flag" != no; then
     BOOST_MAPPED_FILE_CXXFLAG="$flag"
   fi
-  CXXFLAGS="$CXXFLAGS $BOOST_MAPPED_FILE_CXXFLAG"
+  CXXFLAGS="$CXXFLAGS $CLANG_CXXFLAGS $BOOST_MAPPED_FILE_CXXFLAG"
   AC_TRY_COMPILE([
   #include <boost/interprocess/managed_mapped_file.hpp>
   ],[

src/bin/auth/auth_srv.cc

@@ -70,8 +70,6 @@
 
 using namespace std;
 
-using boost::shared_ptr;
-
 using namespace isc;
 using namespace isc::cc;
 using namespace isc::datasrc;
@@ -277,7 +275,7 @@ public:
     AddressList listen_addresses_;
 
     /// The TSIG keyring
-    const shared_ptr<TSIGKeyRing>* keyring_;
+    const boost::shared_ptr<TSIGKeyRing>* keyring_;
 
     /// The data source client list manager
     auth::DataSrcClientsMgr datasrc_clients_mgr_;
@@ -651,7 +649,7 @@ AuthSrvImpl::processNormalQuery(const IOMessage& io_message,
 
     try {
         const ConstQuestionPtr question = *message.beginQuestion();
-        const shared_ptr<datasrc::ClientList>
+        const boost::shared_ptr<datasrc::ClientList>
             list(datasrc_holder.findClientList(question->getClass()));
         if (list) {
             const RRType& qtype = question->getType();
@@ -766,7 +764,7 @@ AuthSrvImpl::processNotify(const IOMessage& io_message, Message& message,
     bool is_auth = false;
     {
         auth::DataSrcClientsMgr::Holder datasrc_holder(datasrc_clients_mgr_);
-        const shared_ptr<datasrc::ClientList> dsrc_clients =
+        const boost::shared_ptr<datasrc::ClientList> dsrc_clients =
             datasrc_holder.findClientList(question->getClass());
         is_auth = dsrc_clients &&
             dsrc_clients->find(question->getName(), true, false).exact_match_;
@@ -900,7 +898,7 @@ AuthSrv::setDNSService(isc::asiodns::DNSServiceBase& dnss) {
 }
 
 void
-AuthSrv::setTSIGKeyRing(const shared_ptr<TSIGKeyRing>* keyring) {
+AuthSrv::setTSIGKeyRing(const boost::shared_ptr<TSIGKeyRing>* keyring) {
     impl_->keyring_ = keyring;
 }
 

src/bin/auth/tests/datasrc_config_unittest.cc

@@ -30,7 +30,6 @@ using namespace isc::config;
 using namespace isc::data;
 using namespace isc::dns;
 using namespace std;
-using namespace boost;
 
 namespace {
 
@@ -57,7 +56,7 @@ private:
     ConstElementPtr configuration_;
 };
 
-typedef shared_ptr<FakeList> ListPtr;
+typedef boost::shared_ptr<FakeList> ListPtr;
 
 // Forward declaration.  We need precise definition of DatasrcConfigTest
 // to complete this function.
@@ -77,8 +76,8 @@ datasrcConfigHandler(DatasrcConfigTest* fake_server, const std::string&,
 
 class DatasrcConfigTest : public ::testing::Test {
 public:
-    void setDataSrcClientLists(shared_ptr<std::map<dns::RRClass, ListPtr> >
-                               new_lists)
+    void setDataSrcClientLists(boost::shared_ptr<std::map<dns::RRClass,
+                               ListPtr> > new_lists)
     {
         lists_.clear();         // first empty it
 
@@ -159,7 +158,7 @@ testConfigureDataSource(DatasrcConfigTest& test,
 {
     // We use customized (faked lists) for the List type.  This makes it
     // possible to easily look that they were called.
-    shared_ptr<std::map<dns::RRClass, ListPtr> > lists =
+    boost::shared_ptr<std::map<dns::RRClass, ListPtr> > lists =
         configureDataSourceGeneric<FakeList>(config);
     test.setDataSrcClientLists(lists);
 }

src/lib/asiodns/tcp_server.cc

@@ -31,8 +31,11 @@
 #include <sys/socket.h>
 #include <errno.h>
 
-using namespace asio;
-using asio::ip::udp;
+// Note: we intentionally avoid 'using namespace asio' to avoid conflicts with
+// std:: definitions in C++11.
+using asio::io_service;
+using asio::buffer;
+using asio::const_buffer;
 using asio::ip::tcp;
 
 using namespace std;
@@ -107,7 +110,7 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
                 CORO_YIELD acceptor_->async_accept(*socket_, *this);
                 if (ec) {
                     using namespace asio::error;
-                    const error_code::value_type err_val = ec.value();
+                    const asio::error_code::value_type err_val = ec.value();
                     // The following two cases can happen when this server is
                     // stopped: operation_aborted in case it's stopped after
                     // starting accept().  bad_descriptor in case it's stopped

src/lib/asiodns/tests/dns_server_unittest.cc

@@ -274,6 +274,7 @@ class TCPClient : public SimpleClient {
         server_ = server;
         socket_.reset(new ip::tcp::socket(service));
         socket_->open(ip::tcp::v6());
+        send_delay_timer_.reset(new deadline_timer(service));
     }
 
 
@@ -324,15 +325,32 @@ class TCPClient : public SimpleClient {
                                 size_t send_bytes)
     {
         if (!error && send_bytes == 2 && send_data_len_delay_ == 0) {
-            sleep(send_data_delay_);
-            socket_->async_send(buffer(data_to_send_.c_str(),
-                                       data_to_send_.size() + 1),
-                    boost::bind(&TCPClient::finishSendHandler, this, _1, _2));
+            // We cannot block here (such as by sleep(3)) since otherwise
+            // the ASIO events may not reliably handled in the server side
+            // as the test expects.  So we use async_wait, and make sure the
+            // control will be given back to the IO service.
+            if (send_data_delay_ > 0) {
+                send_delay_timer_->expires_from_now(boost::posix_time::
+                                                    seconds(send_data_delay_));
+                send_delay_timer_->async_wait(
+                    boost::bind(&TCPClient::sendMessageData, this));
+                return;
+            }
+            sendMessageData();
         }
     }
 
+    void sendMessageData() {
+        socket_->async_send(buffer(data_to_send_.c_str(),
+                                   data_to_send_.size() + 1),
+                            boost::bind(&TCPClient::finishSendHandler, this,
+                                        _1, _2));
+    }
+
     void finishSendHandler(const asio::error_code& error, size_t send_bytes) {
-        if (!error && send_bytes == data_to_send_.size() + 1) {
+        if (error) {
+            getResponseCallBack(error, 0);
+        } else if (send_bytes == data_to_send_.size() + 1) {
             socket_->async_receive(buffer(received_data_, MAX_DATA_LEN),
                    boost::bind(&SimpleClient::getResponseCallBack, this, _1,
                                _2));
@@ -343,6 +361,7 @@ class TCPClient : public SimpleClient {
     ip::tcp::endpoint server_;
     std::string data_to_send_;
     uint16_t data_to_send_len_;
+    boost::shared_ptr<deadline_timer> send_delay_timer_;
 
     size_t send_data_delay_;
     size_t send_data_len_delay_;

src/lib/asiodns/udp_server.cc

@@ -31,8 +31,12 @@
 
 #include <dns/opcode.h>
 
-using namespace asio;
+// Avoid 'using namespace asio' (see tcp_server.cc)
+using asio::io_service;
+using asio::socket_base;
+using asio::buffer;
 using asio::ip::udp;
+using asio::ip::address;
 
 using namespace std;
 using namespace isc::dns;
@@ -56,7 +60,7 @@ struct UDPServer::Data {
      * query, it will only hold parameters until we wait for the
      * first packet. But we do initialize the socket in here.
      */
-    Data(io_service& io_service, const ip::address& addr, const uint16_t port,
+    Data(io_service& io_service, const address& addr, const uint16_t port,
          DNSLookup* lookup, DNSAnswer* answer) :
         io_(io_service), bytes_(0), done_(false),
         lookup_callback_(lookup),
@@ -210,7 +214,7 @@ UDPServer::operator()(asio::error_code ec, size_t length) {
                 // See TCPServer::operator() for details on error handling.
                 if (ec) {
                     using namespace asio::error;
-                    const error_code::value_type err_val = ec.value();
+                    const asio::error_code::value_type err_val = ec.value();
                     if (err_val == operation_aborted ||
                         err_val == bad_descriptor) {
                         return;

src/lib/cc/data.cc

@@ -275,11 +275,11 @@ skipChars(std::istream& in, const char* chars, int& line, int& pos) {
 }
 
 // skip on the input stream to one of the characters in chars
-// if another character is found this function returns false
+// if another character is found this function throws JSONError
 // unless that character is specified in the optional may_skip
 //
-// the character found is left on the stream
-void
+// It returns the found character (as an int value).
+int
 skipTo(std::istream& in, const std::string& file, int& line,
        int& pos, const char* chars, const char* may_skip="")
 {
@@ -298,18 +298,18 @@ skipTo(std::istream& in, const std::string& file, int& line,
                 if (in.peek() == '\n') {
                     pos = 1;
                     ++line;
+                } else {
+                    ++pos;
                 }
                 in.ignore();
-                ++pos;
             }
-            in.putback(c);
-            --pos;
-            return;
+            return (c);
         } else {
             throwJSONError(std::string("'") + std::string(1, c) + "' read, one of \"" + chars + "\" expected", file, line, pos);
         }
     }
     throwJSONError(std::string("EOF read, one of \"") + chars + "\" expected", file, line, pos);
+    return (c); // shouldn't reach here, but some compilers require it
 }
 
 // TODO: Should we check for all other official escapes here (and
@@ -466,10 +466,11 @@ fromStringstreamList(std::istream& in, const std::string& file, int& line,
         if (in.peek() != ']') {
             cur_list_element = Element::fromJSON(in, file, line, pos);
             list->add(cur_list_element);
-            skipTo(in, file, line, pos, ",]", WHITESPACE);
+            c = skipTo(in, file, line, pos, ",]", WHITESPACE);
+        } else {
+            c = in.get();
+            ++pos;
         }
-        c = in.get();
-        pos++;
     }
     return (list);
 }
@@ -492,15 +493,11 @@ fromStringstreamMap(std::istream& in, const std::string& file, int& line,
 
             skipTo(in, file, line, pos, ":", WHITESPACE);
             // skip the :
-            in.ignore();
-            pos++;
 
             ConstElementPtr value = Element::fromJSON(in, file, line, pos);
             map->set(key, value);
 
-            skipTo(in, file, line, pos, ",}", WHITESPACE);
-            c = in.get();
-            pos++;
+            c = skipTo(in, file, line, pos, ",}", WHITESPACE);
         }
     }
     return (map);
@@ -596,6 +593,7 @@ Element::fromJSON(std::istream& in, const std::string& file, int& line,
             case '+':
             case '.':
                 in.putback(c);
+                --pos;
                 element = fromStringstreamNumber(in, pos);
                 el_read = true;
                 break;
@@ -604,17 +602,20 @@ Element::fromJSON(std::istream& in, const std::string& file, int& line,
             case 'f':
             case 'F':
                 in.putback(c);
+                --pos;
                 element = fromStringstreamBool(in, file, line, pos);
                 el_read = true;
                 break;
             case 'n':
             case 'N':
                 in.putback(c);
+                --pos;
                 element = fromStringstreamNull(in, file, line, pos);
                 el_read = true;
                 break;
             case '"':
                 in.putback('"');
+                --pos;
                 element = fromStringstreamString(in, file, line, pos);
                 el_read = true;
                 break;

src/lib/cc/tests/data_unittests.cc

@@ -98,16 +98,24 @@ TEST(Element, from_and_to_json) {
     sv.push_back("\"\xFF\"");
 
     BOOST_FOREACH(const std::string& s, sv) {
-        // test << operator, which uses Element::str()
-        std::ostringstream stream;
-        el = Element::fromJSON(s);
-        stream << *el;
-        EXPECT_EQ(s, stream.str());
-
-        // test toWire(ostream), which should also be the same now
-        std::ostringstream wire_stream;
-        el->toWire(wire_stream);
-        EXPECT_EQ(s, wire_stream.str());
+        // Test two types of fromJSON(): with string and istream.
+        for (int i = 0; i < 2; ++i) {
+            // test << operator, which uses Element::str()
+            if (i == 0) {
+                el = Element::fromJSON(s);
+            } else {
+                std::istringstream iss(s);
+                el = Element::fromJSON(iss);
+            }
+            std::ostringstream stream;
+            stream << *el;
+            EXPECT_EQ(s, stream.str());
+
+            // test toWire(ostream), which should also be the same now
+            std::ostringstream wire_stream;
+            el->toWire(wire_stream);
+            EXPECT_EQ(s, wire_stream.str());
+        }
     }
 
     // some parse errors

src/lib/datasrc/cache_config.h

@@ -17,6 +17,7 @@
 
 #include <exceptions/exceptions.h>
 
+#include <dns/name.h>
 #include <dns/dns_fwd.h>
 #include <cc/data.h>
 #include <datasrc/memory/load_action.h>

src/lib/datasrc/tests/client_list_unittest.cc

@@ -47,9 +47,8 @@ using isc::datasrc::memory::ZoneTableSegment;
 using isc::datasrc::memory::InMemoryZoneFinder;
 using namespace isc::data;
 using namespace isc::dns;
-// don't import the entire boost namespace.  It will unexpectedly hide uintXX_t
-// for some systems.
-using boost::shared_ptr;
+// note: don't use 'using [namespace]' for shared_ptr.  It would conflict with
+// C++ std:: definitions.
 using namespace std;
 
 namespace {
@@ -79,7 +78,7 @@ public:
         if (type == "MasterFiles") {
             return (DataSourcePair(0, DataSourceClientContainerPtr()));
         }
-        shared_ptr<MockDataSourceClient>
+        boost::shared_ptr<MockDataSourceClient>
             ds(new MockDataSourceClient(type, configuration));
         // Make sure it is deleted when the test list is deleted.
         to_delete_.push_back(ds);
@@ -88,7 +87,7 @@ public:
 private:
     // Hold list of data sources created internally, so they are preserved
     // until the end of the test and then deleted.
-    vector<shared_ptr<MockDataSourceClient> > to_delete_;
+    vector<boost::shared_ptr<MockDataSourceClient> > to_delete_;
 };
 
 const char* ds_zones[][3] = {
@@ -144,7 +143,7 @@ public:
             "}]"))
     {
         for (size_t i(0); i < ds_count; ++ i) {
-            shared_ptr<MockDataSourceClient>
+            boost::shared_ptr<MockDataSourceClient>
                 ds(new MockDataSourceClient(ds_zones[i]));
             ds_.push_back(ds);
             ds_info_.push_back(ConfigurableClientList::DataSourceInfo(
@@ -230,7 +229,7 @@ public:
     }
     // Check the positive result is as we expect it.
     void positiveResult(const ClientList::FindResult& result,
-                        const shared_ptr<MockDataSourceClient>& dsrc,
+                        const boost::shared_ptr<MockDataSourceClient>& dsrc,
                         const Name& name, bool exact,
                         const char* test, bool from_cache = false)
     {
@@ -242,10 +241,10 @@ public:
         // alive, even when we don't know what it is.
         // Any better idea how to test it actually keeps the thing
         // alive?
-        EXPECT_NE(shared_ptr<ClientList::FindResult::LifeKeeper>(),
+        EXPECT_NE(boost::shared_ptr<ClientList::FindResult::LifeKeeper>(),
                   result.life_keeper_);
         if (from_cache) {
-            EXPECT_NE(shared_ptr<InMemoryZoneFinder>(),
+            EXPECT_NE(boost::shared_ptr<InMemoryZoneFinder>(),
                       boost::dynamic_pointer_cast<InMemoryZoneFinder>(
                           result.finder_)) << "Finder is not from cache";
             EXPECT_TRUE(NULL !=
@@ -308,7 +307,7 @@ public:
         EXPECT_EQ(type, ds->type_);
         EXPECT_TRUE(Element::fromJSON(params)->equals(*ds->configuration_));
         EXPECT_EQ(cache, list_->getDataSources()[index].cache_ !=
-                  shared_ptr<InMemoryClient>());
+                  boost::shared_ptr<InMemoryClient>());
     }
     ConfigurableClientList::CacheStatus doReload(
         const Name& origin, const string& datasrc_name = "");
@@ -316,9 +315,9 @@ public:
         int numZones, const string& zoneName);
 
     const RRClass rrclass_;
-    shared_ptr<TestedList> list_;
+    boost::shared_ptr<TestedList> list_;
     const ClientList::FindResult negative_result_;
-    vector<shared_ptr<MockDataSourceClient> > ds_;
+    vector<boost::shared_ptr<MockDataSourceClient> > ds_;
     vector<ConfigurableClientList::DataSourceInfo> ds_info_;
     const ConstElementPtr config_elem_, config_elem_zones_;
 };
@@ -401,7 +400,7 @@ TEST_P(ListTest, selfTest) {
     EXPECT_EQ(result::NOTFOUND, ds_[0]->findZone(Name("aaa")).code);
     EXPECT_EQ(result::NOTFOUND, ds_[0]->findZone(Name("zzz")).code);
     // Nothing to keep alive here.
-    EXPECT_EQ(shared_ptr<ClientList::FindResult::LifeKeeper>(),
+    EXPECT_EQ(boost::shared_ptr<ClientList::FindResult::LifeKeeper>(),
                   negative_result_.life_keeper_);
 }
 
@@ -804,7 +803,8 @@ TEST_P(ListTest, cacheZones) {
     checkDS(0, "type1", "[\"example.org\", \"example.com\", \"exmaple.cz\"]",
             true);
 
-    const shared_ptr<InMemoryClient> cache(list_->getDataSources()[0].cache_);
+    const boost::shared_ptr<InMemoryClient> cache(
+        list_->getDataSources()[0].cache_);
     EXPECT_EQ(2, cache->getZoneCount());
 
     EXPECT_EQ(result::SUCCESS, cache->findZone(Name("example.org")).code);
@@ -840,7 +840,8 @@ TEST_P(ListTest, badCache) {
         "}]"));
     list_->configure(elem1, true); // shouldn't cause disruption
     checkDS(0, "test_type", "[\"example.org\"]", true);
-    const shared_ptr<InMemoryClient> cache(list_->getDataSources()[0].cache_);
+    const boost::shared_ptr<InMemoryClient> cache(
+        list_->getDataSources()[0].cache_);
     EXPECT_EQ(1, cache->getZoneCount());
     EXPECT_EQ(result::SUCCESS, cache->findZone(Name("example.org")).code);
     // Now, the zone doesn't give an iterator
@@ -895,7 +896,8 @@ TEST_P(ListTest,
         "}]"));
     list_->configure(elem, true); // no disruption
     checkDS(0, "test_type", "[\"example.org\"]", true);
-    const shared_ptr<InMemoryClient> cache(list_->getDataSources()[0].cache_);
+    const boost::shared_ptr<InMemoryClient> cache(
+        list_->getDataSources()[0].cache_);
 
     // Likewise, reload attempt will fail.
     EXPECT_EQ(ConfigurableClientList::CACHE_NOT_WRITABLE,

src/lib/datasrc/tests/memory/zone_finder_unittest.cc

@@ -44,7 +44,6 @@ using namespace isc::dns;
 using namespace isc::dns::rdata;
 using namespace isc::datasrc;
 using namespace isc::testutils;
-using boost::shared_ptr;
 using namespace isc::datasrc::test;
 using namespace isc::datasrc::memory::test;
 using namespace isc::datasrc::memory;
@@ -1612,7 +1611,7 @@ TEST_F(InMemoryZoneFinderTest, findOrphanRRSIG) {
 // handling)
 TEST_F(InMemoryZoneFinderTest, NSECNonExistentTest) {
     const Name name("example.com.");
-    shared_ptr<ZoneTableSegment> ztable_segment(
+    boost::shared_ptr<ZoneTableSegment> ztable_segment(
          new ZoneTableSegmentMock(class_, mem_sgmt_));
     updater_.reset();
     loadZoneIntoTable(*ztable_segment, name, class_,
@@ -1775,7 +1774,7 @@ TEST_F(InMemoryZoneFinderNSEC3Test, findNSEC3MissingOrigin) {
      setNSEC3HashCreator(&creator);
 
      const Name name("example.com.");
-     shared_ptr<ZoneTableSegment> ztable_segment(
+     boost::shared_ptr<ZoneTableSegment> ztable_segment(
           new ZoneTableSegmentMock(class_, mem_sgmt_));
      updater_.reset();
      loadZoneIntoTable(*ztable_segment, name, class_,

src/lib/datasrc/tests/test_client.cc

@@ -32,7 +32,6 @@
 #include <fstream>
 
 using namespace std;
-using boost::shared_ptr;
 
 using namespace isc::dns;
 
@@ -48,7 +47,7 @@ addRRset(ZoneUpdaterPtr updater, ConstRRsetPtr rrset) {
 }
 }
 
-shared_ptr<DataSourceClient>
+boost::shared_ptr<DataSourceClient>
 createSQLite3Client(RRClass zclass, const Name& zname,
                     const char* const db_file, const char* const zone_file)
 {
@@ -60,7 +59,7 @@ createSQLite3Client(RRClass zclass, const Name& zname,
     return (createSQLite3Client(zclass, zname, db_file, ifs));
 }
 
-shared_ptr<DataSourceClient>
+boost::shared_ptr<DataSourceClient>
 createSQLite3Client(RRClass zclass, const Name& zname,
                     const char* const db_file, istream& rr_stream)
 {
@@ -75,9 +74,10 @@ createSQLite3Client(RRClass zclass, const Name& zname,
                   "Error setting up; command failed: " << install_cmd);
     }
 
-    shared_ptr<SQLite3Accessor> accessor(
+    boost::shared_ptr<SQLite3Accessor> accessor(
         new SQLite3Accessor(db_file, zclass.toText()));
-    shared_ptr<DatabaseClient> client(new DatabaseClient(zclass, accessor));
+    boost::shared_ptr<DatabaseClient> client(new DatabaseClient(zclass,
+                                                                accessor));
 
     ZoneUpdaterPtr updater = client->getUpdater(zname, true);
     masterLoad(rr_stream, zname, zclass, boost::bind(addRRset, updater, _1));

src/lib/datasrc/tests/zone_finder_context_unittest.cc

@@ -42,7 +42,6 @@
 #include <vector>
 
 using namespace std;
-using boost::shared_ptr;
 
 using namespace isc::data;
 using namespace isc::util;
@@ -59,7 +58,7 @@ namespace {
 const char* const TEST_ZONE_FILE = TEST_DATA_DIR "/contexttest.zone";
 
 // Convenient shortcut
-typedef shared_ptr<DataSourceClient> DataSourceClientPtr;
+typedef boost::shared_ptr<DataSourceClient> DataSourceClientPtr;
 
 // This is the type used as the test parameter.  Note that this is
 // intentionally a plain old type (i.e. a function pointer), not a class;
@@ -75,7 +74,7 @@ createInMemoryClient(RRClass zclass, const Name& zname) {
             " \"params\":"
             "  {\"" + zname.toText() + "\": \"" +
             string(TEST_ZONE_FILE) + "\"}}"), true);
-    shared_ptr<ZoneTableSegment> ztable_segment(
+    boost::shared_ptr<ZoneTableSegment> ztable_segment(
         ZoneTableSegment::create(zclass, cache_conf.getSegmentType()));
     memory::ZoneWriter writer(*ztable_segment,
                               cache_conf.getLoadAction(zclass, zname),
@@ -83,8 +82,8 @@ createInMemoryClient(RRClass zclass, const Name& zname) {
     writer.load();
     writer.install();
     writer.cleanup();
-    shared_ptr<InMemoryClient> client(new InMemoryClient(ztable_segment,
-                                                         zclass));
+    boost::shared_ptr<InMemoryClient> client(new InMemoryClient(ztable_segment,
+                                                                zclass));
 
     return (client);
 }

src/lib/dns/masterload.cc

@@ -19,11 +19,13 @@
 #include <cctype>
 #include <cerrno>
 
+#include <boost/bind.hpp>
 #include <boost/scoped_ptr.hpp>
 
 #include <exceptions/exceptions.h>
 
 #include <dns/masterload.h>
+#include <dns/master_loader.h>
 #include <dns/name.h>
 #include <dns/rdata.h>
 #include <dns/rdataclass.h>
@@ -31,6 +33,7 @@
 #include <dns/rrset.h>
 #include <dns/rrttl.h>
 #include <dns/rrtype.h>
+#include <dns/rrcollator.h>
 
 using namespace std;
 using namespace boost;
@@ -39,25 +42,45 @@ using namespace isc::dns::rdata;
 namespace isc {
 namespace dns {
 namespace {
-// A helper function that strips off any comment or whitespace at the end of
-// an RR.
-// This is an incomplete implementation, and cannot handle all such comments;
-// it's considered a short term workaround to deal with some real world
-// cases.
-string
-stripLine(string& s, const Exception& ex) {
-    // Find any ';' in the text data, and locate the position of the last
-    // occurrence.  Note that unless/until we support empty RDATA it
-    // shouldn't be placed at the beginning of the data.
-    const size_t pos_semicolon = s.rfind(';');
-    if (pos_semicolon == 0) {
-        throw ex;
-    } else if (pos_semicolon != string::npos) {
-        s.resize(pos_semicolon);
+void
+callbackWrapper(const RRsetPtr& rrset, MasterLoadCallback callback,
+                const Name* origin)
+{
+    // Origin related validation:
+    //  - reject out-of-zone data
+    //  - reject SOA whose owner is not at the top of zone
+    const NameComparisonResult cmp_result =
+        rrset->getName().compare(*origin);
+    if (cmp_result.getRelation() != NameComparisonResult::EQUAL &&
+        cmp_result.getRelation() != NameComparisonResult::SUBDOMAIN) {
+        isc_throw(MasterLoadError, "Out-of-zone data for " << *origin
+                  << "/" << rrset->getClass() << ": " << rrset->getName());
+    }
+    if (rrset->getType() == RRType::SOA() &&
+        cmp_result.getRelation() != NameComparisonResult::EQUAL) {
+        isc_throw(MasterLoadError, "SOA not at top of zone: "
+                  << *rrset);
+    }
+
+    callback(rrset);
+}
+
+template <typename InputType>
+void
+loadHelper(InputType input, const Name& origin,
+           const RRClass& zone_class, MasterLoadCallback callback)
+{
+    RRCollator rr_collator(boost::bind(callbackWrapper, _1,
+                                       callback, &origin));
+    MasterLoader loader(input, origin, zone_class,
+                        MasterLoaderCallbacks::getNullCallbacks(),
+                        rr_collator.getCallback());
+    try {
+        loader.load();
+    } catch (const MasterLoaderError& ex) {
+        isc_throw(MasterLoadError, ex.what());
     }
-    // Remove any trailing whitespace return the resulting text.
-    s.resize(s.find_last_not_of(" \t") + 1);
-    return (s);
+    rr_collator.flush();
 }
 }
 
@@ -69,154 +92,14 @@ masterLoad(const char* const filename, const Name& origin,
         isc_throw(MasterLoadError, "Name of master file must not be null");
     }
 
-    ifstream ifs;
-    ifs.open(filename, ios_base::in);
-    if (ifs.fail()) {
-        isc_throw(MasterLoadError, "Failed to open master file: " <<
-                  filename << ": " << strerror(errno));
-    }
-    masterLoad(ifs, origin, zone_class, callback, filename);
-    ifs.close();
+    loadHelper<const char*>(filename, origin, zone_class, callback);
 }
 
 void
 masterLoad(istream& input, const Name& origin, const RRClass& zone_class,
-           MasterLoadCallback callback, const char* source)
+           MasterLoadCallback callback, const char*)
 {
-    RRsetPtr rrset;
-    ConstRdataPtr prev_rdata;   // placeholder for special case of RRSIGs
-    string line;
-    unsigned int line_count = 1;
-
-    if (source == NULL) {
-        source = "<unknown>";
-    }
-
-    do {
-        getline(input, line);
-        if (input.bad() || (input.fail() && !input.eof())) {
-            isc_throw(MasterLoadError, "Unexpectedly failed to read a line in "
-                                       << source);
-        }
-
-        // blank/comment lines should be simply skipped.
-        if (line.empty() || line[0] == ';') {
-            continue;
-        }
-
-        // The line shouldn't have leading space (which means omitting the
-        // owner name).
-        if (isspace(line[0])) {
-            isc_throw(MasterLoadError, "Leading space in " << source
-                                       << " at line " << line_count);
-        }
-
-        // Parse a single RR
-        istringstream iss(line);
-        string owner_txt, ttl_txt, rrclass_txt, rrtype_txt;
-        stringbuf rdatabuf;
-        iss >> owner_txt >> ttl_txt >> rrclass_txt >> rrtype_txt >> &rdatabuf;
-        if (iss.bad() || iss.fail()) {
-            isc_throw(MasterLoadError, "Parse failure for a valid RR in "
-                      << source << " at line " << line_count);
-        }
-
-        // This simple version doesn't support relative owner names with a
-        // separate origin.
-        if (owner_txt.empty() || *(owner_txt.end() - 1) != '.') {
-            isc_throw(MasterLoadError, "Owner name is not absolute in "
-                      << source << " at line " << line_count);
-        }
-
-        // XXX: this part is a bit tricky (and less efficient).  We are going
-        // to validate the text for the RR parameters, and throw an exception
-        // if any of them is invalid by converting an underlying exception
-        // to MasterLoadError.  To do that, we need to define the corresponding
-        // variables used for RRset construction outside the try-catch block,
-        // but we don't like to use a temporary variable with a meaningless
-        // initial value.  So we define pointers outside the try block
-        // and allocate/initialize the actual objects within the block.
-        // To make it exception safe we use Boost.scoped_ptr.
-        scoped_ptr<const Name> owner;
-        scoped_ptr<const RRTTL> ttl;
-        scoped_ptr<const RRClass> rrclass;
-        scoped_ptr<const RRType> rrtype;
-        ConstRdataPtr rdata;
-        try {
-            owner.reset(new Name(owner_txt));
-            ttl.reset(new RRTTL(ttl_txt));
-            rrclass.reset(new RRClass(rrclass_txt));
-            rrtype.reset(new RRType(rrtype_txt));
-            string rdtext = rdatabuf.str();
-            try {
-                rdata = createRdata(*rrtype, *rrclass, rdtext);
-            } catch (const Exception& ex) {
-                // If the parse for the RDATA fails, check if it has comments
-                // or whitespace at the end, and if so, retry the conversion
-                // after stripping off the comment or whitespace
-                rdata = createRdata(*rrtype, *rrclass, stripLine(rdtext, ex));
-            }
-        } catch (const Exception& ex) {
-            isc_throw(MasterLoadError, "Invalid RR text in " << source <<
-                                       " at line " << line_count << ": "
-                                       << ex.what());
-        }
-
-        // Origin related validation:
-        //  - reject out-of-zone data
-        //  - reject SOA whose owner is not at the top of zone
-        const NameComparisonResult cmp_result = owner->compare(origin);
-        if (cmp_result.getRelation() != NameComparisonResult::EQUAL &&
-            cmp_result.getRelation() != NameComparisonResult::SUBDOMAIN) {
-            isc_throw(MasterLoadError, "Out-of-zone data for " << origin
-                                       << "/" << rrclass_txt << " in "
-                                       << source << " at line "
-                                       << line_count);
-        }
-        if (*rrtype == RRType::SOA() &&
-            cmp_result.getRelation() != NameComparisonResult::EQUAL) {
-            isc_throw(MasterLoadError, "SOA not at top of zone in "
-                      << source << " at line " << line_count);
-        }
-
-        // Reject RR class mismatching
-        if (*rrclass != zone_class) {
-            isc_throw(MasterLoadError, "RR class (" << rrclass_txt
-                      << ") does not match the zone class (" << zone_class
-                      << ") in " << source << " at line " << line_count);
-        }
-
-        // Everything is okay.  Now create/update RRset with the new RR.
-        // If this is the first RR or the RR type/name is new, we are seeing
-        // a new RRset.
-        bool new_rrset = false;
-        if (!rrset || rrset->getType() != *rrtype ||
-            rrset->getName() != *owner) {
-            new_rrset = true;
-        } else if (rrset->getType() == RRType::RRSIG()) {
-            // We are seeing two consecutive RRSIGs of the same name.
-            // They can be combined iff they have the same type covered.
-            if (dynamic_cast<const generic::RRSIG&>(*rdata).typeCovered() !=
-                dynamic_cast<const generic::RRSIG&>(*prev_rdata).typeCovered())
-            {
-                new_rrset = true;
-            }
-        }
-        if (new_rrset) {
-            // Commit the previous RRset, if any.
-            if (rrset) {
-                callback(rrset);
-            }
-            rrset = RRsetPtr(new RRset(*owner, *rrclass, *rrtype, *ttl));
-        }
-        rrset->addRdata(rdata);
-        prev_rdata = rdata;
-    } while (++line_count, !input.eof());
-
-    // Commit the last RRset, if any.
-    if (rrset) {
-        callback(rrset);
-    }
+    loadHelper<istream&>(input, origin, zone_class, callback);
 }
 
 } // namespace dns

src/lib/dns/masterload.h

@@ -64,86 +64,22 @@ typedef boost::function<void(RRsetPtr)> MasterLoadCallback;
 /// and this function never uses it once it is called.
 /// The callback can freely modify the passed \c RRset.
 ///
-/// This function performs minimum level of validation on the input:
-/// - Each RR is a valid textual representation per the DNS protocol.
-/// - The class of each RR must be identical to the specified RR class.
-/// - The owner name of each RR must be a subdomain of the origin name
-///   (that can be equal to the origin).
-/// - If an SOA RR is included, its owner name must be the origin name.
-/// If any of these validation checks fails, this function throws an
-/// exception of class \c MasterLoadError.
+/// This function internally uses the MasterLoader class, and basically
+/// accepts and rejects input that MasterLoader accepts and rejects,
+/// accordingly.  In addition, this function performs the following validation:
+/// if an SOA RR is included, its owner name must be the origin name.
 ///
 /// It does not perform other semantical checks, however.  For example,
 /// it doesn't check if an NS RR of the origin name is included or if
 /// there is more than one SOA RR.  Such further checks are the caller's
 /// (or the callback's) responsibility.
 ///
-/// <b>Acceptable Format</b>
-///
-/// The current implementation only supports a restricted form of master files
-/// for simplicity.  One easy way to ensure that a handwritten zone file is
-/// acceptable to this implementation is to preprocess it with BIND 9's
-/// named-compilezone tool with both the input and output formats being
-/// "text".
-/// Here is an example:
-/// \code % named-compilezone -f text -F text -o example.com.norm
-///      example.com example.com.zone
-/// \endcode
-/// where example.com.zone is the original zone file for the "example.com"
-/// zone.  The output file is example.com.norm, which should be acceptable
-/// by this implementation.
-///
-/// Below are specific restrictions that this implementation assumes.
-/// Basically, each RR must consist of exactly one line
-/// (so there shouldn't be a multi-line RR) in the following format:
-/// \code  <owner name> <TTL> <RRCLASS> <RRTYPE> <RDATA (single line)>
-/// \endcode
-/// Here are some more details about the restrictions:
-/// - No special directives such as $TTL are supported.
-/// - The owner name must be absolute, that is, it must end with a period.
-/// - "@" is not recognized as a valid owner name.
-/// - Owner names, TTL and RRCLASS cannot be omitted.
-/// - As a corollary, a non blank line must not begin with a space character.
-/// - The order of the RR parameters is fixed, for example, this is acceptable:
-/// \code example.com. 3600 IN A 192.0.2.1
-/// \endcode
-///  but this is not even though it's valid per RFC1035:
-/// \code example.com. IN 3600 A 192.0.2.1
-/// \endcode
-/// - "TTL", "RRCLASS", and "RRTYPE" must be recognizable by the \c RRTTL,
-///   RRClass and RRType class implementations of this library.  In particular,
-///   as of this writing TTL must be a decimal number (a convenient extension
-///   such as "1H" instead of 3600 cannot be used).  Not all standard RR
-///   classes and RR types are supported yet, so the mnemonics of them will
-///   be rejected, too.
-/// - RR TTLs of the same RRset must be the same; even if they are different,
-///   this implementation simply uses the TTL of the first RR.
-///
-/// Blank lines and lines beginning with a semi-colon are allowed, and will
-/// be simply ignored.  Comments cannot coexist with an RR line, however.
-/// For example, this will be rejected:
-/// \code example.com. 3600 IN A 192.0.2.1 ; this is a comment
-/// \endcode
-///
-/// This implementation assumes that RRs of a single RRset are not
-/// interleaved with RRs of a different RRset.
-/// That is, the following sequence shouldn't happen:
-/// \code example.com. 3600 IN A 192.0.2.1
-/// example.com. 3600 IN AAAA 2001:db8::1
-/// example.com. 3600 IN A 192.0.2.2
-/// \endcode
-/// But it does not consider this an error; it will simply regard each RR
-/// as a separate RRset and call the callback with them separately.
-/// It is up to the callback to merge multiple RRsets into one if possible
-/// and necessary.
-///
 /// <b>Exceptions</b>
 ///
 /// This function throws an exception of class \c MasterLoadError in the
 /// following cases:
-/// - Any of the validation checks fails (see the class description).
-/// - The input data is not in the acceptable format (see the details of
-///   the format above).
+/// - Any of the validation checks fails (see above).
+/// - The input data has a syntax error.
 /// - The specified file cannot be opened for loading.
 /// - An I/O error occurs during the loading.
 ///
@@ -196,16 +132,12 @@ typedef boost::function<void(RRsetPtr)> MasterLoadCallback;
 /// The current implementation is in a preliminary level and needs further
 /// extensions.  Some design decisions may also have to be reconsidered as
 /// we gain experiences.  Those include:
-/// - We should be more flexible about the input format.
 /// - We may want to allow optional conditions.  For example, we may want to
 ///   be generous about some validation failures and be able to continue
 ///   parsing.
 /// - Especially if we allow to be generous, we may also want to support
 ///   returning an error code instead of throwing an exception when we
 ///   encounter validation failure.
-/// - We may want to support incremental loading.
-/// - If we add these optional features we may want to introduce a class
-///   that encapsulates loading status and options.
 /// - RRSIGs are handled as separate RRsets, i.e. they are not included in
 ///   the RRset they cover.
 ///
@@ -227,14 +159,16 @@ void masterLoad(const char* const filename, const Name& origin,
 /// All descriptions of the other version apply to this version except those
 /// specific to file I/O.
 ///
+/// Note: The 'source' parameter is now ignored, but it was only used in
+/// exception messages on some error.  So the compatibility effect should be
+/// minimal.
+///
 /// \param input An input stream object that is to emit zone's RRs.
 /// \param origin The origin name of the zone.
 /// \param zone_class The RR class of the zone.
 /// \param callback A callback functor or function that is to be called for
 /// each RRset.
-/// \param source A string to use in error messages if zone content is bad
-/// (e.g. the file name when reading from a file). If this value is NULL,
-/// or left out, the error will use the string '<unknown>'
+/// \param source This parameter is now ignored but left for compatibility.
 void masterLoad(std::istream& input, const Name& origin,
                 const RRClass& zone_class, MasterLoadCallback callback,
                 const char* source = NULL);

src/lib/dns/tests/masterload_unittest.cc

@@ -267,6 +267,15 @@ TEST_F(MasterLoadTest, loadRRNoComment) {
                                       "\"aaa;bbb\"")));
 }
 
+TEST_F(MasterLoadTest, nonAbsoluteOwner) {
+    // If the owner name is not absolute, the zone origin is assumed to be
+    // its origin.
+    rr_stream << "example.com 3600 IN A 192.0.2.1";
+    masterLoad(rr_stream, origin, zclass, callback);
+    EXPECT_EQ(1, results.size());
+    EXPECT_EQ(results[0]->getName(), Name("example.com.example.com"));
+}
+
 TEST_F(MasterLoadTest, loadRREmptyAndComment) {
     // There's no RDATA (invalid in this case) but a comment.  This position
     // shouldn't cause any disruption and should be treated as a normal error.
@@ -356,11 +365,6 @@ TEST_F(MasterLoadTest, loadBadRRText) {
     stringstream rr_stream6("example.com. 3600 IN A");
     EXPECT_THROW(masterLoad(rr_stream6, origin, zclass, callback),
                  MasterLoadError);
-
-    // owner name is not absolute
-    stringstream rr_stream7("example.com 3600 IN A 192.0.2.1");
-    EXPECT_THROW(masterLoad(rr_stream7, origin, zclass, callback),
-                 MasterLoadError);
 }
 
 // This is a helper callback to test the case the input stream becomes bad

src/lib/util/threads/tests/condvar_unittest.cc

@@ -142,8 +142,13 @@ signalAndWait(CondVar* condvar, Mutex* mutex) {
     condvar->wait(*mutex);
 }
 
-#ifndef HAS_UNDEFINED_PTHREAD_BEHAVIOR
-TEST_F(CondVarTest, destroyWhileWait) {
+TEST_F(CondVarTest,
+#ifdef HAS_UNDEFINED_PTHREAD_BEHAVIOR
+       DISABLED_destroyWhileWait
+#else
+       destroyWhileWait
+#endif
+) {
     // We'll destroy a CondVar object while the thread is still waiting
     // on it.  This will trigger an assertion failure.
     if (!isc::util::unittests::runningOnValgrind()) {
@@ -155,7 +160,6 @@ TEST_F(CondVarTest, destroyWhileWait) {
             }, "");
     }
 }
-#endif // !HAS_UNDEFINED_PTHREAD_BEHAVIOR
 
 #ifdef ENABLE_DEBUG
 

src/lib/util/threads/tests/lock_unittest.cc

@@ -86,9 +86,14 @@ TEST(MutexTest, lockNonBlocking) {
 
 #endif // ENABLE_DEBUG
 
-#ifndef HAS_UNDEFINED_PTHREAD_BEHAVIOR
 // Destroying a locked mutex is a bad idea as well
-TEST(MutexTest, destroyLocked) {
+TEST(MutexTest,
+#ifdef HAS_UNDEFINED_PTHREAD_BEHAVIOR
+     DISABLED_destroyLocked
+#else
+     destroyLocked
+#endif
+) {
     if (!isc::util::unittests::runningOnValgrind()) {
         EXPECT_DEATH_IF_SUPPORTED({
             Mutex* mutex = new Mutex;
@@ -99,7 +104,6 @@ TEST(MutexTest, destroyLocked) {
         }, "");
     }
 }
-#endif // !HAS_UNDEFINED_PTHREAD_BEHAVIOR
 
 // In this test, we try to check if a mutex really locks. We could try that
 // with a deadlock, but that's not practical (the test would not end).