Skip to content

Commit

Permalink
Fix failing test_backup_hardlinked_files
Browse files Browse the repository at this point in the history 
Remove assertion that inode numbers were not reused, which was incompatible
with hard links and violated under certain circumstances. Add a test that
reproduces those circumstances.
  • Loading branch information
@qris
qris committed Dec 27, 2018
1 parent ee9cb6e commit 2b26eb1
Show file tree
Hide file tree
Showing 5 changed files with 411 additions and 132 deletions.
199 changes: 110 additions & 89 deletions lib/bbackupd/BackupClientDirectoryRecord.cpp
View file
Expand Up @@ -129,6 +129,60 @@ std::string BackupClientDirectoryRecord::ConvertVssPathToRealPath(
return rVssPath;
}

bool BackupClientDirectoryRecord::ListLocalDirectory(const std::string& local_path,
const std::string& local_path_non_vss, ProgressNotifier& notifier,
std::vector<struct dirent>& entries_out)
{
// read the contents...
DIR *dir_handle = NULL;

try
{
dir_handle = ::opendir(local_path.c_str());
if(dir_handle == 0)
{
// Report the error (logs and eventual email to administrator)
if(errno == EACCES)
{
notifier.NotifyDirListFailed(this, local_path_non_vss,
"Access denied");
}
else
{
notifier.NotifyDirListFailed(this, local_path_non_vss,
strerror(errno));
}

// Ignore this directory for now.
return false;
}

struct dirent *en = 0;

while((en = ::readdir(dir_handle)) != 0)
{
entries_out.push_back(*en);
}

if(::closedir(dir_handle) != 0)
{
THROW_EXCEPTION(CommonException, OSFileError)
}

dir_handle = NULL;
}
catch(...)
{
if(dir_handle != NULL)
{
::closedir(dir_handle);
}
throw;
}

return true;
}

// --------------------------------------------------------------------------
//
// Function
Expand Down Expand Up @@ -242,80 +296,43 @@ void BackupClientDirectoryRecord::SyncDirectory(
// BLOCK
{
// read the contents...
DIR *dirHandle = 0;
try
{
rNotifier.NotifyScanDirectory(this, local_path_non_vss);
rNotifier.NotifyScanDirectory(this, local_path_non_vss);
std::vector<struct dirent> entries;

dirHandle = ::opendir(rLocalPath.c_str());
if(dirHandle == 0)
{
// Report the error (logs and eventual email to administrator)
if (errno == EACCES)
{
rNotifier.NotifyDirListFailed(this, local_path_non_vss,
"Access denied");
}
else
{
rNotifier.NotifyDirListFailed(this, local_path_non_vss,
strerror(errno));
}

SetErrorWhenReadingFilesystemObject(rParams, local_path_non_vss);

// Ignore this directory for now.
return;
}

struct dirent *en = 0;
int num_entries_found = 0;
if(!ListLocalDirectory(rLocalPath, local_path_non_vss, rNotifier, entries))
{
SetErrorWhenReadingFilesystemObject(rParams, local_path_non_vss);

while((en = ::readdir(dirHandle)) != 0)
{
num_entries_found++;
rParams.mrContext.DoKeepAlive();
if(rParams.mpBackgroundTask)
{
rParams.mpBackgroundTask->RunBackgroundTask(
BackgroundTask::Scanning_Dirs,
num_entries_found, 0);
}
// Ignore this directory for now.
return;
}

if (!SyncDirectoryEntry(rParams, rNotifier,
rBackupLocation, rLocalPath,
currentStateChecksum, en, dest_st, dirs,
files, downloadDirectoryRecordBecauseOfFutureFiles))
{
// This entry is not to be backed up.
continue;
}
}

if(::closedir(dirHandle) != 0)
int num_entries_found = 0;
for(struct dirent& entry : entries)
{
num_entries_found++;
rParams.mrContext.DoKeepAlive();
if(rParams.mpBackgroundTask)
{
THROW_EXCEPTION(CommonException, OSFileError)
rParams.mpBackgroundTask->RunBackgroundTask(
BackgroundTask::Scanning_Dirs,
num_entries_found, 0);
}
dirHandle = 0;
}
catch(...)
{
if(dirHandle != 0)

if(!SyncDirectoryEntry(rParams, rNotifier,
rBackupLocation, rLocalPath,
currentStateChecksum, &entry, dest_st, dirs,
files, downloadDirectoryRecordBecauseOfFutureFiles))
{
::closedir(dirHandle);
// This entry is not to be backed up.
continue;
}
throw;
}
}

// Finish off the checksum, and compare with the one currently stored
bool checksumDifferent = true;
currentStateChecksum.Finish();
if(mInitialSyncDone && currentStateChecksum.DigestMatches(mStateChecksum))
{
// The checksum is the same, and there was one to compare with
checksumDifferent = false;
}
bool checksumDifferent = ChecksumIsDifferent(currentStateChecksum);

// Pointer to potentially downloaded store directory info
std::auto_ptr<BackupStoreDirectory> apDirOnStore;
Expand Down Expand Up @@ -1254,32 +1271,37 @@ bool BackupClientDirectoryRecord::UpdateItems(
// Look it up in the current map, and if it's there, use that.
const BackupClientInodeToIDMap &currentIDMap(rContext.GetCurrentIDMap());
int64_t objid = 0, dirid = 0;
if(currentIDMap.Lookup(inodeNum, objid, dirid))
if(currentIDMap.Lookup(inodeNum, objid, dirid) &&
dirid == mObjectID)
{
// Found
if(dirid != mObjectID)
{
BOX_WARNING("Found conflicting parent ID for "
"file ID " << inodeNum << " (" <<
nonVssFilePath << "): expected " <<
mObjectID << " but found " << dirid <<
" (same directory used in two different "
"locations?)");
}

ASSERT(dirid == mObjectID);

// NOTE: If the above assert fails, an inode number has been reused by the OS,
// or there is a problem somewhere. If this happened on a short test run, look
// into it. However, in a long running process this may happen occasionally and
// not indicate anything wrong.
// Run the release version for real life use, where this check is not made.

latestObjectID = objid;
// This could be due to an inode number being reused by the
// OS, or a hard link. We don't particularly care at this
// stage, but we only want to add it to the new ID map once,
// so we don't do it unless the parent directory ID is the
// same as the current directory ID. This could probably be
// improved to avoid losing track of hardlinks that are
// deleted from one location, but without creating false
// renames (or making it not matter, with snapshots).

// TODO: implement real hardlink support.

BOX_TRACE("Copying previous file ID " << inodeNum << " (" <<
nonVssFilePath << ") to new ID map: object ID " <<
BOX_FORMAT_OBJECTID(objid) << " with parent " <<
BOX_FORMAT_OBJECTID(dirid));

idMap.AddToMap(inodeNum, objid, dirid, nonVssFilePath);
}
else
{
// It didn't exist before, but we didn't upload it.
// Hopefully we will as soon as it's old enough.
BOX_TRACE("New file was not uploaded, so not added to new "
"ID map: " << inodeNum << " (" <<
nonVssFilePath << ")");
}
}

if(latestObjectID != 0)
else
{
BOX_TRACE("Storing uploaded file ID " << inodeNum << " (" <<
nonVssFilePath << ") in ID map as object " <<
Expand All @@ -1289,7 +1311,6 @@ bool BackupClientDirectoryRecord::UpdateItems(
mObjectID /* containing directory */,
nonVssFilePath);
}

}

if(fileSynced)
Expand Down Expand Up @@ -1426,7 +1447,7 @@ bool BackupClientDirectoryRecord::UpdateItems(
if(doCreateDirectoryRecord)
{
// New an object for this
psubDirRecord = new BackupClientDirectoryRecord(subDirObjectID, *d);
psubDirRecord = CreateSubdirectoryRecord(subDirObjectID, *d);

// Store in list
try
Expand Down Expand Up @@ -2150,8 +2171,8 @@ void BackupClientDirectoryRecord::Deserialize(Archive & rArchive)
std::string strItem;
rArchive.Read(strItem);

BackupClientDirectoryRecord* pSubDirRecord =
new BackupClientDirectoryRecord(0, "");
BackupClientDirectoryRecord* pSubDirRecord =
new BackupClientDirectoryRecord(0, "");
// will be deserialized anyway, give it id 0 for now

if (!pSubDirRecord)
Expand Down
25 changes: 24 additions & 1 deletion lib/bbackupd/BackupClientDirectoryRecord.h
View file
Expand Up @@ -156,6 +156,7 @@ class BackupClientDirectoryRecord
const Location& rBackupLocation);

int64_t GetObjectID() const { return mObjectID; }
const std::string& GetSubDirName() { return mSubDirName; }

private:
void DeleteSubDirectories();
Expand All @@ -164,7 +165,14 @@ class BackupClientDirectoryRecord
BackupStoreDirectory *pDirOnStore,
const std::string &rLocalPath);

protected: // to allow tests to hook in before UpdateItems() runs
protected:
// This exists only so that it can be overridden in subclasses for testing:
virtual BackupClientDirectoryRecord* CreateSubdirectoryRecord(int64_t remote_dir_id,
const std::string &subdir_name)
{
return new BackupClientDirectoryRecord(remote_dir_id, subdir_name);
}
// To allow tests to hook in before UpdateItems() runs:
virtual bool UpdateItems(SyncParams &rParams,
const std::string &rLocalPath,
const std::string &rRemotePath,
Expand All @@ -173,6 +181,21 @@ class BackupClientDirectoryRecord
std::vector<BackupStoreDirectory::Entry *> &rEntriesLeftOver,
std::vector<std::string> &rFiles,
const std::vector<std::string> &rDirs);
// To allow test_backup_hardlinked_files to workaround implementation differences in whether
// creating a new hardlink, and thus changing the link count, changes the ctime or not:
virtual bool ChecksumIsDifferent(const MD5Digest& new_checksum)
{
return !mInitialSyncDone || !new_checksum.DigestMatches(mStateChecksum);
}
// To allow test_backup_hardlinked_files to find and modify subdirectory records:
virtual std::map<std::string, BackupClientDirectoryRecord *> GetSubDirectories()
{
return mSubDirectories;
}
// To allow test_backup_hardlinked_files to return directory entries in a fixed order:
virtual bool ListLocalDirectory(const std::string& local_path,
const std::string& local_path_non_vss, ProgressNotifier& notifier,
std::vector<struct dirent>& entries_out);

private:
BackupStoreDirectory::Entry* CheckForRename(BackupClientContext& context,
Expand Down
9 changes: 8 additions & 1 deletion lib/bbackupd/BackupDaemon.cpp
View file
Expand Up @@ -2686,7 +2686,8 @@ void BackupDaemon::SetupLocations(BackupClientContext &rClientContext, const Con
if(pLoc->mapDirectoryRecord.get() == NULL)
{
pLoc->mapDirectoryRecord.reset(
new BackupClientDirectoryRecord(existing_remote_dir_id, *pLocName));
CreateLocationRootRecord(existing_remote_dir_id, *pLocName)
);
}

// Read the exclude lists from the Configuration
Expand Down Expand Up @@ -2762,6 +2763,12 @@ void BackupDaemon::SetupLocations(BackupClientContext &rClientContext, const Con
}
}

// This exists only so that it can be overridden in subclasses for testing:
BackupClientDirectoryRecord* BackupDaemon::CreateLocationRootRecord(int64_t remote_dir_id,
const std::string &location_name)
{
return new BackupClientDirectoryRecord(remote_dir_id, location_name);
}

// --------------------------------------------------------------------------
//
Expand Down
6 changes: 5 additions & 1 deletion lib/bbackupd/BackupDaemon.h
View file
Expand Up @@ -152,6 +152,11 @@ public RunStatusProvider, public SysadminNotifier, public BackgroundTask
ProgressNotifier &rProgressNotifier
);

// This exists only so that it can be overridden in subclasses for testing:
virtual BackupClientDirectoryRecord* CreateLocationRootRecord(int64_t remote_dir_id,
const std::string &location_name);
virtual void CommitIDMapsAfterSync();

private:
void DeleteAllLocations();
void SetupLocations(BackupClientContext &rClientContext, const Configuration &rLocationsConf);
Expand All @@ -165,7 +170,6 @@ public RunStatusProvider, public SysadminNotifier, public BackgroundTask
void FillIDMapVector(std::vector<BackupClientInodeToIDMap *> &rVector, bool NewMaps);

void SetupIDMapsForSync();
void CommitIDMapsAfterSync();
void DeleteCorruptBerkelyDbFiles();

void MakeMapBaseName(unsigned int MountNumber, std::string &rNameOut) const;
Expand Down

0 comments on commit 2b26eb1

Please sign in to comment.