From f5f52667abb7487a2db7e51adbf0764b14c0a3c0 Mon Sep 17 00:00:00 2001 From: "Michael P. Gerlek" Date: Tue, 26 May 2015 10:38:49 -0400 Subject: [PATCH 1/2] better error handling, as per #927 made return codes and error handling consistent removed derived exception types added missing finalize() call (which caused random crashes), and enforced resetting of m_statement (which is shared between insert() and query()) (nothing readily unit-testable) --- plugins/sqlite/io/SQLiteCommon.hpp | 205 ++++++++++++++--------------- plugins/sqlite/io/SQLiteReader.cpp | 8 +- plugins/sqlite/io/SQLiteWriter.cpp | 4 +- 3 files changed, 102 insertions(+), 115 deletions(-) diff --git a/plugins/sqlite/io/SQLiteCommon.hpp b/plugins/sqlite/io/SQLiteCommon.hpp index 320c41f15f..f1b6ae7a30 100644 --- a/plugins/sqlite/io/SQLiteCommon.hpp +++ b/plugins/sqlite/io/SQLiteCommon.hpp @@ -47,31 +47,6 @@ namespace pdal { - - class sqlite_driver_error : public pdal_error - { - public: - sqlite_driver_error(std::string const& msg) - : pdal_error(msg) - {} - }; - - class connection_failed : public sqlite_driver_error - { - public: - connection_failed(std::string const& msg) - : sqlite_driver_error(msg) - {} - }; - - class buffer_too_small : public sqlite_driver_error - { - public: - buffer_too_small(std::string const& msg) - : sqlite_driver_error(msg) - {} - }; - class column { public: @@ -170,6 +145,7 @@ class SQLite sqlite3_config(SQLITE_CONFIG_LOG, log_callback, this); sqlite3_initialize(); m_log->get(LogLevel::Debug3) << "Set up config " << std::endl; + m_log->get(LogLevel::Debug3) << "SQLite version: " << sqlite3_libversion() << std::endl; } ~SQLite() @@ -200,7 +176,7 @@ class SQLite { if ( ! m_connection.size() ) { - throw connection_failed("unable to connect to sqlite3 database, no connection string was given!"); + throw pdal_error("unable to connect to sqlite3 database, no connection string was given!"); } int flags = SQLITE_OPEN_NOMUTEX; @@ -215,25 +191,29 @@ class SQLite flags |= SQLITE_OPEN_READONLY; } - int code = sqlite3_open_v2(m_connection.c_str(), &m_session, flags, 0); - if ( (code != SQLITE_OK) ) + int status = sqlite3_open_v2(m_connection.c_str(), &m_session, flags, 0); + if (status != SQLITE_OK) { - check_error("unable to connect to database"); + error("sqlite3_open_v2: unable to connect to database"); } } - void execute(std::string const& sql, std::string errmsg="") + void execute(std::string const& sql, const std::string& userErrorMsg="") { if (!m_session) - throw sqlite_driver_error("Session not opened!"); + { + throw pdal_error("Session not opened!"); + } m_log->get(LogLevel::Debug3) << "Executing '" << sql <<"'"<< std::endl; - int code = sqlite3_exec(m_session, sql.c_str(), NULL, NULL, NULL); - if (code != SQLITE_OK) + int status = sqlite3_exec(m_session, sql.c_str(), NULL, NULL, NULL); + if (status != SQLITE_OK) { std::ostringstream oss; - oss << errmsg <<" '" << sql << "'"; - throw sqlite_driver_error(oss.str()); + oss << "sqlite3_exec: " << userErrorMsg + << std::endl + << "SQL: \"" << sql << "\""; + error(oss.str()); } } @@ -265,32 +245,30 @@ class SQLite m_position = 0; m_columns.clear(); m_data.clear(); - sqlite3_reset(m_statement); - + assert(!m_statement); + + int status; + m_log->get(LogLevel::Debug3) << "Querying '" << query.c_str() <<"'"<< std::endl; char const* tail = 0; // unused; - int res = sqlite3_prepare_v2(m_session, - query.c_str(), - static_cast(query.size()), - &m_statement, - &tail); - if (res != SQLITE_OK) + status = sqlite3_prepare_v2(m_session, + query.c_str(), + static_cast(query.size()), + &m_statement, + &tail); + if (status != SQLITE_OK) { - char const* zErrMsg = sqlite3_errmsg(m_session); - - std::ostringstream ss; - ss << "sqlite3_statement_backend::prepare: " - << zErrMsg; - throw sqlite_driver_error(ss.str()); + error("sqlite3_prepare_v2 (query)"); } + int numCols = -1; - while (res != SQLITE_DONE) + while (status != SQLITE_DONE) { - res = sqlite3_step(m_statement); + status = sqlite3_step(m_statement); - if (SQLITE_ROW == res) + if (SQLITE_ROW == status) { // only need to set the number of columns once if (-1 == numCols) @@ -345,7 +323,23 @@ class SQLite } m_data.push_back(r); } + else if (status == SQLITE_DONE) + { + // ok + } + else + { + error("sqlite3_step"); + } } + + status = sqlite3_finalize(m_statement); + if (status != SQLITE_OK) + { + error("sqlite3_finalize"); + } + + m_statement = NULL; } bool next() @@ -381,75 +375,75 @@ class SQLite return (int64_t)sqlite3_last_insert_rowid(m_session); } - bool insert(std::string const& statement, records const& rs) + void insert(std::string const& statement, records const& rs) { + int status; + records::size_type rows = rs.size(); - int res = sqlite3_prepare_v2(m_session, - statement.c_str(), - static_cast(statement.size()), - &m_statement, - 0); - m_log->get(LogLevel::Debug3) << "Inserting '" << statement << "'"<< - std::endl; - - if (res != SQLITE_OK) + assert(!m_statement); + status = sqlite3_prepare_v2(m_session, + statement.c_str(), + static_cast(statement.size()), + &m_statement, + 0); + if (status != SQLITE_OK) { - char const* zErrMsg = sqlite3_errmsg(m_session); - - std::ostringstream ss; - ss << "sqlite insert prepare: " - << zErrMsg; - throw sqlite_driver_error(ss.str()); + error("sqlite3_prepare_v2 (insert)"); } + + m_log->get(LogLevel::Debug3) << "Inserting '" << statement << "'"<< + std::endl; for (records::size_type r = 0; r < rows; ++r) { int const totalPositions = static_cast(rs[0].size()); for (int pos = 0; pos <= totalPositions-1; ++pos) { - int didBind = SQLITE_OK; const column& c = rs[r][pos]; if (c.null) { - didBind = sqlite3_bind_null(m_statement, pos+1); + status = sqlite3_bind_null(m_statement, pos+1); } else if (c.blobLen != 0) { - didBind = sqlite3_bind_blob(m_statement, pos+1, - &(c.blobBuf.front()), - static_cast(c.blobLen), - SQLITE_STATIC); + status = sqlite3_bind_blob(m_statement, pos+1, + &(c.blobBuf.front()), + static_cast(c.blobLen), + SQLITE_STATIC); } else { - didBind = sqlite3_bind_text(m_statement, pos+1, - c.data.c_str(), static_cast(c.data.length()), - SQLITE_STATIC); + status = sqlite3_bind_text(m_statement, pos+1, + c.data.c_str(), + static_cast(c.data.length()), + SQLITE_STATIC); } - if (SQLITE_OK != didBind) + if (SQLITE_OK != status) { std::ostringstream oss; - oss << "Failure to bind row number '" - << r <<"' at position number '" <get(LogLevel::Debug3) << "SpatiaLite version: " << getSpatialiteVersion() << std::endl; return true; @@ -510,13 +503,10 @@ class SQLite bool doesTableExist(std::string const& name) { - std::ostringstream oss; + const std::string sql("SELECT name FROM sqlite_master WHERE type = 'table'"); - oss << "SELECT name FROM sqlite_master WHERE type = \"table\""; - - query(oss.str()); + query(sql); - std::ostringstream debug; do { const row* r = get(); @@ -563,16 +553,15 @@ class SQLite std::map m_columns; std::vector m_types; - void check_error(std::string const& msg) + void error(std::string const& userMssg) { - const char *zErrMsg = sqlite3_errmsg(m_session); + char const* sqlMssg = sqlite3_errmsg(m_session); + std::ostringstream ss; - ss << msg << " sqlite error: " << zErrMsg; - throw sqlite_driver_error(ss.str()); + ss << "sqlite error: " << userMssg << std::endl + << sqlMssg; + throw pdal_error(ss.str()); } - - - }; } // namespace pdal diff --git a/plugins/sqlite/io/SQLiteReader.cpp b/plugins/sqlite/io/SQLiteReader.cpp index 78d0fc2083..d2c6f89d9e 100644 --- a/plugins/sqlite/io/SQLiteReader.cpp +++ b/plugins/sqlite/io/SQLiteReader.cpp @@ -64,13 +64,11 @@ void SQLiteReader::initialize() if (!bHaveSpatialite) { - std::ostringstream oss; - oss << "no spatialite enabled!"; - throw sqlite_driver_error(oss.str()); + throw pdal_error("no spatialite enabled!"); } } - catch (sqlite_driver_error const& e) + catch (pdal_error const& e) { std::stringstream oss; oss << "Unable to connect to database with error '" << e.what() << "'"; @@ -188,7 +186,7 @@ void SQLiteReader::addDimensions(PointLayoutPtr layout) m_session->query(q); const row* r = m_session->get(); // First result better have our schema if (!r) - throw sqlite_driver_error("Unable to select schema from query!"); + throw pdal_error("Unable to select schema from query!"); column const& s = r->at(0); // First column is schema diff --git a/plugins/sqlite/io/SQLiteWriter.cpp b/plugins/sqlite/io/SQLiteWriter.cpp index 55e397056a..828a051c2f 100644 --- a/plugins/sqlite/io/SQLiteWriter.cpp +++ b/plugins/sqlite/io/SQLiteWriter.cpp @@ -80,7 +80,7 @@ void SQLiteWriter::processOptions(const Options& options) options.getValueOrDefault("filename", ""); if (!m_connection.size()) - throw sqlite_driver_error("unable to connect to database, " + throw pdal_error("unable to connect to database, " "no connection string was given!"); } m_block_table = @@ -119,7 +119,7 @@ void SQLiteWriter::initialize() } } - catch (sqlite_driver_error const& e) + catch (pdal_error const& e) { std::stringstream oss; oss << "Unable to connect to database with error '" << e.what() << "'"; From 0d383c66f96be48fcda66dd7721be4942dcbfc6f Mon Sep 17 00:00:00 2001 From: "Michael P. Gerlek" Date: Tue, 26 May 2015 10:41:30 -0400 Subject: [PATCH 2/2] explicitly scope the database read & write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit otherwise we can’t be assured the dtors will be called so the files get closed out properly --- plugins/sqlite/test/SQLiteTest.cpp | 75 ++++++++++++++++-------------- 1 file changed, 41 insertions(+), 34 deletions(-) diff --git a/plugins/sqlite/test/SQLiteTest.cpp b/plugins/sqlite/test/SQLiteTest.cpp index cc730b9708..a864925dd0 100644 --- a/plugins/sqlite/test/SQLiteTest.cpp +++ b/plugins/sqlite/test/SQLiteTest.cpp @@ -82,6 +82,8 @@ void testReadWrite(bool compression, bool scaling) std::string tempFilename = getSQLITEOptions().getValueOrThrow("connection"); + FileUtils::deleteFile(tempFilename); + Options sqliteOptions = getSQLITEOptions(); if (scaling) { @@ -90,50 +92,55 @@ void testReadWrite(bool compression, bool scaling) } sqliteOptions.add("compression", compression, ""); - // remove file from earlier run, if needed - std::string temp_filename = + { + // remove file from earlier run, if needed + std::string temp_filename = sqliteOptions.getValueOrThrow("connection"); - Options lasReadOpts; - lasReadOpts.add("filename", Support::datapath("las/1.2-with-color.las")); - lasReadOpts.add("count", 11); + Options lasReadOpts; + lasReadOpts.add("filename", Support::datapath("las/1.2-with-color.las")); + lasReadOpts.add("count", 11); - LasReader reader; - reader.setOptions(lasReadOpts); + LasReader reader; + reader.setOptions(lasReadOpts); - StageFactory f; - std::unique_ptr sqliteWriter(f.createStage("writers.sqlite")); - sqliteWriter->setOptions(sqliteOptions); - sqliteWriter->setInput(reader); + StageFactory f; + std::unique_ptr sqliteWriter(f.createStage("writers.sqlite")); + sqliteWriter->setOptions(sqliteOptions); + sqliteWriter->setInput(reader); - PointTable table; - sqliteWriter->prepare(table); - sqliteWriter->execute(table); - - // Done - now read back. - std::unique_ptr sqliteReader(f.createStage("readers.sqlite")); - sqliteReader->setOptions(sqliteOptions); + PointTable table; + sqliteWriter->prepare(table); + sqliteWriter->execute(table); + } + + { + // Done - now read back. + StageFactory f; + std::unique_ptr sqliteReader(f.createStage("readers.sqlite")); + sqliteReader->setOptions(sqliteOptions); - PointTable table2; - sqliteReader->prepare(table2); - PointViewSet viewSet = sqliteReader->execute(table2); - EXPECT_EQ(viewSet.size(), 1U); - PointViewPtr view = *viewSet.begin(); + PointTable table2; + sqliteReader->prepare(table2); + PointViewSet viewSet = sqliteReader->execute(table2); + EXPECT_EQ(viewSet.size(), 1U); + PointViewPtr view = *viewSet.begin(); - using namespace Dimension; + using namespace Dimension; - uint16_t reds[] = {68, 54, 112, 178, 134, 99, 90, 106, 106, 100, 64}; - for (PointId idx = 0; idx < 11; idx++) - { - uint16_t r = view->getFieldAs(Id::Red, idx); - EXPECT_EQ(r, reds[idx]); + uint16_t reds[] = {68, 54, 112, 178, 134, 99, 90, 106, 106, 100, 64}; + for (PointId idx = 0; idx < 11; idx++) + { + uint16_t r = view->getFieldAs(Id::Red, idx); + EXPECT_EQ(r, reds[idx]); + } + int32_t x = view->getFieldAs(Id::X, 10); + EXPECT_EQ(x, 636038); + double xd = view->getFieldAs(Id::X, 10); + EXPECT_FLOAT_EQ(xd, 636037.53); } - int32_t x = view->getFieldAs(Id::X, 10); - EXPECT_EQ(x, 636038); - double xd = view->getFieldAs(Id::X, 10); - EXPECT_FLOAT_EQ(xd, 636037.53); - FileUtils::deleteFile(tempFilename); + FileUtils::deleteFile(tempFilename); }