BIND 10 trac931_2, updated. e156ec53472f22bbb890ebc76a1acdd9a9eda70c [trac931] Different test for the double read
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed May 25 11:33:51 UTC 2011
The branch, trac931_2 has been updated
via e156ec53472f22bbb890ebc76a1acdd9a9eda70c (commit)
via d694e5ded809ef2842eb5101e4d03e6e657e4671 (commit)
via 5d33e8c43d18b6aef9461086c8fbd5ee493b55df (commit)
from b2c418d1d6aed9e2eb599941218e1f8c0d13a445 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit e156ec53472f22bbb890ebc76a1acdd9a9eda70c
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Wed May 25 13:33:32 2011 +0200
[trac931] Different test for the double read
commit d694e5ded809ef2842eb5101e4d03e6e657e4671
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Wed May 25 12:42:06 2011 +0200
Revert "[trac931] Use uint32_t for fixed-sized lengths"
This reverts commit 6342eda010ae512cb972e70f3824ca0de638c293.
commit 5d33e8c43d18b6aef9461086c8fbd5ee493b55df
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Wed May 25 12:40:04 2011 +0200
Revert the double read bug test
Revert "[trac931_2] make sure the workaround -Wno-unused-parameter follows -Wextra"
Revert "[trac931] Disable warning on one needed file"
Revert "[trac931] Missing includes"
Revert "[trac931] Test reproducing the double read bug"
This reverts commit ec6446047b5007290ca4d6d31e67239c424f33bb.
This reverts commit 990a0cff9891fce08b3e163720dfa08fffdede5f.
This reverts commit 54f86d7796c55289518befaefdcdd7d84ebefa88.
This reverts commit 37ded0b31a0a0dcadb93da7bd85248b90d9af57f.
-----------------------------------------------------------------------
Summary of changes:
src/lib/cc/session.cc | 20 ++--
src/lib/cc/tests/session_unittests.cc | 5 +
src/lib/config/ccsession.cc | 6 +-
src/lib/config/ccsession.h | 15 +--
src/lib/config/tests/Makefile.am | 12 +--
src/lib/config/tests/ccsession_unittests.cc | 123 ++++----------------
.../config/tests/data_def_unittests_config.h.in | 1 -
src/lib/config/tests/fake_session.cc | 22 +++-
src/lib/config/tests/fake_session.h | 9 ++
src/lib/server_common/tests/keyring_test.cc | 2 +-
10 files changed, 78 insertions(+), 137 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/cc/session.cc b/src/lib/cc/session.cc
index 847ffba..e911a86 100644
--- a/src/lib/cc/session.cc
+++ b/src/lib/cc/session.cc
@@ -79,11 +79,11 @@ public:
{}
void establish(const char& socket_file);
void disconnect();
- void writeData(const void* data, uint32_t datalen);
- uint32_t readDataLength();
+ void writeData(const void* data, size_t datalen);
+ size_t readDataLength();
// Blocking read. Will throw a SessionTimeout if the timeout value
// (in seconds) is thrown. If timeout is 0 it will block forever
- void readData(void* data, uint32_t datalen);
+ void readData(void* data, size_t datalen);
void startRead(boost::function<void()> user_handler);
void setTimeout(size_t seconds) { timeout_ = seconds; };
size_t getTimeout() const { return timeout_; };
@@ -94,7 +94,7 @@ public:
private:
void internalRead(const asio::error_code& error,
- uint32_t bytes_transferred);
+ size_t bytes_transferred);
private:
io_service& io_service_;
@@ -136,7 +136,7 @@ SessionImpl::disconnect() {
}
void
-SessionImpl::writeData(const void* data, uint32_t datalen) {
+SessionImpl::writeData(const void* data, size_t datalen) {
try {
asio::write(socket_, asio::buffer(data, datalen));
} catch (const asio::system_error& asio_ex) {
@@ -144,9 +144,9 @@ SessionImpl::writeData(const void* data, uint32_t datalen) {
}
}
-uint32_t
+size_t
SessionImpl::readDataLength() {
- uint32_t ret_len = data_length_;
+ size_t ret_len = data_length_;
if (ret_len == 0) {
readData(&data_length_, sizeof(data_length_));
@@ -161,7 +161,7 @@ SessionImpl::readDataLength() {
}
void
-SessionImpl::readData(void* data, uint32_t datalen) {
+SessionImpl::readData(void* data, size_t datalen) {
boost::optional<asio::error_code> read_result;
boost::optional<asio::error_code> timer_result;
@@ -227,7 +227,7 @@ SessionImpl::startRead(boost::function<void()> user_handler) {
void
SessionImpl::internalRead(const asio::error_code& error,
- uint32_t bytes_transferred)
+ size_t bytes_transferred)
{
if (!error) {
assert(bytes_transferred == sizeof(data_length_));
@@ -349,7 +349,7 @@ bool
Session::recvmsg(ConstElementPtr& env, ConstElementPtr& msg,
bool nonblock, int seq)
{
- uint32_t length = impl_->readDataLength();
+ size_t length = impl_->readDataLength();
if (hasQueuedMsgs()) {
ConstElementPtr q_el;
for (int i = 0; i < impl_->queue_->size(); i++) {
diff --git a/src/lib/cc/tests/session_unittests.cc b/src/lib/cc/tests/session_unittests.cc
index 5f6e595..e64925c 100644
--- a/src/lib/cc/tests/session_unittests.cc
+++ b/src/lib/cc/tests/session_unittests.cc
@@ -235,4 +235,9 @@ TEST_F(SessionTest, run_with_handler_timeout) {
ASSERT_THROW(my_io_service.run(), SessionTimeout);
}
+// Test it throws if we start it twice
+TEST_F(SessionTest, doubleStart) {
+ sess.start();
+}
+
diff --git a/src/lib/config/ccsession.cc b/src/lib/config/ccsession.cc
index 02472ce..2dffbc5 100644
--- a/src/lib/config/ccsession.cc
+++ b/src/lib/config/ccsession.cc
@@ -193,7 +193,7 @@ ModuleCCSession::ModuleCCSession(
isc::data::ConstElementPtr new_config),
isc::data::ConstElementPtr(*command_handler)(
const std::string& command, isc::data::ConstElementPtr args),
- bool start_immediately, const char* socket_path
+ bool start_immediately
) :
started_(false),
session_(session)
@@ -205,7 +205,7 @@ ModuleCCSession::ModuleCCSession(
config_handler_ = config_handler;
command_handler_ = command_handler;
- session_.establish(socket_path);
+ session_.establish(NULL);
session_.subscribe(module_name_, "*");
//session_.subscribe("Boss", "*");
//session_.subscribe("statistics", "*");
@@ -252,6 +252,8 @@ ModuleCCSession::start() {
// register callback for asynchronous read
session_.startRead(boost::bind(&ModuleCCSession::startCheck, this));
+
+ started_ = true;
}
/// Validates the new config values, if they are correct,
diff --git a/src/lib/config/ccsession.h b/src/lib/config/ccsession.h
index 379d055..9274649 100644
--- a/src/lib/config/ccsession.h
+++ b/src/lib/config/ccsession.h
@@ -171,13 +171,11 @@ public:
* @param command_handler A callback function pointer to be called when
* a control command from a remote agent needs to be performed on the
* local module.
- * @param start_immediately If true (default), start listening to new
- * commands and configuration changes asynchronously at the end of the
- * constructor; if false, it will be delayed until the start() method is
- * explicitly called. (This is a short term workaround for an
- * initialization trouble. We'll need to develop a cleaner solution, and
- * then remove this knob)
- * @param socket_path The path to the socket. For testing purposes.
+ * @start_immediately If true (default), start listening to new commands
+ * and configuration changes asynchronously at the end of the constructor;
+ * if false, it will be delayed until the start() method is explicitly
+ * called. (This is a short term workaround for an initialization trouble.
+ * We'll need to develop a cleaner solution, and then remove this knob)
*/
ModuleCCSession(const std::string& spec_file_name,
isc::cc::AbstractSession& session,
@@ -186,8 +184,7 @@ public:
isc::data::ConstElementPtr(*command_handler)(
const std::string& command,
isc::data::ConstElementPtr args) = NULL,
- bool start_immediately = true,
- const char* socket_path = NULL
+ bool start_immediately = true
);
/// Start receiving new commands and configuration changes asynchronously.
diff --git a/src/lib/config/tests/Makefile.am b/src/lib/config/tests/Makefile.am
index 293f3ef..0d2c29b 100644
--- a/src/lib/config/tests/Makefile.am
+++ b/src/lib/config/tests/Makefile.am
@@ -16,17 +16,8 @@ libfake_session_la_SOURCES = fake_session.h fake_session.cc
TESTS =
if HAVE_GTEST
-# We build this file with disabled warnings because of ASIO
-lib_LTLIBRARIES += libasiobased.la
-libasiobased_la_SOURCES = ccsession_unittests.cc
-libasiobased_la_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
-# Note: the ordering matters: -Wno-... must follow -Wextra (defined in
-# B10_CXXFLAGS)
-libasiobased_la_CXXFLAGS = $(AM_CXXFLAGS) -Wno-unused-parameter
-libasiobased_la_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS)
-
TESTS += run_unittests
-run_unittests_SOURCES = module_spec_unittests.cc config_data_unittests.cc run_unittests.cc
+run_unittests_SOURCES = ccsession_unittests.cc module_spec_unittests.cc config_data_unittests.cc run_unittests.cc
run_unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
run_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS)
@@ -35,7 +26,6 @@ run_unittests_LDADD += $(top_builddir)/src/lib/cc/libcc.la
run_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la
run_unittests_LDADD += $(top_builddir)/src/lib/log/liblog.la
run_unittests_LDADD += libfake_session.la
-run_unittests_LDADD += libasiobased.la
run_unittests_LDADD += $(top_builddir)/src/lib/config/libcfgclient.la
endif
diff --git a/src/lib/config/tests/ccsession_unittests.cc b/src/lib/config/tests/ccsession_unittests.cc
index 44f98bd..f143e36 100644
--- a/src/lib/config/tests/ccsession_unittests.cc
+++ b/src/lib/config/tests/ccsession_unittests.cc
@@ -12,16 +12,6 @@
// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
// PERFORMANCE OF THIS SOFTWARE.
-#include <asio.hpp>
-#include <boost/bind.hpp>
-
-// These 3 are for delayedStart test. For windows and such, probably disabling
-// that test would be best option, it is not that important and while the test
-// itself uses unix-specific features, it tests OS-agnostic part of code.
-#include <unistd.h> // For fork in one of the tests
-#include <sys/types.h> // And for kill
-#include <signal.h>
-
#include <config.h>
#include <gtest/gtest.h>
@@ -34,8 +24,6 @@
#include <config/tests/data_def_unittests_config.h>
-#include <unistd.h>
-
using namespace isc::data;
using namespace isc::config;
using namespace isc::cc;
@@ -374,7 +362,7 @@ TEST_F(CCSessionTest, remoteConfig) {
std::string module_name;
int item1;
- ModuleCCSession mccs(ccspecfile("spec1.spec"), session, NULL, NULL);
+ ModuleCCSession mccs(ccspecfile("spec1.spec"), session, NULL, NULL, false);
EXPECT_TRUE(session.haveSubscription("Spec1", "*"));
// first simply connect, with no config values, and see we get
@@ -517,7 +505,8 @@ TEST_F(CCSessionTest, ignoreRemoteConfigCommands) {
session.getMessages()->add(createAnswer(0, el("{ }")));
EXPECT_FALSE(session.haveSubscription("Spec29", "*"));
- ModuleCCSession mccs(ccspecfile("spec29.spec"), session, my_config_handler, my_command_handler);
+ ModuleCCSession mccs(ccspecfile("spec29.spec"), session, my_config_handler,
+ my_command_handler, false);
EXPECT_TRUE(session.haveSubscription("Spec29", "*"));
EXPECT_EQ(2, session.getMsgQueue()->size());
@@ -567,97 +556,31 @@ TEST_F(CCSessionTest, initializationFail) {
EXPECT_TRUE(session.haveSubscription("Spec29", "*"));
}
-void
-dumpSocket(unsigned char* data, asio::local::stream_protocol::socket* socket,
- asio::error_code)
-{
- asio::async_read(*socket, asio::buffer(data, 1),
- boost::bind(&dumpSocket, data, socket, _1));
+// Test it throws when we try to start it twice (once from the constructor)
+TEST_F(CCSessionTest, doubleStartImplicit) {
+ ModuleCCSession mccs(ccspecfile("spec29.spec"), session, NULL, NULL);
+ EXPECT_THROW(mccs.start(), CCSessionError);
}
-// Format the wire data for send
-void
-putTestData(size_t& length, unsigned char* data, ConstElementPtr routing,
- ConstElementPtr msg)
-{
- std::string r(routing->str());
- std::string m(msg->str());
- uint32_t l(r.size() + m.size() + 2);
- l = htonl(l);
- uint16_t h(r.size());
- h = htons(h);
- memcpy(data + length, &l, 4);
- length += 4;
- memcpy(data + length, &h, 2);
- length += 2;
- memcpy(data + length, r.c_str(), r.size());
- length += r.size();
- memcpy(data + length, m.c_str(), m.size());
- length += m.size();
+// The same, but both starts are explicit
+TEST_F(CCSessionTest, doubleStartExplicit) {
+ ModuleCCSession mccs(ccspecfile("spec29.spec"), session, NULL, NULL,
+ false);
+ mccs.start();
+ EXPECT_THROW(mccs.start(), CCSessionError);
}
-void
-emptyHandler(asio::error_code, size_t) {}
-
+// Test we can request synchronous receive before we start the session,
+// and check there's the mechanism if we do it after
TEST_F(CCSessionTest, delayedStart) {
- // There's a problem when mixing synchronous and asynchronous
- // reads, because both will read 4 bytes as length and mess it
- // up.
- //
- // This tests our workaround for it, starting the asynchronous
- // calls later.
-
- // FIXME Is this OK in a test? Using asio directly?
- // The cc library seems to do the same, we don't have
- // asiolink wrappers for local sockets
- //
- // We use separate service for each process, just to make sure
- // nothing bad happens
- asio::io_service forkService;
- ::unlink(TEST_SOCKET_PATH);
- asio::local::stream_protocol::endpoint ep(TEST_SOCKET_PATH);
- asio::local::stream_protocol::acceptor acceptor(forkService, ep);
-
- // We fork and we'll do the feeding with data from there.
- pid_t child(fork());
- ASSERT_NE(-1, child);
- if (!child) {
- asio::local::stream_protocol::socket socket(forkService);
- acceptor.accept(socket);
- // We just empty everything from the socket
- unsigned char dump;
- dumpSocket(&dump, &socket, asio::error_code());
- // And schedule writing of the data
- size_t length(0);
- // That is enough for our tests, no need for dynamic allocation
- unsigned char data[4096];
- putTestData(length, data, el("{}"), el("{\"lname\": \"mock\"}"));
- putTestData(length, data, el("{\"reply\": 0}"), createAnswer());
- putTestData(length, data, el("{\"reply\": 3}"), el("{\"a\": 42}"));
- asio::async_write(socket, asio::buffer(data, length), &emptyHandler);
- forkService.run();
- } else {
- try {
- asio::io_service sessionService;
- asio::io_service::work w(sessionService);
- Session session(sessionService);
- // Start the session, but defer the reads
- ModuleCCSession mccs(ccspecfile("spec2.spec"), session,
- NULL, NULL, false, TEST_SOCKET_PATH);
- ConstElementPtr env, answer;
- // Ask for the data there
- // If there are two scheduled reads, this'll probably fail
- session.group_recvmsg(env, answer, false, 3);
- // Check that we received what we shoud have
- EXPECT_TRUE(el("{\"a\": 42}")->equals(*answer));
- kill(child, SIGTERM);
- }
- catch (...) {
- // Just make sure the child is killed no matter what
- kill(child, SIGTERM);
- throw;
- }
- }
+ ModuleCCSession mccs(ccspecfile("spec2.spec"), session, NULL, NULL, false);
+ session.getMessages()->add(createAnswer());
+ ConstElementPtr env, answer;
+ EXPECT_NO_THROW(session.group_recvmsg(env, answer, false, 3));
+ mccs.start();
+ session.getMessages()->add(createAnswer());
+ EXPECT_THROW(session.group_recvmsg(env, answer, false, 3),
+ FakeSession::DoubleRead);
}
}
diff --git a/src/lib/config/tests/data_def_unittests_config.h.in b/src/lib/config/tests/data_def_unittests_config.h.in
index 27abd6b..80e9cfa 100644
--- a/src/lib/config/tests/data_def_unittests_config.h.in
+++ b/src/lib/config/tests/data_def_unittests_config.h.in
@@ -13,4 +13,3 @@
// PERFORMANCE OF THIS SOFTWARE.
#define TEST_DATA_PATH "@abs_srcdir@/testdata"
-#define TEST_SOCKET_PATH "@abs_builddir@/test.sock"
diff --git a/src/lib/config/tests/fake_session.cc b/src/lib/config/tests/fake_session.cc
index 5f79d48..2b216e7 100644
--- a/src/lib/config/tests/fake_session.cc
+++ b/src/lib/config/tests/fake_session.cc
@@ -71,7 +71,8 @@ FakeSession::FakeSession(isc::data::ElementPtr initial_messages,
isc::data::ElementPtr msg_queue) :
messages_(initial_messages),
subscriptions_(subscriptions),
- msg_queue_(msg_queue)
+ msg_queue_(msg_queue),
+ started_(false)
{
}
@@ -84,6 +85,7 @@ FakeSession::disconnect() {
void
FakeSession::startRead(boost::function<void()>) {
+ started_ = true;
}
void
@@ -91,7 +93,13 @@ FakeSession::establish(const char*) {
}
bool
-FakeSession::recvmsg(ConstElementPtr& msg, bool, int) {
+FakeSession::recvmsg(ConstElementPtr& msg, bool nonblock, int) {
+ if (started_ && !nonblock) {
+ // This would schedule another read for length, leading to
+ // corputed data
+ isc_throw(DoubleRead, "Second read scheduled from recvmsg");
+ }
+
//cout << "[XX] client asks for message " << endl;
if (messages_ &&
messages_->getType() == Element::list &&
@@ -105,7 +113,15 @@ FakeSession::recvmsg(ConstElementPtr& msg, bool, int) {
}
bool
-FakeSession::recvmsg(ConstElementPtr& env, ConstElementPtr& msg, bool, int) {
+FakeSession::recvmsg(ConstElementPtr& env, ConstElementPtr& msg, bool nonblock,
+ int)
+{
+ if (started_ && !nonblock) {
+ // This would schedule another read for length, leading to
+ // corputed data
+ isc_throw(DoubleRead, "Second read scheduled from recvmsg");
+ }
+
//cout << "[XX] client asks for message and env" << endl;
env = ElementPtr();
if (messages_ &&
diff --git a/src/lib/config/tests/fake_session.h b/src/lib/config/tests/fake_session.h
index ac8e291..85e47d5 100644
--- a/src/lib/config/tests/fake_session.h
+++ b/src/lib/config/tests/fake_session.h
@@ -42,6 +42,14 @@ public:
isc::data::ElementPtr msg_queue);
virtual ~FakeSession();
+ // This is thrown if two reads for length at once are scheduled at once.
+ // Such thing does bad things currently (see discussion in ticket #931).
+ class DoubleRead : public Exception {
+ public:
+ DoubleRead(const char* file, size_t line, const char* what) :
+ Exception(file, line, what) {}
+ };
+
virtual void startRead(boost::function<void()> read_callback);
virtual void establish(const char* socket_file = NULL);
@@ -89,6 +97,7 @@ private:
const isc::data::ElementPtr messages_;
isc::data::ElementPtr subscriptions_;
isc::data::ElementPtr msg_queue_;
+ bool started_;
};
} // namespace cc
} // namespace isc
diff --git a/src/lib/server_common/tests/keyring_test.cc b/src/lib/server_common/tests/keyring_test.cc
index 6d2f226..4d61950 100644
--- a/src/lib/server_common/tests/keyring_test.cc
+++ b/src/lib/server_common/tests/keyring_test.cc
@@ -38,7 +38,7 @@ public:
specfile(std::string(TEST_DATA_PATH) + "/spec.spec")
{
session.getMessages()->add(createAnswer());
- mccs.reset(new ModuleCCSession(specfile, session, NULL, NULL));
+ mccs.reset(new ModuleCCSession(specfile, session, NULL, NULL, false));
}
isc::cc::FakeSession session;
std::auto_ptr<ModuleCCSession> mccs;
More information about the bind10-changes
mailing list