[2903] further simplify SyncUDPServer: now remove checkin and answer callbacks.

Its constructor and member variables are also simplified accordingly.

src/lib/asiodns/dns_service.cc

@@ -58,9 +58,15 @@ public:
     template<class Ptr, class Server> void addServerFromFD(int fd, int af) {
         Ptr server(new Server(io_service_.get_io_service(), fd, af, checkin_,
                               lookup_, answer_));
-        server->setTCPRecvTimeout(tcp_recv_timeout_);
-        (*server)();
-        servers_.push_back(server);
+        startServer(server);
+    }
+
+    // SyncUDPServer has different constructor signature so it cannot be
+    // templated.
+    void addSyncUDPServerFromFD(int fd, int af) {
+        SyncUDPServerPtr server(new SyncUDPServer(io_service_.get_io_service(),
+                                                  fd, af, lookup_));
+        startServer(server);
     }
 
     void setTCPRecvTimeout(size_t timeout) {
@@ -72,6 +78,13 @@ public:
             (*it)->setTCPRecvTimeout(timeout);
         }
     }
+
+private:
+    void startServer(DNSServerPtr server) {
+        server->setTCPRecvTimeout(tcp_recv_timeout_);
+        (*server)();
+        servers_.push_back(server);
+    }
 };
 
 DNSService::DNSService(IOService& io_service, SimpleCallback* checkin,
@@ -95,8 +108,7 @@ void DNSService::addServerUDPFromFD(int fd, int af, ServerFlag options) {
                   << options);
     }
     if ((options & SERVER_SYNC_OK) != 0) {
-        impl_->addServerFromFD<DNSServiceImpl::SyncUDPServerPtr,
-            SyncUDPServer>(fd, af);
+        impl_->addSyncUDPServerFromFD(fd, af);
     } else {
         impl_->addServerFromFD<DNSServiceImpl::UDPServerPtr, UDPServer>(
             fd, af);

src/lib/asiodns/sync_udp_server.cc

@@ -39,12 +39,11 @@ namespace isc {
 namespace asiodns {
 
 SyncUDPServer::SyncUDPServer(asio::io_service& io_service, const int fd,
-                             const int af, asiolink::SimpleCallback*,
-                             DNSLookup* lookup, DNSAnswer* answer) :
+                             const int af, DNSLookup* lookup) :
     output_buffer_(new isc::util::OutputBuffer(0)),
     query_(new isc::dns::Message(isc::dns::Message::PARSE)),
     answer_(new isc::dns::Message(isc::dns::Message::RENDER)),
-    lookup_callback_(lookup), answer_callback_(answer), stopped_(false)
+    lookup_callback_(lookup), stopped_(false)
 {
     if (af != AF_INET && af != AF_INET6) {
         isc_throw(InvalidParameter, "Address family must be either AF_INET "
@@ -130,8 +129,6 @@ SyncUDPServer::handleRead(const asio::error_code& ec, const size_t length) {
 
     if (done_) {
         // Good, there's an answer.
-        // Call the answer callback to render it.
-        (*answer_callback_)(message, query_, answer_, output_buffer_);
 
         asio::error_code ec;
         socket_->send_to(asio::const_buffers_1(output_buffer_->getData(),

src/lib/asiodns/sync_udp_server.h

@@ -46,30 +46,32 @@ public:
     ///
     /// Due to the nature of this server, it's meaningless if the lookup
     /// callback is NULL.  So the constructor explicitly rejects that case
-    /// with an exception.
+    /// with an exception.  Likewise, it doesn't take "checkin" or "answer"
+    /// callbacks.  In fact, calling "checkin" from receive callback does not
+    /// make sense for any of the DNSServer variants; "answer" callback is
+    /// simply unnecessary for this class because a complete answer is build
+    /// in the lookup callback (it's the user's responsibility to guarantee
+    /// that condition).
     ///
     /// \param io_service the asio::io_service to work with
     /// \param fd the file descriptor of opened UDP socket
     /// \param af address family, either AF_INET or AF_INET6
-    /// \param checkin the callbackprovider for non-DNS events (unused)
     /// \param lookup the callbackprovider for DNS lookup events (must not be
     ///        NULL)
-    /// \param answer the callbackprovider for DNS answer events
     ///
     /// \throw isc::InvalidParameter if af is neither AF_INET nor AF_INET6
     /// \throw isc::InvalidParameter lookup is NULL
     /// \throw isc::asiolink::IOError when a low-level error happens, like the
     ///     fd is not a valid descriptor.
     SyncUDPServer(asio::io_service& io_service, const int fd, const int af,
-                  isc::asiolink::SimpleCallback* checkin = NULL,
-                  DNSLookup* lookup = NULL, DNSAnswer* answer = NULL);
+                  DNSLookup* lookup);
 
     /// \brief Start the SyncUDPServer.
     ///
     /// This is the function operator to keep interface with other server
     /// classes. They need that because they're coroutines.
     virtual void operator()(asio::error_code ec = asio::error_code(),
-                    size_t length = 0);
+                            size_t length = 0);
 
     /// \brief Calls the lookup callback
     virtual void asyncLookup() {
@@ -128,9 +130,8 @@ private:
     std::auto_ptr<asio::ip::udp::socket> socket_;
     // Place the socket puts the sender of a packet when it is received
     asio::ip::udp::endpoint sender_;
-    // Callbacks
+    // Callback
     const DNSLookup* lookup_callback_;
-    const DNSAnswer* answer_callback_;
     // Answers from the lookup callback (not sent directly, but signalled
     // through resume()
     bool resume_called_, done_;

src/lib/asiodns/tests/dns_server_unittest.cc

@@ -117,7 +117,7 @@ public:
     DummyLookup() :
         allow_resume_(true)
     { }
-    void operator()(const IOMessage& io_message,
+    virtual void operator()(const IOMessage& io_message,
             isc::dns::MessagePtr message,
             isc::dns::MessagePtr answer_message,
             isc::util::OutputBufferPtr buffer,
@@ -147,6 +147,24 @@ class SimpleAnswer : public DNSAnswer, public ServerStopper {
 
 };
 
+/// \brief Mixture of DummyLookup and SimpleAnswer: build the answer in the
+/// lookup callback.  Used with SyncUDPServer.
+class SyncDummyLookup : public DummyLookup {
+public:
+    virtual void operator()(const IOMessage& io_message,
+                            isc::dns::MessagePtr message,
+                            isc::dns::MessagePtr answer_message,
+                            isc::util::OutputBufferPtr buffer,
+                            DNSServer* server) const
+    {
+        buffer->writeData(io_message.getData(), io_message.getDataSize());
+        stopServer();
+        if (allow_resume_) {
+            server->resume(true);
+        }
+    }
+};
+
 // \brief simple client, send one string to server and wait for response
 //  in case, server stopped and client can't get response, there is a timer wait
 //  for specified seconds (the value is just a estimate since server process logic is quite
@@ -353,6 +371,7 @@ class DNSServerTestBase : public::testing::Test {
             server_address_(ip::address::from_string(server_ip)),
             checker_(new DummyChecker()),
             lookup_(new DummyLookup()),
+            sync_lookup_(new SyncDummyLookup()),
             answer_(new SimpleAnswer()),
             udp_client_(new UDPClient(service,
                                       ip::udp::endpoint(server_address_,
@@ -375,6 +394,7 @@ class DNSServerTestBase : public::testing::Test {
             }
             delete checker_;
             delete lookup_;
+            delete sync_lookup_;
             delete answer_;
             delete udp_server_;
             delete udp_client_;
@@ -421,7 +441,8 @@ class DNSServerTestBase : public::testing::Test {
         asio::io_service service;
         const ip::address server_address_;
         DummyChecker* const checker_;
-        DummyLookup*  const lookup_;
+        DummyLookup* lookup_;     // we need to replace it in some cases
+        SyncDummyLookup*  const sync_lookup_;
         SimpleAnswer* const answer_;
         UDPClient*    const udp_client_;
         TCPClient*    const tcp_client_;
@@ -482,19 +503,34 @@ private:
 protected:
     // Using SetUp here so we can ASSERT_*
     void SetUp() {
-        const int fdUDP(getFd(SOCK_DGRAM));
-        ASSERT_NE(-1, fdUDP) << strerror(errno);
-        this->udp_server_ = new UDPServerClass(this->service, fdUDP, AF_INET6,
-                                               this->checker_, this->lookup_,
-                                               this->answer_);
-        const int fdTCP(getFd(SOCK_STREAM));
-        ASSERT_NE(-1, fdTCP) << strerror(errno);
-        this->tcp_server_ = new TCPServer(this->service, fdTCP, AF_INET6,
+        const int fd_udp(getFd(SOCK_DGRAM));
+        ASSERT_NE(-1, fd_udp) << strerror(errno);
+        this->udp_server_ = createServer(fd_udp, AF_INET6);
+        const int fd_tcp(getFd(SOCK_STREAM));
+        ASSERT_NE(-1, fd_tcp) << strerror(errno);
+        this->tcp_server_ = new TCPServer(this->service, fd_tcp, AF_INET6,
                                           this->checker_, this->lookup_,
                                           this->answer_);
     }
+
+    // A helper factory of the tested UDP server class: allow customization
+    // by template specialization.
+    UDPServerClass* createServer(int fd, int af) {
+        return (new UDPServerClass(this->service, fd, af,
+                                   this->checker_, this->lookup_,
+                                   this->answer_));
+    }
 };
 
+// Specialization for SyncUDPServer.  It needs to use SyncDummyLookup.
+template<>
+SyncUDPServer*
+FdInit<SyncUDPServer>::createServer(int fd, int af) {
+    delete this->lookup_;
+    this->lookup_ = new SyncDummyLookup;
+    return (new SyncUDPServer(this->service, fd, af, this->lookup_));
+}
+
 // This makes it the template as gtest wants it.
 template<class Parent>
 class DNSServerTest : public Parent { };
@@ -518,7 +554,7 @@ asio::io_service* DNSServerTestBase<UDPServerClass>::current_service(NULL);
 
 // Test whether server stopped successfully after client get response
 // client will send query and start to wait for response, once client
-// get response, udp server will be stopped, the io service won't quit
+// get response, UDP server will be stopped, the io service won't quit
 // if udp server doesn't stop successfully.
 TYPED_TEST(DNSServerTest, stopUDPServerAfterOneQuery) {
     this->testStopServerByStopper(this->udp_server_, this->udp_client_,
@@ -555,12 +591,13 @@ TYPED_TEST(DNSServerTest, stopUDPServerDuringQueryLookup) {
     EXPECT_TRUE(this->serverStopSucceed());
 }
 
-// Test whether udp server stopped successfully during composing answer
-TYPED_TEST(DNSServerTest, stopUDPServerDuringPrepareAnswer) {
-    this->testStopServerByStopper(this->udp_server_, this->udp_client_,
-                                  this->answer_);
-    EXPECT_EQ(std::string(""), this->udp_client_->getReceivedData());
-    EXPECT_TRUE(this->serverStopSucceed());
+// Test whether UDP server stopped successfully during composing answer.
+// Only works for (non-sync) server because SyncUDPServer doesn't use answer
+// callback.
+TEST_F(AsyncServerTest, stopUDPServerDuringPrepareAnswer) {
+    testStopServerByStopper(udp_server_, udp_client_, answer_);
+    EXPECT_EQ(std::string(""), udp_client_->getReceivedData());
+    EXPECT_TRUE(serverStopSucceed());
 }
 
 void
@@ -572,7 +609,7 @@ stopServerManyTimes(DNSServer *server, unsigned int times) {
 
 // Test whether udp server stop interface can be invoked several times without
 // throw any exception
-TYPED_TEST(DNSServerTest, stopUDPServeMoreThanOnce) {
+TYPED_TEST(DNSServerTest, stopUDPServerMoreThanOnce) {
     ASSERT_NO_THROW({
         boost::function<void()> stop_server_3_times
             = boost::bind(stopServerManyTimes, this->udp_server_, 3);
@@ -675,14 +712,15 @@ TYPED_TEST(DNSServerTest, stopTCPServeMoreThanOnce) {
 TYPED_TEST(DNSServerTestBase, invalidFamily) {
     // We abuse DNSServerTestBase for this test, as we don't need the
     // initialization.
-    EXPECT_THROW(TypeParam(this->service, 0, AF_UNIX, this->checker_,
-                           this->lookup_, this->answer_),
-                 isc::InvalidParameter);
     EXPECT_THROW(TCPServer(this->service, 0, AF_UNIX, this->checker_,
                            this->lookup_, this->answer_),
                  isc::InvalidParameter);
 }
 
+TYPED_TEST(DNSServerTest, invalidFamilyUDP) {
+    EXPECT_THROW(this->createServer(0, AF_UNIX), isc::InvalidParameter);
+}
+
 // It raises an exception when invalid address family is passed
 TYPED_TEST(DNSServerTestBase, invalidTCPFD) {
     // We abuse DNSServerTestBase for this test, as we don't need the
@@ -701,7 +739,7 @@ TYPED_TEST(DNSServerTestBase, invalidTCPFD) {
                  isc::asiolink::IOError);
 }
 
-TYPED_TEST(DNSServerTestBase, DISABLED_invalidUDPFD) {
+TYPED_TEST(DNSServerTest, DISABLED_invalidUDPFD) {
     /*
      FIXME: The UDP server doesn't fail reliably with an invalid FD.
      We need to find a way to trigger it reliably (it seems epoll
@@ -709,9 +747,7 @@ TYPED_TEST(DNSServerTestBase, DISABLED_invalidUDPFD) {
      not the others, maybe we could make it run this at least on epoll-based
      systems).
     */
-    EXPECT_THROW(TypeParam(this->service, -1, AF_INET, this->checker_,
-                           this->lookup_, this->answer_),
-                 isc::asiolink::IOError);
+    EXPECT_THROW(this->createServer(-1, AF_INET), isc::asiolink::IOError);
 }
 
 // Check it rejects some of the unsupported operations
@@ -729,7 +765,7 @@ TEST_F(SyncServerTest, mustResume) {
 
 // SyncUDPServer doesn't allow NULL lookup callback.
 TEST_F(SyncServerTest, nullLookupCallback) {
-    EXPECT_THROW(SyncUDPServer(service, 0, AF_INET, checker_, NULL, answer_),
+    EXPECT_THROW(SyncUDPServer(service, 0, AF_INET, NULL),
                  isc::InvalidParameter);
 }