Browse Source

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.
Michal 'vorner' Vaner 14 years ago
parent
commit
5d33e8c43d

+ 2 - 2
src/lib/config/ccsession.cc

@@ -193,7 +193,7 @@ ModuleCCSession::ModuleCCSession(
         isc::data::ConstElementPtr new_config),
         isc::data::ConstElementPtr new_config),
     isc::data::ConstElementPtr(*command_handler)(
     isc::data::ConstElementPtr(*command_handler)(
         const std::string& command, isc::data::ConstElementPtr args),
         const std::string& command, isc::data::ConstElementPtr args),
-    bool start_immediately, const char* socket_path
+    bool start_immediately
     ) :
     ) :
     started_(false),
     started_(false),
     session_(session)
     session_(session)
@@ -205,7 +205,7 @@ ModuleCCSession::ModuleCCSession(
     config_handler_ = config_handler;
     config_handler_ = config_handler;
     command_handler_ = command_handler;
     command_handler_ = command_handler;
 
 
-    session_.establish(socket_path);
+    session_.establish(NULL);
     session_.subscribe(module_name_, "*");
     session_.subscribe(module_name_, "*");
     //session_.subscribe("Boss", "*");
     //session_.subscribe("Boss", "*");
     //session_.subscribe("statistics", "*");
     //session_.subscribe("statistics", "*");

+ 6 - 9
src/lib/config/ccsession.h

@@ -171,13 +171,11 @@ public:
      * @param command_handler A callback function pointer to be called when
      * @param command_handler A callback function pointer to be called when
      * a control command from a remote agent needs to be performed on the
      * a control command from a remote agent needs to be performed on the
      * local module.
      * local module.
-     * @param start_immediately If true (default), start listening to new
+     * @start_immediately If true (default), start listening to new commands
-     * commands and configuration changes asynchronously at the end of the
+     * and configuration changes asynchronously at the end of the constructor;
-     * constructor; if false, it will be delayed until the start() method is
+     * if false, it will be delayed until the start() method is explicitly
-     * explicitly called. (This is a short term workaround for an
+     * called. (This is a short term workaround for an initialization trouble.
-     * initialization trouble. We'll need to develop a cleaner solution, and
+     * We'll need to develop a cleaner solution, and then remove this knob)
-     * then remove this knob)
-     * @param socket_path The path to the socket. For testing purposes.
      */
      */
     ModuleCCSession(const std::string& spec_file_name,
     ModuleCCSession(const std::string& spec_file_name,
                     isc::cc::AbstractSession& session,
                     isc::cc::AbstractSession& session,
@@ -186,8 +184,7 @@ public:
                     isc::data::ConstElementPtr(*command_handler)(
                     isc::data::ConstElementPtr(*command_handler)(
                         const std::string& command,
                         const std::string& command,
                         isc::data::ConstElementPtr args) = NULL,
                         isc::data::ConstElementPtr args) = NULL,
-                    bool start_immediately = true,
+                    bool start_immediately = true
-                    const char* socket_path = NULL
                     );
                     );
 
 
     /// Start receiving new commands and configuration changes asynchronously.
     /// Start receiving new commands and configuration changes asynchronously.

+ 1 - 11
src/lib/config/tests/Makefile.am

@@ -16,17 +16,8 @@ libfake_session_la_SOURCES = fake_session.h fake_session.cc
 
 
 TESTS =
 TESTS =
 if HAVE_GTEST
 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
 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_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
 run_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS)
 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/exceptions/libexceptions.la
 run_unittests_LDADD += $(top_builddir)/src/lib/log/liblog.la
 run_unittests_LDADD += $(top_builddir)/src/lib/log/liblog.la
 run_unittests_LDADD += libfake_session.la
 run_unittests_LDADD += libfake_session.la
-run_unittests_LDADD += libasiobased.la
 run_unittests_LDADD += $(top_builddir)/src/lib/config/libcfgclient.la
 run_unittests_LDADD += $(top_builddir)/src/lib/config/libcfgclient.la
 
 
 endif
 endif

+ 0 - 105
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
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 // 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 <config.h>
 
 
 #include <gtest/gtest.h>
 #include <gtest/gtest.h>
@@ -34,8 +24,6 @@
 
 
 #include <config/tests/data_def_unittests_config.h>
 #include <config/tests/data_def_unittests_config.h>
 
 
-#include <unistd.h>
-
 using namespace isc::data;
 using namespace isc::data;
 using namespace isc::config;
 using namespace isc::config;
 using namespace isc::cc;
 using namespace isc::cc;
@@ -567,97 +555,4 @@ TEST_F(CCSessionTest, initializationFail) {
     EXPECT_TRUE(session.haveSubscription("Spec29", "*"));
     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));
-}
-
-// 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();
-}
-
-void
-emptyHandler(asio::error_code, size_t) {}
-
-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;
-        }
-    }
-}
-
 }
 }

+ 0 - 1
src/lib/config/tests/data_def_unittests_config.h.in

@@ -13,4 +13,3 @@
 // PERFORMANCE OF THIS SOFTWARE.
 // PERFORMANCE OF THIS SOFTWARE.
 
 
 #define TEST_DATA_PATH "@abs_srcdir@/testdata"
 #define TEST_DATA_PATH "@abs_srcdir@/testdata"
-#define TEST_SOCKET_PATH "@abs_builddir@/test.sock"