From 41b716a473e2efae0993a0dc323cd786c329ff7a Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Wed, 6 May 2015 21:29:39 +0000 Subject: Fix missing account lock while checking for errors. The old assertion, that the write lock file exists before starting checking, was erroneously passing before when no lock was held, because the lockfile was never deleted. Now that we delete it when unlocking the account, this started causing test failures. Changed the way that accounts are checked for errors to use a function that acquires a write lock first, and modified test to disconnect open clients before starting checking the account, to fix it. --- lib/backupstore/BackupStoreAccounts.cpp | 14 ++++++++--- lib/backupstore/BackupStoreAccounts.h | 3 ++- lib/backupstore/StoreTestUtils.cpp | 3 ++- test/backupstorefix/testbackupstorefix.cpp | 37 ++++++------------------------ 4 files changed, 22 insertions(+), 35 deletions(-) diff --git a/lib/backupstore/BackupStoreAccounts.cpp b/lib/backupstore/BackupStoreAccounts.cpp index 5f51917b..d1c7430f 100644 --- a/lib/backupstore/BackupStoreAccounts.cpp +++ b/lib/backupstore/BackupStoreAccounts.cpp @@ -594,7 +594,8 @@ bool BackupStoreAccountsControl::OpenAccount(int32_t ID, std::string &rRootDirOu return true; } -int BackupStoreAccountsControl::CheckAccount(int32_t ID, bool FixErrors, bool Quiet) +int BackupStoreAccountsControl::CheckAccount(int32_t ID, bool FixErrors, bool Quiet, + bool ReturnNumErrorsFound) { std::string rootDir; int discSetNum; @@ -612,8 +613,15 @@ int BackupStoreAccountsControl::CheckAccount(int32_t ID, bool FixErrors, bool Qu // Check it BackupStoreCheck check(rootDir, discSetNum, ID, FixErrors, Quiet); check.Check(); - - return check.ErrorsFound()?1:0; + + if(ReturnNumErrorsFound) + { + return check.GetNumErrorsFound(); + } + else + { + return check.ErrorsFound() ? 1 : 0; + } } int BackupStoreAccountsControl::CreateAccount(int32_t ID, int32_t DiscNumber, diff --git a/lib/backupstore/BackupStoreAccounts.h b/lib/backupstore/BackupStoreAccounts.h index 5ee67312..3c2e4210 100644 --- a/lib/backupstore/BackupStoreAccounts.h +++ b/lib/backupstore/BackupStoreAccounts.h @@ -76,7 +76,8 @@ public: int PrintAccountInfo(int32_t ID); int SetAccountEnabled(int32_t ID, bool enabled); int DeleteAccount(int32_t ID, bool AskForConfirmation); - int CheckAccount(int32_t ID, bool FixErrors, bool Quiet); + int CheckAccount(int32_t ID, bool FixErrors, bool Quiet, + bool ReturnNumErrorsFound = false); int CreateAccount(int32_t ID, int32_t DiscNumber, int32_t SoftLimit, int32_t HardLimit); int HousekeepAccountNow(int32_t ID); diff --git a/lib/backupstore/StoreTestUtils.cpp b/lib/backupstore/StoreTestUtils.cpp index 18cb08ba..e0709876 100644 --- a/lib/backupstore/StoreTestUtils.cpp +++ b/lib/backupstore/StoreTestUtils.cpp @@ -276,7 +276,8 @@ int check_account_for_errors(Log::Level log_level) BackupStoreAccountsControl control(*config); int errors_fixed = control.CheckAccount(0x01234567, true, // FixErrors - false); // Quiet + false, // Quiet + true); // ReturnNumErrorsFound return errors_fixed; } diff --git a/test/backupstorefix/testbackupstorefix.cpp b/test/backupstorefix/testbackupstorefix.cpp index 68525a5c..ce14764c 100644 --- a/test/backupstorefix/testbackupstorefix.cpp +++ b/test/backupstorefix/testbackupstorefix.cpp @@ -77,26 +77,6 @@ std::map objectIsDir; ::system(BBSTOREACCOUNTS " -c testfiles/bbstored.conf check 01234567"); \ ::system(BBSTOREACCOUNTS " -c testfiles/bbstored.conf check 01234567 fix"); -bool check_fix_internal(int expected_num_errors) -{ - BackupStoreCheck checker(accountRootDir, discSetNum, - 0x01234567, true /* FixErrors */, false /* Quiet */); - checker.Check(); - if (expected_num_errors == -1) - { - TEST_THAT(checker.ErrorsFound()); - return checker.ErrorsFound(); - } - else - { - TEST_EQUAL(expected_num_errors, checker.GetNumErrorsFound()); - return checker.GetNumErrorsFound() == expected_num_errors; - } -} - -#define RUN_CHECK_INTERNAL(expected_num_errors) \ - TEST_THAT(check_fix_internal(expected_num_errors)) - // Get ID of an object given a filename int32_t getID(const char *name) { @@ -435,7 +415,7 @@ void check_root_dir_ok(dir_en_check after_entries[], { // Check the store, check that the error is detected and // repaired, by removing x1 from the directory. - RUN_CHECK_INTERNAL(0); + TEST_EQUAL(0, check_account_for_errors()); // Read the directory back in, check that it's empty BackupStoreDirectory dir; @@ -450,7 +430,7 @@ void check_and_fix_root_dir(dir_en_check after_entries[], { // Check the store, check that the error is detected and // repaired. - RUN_CHECK_INTERNAL(-1); + TEST_THAT(check_account_for_errors() > 0); check_root_dir_ok(after_entries, after_deps); } @@ -491,6 +471,7 @@ int test(int argc, const char *argv[]) std::string file_path = "testfiles/TestDir1/cannes/ict/metegoguered/oats"; int x1id = fake_upload(client, file_path, 0); + client.QueryFinished(); // Now break the reverse dependency by deleting x1 (the file, // not the directory entry) @@ -523,6 +504,7 @@ int test(int argc, const char *argv[]) fs.Close(); int x1aid = fake_upload(client, file_path, x1id); + client.QueryFinished(); // Check that we've ended up with the right preconditions // for the tests below. @@ -590,16 +572,11 @@ int test(int argc, const char *argv[]) read_bb_dir(1 /* root */, dir); TEST_THAT(dir.FindEntryByID(0x1234567890123456LL) != 0); - // Check it - BackupStoreCheck checker(accountRootDir, discSetNum, - 0x01234567, true /* FixErrors */, false /* Quiet */); - checker.Check(); - // Should just be greater than 1 really, we don't know quite // how good the checker is (or will become) at spotting errors! // But this will help us catch changes in checker behaviour, // so it's not a bad thing to test. - TEST_EQUAL(2, checker.GetNumErrorsFound()); + TEST_EQUAL(2, check_account_for_errors()); file = RaidFileRead::Open(discSetNum, fn); dir.ReadFromStream(*file, IOStream::TimeOutInfinite); @@ -754,7 +731,7 @@ int test(int argc, const char *argv[]) // ERROR: BlocksInCurrentFiles changed from 228 to 226 // ERROR: NumCurrentFiles changed from 114 to 113 // WARNING: Reference count of object 0x44 changed from 1 to 0 - RUN_CHECK_INTERNAL(5); + TEST_EQUAL(5, check_account_for_errors()); { std::auto_ptr usage = BackupProtocolLocal2(0x01234567, "test", @@ -862,7 +839,7 @@ int test(int argc, const char *argv[]) // ERROR: NumFiles changed from 113 to 110 // WARNING: Reference count of object 0x3e changed from 1 to 0 - RUN_CHECK_INTERNAL(12); + TEST_EQUAL(12, check_account_for_errors()); { std::auto_ptr usage = -- cgit v1.2.3