From 7776a37d651f0afdf61f150fba50884f2ad3ac5d Mon Sep 17 00:00:00 2001 From: shess Date: Tue, 8 Mar 2016 21:30:30 -0800 Subject: [PATCH] [sqlite] sql::Recovery working under USE_SYSTEM_SQLITE. Removes the USE_SYSTEM_SQLITE conditional sections in sql::Recover implementation and tests. third_party/sqlite includes sqlite_recover library for the USE_SYSTEM_SQLITE case, with recover.c compiled against the system version of SQLite. Additionally remove interiorCursorEOF() from recover.c, as it was unused, rather than adding a compile flag to allow it. BUG=584407 Review URL: https://codereview.chromium.org/1757653002 Cr-Commit-Position: refs/heads/master@{#380076} --- sql/BUILD.gn | 2 ++ sql/recovery.cc | 19 +------------------ sql/recovery_unittest.cc | 4 ---- sql/sql.gyp | 12 ++++++++++++ third_party/sqlite/BUILD.gn | 12 ++++++++++++ third_party/sqlite/sqlite.gyp | 15 +++++++++++++++ third_party/sqlite/src/src/recover.c | 15 --------------- 7 files changed, 42 insertions(+), 37 deletions(-) diff --git a/sql/BUILD.gn b/sql/BUILD.gn index 4ffa421df12f8f..8c1cbb79997e2f 100644 --- a/sql/BUILD.gn +++ b/sql/BUILD.gn @@ -113,6 +113,8 @@ test("sql_unittests") { # '../testing/android/native_test.gyp:native_test_native_code', # ], #}], + + # TODO(GYP): dep on copy_test_data_ios action. } if (is_android) { diff --git a/sql/recovery.cc b/sql/recovery.cc index b79fe08598ec33..7b9ca9f6fee657 100644 --- a/sql/recovery.cc +++ b/sql/recovery.cc @@ -98,11 +98,7 @@ void RecordRecoveryEvent(RecoveryEventType recovery_event) { // static bool Recovery::FullRecoverySupported() { // TODO(shess): See comment in Init(). -#if defined(USE_SYSTEM_SQLITE) - return false; -#else return true; -#endif } // static @@ -203,16 +199,7 @@ bool Recovery::Init(const base::FilePath& db_path) { return false; } - // TODO(shess): Figure out a story for USE_SYSTEM_SQLITE. The - // virtual table implementation relies on SQLite internals for some - // types and functions, which could be copied inline to make it - // standalone. Or an alternate implementation could try to read - // through errors entirely at the SQLite level. - // - // For now, defer to the caller. The setup will succeed, but the - // later CREATE VIRTUAL TABLE call will fail, at which point the - // caller can fire Unrecoverable(). -#if !defined(USE_SYSTEM_SQLITE) + // Enable the recover virtual table for this connection. int rc = recoverVtableInit(recover_db_.db_); if (rc != SQLITE_OK) { RecordRecoveryEvent(RECOVERY_FAILED_VIRTUAL_TABLE_INIT); @@ -220,10 +207,6 @@ bool Recovery::Init(const base::FilePath& db_path) { << recover_db_.GetErrorMessage(); return false; } -#else - // If this is infrequent enough, just wire it to Raze(). - RecordRecoveryEvent(RECOVERY_FAILED_VIRTUAL_TABLE_SYSTEM_SQLITE); -#endif // Turn on |SQLITE_RecoveryMode| for the handle, which allows // reading certain broken databases. diff --git a/sql/recovery_unittest.cc b/sql/recovery_unittest.cc index e4fdb7b189ebe0..d3b04b1eb900a8 100644 --- a/sql/recovery_unittest.cc +++ b/sql/recovery_unittest.cc @@ -172,9 +172,6 @@ TEST_F(SQLRecoveryTest, RecoverBasic) { ExecuteWithResults(&db(), kXSql, "|", "\n")); } -// The recovery virtual table is only supported for Chromium's SQLite. -#if !defined(USE_SYSTEM_SQLITE) - // Test operation of the virtual table used by sql::Recovery. TEST_F(SQLRecoveryTest, VirtualTable) { const char kCreateSql[] = "CREATE TABLE x (t TEXT)"; @@ -737,7 +734,6 @@ TEST_F(SQLRecoveryTest, Bug387868) { EXPECT_TRUE(sql::Recovery::Recovered(std::move(recovery))); } } -#endif // !defined(USE_SYSTEM_SQLITE) // Memory-mapped I/O interacts poorly with I/O errors. Make sure the recovery // database doesn't accidentally enable it. diff --git a/sql/sql.gyp b/sql/sql.gyp index 30bd76dde61170..cde30576cb9269 100644 --- a/sql/sql.gyp +++ b/sql/sql.gyp @@ -114,6 +114,18 @@ '../testing/android/native_test.gyp:native_test_native_code', ], }], + ['OS == "ios"', { + 'actions': [{ + 'action_name': 'copy_test_data', + 'variables': { + 'test_data_files': [ + 'test/data', + ], + 'test_data_prefix' : 'sql', + }, + 'includes': [ '../build/copy_test_data_ios.gypi' ], + }], + }], ], # TODO(jschuh): crbug.com/167187 fix size_t to int truncations. 'msvs_disabled_warnings': [4267, ], diff --git a/third_party/sqlite/BUILD.gn b/third_party/sqlite/BUILD.gn index 8400837db8c2d3..c6fb95d2486846 100644 --- a/third_party/sqlite/BUILD.gn +++ b/third_party/sqlite/BUILD.gn @@ -223,6 +223,9 @@ if (use_system_sqlite) { source_set("sqlite") { public_configs = [ ":sqlite_config" ] + deps = [ + ":sqlite_recover", + ] if (is_ios) { deps = [ ":sqlite_regexp", @@ -230,6 +233,15 @@ if (use_system_sqlite) { } } + source_set("sqlite_recover") { + sources = [ + # TODO(shess): Move out of the SQLite source tree, perhaps to ext/. + "src/src/recover.c", + "src/src/recover.h", + "src/src/recover_varint.c", + ] + } + if (is_ios) { source_set("sqlite_regexp") { defines = [ diff --git a/third_party/sqlite/sqlite.gyp b/third_party/sqlite/sqlite.gyp index fca90060385835..2f3f67ea53805c 100644 --- a/third_party/sqlite/sqlite.gyp +++ b/third_party/sqlite/sqlite.gyp @@ -64,6 +64,10 @@ ], }, + 'dependencies': [ + 'sqlite_recover', + ], + 'conditions': [ ['OS == "ios"', { 'dependencies': [ @@ -215,6 +219,17 @@ # crbug.com/422251 '../../build/android/disable_gcc_lto.gypi', ], + }, { + # Virtual table used by sql::Recovery to recover corrupt databases, for + # use with USE_SYSTEM_SQLITE. + 'target_name': 'sqlite_recover', + 'type': 'static_library', + 'sources': [ + # TODO(shess): Move out of the SQLite source tree, perhaps to ext/. + 'src/src/recover_varint.c', + 'src/src/recover.c', + 'src/src/recover.h', + ], }, ], 'conditions': [ diff --git a/third_party/sqlite/src/src/recover.c b/third_party/sqlite/src/src/recover.c index 9ad2d1c5c05ad5..bb2ae9e79b8df4 100644 --- a/third_party/sqlite/src/src/recover.c +++ b/third_party/sqlite/src/src/recover.c @@ -701,8 +701,6 @@ static int getRootPage(sqlite3 *db, const char *zDb, const char *zTable, * interiorCursorDestroy - release all resources associated with the * cursor and any parent cursors. * interiorCursorCreate - create a cursor with the given parent and page. - * interiorCursorEOF - returns true if neither the cursor nor the - * parent cursors can return any more data. * interiorCursorNextPage - fetch the next child page from the cursor. * * Logically, interiorCursorNextPage() returns the next child page @@ -719,11 +717,6 @@ static int getRootPage(sqlite3 *db, const char *zDb, const char *zTable, * Note that while interiorCursorNextPage() will refuse to follow * loops, it does not keep track of pages returned for purposes of * preventing duplication. - * - * Note that interiorCursorEOF() could return false (not at EOF), and - * interiorCursorNextPage() could still return SQLITE_DONE. This - * could happen if there are more cells to iterate in an interior - * page, but those cells refer to invalid pages. */ typedef struct RecoverInteriorCursor RecoverInteriorCursor; struct RecoverInteriorCursor { @@ -839,14 +832,6 @@ static unsigned interiorCursorChildPage(RecoverInteriorCursor *pCursor){ return 0; } -static int interiorCursorEOF(RecoverInteriorCursor *pCursor){ - /* Find a parent with remaining children. EOF if none found. */ - while( pCursor && pCursor->iChild>=pCursor->nChildren ){ - pCursor = pCursor->pParent; - } - return pCursor==NULL; -} - /* Internal helper. Used to detect if iPage would cause a loop. */ static int interiorCursorPageInUse(RecoverInteriorCursor *pCursor, unsigned iPage){