Commit f0f818b2 authored by Evan Hunt's avatar Evan Hunt

Addressed some review comments, including:

 - added diagrams to asiolink documentation
 - eliminated improper error return in TCPServer operator()
 - moved UDPEndpoint, TCPEndpoint, UDPSOcket, TCPSOcket implementation
   code into internal/udpdns.h and internal/tcpdns.h
 - RecursiveQuery ns_addr_ member now an IOAddress rather than asio address
 - add method headers in recursor.h and auth_srv.h
 - change asio_link unittest name to asiolink


git-svn-id: svn://bind10.isc.org/svn/bind10/branches/trac327@3177 e5f2f494-b856-4b98-b285-d166d9295462
parent ad55ea2d
......@@ -174,8 +174,8 @@ class ConfigChecker : public SimpleCallback {
public:
ConfigChecker(AuthSrv* srv) : server_(srv) {}
virtual void operator()(const IOMessage& io_message UNUSED_PARAM) const {
if (server_->configSession()->hasQueuedMsgs()) {
server_->configSession()->checkCommand();
if (server_->getConfigSession()->hasQueuedMsgs()) {
server_->getConfigSession()->checkCommand();
}
}
private:
......@@ -270,7 +270,7 @@ AuthSrv::setConfigSession(ModuleCCSession* config_session) {
}
ModuleCCSession*
AuthSrv::configSession() const {
AuthSrv::getConfigSession() const {
return (impl_->config_session_);
}
......
......@@ -30,10 +30,6 @@ class AbstractXfroutClient;
}
}
namespace asiolink {
class IOMessage;
}
class AuthSrvImpl;
class AuthSrv {
......@@ -58,30 +54,37 @@ public:
isc::xfr::AbstractXfroutClient& xfrout_client);
~AuthSrv();
//@}
/// \return \c true if the \message contains a response to be returned;
/// otherwise \c false.
/// \brief Process an incoming DNS message, then signal 'server' to resume
void processMessage(const asiolink::IOMessage& io_message,
isc::dns::MessagePtr message,
isc::dns::OutputBufferPtr buffer,
asiolink::DNSServer* server);
// \brief Set and get verbose mode
void setVerbose(bool on);
bool getVerbose() const;
isc::data::ConstElementPtr updateConfig(isc::data::ConstElementPtr config);
isc::config::ModuleCCSession* configSession() const;
/// \brief Set and get the config session
void setConfigSession(isc::config::ModuleCCSession* config_session);
isc::config::ModuleCCSession* getConfigSession() const;
/// \brief Handle commands from the config session
isc::data::ConstElementPtr updateConfig(isc::data::ConstElementPtr config);
/// \brief Assign an ASIO IO Service queue to this Recursor object
void setIOService(asiolink::IOService& ios) { io_service_ = &ios; }
/// \brief Return this object's ASIO IO Service queue
asiolink::IOService& getIOService() const { return (*io_service_); }
asiolink::DNSLookup* getDNSLookupProvider() const {
return (dns_lookup_);
}
asiolink::DNSAnswer* getDNSAnswerProvider() const {
return (dns_answer_);
}
asiolink::SimpleCallback* getCheckinProvider() const {
return (checkin_);
}
/// \brief Return pointer to the DNS Lookup callback function
asiolink::DNSLookup* getDNSLookupProvider() const { return (dns_lookup_); }
/// \brief Return pointer to the DNS Answer callback function
asiolink::DNSAnswer* getDNSAnswerProvider() const { return (dns_answer_); }
/// \brief Return pointer to the Checkin callback function
asiolink::SimpleCallback* getCheckinProvider() const { return (checkin_); }
///
/// Note: this interface is tentative. We'll revisit the ASIO and session
......
......@@ -55,17 +55,17 @@ using namespace asiolink;
namespace {
bool verbose_mode = false;
static bool verbose_mode = false;
const string PROGRAM = "Auth";
const char* DNSPORT = "5300";
static const string PROGRAM = "Auth";
static const char* DNSPORT = "5300";
/* need global var for config/command handlers.
* todo: turn this around, and put handlers in the authserver
* class itself? */
AuthSrv *auth_server;
static AuthSrv *auth_server;
IOService* io_service;
static IOService* io_service;
ConstElementPtr
my_config_handler(ConstElementPtr new_config) {
......@@ -89,7 +89,14 @@ my_command_handler(const string& command, ConstElementPtr args) {
void
usage() {
cerr << "Usage: b10-auth [-a address] [-p port] [-4|-6] [-nv]" << endl;
cerr << "Usage: b10-auth [-a address] [-p port] [-4|-6] [-nv]" << endl;
cerr << "\t-a: specify the address to listen on (default: all) " << endl;
cerr << "\t-p: specify the port to listen on (default: 5300)" << endl;
cerr << "\t-4: listen on all IPv4 addresses (incompatible with -a)" << endl;
cerr << "\t-4: listen on all IPv6 addresses (incompatible with -a)" << endl;
cerr << "\t-n: do not cache answers in memory" << endl;
cerr << "\t-u: change process UID to the specified user" << endl;
cerr << "\t-v: verbose output" << endl;
exit(1);
}
} // end of anonymous namespace
......@@ -141,12 +148,14 @@ main(int argc, char* argv[]) {
}
if (!use_ipv4 && !use_ipv6) {
cerr << "[b10-auth] Error: -4 and -6 can't coexist" << endl;
cerr << "[b10-auth] Error: Cannot specify both -4 and -6 "
<< "at the same time" << endl;
usage();
}
if ((!use_ipv4 || !use_ipv6) && address != NULL) {
cerr << "[b10-auth] Error: -4|-6 and -a can't coexist" << endl;
cerr << "[b10-auth] Error: Cannot specify -4 or -6 "
<< "at the same time as -a" << endl;
usage();
}
......
......@@ -56,13 +56,13 @@ using namespace asiolink;
namespace {
bool verbose_mode = false;
static bool verbose_mode = false;
const string PROGRAM = "Recurse";
const char* DNSPORT = "5300";
static const string PROGRAM = "Recurse";
static const char* DNSPORT = "5300";
IOService* io_service;
Recursor *recursor;
static IOService* io_service;
static Recursor *recursor;
ConstElementPtr
my_config_handler(ConstElementPtr new_config) {
......@@ -86,8 +86,16 @@ my_command_handler(const string& command, ConstElementPtr args) {
void
usage() {
cerr << "Usage: b10-recurse -f nameserver [-a address] [-p port] "
"[-4|-6] [-nv]" << endl;
cerr << "Usage: b10-recurse -f nameserver [-a address] [-p port] "
"[-4|-6] [-v]" << endl;
cerr << "\t-f: specify the nameserver to which queries should be forwarded"
<< endl;
cerr << "\t-a: specify the address to listen on (default: all)" << endl;
cerr << "\t-p: specify the port to listen on (default: 5300)" << endl;
cerr << "\t-4: listen on all IPv4 addresses (incompatible with -a)" << endl;
cerr << "\t-4: listen on all IPv6 addresses (incompatible with -a)" << endl;
cerr << "\t-u: change process UID to the specified user" << endl;
cerr << "\t-v: verbose output" << endl;
exit(1);
}
} // end of anonymous namespace
......@@ -99,9 +107,9 @@ main(int argc, char* argv[]) {
const char* address = NULL;
const char* forward = NULL;
const char* uid = NULL;
bool use_ipv4 = true, use_ipv6 = true, cache = true;
bool use_ipv4 = true, use_ipv6 = true;
while ((ch = getopt(argc, argv, "46a:f:np:u:v")) != -1) {
while ((ch = getopt(argc, argv, "46a:f:p:u:v")) != -1) {
switch (ch) {
case '4':
// Note that -4 means "ipv4 only", we need to set "use_ipv6" here,
......@@ -114,9 +122,6 @@ main(int argc, char* argv[]) {
// The same note as -4 applies.
use_ipv4 = false;
break;
case 'n':
cache = false;
break;
case 'a':
address = optarg;
break;
......@@ -143,12 +148,14 @@ main(int argc, char* argv[]) {
}
if (!use_ipv4 && !use_ipv6) {
cerr << "[b10-recurse] Error: -4 and -6 can't coexist" << endl;
cerr << "[b10-auth] Error: Cannot specify both -4 and -6 "
<< "at the same time" << endl;
usage();
}
if ((!use_ipv4 || !use_ipv6) && address != NULL) {
cerr << "[b10-recurse] Error: -4|-6 and -a can't coexist" << endl;
cerr << "[b10-auth] Error: Cannot specify -4 or -6 "
<< "at the same time as -a" << endl;
usage();
}
......@@ -172,7 +179,7 @@ main(int argc, char* argv[]) {
}
recursor = new Recursor(*forward);
recursor ->setVerbose(verbose_mode);
recursor->setVerbose(verbose_mode);
cout << "[b10-recurse] Server created." << endl;
SimpleCallback* checkin = recursor->getCheckinProvider();
......
......@@ -85,6 +85,7 @@ public:
void processNormalQuery(const Question& question, MessagePtr message,
OutputBufferPtr buffer,
DNSServer* server);
ModuleCCSession* config_session_;
bool verbose_mode_;
......@@ -271,8 +272,8 @@ class ConfigCheck : public SimpleCallback {
public:
ConfigCheck(Recursor* srv) : server_(srv) {}
virtual void operator()(const IOMessage& io_message UNUSED_PARAM) const {
if (server_->configSession()->hasQueuedMsgs()) {
server_->configSession()->checkCommand();
if (server_->getConfigSession()->hasQueuedMsgs()) {
server_->getConfigSession()->checkCommand();
}
}
private:
......@@ -316,7 +317,7 @@ Recursor::setConfigSession(ModuleCCSession* config_session) {
}
ModuleCCSession*
Recursor::configSession() const {
Recursor::getConfigSession() const {
return (impl_->config_session_);
}
......
......@@ -24,16 +24,6 @@
#include <asiolink/asiolink.h>
namespace isc {
namespace dns {
class InputBuffer;
}
}
namespace asiolink {
class IOMessage;
}
class RecursorImpl;
class Recursor {
......@@ -56,30 +46,38 @@ public:
Recursor(const char& forward);
~Recursor();
//@}
/// \return \c true if the \message contains a response to be returned;
/// otherwise \c false.
/// \brief Process an incoming DNS message, then signal 'server' to resume
void processMessage(const asiolink::IOMessage& io_message,
isc::dns::MessagePtr message,
isc::dns::OutputBufferPtr buffer,
asiolink::DNSServer* server);
// \brief Set and get verbose mode
void setVerbose(bool on);
bool getVerbose() const;
isc::data::ConstElementPtr updateConfig(isc::data::ConstElementPtr config);
isc::config::ModuleCCSession* configSession() const;
/// \brief Set and get the config session
isc::config::ModuleCCSession* getConfigSession() const;
void setConfigSession(isc::config::ModuleCCSession* config_session);
/// \brief Handle commands from the config session
isc::data::ConstElementPtr updateConfig(isc::data::ConstElementPtr config);
/// \brief Assign an ASIO IO Service queue to this Recursor object
void setIOService(asiolink::IOService& ios);
/// \brief Return this object's ASIO IO Service queue
asiolink::IOService& getIOService() const { return (*io_); }
asiolink::DNSLookup* getDNSLookupProvider() {
return (dns_lookup_);
}
asiolink::DNSAnswer* getDNSAnswerProvider() {
return (dns_answer_);
}
asiolink::SimpleCallback* getCheckinProvider() {
return (checkin_);
}
/// \brief Return pointer to the DNS Lookup callback function
asiolink::DNSLookup* getDNSLookupProvider() { return (dns_lookup_); }
/// \brief Return pointer to the DNS Answer callback function
asiolink::DNSAnswer* getDNSAnswerProvider() { return (dns_answer_); }
/// \brief Return pointer to the Checkin callback function
asiolink::SimpleCallback* getCheckinProvider() { return (checkin_); }
private:
RecursorImpl* impl_;
......
......@@ -457,19 +457,4 @@ TEST_F(RecursorTest, notifyFail) {
Opcode::NOTIFY().getCode(), QR_FLAG, 0, 0, 0, 0);
}
void
updateConfig(Recursor* server, const char* const dbfile,
const bool expect_success)
{
ConstElementPtr config_answer =
server->updateConfig(Element::fromJSON(dbfile));
EXPECT_EQ(Element::map, config_answer->getType());
EXPECT_TRUE(config_answer->contains("result"));
ConstElementPtr result = config_answer->get("result");
EXPECT_EQ(Element::list, result->getType());
EXPECT_EQ(expect_success ? 0 : 1, result->get(0)->intValue());
}
}
......@@ -67,14 +67,17 @@ used by a DNS Server to communicate with the module that called it.
They are abstract-only classes whose concrete implementations
are supplied by the calling module.
Note that the DNSLookup callback runs asynchronously. A concrete
implementation must be sure to call the server's "resume" method when
The DNSLookup callback always runs asynchronously. Concrete
implementations must be sure to call the server's "resume" method when
it is finished.
In an authoritative server, the DNSLookup implementation would examine
the query, look up the answer, then call "resume". In a recursive server,
it would initiate a DNSQuery, which in turn would be responsible for
calling the server's "resume" method.
the query, look up the answer, then call "resume". (See the diagram
in doc/auth_process.jpg.)
In a recursive server, the DNSLookup impelemtation would initiate a
DNSQuery, which in turn would be responsible for calling the server's
"resume" method. (See the diagram in doc/recursive_process.jpg.)
A DNSQuery object is intended to handle resolution of a query over
the network when the local authoritative data sources or cache are not
......
......@@ -172,15 +172,8 @@ IOService::get_io_service() {
RecursiveQuery::RecursiveQuery(IOService& io_service, const char& forward,
uint16_t port) :
io_service_(io_service), port_(port)
{
error_code err;
ns_addr_ = ip::address::from_string(&forward, err);
if (err) {
isc_throw(IOError, "Invalid IP address '" << &ns_addr_ << "': "
<< err.message());
}
}
io_service_(io_service), ns_addr_(&forward), port_(port)
{}
void
RecursiveQuery::sendQuery(const Question& question, OutputBufferPtr buffer,
......
......@@ -222,8 +222,10 @@ public:
///
/// These methods all make their calls indirectly via the "self_"
/// pointer, ensuring that the functions ultimately invoked will be
/// the ones in the derived class.
///
/// the ones in the derived class. This makes it possible to pass
/// instances of derived classes as references to this base class
/// without losing access to derived class data.
///
//@{
/// \brief The funtion operator
virtual void operator()(asio::error_code ec = asio::error_code(),
......@@ -464,7 +466,7 @@ public:
DNSServer* server);
private:
IOService& io_service_;
asio::ip::address ns_addr_;
IOAddress ns_addr_;
uint16_t port_;
};
......
......@@ -54,10 +54,29 @@ public:
{}
~TCPEndpoint() { delete asio_endpoint_placeholder_; }
virtual IOAddress getAddress() const;
virtual uint16_t getPort() const;
virtual short getProtocol() const;
virtual short getFamily() const;
inline IOAddress getAddress() const {
return (asio_endpoint_.address());
}
inline uint16_t getPort() const {
return (asio_endpoint_.port());
}
inline short getProtocol() const {
return (asio_endpoint_.protocol().protocol());
}
inline short getFamily() const {
return (asio_endpoint_.protocol().family());
}
// This is not part of the exosed IOEndpoint API but allows
// direct access to the ASIO implementation of the endpoint
inline const asio::ip::tcp::endpoint& getASIOEndpoint() const {
return (asio_endpoint_);
}
private:
const asio::ip::tcp::endpoint* asio_endpoint_placeholder_;
const asio::ip::tcp::endpoint& asio_endpoint_;
......@@ -70,8 +89,10 @@ private:
TCPSocket& operator=(const TCPSocket& source);
public:
TCPSocket(asio::ip::tcp::socket& socket) : socket_(socket) {}
virtual int getNative() const;
virtual int getProtocol() const;
inline int getNative() const { return (socket_.native()); }
inline int getProtocol() const { return (IPPROTO_TCP); }
private:
asio::ip::tcp::socket& socket_;
};
......
......@@ -52,10 +52,29 @@ public:
{}
~UDPEndpoint() { delete asio_endpoint_placeholder_; }
virtual IOAddress getAddress() const;
virtual uint16_t getPort() const;
virtual short getProtocol() const;
virtual short getFamily() const;
inline IOAddress getAddress() const {
return (asio_endpoint_.address());
}
inline uint16_t getPort() const {
return (asio_endpoint_.port());
}
inline short getProtocol() const {
return (asio_endpoint_.protocol().protocol());
}
inline short getFamily() const {
return (asio_endpoint_.protocol().family());
}
// This is not part of the exosed IOEndpoint API but allows
// direct access to the ASIO implementation of the endpoint
inline const asio::ip::udp::endpoint& getASIOEndpoint() const {
return (asio_endpoint_);
}
private:
const asio::ip::udp::endpoint* asio_endpoint_placeholder_;
const asio::ip::udp::endpoint& asio_endpoint_;
......@@ -67,8 +86,10 @@ private:
UDPSocket& operator=(const UDPSocket& source);
public:
UDPSocket(asio::ip::udp::socket& socket) : socket_(socket) {}
virtual int getNative() const;
virtual int getProtocol() const;
virtual int getNative() const { return (socket_.native()); }
virtual int getProtocol() const { return (IPPROTO_UDP); }
private:
asio::ip::udp::socket& socket_;
};
......@@ -161,8 +182,7 @@ class UDPQuery : public coroutine {
public:
explicit UDPQuery(asio::io_service& io_service,
const isc::dns::Question& q,
const asio::ip::address& addr,
uint16_t port,
const IOAddress& addr, uint16_t port,
isc::dns::OutputBufferPtr buffer,
DNSServer* server);
void operator()(asio::error_code ec = asio::error_code(),
......
......@@ -41,46 +41,6 @@ using namespace std;
using namespace isc::dns;
namespace asiolink {
/// The following functions provide TCP-specific concrete implementations
/// of the \c IOEndpoint and \c IOSocket classes.
///
/// \brief Returns the address of an endpoint
IOAddress
TCPEndpoint::getAddress() const {
return (asio_endpoint_.address());
}
/// \brief Returns the port of an endpoint
uint16_t
TCPEndpoint::getPort() const {
return (asio_endpoint_.port());
}
/// \brief Returns the protocol number of an endpoint.
short
TCPEndpoint::getProtocol() const {
return (asio_endpoint_.protocol().protocol());
}
/// \brief Returns the address family of an endpoint.
short
TCPEndpoint::getFamily() const {
return (asio_endpoint_.protocol().family());
}
/// \brief Returns the native socket descriptor for a socket.
int
TCPSocket::getNative() const {
return (socket_.native());
}
/// \brief Returns the protocol number for a socket (since this
/// is the TCP-specific implementation, that is always IPPROTO_TCP).
int
TCPSocket::getProtocol() const {
return (IPPROTO_TCP);
}
/// The following functions implement the \c UDPServer class.
///
/// The constructor
......@@ -108,11 +68,6 @@ TCPServer::TCPServer(io_service& io_service,
void
TCPServer::operator()(error_code ec, size_t length) {
/// In the event of an error, just exit. The coroutine will be deleted.
if (ec) {
return;
}
/// Because the coroutine reeentry block is implemented as
/// a switch statement, inline variable declarations are not
/// permitted. Certain variables used below can be declared here.
......@@ -126,8 +81,11 @@ TCPServer::operator()(error_code ec, size_t length) {
/// Create a socket to listen for connections
socket_.reset(new tcp::socket(acceptor_->get_io_service()));
/// Wait for new connections
CORO_YIELD acceptor_->async_accept(*socket_, *this);
/// Wait for new connections. In the event of error,
/// try again
do {
CORO_YIELD acceptor_->async_accept(*socket_, *this);
} while (!ec);
/// Fork the coroutine by creating a copy of this one and
/// scheduling it on the ASIO service queue. The parent
......@@ -140,9 +98,12 @@ TCPServer::operator()(error_code ec, size_t length) {
/// asynchronous read call.
data_ = boost::shared_ptr<char>(new char[MAX_LENGTH]);
/// First, read the message length.
/// Read the message, in two parts. First, the message length:
CORO_YIELD async_read(*socket_, asio::buffer(data_.get(),
TCP_MESSAGE_LENGTHSIZE), *this);
if (ec) {
CORO_YIELD return;
}
/// Now read the message itself. (This is done in a different scope
/// to allow inline variable declarations.)
......@@ -152,6 +113,10 @@ TCPServer::operator()(error_code ec, size_t length) {
async_read(*socket_, asio::buffer(data_.get(), msglen), *this);
}
if (ec) {
CORO_YIELD return;
}
// Create an \c IOMessage object to store the query.
//
// (XXX: It would be good to write a factory function
......
......@@ -16,7 +16,7 @@ if HAVE_GTEST
TESTS += run_unittests
run_unittests_SOURCES = $(top_srcdir)/src/lib/dns/tests/unittest_util.h
run_unittests_SOURCES += $(top_srcdir)/src/lib/dns/tests/unittest_util.cc
run_unittests_SOURCES += asio_link_unittest.cc
run_unittests_SOURCES += asiolink_unittest.cc
run_unittests_SOURCES += run_unittests.cc
run_unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
run_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS)
......