From e6ef4d9e1d545da3a0a3eeee79d57eed9fb9738a Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Thu, 17 Aug 2017 20:27:29 +0100 Subject: [PATCH] Initial refactor of BackupAccountControl to use BackupFileSystem Abstract away some account operations using BackupFileSystem so that they can be implemented differently for S3 accounts. In particular, account opening and NamedLock usage were targets for this refactor. BackupStoreInfo creation and opening ended up being partially addressed as well. This significantly reduces the diffs to the s3_support branch. --- lib/backupstore/BackupAccountControl.cpp | 183 ++++++++++++++--------- lib/backupstore/BackupAccountControl.h | 11 +- lib/backupstore/BackupFileSystem.h | 8 + lib/backupstore/BackupStoreAccounts.cpp | 68 --------- lib/backupstore/BackupStoreAccounts.h | 3 - lib/backupstore/BackupStoreException.txt | 5 +- lib/backupstore/BackupStoreInfo.cpp | 3 +- lib/backupstore/BackupStoreInfo.h | 18 ++- 8 files changed, 145 insertions(+), 154 deletions(-) diff --git a/lib/backupstore/BackupAccountControl.cpp b/lib/backupstore/BackupAccountControl.cpp index 76b4dc973..61b633fea 100644 --- a/lib/backupstore/BackupAccountControl.cpp +++ b/lib/backupstore/BackupAccountControl.cpp @@ -26,13 +26,28 @@ #include "Configuration.h" #include "HTTPResponse.h" #include "HousekeepStoreAccount.h" -#include "NamedLock.h" #include "RaidFileController.h" +#include "RaidFileWrite.h" #include "UnixUser.h" #include "Utils.h" #include "MemLeakFindOn.h" +#define OPEN_ACCOUNT(read_write) \ + try \ + { \ + OpenAccount(read_write); \ + } \ + catch(BoxException &e) \ + { \ + if(EXCEPTION_IS_TYPE(e, BackupStoreException, CouldNotLockStoreAccount)) \ + { \ + BOX_ERROR("Failed to open account: " << e.what()); \ + return 1; \ + } \ + throw; \ + } + void BackupAccountControl::CheckSoftHardLimits(int64_t SoftLimit, int64_t HardLimit) { if(SoftLimit > HardLimit) @@ -113,14 +128,7 @@ int BackupStoreAccountControl::BlockSizeOfDiscSet(int discSetNum) int BackupStoreAccountControl::SetLimit(const char *SoftLimitStr, const char *HardLimitStr) { - NamedLock writeLock; - - if(!OpenAccount(&writeLock)) - { - BOX_ERROR("Failed to open account " << BOX_FORMAT_ACCOUNT(mAccountID) - << " to change limits."); - return 1; - } + OPEN_ACCOUNT(true); // readWrite // Load the info std::auto_ptr info(BackupStoreInfo::Load(mAccountID, mRootDir, @@ -145,14 +153,7 @@ int BackupStoreAccountControl::SetLimit(const char *SoftLimitStr, int BackupStoreAccountControl::SetAccountName(const std::string& rNewAccountName) { - NamedLock writeLock; - - if(!OpenAccount(&writeLock)) - { - BOX_ERROR("Failed to open account " << BOX_FORMAT_ACCOUNT(mAccountID) - << " to change name."); - return 1; - } + OPEN_ACCOUNT(true); // readWrite // Load the info std::auto_ptr info(BackupStoreInfo::Load(mAccountID, @@ -171,17 +172,9 @@ int BackupStoreAccountControl::SetAccountName(const std::string& rNewAccountName int BackupStoreAccountControl::PrintAccountInfo() { - if(!OpenAccount(NULL /* no write lock needed for this read-only operation */)) - { - BOX_ERROR("Failed to open account " << BOX_FORMAT_ACCOUNT(mAccountID) - << " to display info."); - return 1; - } + OPEN_ACCOUNT(false); // !readWrite - // Load it in - std::auto_ptr ap_info(BackupStoreInfo::Load(mAccountID, - mRootDir, mDiscSetNum, true /* ReadOnly */)); - BackupStoreInfo& info(*ap_info); + BackupStoreInfo& info(mapFileSystem->GetBackupStoreInfo(true)); // ReadOnly int BlockSize = GetBlockSize(); // Then print out lots of info @@ -231,14 +224,7 @@ int BackupStoreAccountControl::PrintAccountInfo() int BackupStoreAccountControl::SetAccountEnabled(bool enabled) { - NamedLock writeLock; - - if(!OpenAccount(&writeLock)) - { - BOX_ERROR("Failed to open account " << BOX_FORMAT_ACCOUNT(mAccountID) - << " to change enabled flag."); - return 1; - } + OPEN_ACCOUNT(true); // readWrite // Load it in std::auto_ptr info(BackupStoreInfo::Load(mAccountID, @@ -248,17 +234,47 @@ int BackupStoreAccountControl::SetAccountEnabled(bool enabled) return 0; } -int BackupStoreAccountControl::DeleteAccount(bool AskForConfirmation) + +int BackupAccountControl::CreateAccount(int32_t AccountID, int32_t SoftLimit, + int32_t HardLimit, const std::string& AccountName) { - NamedLock writeLock; + OPEN_ACCOUNT(true); // readWrite - // Obtain a write lock, as the daemon user - if(!OpenAccount(&writeLock)) - { - BOX_ERROR("Failed to open account " << BOX_FORMAT_ACCOUNT(mAccountID) - << " for deletion."); - return 1; - } + BackupStoreInfo& info(mapFileSystem->GetBackupStoreInfo(false)); // !ReadOnly + info.SetAccountName(AccountName); + + // And an empty directory + BackupStoreDirectory rootDir(BACKUPSTORE_ROOT_DIRECTORY_ID, + BACKUPSTORE_ROOT_DIRECTORY_ID); + mapFileSystem->PutDirectory(rootDir); + int64_t rootDirSize = rootDir.GetUserInfo1_SizeInBlocks(); + + // Update the store info to reflect the size of the root directory + info.ChangeBlocksUsed(rootDirSize); + info.ChangeBlocksInDirectories(rootDirSize); + info.AdjustNumDirectories(1); + int64_t id = info.AllocateObjectID(); + ASSERT(id == BACKUPSTORE_ROOT_DIRECTORY_ID); + + mapFileSystem->PutBackupStoreInfo(info); + + // Now get the info file again, and report any differences, to check that it + // really worked. + std::auto_ptr copy = mapFileSystem->GetBackupStoreInfoUncached(); + ASSERT(info.ReportChangesTo(*copy) == 0); + + // We also need to create and upload a fresh refcount DB. + BackupStoreRefCountDatabase& refcount( + mapFileSystem->GetPotentialRefCountDatabase()); + refcount.Commit(); + + return 0; +} + +int BackupStoreAccountControl::DeleteAccount(bool AskForConfirmation) +{ + // We definitely need a lock to do something this destructive! + OPEN_ACCOUNT(true); // readWrite // Check user really wants to do this if(AskForConfirmation) @@ -322,7 +338,7 @@ int BackupStoreAccountControl::DeleteAccount(bool AskForConfirmation) // NamedLock will throw an exception if it can't delete the lockfile, // which it can't if it doesn't exist. Now that we've deleted the account, // nobody can open it anyway, so it's safe to unlock. - writeLock.ReleaseLock(); + mapFileSystem->ReleaseLock(); int retcode = 0; @@ -361,8 +377,21 @@ int BackupStoreAccountControl::DeleteAccount(bool AskForConfirmation) return retcode; } -bool BackupStoreAccountControl::OpenAccount(NamedLock* pLock) +void BackupStoreAccountControl::OpenAccount(bool readWrite) { + if(mapFileSystem.get()) + { + // Can use existing BackupFileSystem. + + if(readWrite && !mapFileSystem->HaveLock()) + { + // Need to acquire a lock first. + mapFileSystem->GetLock(); + } + + return; + } + // Load in the account database std::auto_ptr db( BackupStoreAccountDatabase::Read( @@ -371,9 +400,9 @@ bool BackupStoreAccountControl::OpenAccount(NamedLock* pLock) // Exists? if(!db->EntryExists(mAccountID)) { - BOX_ERROR("Account " << BOX_FORMAT_ACCOUNT(mAccountID) << - " does not exist."); - return false; + THROW_EXCEPTION_MESSAGE(BackupStoreException, AccountDoesNotExist, + "Failed to open account " << BOX_FORMAT_ACCOUNT(mAccountID) << + ": does not exist"); } // Get info from the database @@ -396,29 +425,22 @@ bool BackupStoreAccountControl::OpenAccount(NamedLock* pLock) // Username specified, change... mapChangeUser.reset(new UnixUser(username)); mapChangeUser->ChangeProcessUser(true /* temporary */); - // Change will be undone when apUser goes out of scope - // in the caller. + // Change will be undone when this BackupStoreAccountControl is destroyed } - if(pLock) + mapFileSystem.reset(new RaidBackupFileSystem(mAccountID, mRootDir, mDiscSetNum)); + + if(readWrite) { - acc.LockAccount(mAccountID, *pLock); + mapFileSystem->GetLock(); } - - return true; } int BackupStoreAccountControl::CheckAccount(bool FixErrors, bool Quiet, bool ReturnNumErrorsFound) { - NamedLock writeLock; - - if(!OpenAccount(FixErrors ? &writeLock : NULL)) // don't need a write lock if not making changes - { - BOX_ERROR("Failed to open account " << BOX_FORMAT_ACCOUNT(mAccountID) - << " for checking."); - return 1; - } + // Don't need a write lock if not making changes. + OPEN_ACCOUNT(FixErrors); // readWrite // Check it BackupStoreCheck check(mRootDir, mDiscSetNum, mAccountID, FixErrors, Quiet); @@ -450,6 +472,11 @@ int BackupStoreAccountControl::CreateAccount(int32_t DiscNumber, return 1; } + db->AddEntry(mAccountID, DiscNumber); + + // As the original user, write the modified account database back + db->Write(); + // Get the user under which the daemon runs std::string username; { @@ -460,9 +487,27 @@ int BackupStoreAccountControl::CreateAccount(int32_t DiscNumber, } } - // Create it. + // Become the user specified in the config file? + if(!username.empty()) + { + // Username specified, change... + mapChangeUser.reset(new UnixUser(username)); + mapChangeUser->ChangeProcessUser(true /* temporary */); + // Change will be undone when this BackupStoreAccountControl is destroyed + } + + // Get directory name: BackupStoreAccounts acc(*db); - acc.Create(mAccountID, DiscNumber, SoftLimit, HardLimit, username); + acc.GetAccountRoot(mAccountID, mRootDir, mDiscSetNum); + + // Create the account root directory on disc: + RaidFileWrite::CreateDirectory(mDiscSetNum, mRootDir, true /* recursive */); + + // Create the BackupStoreInfo and BackupStoreRefCountDatabase files: + BackupStoreInfo::CreateNew(mAccountID, mRootDir, mDiscSetNum, SoftLimit, HardLimit); + + mapFileSystem.reset(new RaidBackupFileSystem(mAccountID, mRootDir, mDiscSetNum)); + BackupAccountControl::CreateAccount(mAccountID, SoftLimit, HardLimit, ""); BOX_NOTICE("Account " << BOX_FORMAT_ACCOUNT(mAccountID) << " created."); @@ -472,12 +517,8 @@ int BackupStoreAccountControl::CreateAccount(int32_t DiscNumber, int BackupStoreAccountControl::HousekeepAccountNow() { - if(!OpenAccount(NULL /* housekeeping locks the account itself */)) - { - BOX_ERROR("Failed to open account " << BOX_FORMAT_ACCOUNT(mAccountID) - << " for housekeeping."); - return 1; - } + // Housekeeping locks the account itself, so we can't. + OPEN_ACCOUNT(false); // readWrite HousekeepStoreAccount housekeeping(mAccountID, mRootDir, mDiscSetNum, NULL); bool success = housekeeping.DoHousekeeping(); diff --git a/lib/backupstore/BackupAccountControl.h b/lib/backupstore/BackupAccountControl.h index f05bc730d..9b33b3b62 100644 --- a/lib/backupstore/BackupAccountControl.h +++ b/lib/backupstore/BackupAccountControl.h @@ -14,13 +14,14 @@ #include "BackupStoreAccountDatabase.h" #include "BackupFileSystem.h" -#include "NamedLock.h" #include "S3Client.h" #include "UnixUser.h" class BackupStoreDirectory; class BackupStoreInfo; class Configuration; +class NamedLock; +class UnixUser; class BackupAccountControl { @@ -29,6 +30,7 @@ class BackupAccountControl bool mMachineReadableOutput; std::auto_ptr mapFileSystem; + virtual void OpenAccount(bool readWrite) = 0; virtual int GetBlockSize() { return mapFileSystem->GetBlockSize(); @@ -45,6 +47,8 @@ class BackupAccountControl int64_t SizeStringToBlocks(const char *string, int BlockSize); std::string BlockSizeToString(int64_t Blocks, int64_t MaxBlocks, int BlockSize); int PrintAccountInfo(const BackupStoreInfo& info, int BlockSize); + int CreateAccount(int32_t AccountID, int32_t SoftLimit, int32_t HardLimit, + const std::string& AccountName); }; @@ -56,6 +60,8 @@ class BackupStoreAccountControl : public BackupAccountControl int mDiscSetNum; std::auto_ptr mapChangeUser; // used to reset uid when we return + virtual void OpenAccount(bool readWrite); + public: BackupStoreAccountControl(const Configuration& config, int32_t AccountID, bool machineReadableOutput = false) @@ -68,7 +74,6 @@ class BackupStoreAccountControl : public BackupAccountControl return BlockSizeOfDiscSet(mDiscSetNum); } int BlockSizeOfDiscSet(int discSetNum); - bool OpenAccount(NamedLock* pLock); int SetLimit(const char *SoftLimitStr, const char *HardLimitStr); int SetAccountName(const std::string& rNewAccountName); @@ -88,6 +93,8 @@ class S3BackupAccountControl : public BackupAccountControl std::auto_ptr mapS3Client; // mapFileSystem is inherited from BackupAccountControl + virtual void OpenAccount(bool readWrite) { } + public: S3BackupAccountControl(const Configuration& config, bool machineReadableOutput = false); diff --git a/lib/backupstore/BackupFileSystem.h b/lib/backupstore/BackupFileSystem.h index a67a4f29a..5e62aef89 100644 --- a/lib/backupstore/BackupFileSystem.h +++ b/lib/backupstore/BackupFileSystem.h @@ -63,6 +63,14 @@ class BackupFileSystem virtual BackupStoreInfo& GetBackupStoreInfo(bool ReadOnly, bool Refresh = false); virtual void PutBackupStoreInfo(BackupStoreInfo& rInfo) = 0; + virtual std::auto_ptr GetBackupStoreInfoUncached() + { + // Return a BackupStoreInfo freshly retrieved from storage, read-only to + // prevent accidentally making changes to this copy, which can't be saved + // back to the BackupFileSystem. + return GetBackupStoreInfoInternal(true); // ReadOnly + } + // GetPotentialRefCountDatabase() returns the current potential database if it // has been already obtained and not closed, otherwise creates a new one. // This same database will never be returned by both this function and diff --git a/lib/backupstore/BackupStoreAccounts.cpp b/lib/backupstore/BackupStoreAccounts.cpp index 462a11adc..856c7a620 100644 --- a/lib/backupstore/BackupStoreAccounts.cpp +++ b/lib/backupstore/BackupStoreAccounts.cpp @@ -59,74 +59,6 @@ BackupStoreAccounts::~BackupStoreAccounts() } - -// -------------------------------------------------------------------------- -// -// Function -// Name: BackupStoreAccounts::Create(int32_t, int, int64_t, int64_t, const std::string &) -// Purpose: Create a new account on the specified disc set. -// If rAsUsername is not empty, then the account information will be written under the -// username specified. -// Created: 2003/08/21 -// -// -------------------------------------------------------------------------- -void BackupStoreAccounts::Create(int32_t ID, int DiscSet, int64_t SizeSoftLimit, int64_t SizeHardLimit, const std::string &rAsUsername) -{ - // Create the entry in the database - BackupStoreAccountDatabase::Entry Entry(mrDatabase.AddEntry(ID, - DiscSet)); - - { - // Become the user specified in the config file? - std::auto_ptr user; - if(!rAsUsername.empty()) - { - // Username specified, change... - user.reset(new UnixUser(rAsUsername.c_str())); - user->ChangeProcessUser(true /* temporary */); - // Change will be undone at the end of this function - } - - // Get directory name - std::string dirName(MakeAccountRootDir(ID, DiscSet)); - - // Create a directory on disc - RaidFileWrite::CreateDirectory(DiscSet, dirName, true /* recursive */); - - // Create an info file - BackupStoreInfo::CreateNew(ID, dirName, DiscSet, SizeSoftLimit, SizeHardLimit); - - // And an empty directory - BackupStoreDirectory rootDir(BACKUPSTORE_ROOT_DIRECTORY_ID, BACKUPSTORE_ROOT_DIRECTORY_ID); - int64_t rootDirSize = 0; - // Write it, knowing the directory scheme - { - RaidFileWrite rf(DiscSet, dirName + "o01"); - rf.Open(); - rootDir.WriteToStream(rf); - rootDirSize = rf.GetDiscUsageInBlocks(); - rf.Commit(true); - } - - // Update the store info to reflect the size of the root directory - std::auto_ptr info(BackupStoreInfo::Load(ID, dirName, DiscSet, false /* ReadWrite */)); - info->ChangeBlocksUsed(rootDirSize); - info->ChangeBlocksInDirectories(rootDirSize); - info->AdjustNumDirectories(1); - - // Save it back - info->Save(); - - // Create the refcount database - BackupStoreRefCountDatabase::Create(Entry)->Commit(); - } - - // As the original user... - // Write the database back - mrDatabase.Write(); -} - - // -------------------------------------------------------------------------- // // Function diff --git a/lib/backupstore/BackupStoreAccounts.h b/lib/backupstore/BackupStoreAccounts.h index 7c7e7c0d1..0e3948e15 100644 --- a/lib/backupstore/BackupStoreAccounts.h +++ b/lib/backupstore/BackupStoreAccounts.h @@ -33,9 +33,6 @@ class BackupStoreAccounts BackupStoreAccounts(const BackupStoreAccounts &rToCopy); public: - void Create(int32_t ID, int DiscSet, int64_t SizeSoftLimit, - int64_t SizeHardLimit, const std::string &rAsUsername); - bool AccountExists(int32_t ID); void GetAccountRoot(int32_t ID, std::string &rRootDirOut, int &rDiscSetOut) const; static std::string GetAccountRoot(const diff --git a/lib/backupstore/BackupStoreException.txt b/lib/backupstore/BackupStoreException.txt index 21f02f886..c64c99526 100644 --- a/lib/backupstore/BackupStoreException.txt +++ b/lib/backupstore/BackupStoreException.txt @@ -77,5 +77,6 @@ InvalidEtagHeader 73 The S3 HTTP response contain a malformed or unexpected ETa FileSystemNotInitialised 74 No BackupFileSystem has been configured for this account ObjectDoesNotExist 75 The specified object ID does not exist in the store, or is not of this type AccountAlreadyExists 76 Tried to create an account that already exists -FailedToCreateTemporaryFile 77 Failed to create a temporary file -BadConfiguration 78 The configuration file contains an error or invalid value +AccountDoesNotExist 77 Tried to open an account that does not exist +FailedToCreateTemporaryFile 78 Failed to create a temporary file +BadConfiguration 79 The configuration file contains an error or invalid value diff --git a/lib/backupstore/BackupStoreInfo.cpp b/lib/backupstore/BackupStoreInfo.cpp index efe3f7bb9..6d97c79be 100644 --- a/lib/backupstore/BackupStoreInfo.cpp +++ b/lib/backupstore/BackupStoreInfo.cpp @@ -81,7 +81,7 @@ void BackupStoreInfo::CreateNew(int32_t AccountID, const std::string &rRootDir, info.mAccountID = AccountID; info.mDiscSet = DiscSet; info.mReadOnly = false; - info.mLastObjectIDUsed = 1; + info.mLastObjectIDUsed = 0; info.mBlocksSoftLimit = BlockSoftLimit; info.mBlocksHardLimit = BlockHardLimit; @@ -119,6 +119,7 @@ BackupStoreInfo::BackupStoreInfo(int32_t AccountID, const std::string &FileName, mExtraData.SetForReading(); // extra data is empty in this case } + // -------------------------------------------------------------------------- // // Function diff --git a/lib/backupstore/BackupStoreInfo.h b/lib/backupstore/BackupStoreInfo.h index 0576919b5..8d2695845 100644 --- a/lib/backupstore/BackupStoreInfo.h +++ b/lib/backupstore/BackupStoreInfo.h @@ -70,11 +70,7 @@ END_STRUCTURE_PACKING_FOR_WIRE class BackupStoreInfo { friend class BackupStoreCheck; -public: - ~BackupStoreInfo(); -private: - // Creation through static functions only - BackupStoreInfo(); + // No copying allowed BackupStoreInfo(const BackupStoreInfo &); @@ -82,14 +78,22 @@ class BackupStoreInfo // Create a New account, saving a blank info object to the disc static void CreateNew(int32_t AccountID, const std::string &rRootDir, int DiscSet, int64_t BlockSoftLimit, int64_t BlockHardLimit); - BackupStoreInfo(int32_t AccountID, const std::string &FileName, - int64_t BlockSoftLimit, int64_t BlockHardLimit); +private: + +protected: + BackupStoreInfo(); + +public: // Load it from the store // TODO FIXME: remove this RaidFile version of Load(), let BackupFileSystem // handle the RaidFile part. static std::auto_ptr Load(int32_t AccountID, const std::string &rRootDir, int DiscSet, bool ReadOnly, int64_t *pRevisionID = 0); + BackupStoreInfo(int32_t AccountID, const std::string &FileName, + int64_t BlockSoftLimit, int64_t BlockHardLimit); + ~BackupStoreInfo(); + // Load it from a stream (file or RaidFile) static std::auto_ptr Load(IOStream& rStream, const std::string FileName, bool ReadOnly);