Skip to content

GitLab

Commit ac15a86e authored by Jelte Jansen's avatar Jelte Jansen
Browse files

[1062] rest of the review comments

parent b19a36e3
Hide whitespace changes
Inline Side-by-side
src/lib/datasrc/database.cc
View file @ ac15a86e
......@@ -93,11 +93,15 @@ void addOrCreate(isc::dns::RRsetPtr& rrset,
if (!rrset) {
rrset.reset(new isc::dns::RRset(name, cls, type, ttl));
} else {
if (ttl < rrset->getTTL()) {
rrset->setTTL(ttl);
}
// This is a check to make sure find() is not messing things up
assert(type == rrset->getType());
if (ttl != rrset->getTTL()) {
if (ttl < rrset->getTTL()) {
rrset->setTTL(ttl);
}
logger.info(DATASRC_DATABASE_FIND_TTL_MISMATCH)
.arg(name).arg(cls).arg(type).arg(rrset->getTTL());
}
}
try {
rrset->addRdata(isc::dns::rdata::createRdata(type, cls, rdata_str));
......@@ -170,9 +174,9 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
try {
connection_->searchForRecords(zone_id_, name.toText());
std::string columns[DatabaseConnection::RecordColumnCount];
std::string columns[DatabaseConnection::RECORDCOLUMNCOUNT];
while (connection_->getNextRecord(columns,
DatabaseConnection::RecordColumnCount)) {
DatabaseConnection::RECORDCOLUMNCOUNT)) {
if (!records_found) {
records_found = true;
}
......
src/lib/datasrc/database.h
View file @ ac15a86e
......@@ -93,7 +93,7 @@ public:
* In the case of a database error, a DatasourceError is thrown.
*
* The columns passed is an array of std::strings consisting of
* DatabaseConnection::RecordColumnCount elements, the elements of which
* DatabaseConnection::RECORDCOLUMNCOUNT elements, the elements of which
* are defined in DatabaseConnection::RecordColumns, in their basic
* string representation.
*
......@@ -146,7 +146,7 @@ public:
};
/// The number of fields the columns array passed to getNextRecord should have
static const size_t RecordColumnCount = 4;
static const size_t RECORDCOLUMNCOUNT = 4;
/**
* \brief Returns a string identifying this dabase backend
......
src/lib/datasrc/datasrc_messages.mes
View file @ ac15a86e
......@@ -73,6 +73,11 @@ The error message contains specific information about the error.
Debug information. The database data source is looking up records with the given
name and type in the database.
% DATASRC_DATABASE_FIND_TTL_MISMATCH TTL values differ for elements of %1/%2/%3, setting to %4
The datasource backend provided resource records for the given RRset with
different TTL values. The TTL of the RRSET is set to the lowest value, which
is printed in the log message.
% DATASRC_DATABASE_FIND_UNCAUGHT_ERROR uncaught general error retrieving data from datasource %1: %2
There was an uncaught general exception while reading data from a datasource.
This most likely points to a logic error in the code, and can be considered a
......
src/lib/datasrc/sqlite3_connection.cc
View file @ ac15a86e
......@@ -325,21 +325,17 @@ SQLite3Connection::getZone(const isc::dns::Name& name) const {
void
SQLite3Connection::searchForRecords(int zone_id, const std::string& name) {
resetSearch();
int result;
result = sqlite3_bind_int(dbparameters_->q_any_, 1, zone_id);
if (result != SQLITE_OK) {
if (sqlite3_bind_int(dbparameters_->q_any_, 1, zone_id) != SQLITE_OK) {
isc_throw(DataSourceError,
"Error in sqlite3_bind_int() for zone_id " <<
zone_id << ", sqlite3 result code: " << result);
zone_id << ": " << sqlite3_errmsg(dbparameters_->db_));
}
// use transient since name is a ref and may disappear
result = sqlite3_bind_text(dbparameters_->q_any_, 2, name.c_str(), -1,
SQLITE_TRANSIENT);
if (result != SQLITE_OK) {
if (sqlite3_bind_text(dbparameters_->q_any_, 2, name.c_str(), -1,
SQLITE_TRANSIENT) != SQLITE_OK) {
isc_throw(DataSourceError,
"Error in sqlite3_bind_text() for name " <<
name << ", sqlite3 result code: " << result);
name << ": " << sqlite3_errmsg(dbparameters_->db_));
}
}
......@@ -349,10 +345,23 @@ namespace {
// might not be directly convertable
// In case sqlite3_column_text() returns NULL, we just make it an
// empty string.
// The sqlite3parameters value is only used to check the error code if
// ucp == NULL
const char*
convertToPlainChar(const unsigned char* ucp) {
convertToPlainChar(const unsigned char* ucp,
SQLite3Parameters* dbparameters) {
if (ucp == NULL) {
return ("");
// The field can really be NULL, in which case we return an
// empty string, or sqlite may have run out of memory, in
// which case we raise an error
if (dbparameters &&
sqlite3_errcode(dbparameters->db_) == SQLITE_NOMEM) {
isc_throw(DataSourceError,
"Sqlite3 backend encountered a memory allocation "
"error in sqlite3_column_text()");
} else {
return ("");
}
}
const void* p = ucp;
return (static_cast<const char*>(p));
......@@ -361,35 +370,35 @@ convertToPlainChar(const unsigned char* ucp) {
bool
SQLite3Connection::getNextRecord(std::string columns[], size_t column_count) {
try {
sqlite3_stmt* current_stmt = dbparameters_->q_any_;
const int rc = sqlite3_step(current_stmt);
if (column_count != RECORDCOLUMNCOUNT) {
isc_throw(DataSourceError,
"Datasource backend caller did not pass a column array "
"of size " << RECORDCOLUMNCOUNT <<
" to getNextRecord()");
}
if (column_count != RecordColumnCount) {
isc_throw(DataSourceError,
"Datasource backend caller did not pass a column array "
"of size " << RecordColumnCount <<
" to getNextRecord()");
}
sqlite3_stmt* current_stmt = dbparameters_->q_any_;
const int rc = sqlite3_step(current_stmt);
if (rc == SQLITE_ROW) {
for (int column = 0; column < column_count; ++column) {
if (rc == SQLITE_ROW) {
for (int column = 0; column < column_count; ++column) {
try {
columns[column] = convertToPlainChar(sqlite3_column_text(
current_stmt, column));
current_stmt, column),
dbparameters_);
} catch (const std::bad_alloc&) {
isc_throw(DataSourceError,
"bad_alloc in Sqlite3Connection::getNextRecord");
}
return (true);
} else if (rc == SQLITE_DONE) {
// reached the end of matching rows
resetSearch();
return (false);
}
return (true);
} else if (rc == SQLITE_DONE) {
// reached the end of matching rows
resetSearch();
isc_throw(DataSourceError,
"Unexpected failure in sqlite3_step (sqlite result code " << rc << ")");
} catch (const std::bad_alloc&) {
isc_throw(DataSourceError,
"bad_alloc in Sqlite3Connection::getNextRecord");
return (false);
}
isc_throw(DataSourceError, "Unexpected failure in sqlite3_step: " <<
sqlite3_errmsg(dbparameters_->db_));
// Compilers might not realize isc_throw always throws
return (false);
}
......
src/lib/datasrc/tests/database_unittest.cc
View file @ ac15a86e
......@@ -16,12 +16,15 @@
#include <dns/name.h>
#include <dns/rrttl.h>
#include <dns/rrset.h>
#include <exceptions/exceptions.h>
#include <datasrc/database.h>
#include <datasrc/zone.h>
#include <datasrc/data_source.h>
#include <testutils/dnsmessage_test.h>
#include <map>
using namespace isc::datasrc;
......@@ -89,7 +92,7 @@ public:
throw std::exception();
}
if (column_count != DatabaseConnection::RecordColumnCount) {
if (column_count != DatabaseConnection::RECORDCOLUMNCOUNT) {
isc_throw(DataSourceError, "Wrong column count in getNextRecord");
}
if (cur_record < cur_name.size()) {
......@@ -328,18 +331,31 @@ doFindTest(shared_ptr<DatabaseClient::Finder> finder,
const isc::dns::RRType& expected_type,
const isc::dns::RRTTL expected_ttl,
ZoneFinder::Result expected_result,
unsigned int expected_rdata_count,
unsigned int expected_signature_count)
const std::vector<std::string>& expected_rdatas,
const std::vector<std::string>& expected_sig_rdatas)
{
ZoneFinder::FindResult result =
finder->find(name, type, NULL, ZoneFinder::FIND_DEFAULT);
ASSERT_EQ(expected_result, result.code) << name << " " << type;
if (expected_rdata_count > 0) {
EXPECT_EQ(expected_rdata_count, result.rrset->getRdataCount());
if (expected_rdatas.size() > 0) {
EXPECT_EQ(expected_rdatas.size(), result.rrset->getRdataCount());
EXPECT_EQ(expected_ttl, result.rrset->getTTL());
EXPECT_EQ(expected_type, result.rrset->getType());
if (expected_signature_count > 0) {
EXPECT_EQ(expected_signature_count,
isc::dns::RRsetPtr expected_rrset(
new isc::dns::RRset(name, finder->getClass(),
expected_type, expected_ttl));
for (unsigned int i = 0; i < expected_rdatas.size(); ++i) {
expected_rrset->addRdata(
isc::dns::rdata::createRdata(expected_type,
finder->getClass(),
expected_rdatas[i]));
}
isc::testutils::rrsetCheck(expected_rrset, result.rrset);
if (expected_sig_rdatas.size() > 0) {
// TODO same for sigrrset
EXPECT_EQ(expected_sig_rdatas.size(),
result.rrset->getRRsig()->getRdataCount());
} else {
EXPECT_EQ(isc::dns::RRsetPtr(), result.rrset->getRRsig());
......@@ -357,110 +373,224 @@ TEST_F(DatabaseClientTest, find) {
dynamic_pointer_cast<DatabaseClient::Finder>(zone.zone_finder));
EXPECT_EQ(42, finder->zone_id());
EXPECT_FALSE(current_connection_->searchRunning());
std::vector<std::string> expected_rdatas;
std::vector<std::string> expected_sig_rdatas;
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("192.0.2.1");
doFindTest(finder, isc::dns::Name("www.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::A(),
isc::dns::RRTTL(3600),
ZoneFinder::SUCCESS, 1, 0);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("192.0.2.1");
expected_rdatas.push_back("192.0.2.2");
doFindTest(finder, isc::dns::Name("www2.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::A(),
isc::dns::RRTTL(3600),
ZoneFinder::SUCCESS, 2, 0);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("2001:db8::1");
expected_rdatas.push_back("2001:db8::2");
doFindTest(finder, isc::dns::Name("www.example.org."),
isc::dns::RRType::AAAA(), isc::dns::RRType::AAAA(),
isc::dns::RRTTL(3600),
ZoneFinder::SUCCESS, 2, 0);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
doFindTest(finder, isc::dns::Name("www.example.org."),
isc::dns::RRType::TXT(), isc::dns::RRType::TXT(),
isc::dns::RRTTL(3600),
ZoneFinder::NXRRSET, 0, 0);
ZoneFinder::NXRRSET,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("www.example.org.");
doFindTest(finder, isc::dns::Name("cname.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::CNAME(),
isc::dns::RRTTL(3600),
ZoneFinder::CNAME, 1, 0);
ZoneFinder::CNAME,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("www.example.org.");
doFindTest(finder, isc::dns::Name("cname.example.org."),
isc::dns::RRType::CNAME(), isc::dns::RRType::CNAME(),
isc::dns::RRTTL(3600),
ZoneFinder::SUCCESS, 1, 0);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
doFindTest(finder, isc::dns::Name("doesnotexist.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::A(),
isc::dns::RRTTL(3600),
ZoneFinder::NXDOMAIN, 0, 0);
ZoneFinder::NXDOMAIN,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("192.0.2.1");
expected_sig_rdatas.push_back("A 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
expected_sig_rdatas.push_back("A 5 3 3600 20000101000000 20000201000000 12346 example.org. FAKEFAKEFAKE");
doFindTest(finder, isc::dns::Name("signed1.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::A(),
isc::dns::RRTTL(3600),
ZoneFinder::SUCCESS, 1, 2);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("2001:db8::1");
expected_rdatas.push_back("2001:db8::2");
expected_sig_rdatas.push_back("AAAA 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
doFindTest(finder, isc::dns::Name("signed1.example.org."),
isc::dns::RRType::AAAA(), isc::dns::RRType::AAAA(),
isc::dns::RRTTL(3600),
ZoneFinder::SUCCESS, 2, 1);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
doFindTest(finder, isc::dns::Name("signed1.example.org."),
isc::dns::RRType::TXT(), isc::dns::RRType::TXT(),
isc::dns::RRTTL(3600),
ZoneFinder::NXRRSET, 0, 0);
ZoneFinder::NXRRSET,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("www.example.org.");
expected_sig_rdatas.push_back("CNAME 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
doFindTest(finder, isc::dns::Name("signedcname1.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::CNAME(),
isc::dns::RRTTL(3600),
ZoneFinder::CNAME, 1, 1);
ZoneFinder::CNAME,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("192.0.2.1");
expected_sig_rdatas.push_back("A 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
expected_sig_rdatas.push_back("A 5 3 3600 20000101000000 20000201000000 12346 example.org. FAKEFAKEFAKE");
doFindTest(finder, isc::dns::Name("signed2.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::A(),
isc::dns::RRTTL(3600),
ZoneFinder::SUCCESS, 1, 2);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("2001:db8::2");
expected_rdatas.push_back("2001:db8::1");
expected_sig_rdatas.push_back("AAAA 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
doFindTest(finder, isc::dns::Name("signed2.example.org."),
isc::dns::RRType::AAAA(), isc::dns::RRType::AAAA(),
isc::dns::RRTTL(3600),
ZoneFinder::SUCCESS, 2, 1);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
doFindTest(finder, isc::dns::Name("signed2.example.org."),
isc::dns::RRType::TXT(), isc::dns::RRType::TXT(),
isc::dns::RRTTL(3600),
ZoneFinder::NXRRSET, 0, 0);
ZoneFinder::NXRRSET,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("www.example.org.");
expected_sig_rdatas.push_back("CNAME 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
doFindTest(finder, isc::dns::Name("signedcname2.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::CNAME(),
isc::dns::RRTTL(3600),
ZoneFinder::CNAME, 1, 1);
ZoneFinder::CNAME,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("192.0.2.1");
expected_sig_rdatas.push_back("A 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
doFindTest(finder, isc::dns::Name("acnamesig1.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::A(),
isc::dns::RRTTL(3600),
ZoneFinder::SUCCESS, 1, 1);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("192.0.2.1");
expected_sig_rdatas.push_back("A 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
doFindTest(finder, isc::dns::Name("acnamesig2.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::A(),
isc::dns::RRTTL(3600),
ZoneFinder::SUCCESS, 1, 1);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("192.0.2.1");
expected_sig_rdatas.push_back("A 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
doFindTest(finder, isc::dns::Name("acnamesig3.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::A(),
isc::dns::RRTTL(3600),
ZoneFinder::SUCCESS, 1, 1);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("192.0.2.1");
expected_rdatas.push_back("192.0.2.2");
doFindTest(finder, isc::dns::Name("ttldiff1.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::A(),
isc::dns::RRTTL(360),
ZoneFinder::SUCCESS, 2, 0);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("192.0.2.1");
expected_rdatas.push_back("192.0.2.2");
doFindTest(finder, isc::dns::Name("ttldiff2.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::A(),
isc::dns::RRTTL(360),
ZoneFinder::SUCCESS, 2, 0);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
EXPECT_THROW(finder->find(isc::dns::Name("badcname1.example.org."),
isc::dns::RRType::A(),
NULL, ZoneFinder::FIND_DEFAULT),
......@@ -498,7 +628,6 @@ TEST_F(DatabaseClientTest, find) {
EXPECT_FALSE(current_connection_->searchRunning());
// Trigger the hardcoded exceptions and see if find() has cleaned up
/*
EXPECT_THROW(finder->find(isc::dns::Name("dsexception.in.search."),
isc::dns::RRType::A(),
NULL, ZoneFinder::FIND_DEFAULT),
......@@ -514,7 +643,7 @@ TEST_F(DatabaseClientTest, find) {
NULL, ZoneFinder::FIND_DEFAULT),
std::exception);
EXPECT_FALSE(current_connection_->searchRunning());
*/
EXPECT_THROW(finder->find(isc::dns::Name("dsexception.in.getnext."),
isc::dns::RRType::A(),
NULL, ZoneFinder::FIND_DEFAULT),
......@@ -534,12 +663,16 @@ TEST_F(DatabaseClientTest, find) {
// This RRSIG has the wrong sigtype field, which should be
// an error if we decide to keep using that field
// Right now the field is ignored, so it does not error
expected_rdatas.clear();
expected_sig_rdatas.clear();
expected_rdatas.push_back("192.0.2.1");
expected_sig_rdatas.push_back("A 5 3 3600 20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE");
doFindTest(finder, isc::dns::Name("badsigtype.example.org."),
isc::dns::RRType::A(), isc::dns::RRType::A(),
isc::dns::RRTTL(3600),
ZoneFinder::SUCCESS, 1, 1);
ZoneFinder::SUCCESS,
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_connection_->searchRunning());
}
}
src/lib/datasrc/tests/sqlite3_connection_unittest.cc
View file @ ac15a86e
......@@ -17,7 +17,6 @@
#include <dns/rrclass.h>
#include <gtest/gtest.h>
#include <boost/shared_ptr.hpp>
#include <boost/scoped_ptr.hpp>
using namespace isc::datasrc;
......@@ -135,7 +134,7 @@ TEST_F(SQLite3Conn, getRecords) {
const int zone_id = zone_info.second;
ASSERT_EQ(1, zone_id);
const size_t column_count = DatabaseConnection::RecordColumnCount;
const size_t column_count = DatabaseConnection::RECORDCOLUMNCOUNT;
std::string columns[column_count];
// without search, getNext() should return false
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment