Martchus
/
tagparser
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Use `std::filesystem` in backup helper code 

* Fix applying changes to symlinks so that the target is modified in any
  case (and not just if a rewrite isn't necessary)
* Avoid using `std::rename` and `std::remove` because they might not work
  under Windows when the path contains non-ASCII characters
* Simplify code, remove `isRelative()`
This commit is contained in:
Martchus 2021-09-11 23:05:22 +02:00
parent 8a48914bcc
commit e277070e9c
8 changed files with 101 additions and 86 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
CMakeLists.txt
View File

@ -10,7 +10,7 @@ set(META_APP_URL "https://github.com/${META_APP_AUTHOR}/${META_PROJECT_NAME}")
set(META_APP_DESCRIPTION "C++ library for reading and writing MP4 (iTunes), ID3, Vorbis, Opus, FLAC and Matroska tags")
set(META_VERSION_MAJOR 10)
set(META_VERSION_MINOR 2)
set(META_VERSION_PATCH 0)
set(META_VERSION_PATCH 1)
set(META_REQUIRED_CPP_UNIT_VERSION 1.14.0)
set(META_ADD_DEFAULT_CPP_UNIT_TEST_APPLICATION ON)

144
backuphelper.cpp
View File

@ -47,48 +47,39 @@ namespace BackupHelper {
void restoreOriginalFileFromBackupFile(
const std::string &originalPath, const std::string &backupPath, NativeFileStream &originalStream, NativeFileStream &backupStream)
{
// ensure the original stream is closed
if (originalStream.is_open()) {
originalStream.close();
}
// check whether backup file actually exists and close the backup stream afterwards
const auto backupPathForOpen = BasicFileInfo::pathForOpen(backupPath);
// ensure streams are closed but don't handle any errors anymore at this point
originalStream.exceptions(ios_base::goodbit);
backupStream.exceptions(ios_base::goodbit);
originalStream.close();
backupStream.close();
originalStream.clear();
backupStream.clear();
backupStream.open(backupPathForOpen.data(), ios_base::in | ios_base::binary);
if (backupStream.is_open()) {
backupStream.close();
} else {
// restore usual exception handling of the streams
originalStream.exceptions(ios_base::badbit | ios_base::failbit);
backupStream.exceptions(ios_base::badbit | ios_base::failbit);
// check whether backup file actually exists and close the backup stream afterwards
const auto originalPathForOpen = std::filesystem::path(BasicFileInfo::pathForOpen(originalPath));
const auto backupPathForOpen = std::filesystem::path(BasicFileInfo::pathForOpen(backupPath));
auto ec = std::error_code();
if (!std::filesystem::exists(backupPathForOpen, ec) && !ec) {
throw std::ios_base::failure("Backup/temporary file has not been created.");
}
// remove original file and restore backup
const auto originalPathForOpen = BasicFileInfo::pathForOpen(originalPath);
std::remove(originalPathForOpen.data());
if (std::rename(backupPathForOpen.data(), originalPathForOpen.data()) == 0) {
return;
}
// can't rename/move the file (maybe backup dir on another partition) -> make a copy instead
try {
// need to open all streams again
backupStream.exceptions(ios_base::failbit | ios_base::badbit);
originalStream.exceptions(ios_base::failbit | ios_base::badbit);
backupStream.open(backupPathForOpen.data(), ios_base::in | ios_base::binary);
originalStream.open(originalPathForOpen.data(), ios_base::out | ios_base::binary);
originalStream << backupStream.rdbuf();
originalStream.flush();
// TODO: callback for progress updates
} catch (const std::ios_base::failure &failure) {
throw std::ios_base::failure("Unable to restore original file from backup file \"" % backupPath % "\" after failure: " + failure.what());
}
}
/*!
* \brief Returns whether the specified \a path is relative.
*/
static bool isRelative(const std::string &path)
{
return path.empty() || (path.front() != '/' && (path.size() < 2 || path[1] != ':'));
// remove original file and restore backup
std::filesystem::remove(originalPath, ec);
if (ec) {
throw std::ios_base::failure("Unable to remove original file: " + ec.message());
}
std::filesystem::rename(backupPathForOpen, originalPathForOpen, ec);
if (ec) {
// try making a copy instead, maybe backup dir is on another partition
std::filesystem::copy_file(backupPathForOpen, originalPathForOpen, ec);
}
if (ec) {
throw std::ios_base::failure("Unable to restore original file from backup file \"" % backupPath % "\" after failure: " + ec.message());
}
}
/*!
@ -120,10 +111,11 @@ void createBackupFile(const std::string &backupDir, const std::string &originalP
NativeFileStream &backupStream)
{
// determine dirs
const auto backupDirRelative(isRelative(backupDir));
const auto originalDir(backupDirRelative ? BasicFileInfo::containingDirectory(originalPath) : string());
const auto backupDirRelative = std::filesystem::path(backupDir).is_relative();
const auto originalDir = backupDirRelative ? BasicFileInfo::containingDirectory(originalPath) : string();
// determine the backup path
auto ec = std::error_code();
for (unsigned int i = 0;; ++i) {
if (backupDir.empty()) {
if (i) {
@ -150,7 +142,7 @@ void createBackupFile(const std::string &backupDir, const std::string &originalP
}
// test whether the backup path is still unused; otherwise continue loop
if (auto ec = std::error_code(); !std::filesystem::exists(BasicFileInfo::pathForOpen(backupPath), ec)) {
if (!std::filesystem::exists(BasicFileInfo::pathForOpen(backupPath), ec)) {
break;
}
}
@ -161,26 +153,15 @@ void createBackupFile(const std::string &backupDir, const std::string &originalP
}
// rename original file
if (std::rename(BasicFileInfo::pathForOpen(originalPath).data(), BasicFileInfo::pathForOpen(backupPath).data())) {
// can't rename/move the file (maybe backup dir on another partition) -> make a copy instead
try {
backupStream.exceptions(ios_base::failbit | ios_base::badbit);
originalStream.exceptions(ios_base::failbit | ios_base::badbit);
// ensure backupStream is opened as write-only
if (backupStream.is_open()) {
backupStream.close();
}
backupStream.open(BasicFileInfo::pathForOpen(backupPath).data(), ios_base::out | ios_base::binary);
// ensure originalStream is opened with read permissions
originalStream.open(BasicFileInfo::pathForOpen(originalPath).data(), ios_base::in | ios_base::binary);
// do the actual copying
backupStream << originalStream.rdbuf();
backupStream.flush();
// streams are closed in the next try-block
// TODO: callback for progress updates
} catch (const std::ios_base::failure &failure) {
throw std::ios_base::failure(argsToString("Unable to rename original file before rewriting it: ", failure.what()));
}
const auto backupPathForOpen = BasicFileInfo::pathForOpen(backupPath);
std::filesystem::rename(originalPath, backupPathForOpen, ec);
if (ec) {
// try making a copy instead, maybe backup dir is on another partition
std::filesystem::copy_file(originalPath, backupPathForOpen, ec);
}
if (ec) {
throw std::ios_base::failure(
argsToString("Unable to create backup file \"", backupPathForOpen, "\" of \"", originalPath, "\" before rewriting it: " + ec.message()));
}
// manage streams
@ -197,16 +178,34 @@ void createBackupFile(const std::string &backupDir, const std::string &originalP
backupStream.exceptions(ios_base::failbit | ios_base::badbit);
backupStream.open(BasicFileInfo::pathForOpen(backupPath).data(), ios_base::in | ios_base::binary);
} catch (const std::ios_base::failure &failure) {
// can't open the new file
// -> try to re-rename backup file in the error case to restore previous state
if (std::rename(BasicFileInfo::pathForOpen(backupPath).data(), BasicFileInfo::pathForOpen(originalPath).data())) {
// try to restore the previous state in the error case
try {
restoreOriginalFileFromBackupFile(originalPath, backupPath, originalStream, backupStream);
} catch (const std::ios_base::failure &) {
throw std::ios_base::failure("Unable to restore original file from backup file \"" % backupPath % "\" after failure: " + failure.what());
} else {
throw std::ios_base::failure(argsToString("Unable to open backup file: ", failure.what()));
}
throw std::ios_base::failure(argsToString("Unable to open backup file: ", failure.what()));
}
}
/*!
* \brief Creates a backup file like createBackupFile() but canonicalizes \a originalPath before doing the backup.
* \remarks
* - This function sets \a originalPath to be a canonical path.
* - Using this function (instead of createBackupFile()) is recommended so the actual file is being altered.
*/
void createBackupFileCanonical(const std::string &backupDir, std::string &originalPath, std::string &backupPath,
CppUtilities::NativeFileStream &originalStream, CppUtilities::NativeFileStream &backupStream)
{
auto ec = std::error_code();
if (const auto canonicalPath = std::filesystem::canonical(BasicFileInfo::pathForOpen(originalPath), ec); !ec) {
originalPath = canonicalPath.string();
} else {
throw std::ios_base::failure("Unable to canonicalize path of original file before rewriting it: " + ec.message());
}
createBackupFile(backupDir, originalPath, backupPath, originalStream, backupStream);
}
/*!
* \brief Handles a failure/abort which occurred after the file has been modified.
*
@ -228,6 +227,17 @@ void createBackupFile(const std::string &backupDir, const std::string &originalP
*/
void handleFailureAfterFileModified(MediaFileInfo &fileInfo, const std::string &backupPath, NativeFileStream &outputStream,
NativeFileStream &backupStream, Diagnostics &diag, const std::string &context)
{
handleFailureAfterFileModifiedCanonical(fileInfo, fileInfo.path(), backupPath, outputStream, backupStream, diag, context);
}
/*!
* \brief Handles a failure/abort which occurred after the file has been modified.
* \remarks Same as handleFailureAfterFileModified() but allows specifying the original path instead of just using the
* path from \a mediaFileInfo.
*/
void handleFailureAfterFileModifiedCanonical(MediaFileInfo &fileInfo, const std::string &originalPath, const std::string &backupPath,
CppUtilities::NativeFileStream &outputStream, CppUtilities::NativeFileStream &backupStream, Diagnostics &diag, const std::string &context)
{
// reset the associated container in any case
if (fileInfo.container()) {
@ -242,7 +252,7 @@ void handleFailureAfterFileModified(MediaFileInfo &fileInfo, const std::string &
// a temp/backup file has been created -> restore original file
diag.emplace_back(DiagLevel::Information, "Rewriting the file to apply changed tag information has been aborted.", context);
try {
restoreOriginalFileFromBackupFile(fileInfo.path(), backupPath, outputStream, backupStream);
restoreOriginalFileFromBackupFile(originalPath, backupPath, outputStream, backupStream);
diag.emplace_back(DiagLevel::Warning, "The original file has been restored.", context);
} catch (const std::ios_base::failure &failure) {
diag.emplace_back(DiagLevel::Critical, argsToString("The original file could not be restored: ", failure.what()), context);
@ -257,7 +267,7 @@ void handleFailureAfterFileModified(MediaFileInfo &fileInfo, const std::string &
// a temp/backup file has been created -> restore original file
diag.emplace_back(DiagLevel::Critical, "Rewriting the file to apply changed tag information failed.", context);
try {
restoreOriginalFileFromBackupFile(fileInfo.path(), backupPath, outputStream, backupStream);
restoreOriginalFileFromBackupFile(originalPath, backupPath, outputStream, backupStream);
diag.emplace_back(DiagLevel::Warning, "The original file has been restored.", context);
} catch (const std::ios_base::failure &failure) {
diag.emplace_back(DiagLevel::Critical, argsToString("The original file could not be restored: ", failure.what()), context);
@ -272,7 +282,7 @@ void handleFailureAfterFileModified(MediaFileInfo &fileInfo, const std::string &
// a temp/backup file has been created -> restore original file
diag.emplace_back(DiagLevel::Critical, "An IO error occurred when rewriting the file to apply changed tag information.", context);
try {
restoreOriginalFileFromBackupFile(fileInfo.path(), backupPath, outputStream, backupStream);
restoreOriginalFileFromBackupFile(originalPath, backupPath, outputStream, backupStream);
diag.emplace_back(DiagLevel::Warning, "The original file has been restored.", context);
} catch (const std::ios_base::failure &failure) {
diag.emplace_back(DiagLevel::Critical, argsToString("The original file could not be restored: ", failure.what()), context);

7
backuphelper.h
View File

@ -16,9 +16,14 @@ TAG_PARSER_EXPORT void restoreOriginalFileFromBackupFile(const std::string &orig
CppUtilities::NativeFileStream &originalStream, CppUtilities::NativeFileStream &backupStream);
TAG_PARSER_EXPORT void createBackupFile(const std::string &backupDir, const std::string &originalPath, std::string &backupPath,
CppUtilities::NativeFileStream &originalStream, CppUtilities::NativeFileStream &backupStream);
TAG_PARSER_EXPORT void handleFailureAfterFileModified(MediaFileInfo &mediaFileInfo, const std::string &backupPath,
TAG_PARSER_EXPORT void createBackupFileCanonical(const std::string &backupDir, std::string &originalPath, std::string &backupPath,
CppUtilities::NativeFileStream &originalStream, CppUtilities::NativeFileStream &backupStream);
TAG_PARSER_EXPORT void handleFailureAfterFileModified(MediaFileInfo &fileInfo, const std::string &backupPath,
CppUtilities::NativeFileStream &outputStream, CppUtilities::NativeFileStream &backupStream, Diagnostics &diag,
const std::string &context = "making file");
TAG_PARSER_EXPORT void handleFailureAfterFileModifiedCanonical(MediaFileInfo &fileInfo, const std::string &originalPath,
const std::string &backupPath, CppUtilities::NativeFileStream &outputStream, CppUtilities::NativeFileStream &backupStream, Diagnostics &diag,
const std::string &context = "making file");
} // namespace BackupHelper

8
matroska/matroskacontainer.cpp
View File

@ -1503,7 +1503,7 @@ void MatroskaContainer::internalMakeFile(Diagnostics &diag, AbortableProgressFee
progress.nextStepOrStop("Preparing streams ...");
// -> define variables needed to handle output stream and backup stream (required when rewriting the file)
string backupPath;
string originalPath = fileInfo().path(), backupPath;
NativeFileStream &outputStream = fileInfo().stream();
NativeFileStream backupStream; // create a stream to open the backup/original file for the case rewriting the file is required
BinaryWriter outputWriter(&outputStream);
@ -1513,9 +1513,9 @@ void MatroskaContainer::internalMakeFile(Diagnostics &diag, AbortableProgressFee
if (fileInfo().saveFilePath().empty()) {
// move current file to temp dir and reopen it as backupStream, recreate original file
try {
BackupHelper::createBackupFile(fileInfo().backupDirectory(), fileInfo().path(), backupPath, outputStream, backupStream);
BackupHelper::createBackupFileCanonical(fileInfo().backupDirectory(), originalPath, backupPath, outputStream, backupStream);
// recreate original file, define buffer variables
outputStream.open(BasicFileInfo::pathForOpen(fileInfo().path()).data(), ios_base::out | ios_base::binary | ios_base::trunc);
outputStream.open(originalPath, ios_base::out | ios_base::binary | ios_base::trunc);
} catch (const std::ios_base::failure &failure) {
diag.emplace_back(
DiagLevel::Critical, argsToString("Creation of temporary file (to rewrite the original file) failed: ", failure.what()), context);
@ -1874,7 +1874,7 @@ void MatroskaContainer::internalMakeFile(Diagnostics &diag, AbortableProgressFee
// handle errors (which might have been occurred after renaming/creating backup file)
} catch (...) {
BackupHelper::handleFailureAfterFileModified(fileInfo(), backupPath, outputStream, backupStream, diag, context);
BackupHelper::handleFailureAfterFileModifiedCanonical(fileInfo(), originalPath, backupPath, outputStream, backupStream, diag, context);
}
}

8
mediafileinfo.cpp
View File

@ -1701,7 +1701,7 @@ void MediaFileInfo::makeMp3File(Diagnostics &diag, AbortableProgressFeedback &pr
// setup stream(s) for writing
// -> define variables needed to handle output stream and backup stream (required when rewriting the file)
string backupPath;
string originalPath = path(), backupPath;
NativeFileStream &outputStream = stream();
NativeFileStream backupStream; // create a stream to open the backup/original file for the case rewriting the file is required
@ -1709,9 +1709,9 @@ void MediaFileInfo::makeMp3File(Diagnostics &diag, AbortableProgressFeedback &pr
if (m_saveFilePath.empty()) {
// move current file to temp dir and reopen it as backupStream, recreate original file
try {
BackupHelper::createBackupFile(backupDirectory(), path(), backupPath, outputStream, backupStream);
BackupHelper::createBackupFileCanonical(backupDirectory(), originalPath, backupPath, outputStream, backupStream);
// recreate original file, define buffer variables
outputStream.open(BasicFileInfo::pathForOpen(path()).data(), ios_base::out | ios_base::binary | ios_base::trunc);
outputStream.open(originalPath, ios_base::out | ios_base::binary | ios_base::trunc);
} catch (const std::ios_base::failure &failure) {
diag.emplace_back(
DiagLevel::Critical, argsToString("Creation of temporary file (to rewrite the original file) failed: ", failure.what()), context);
@ -1855,7 +1855,7 @@ void MediaFileInfo::makeMp3File(Diagnostics &diag, AbortableProgressFeedback &pr
}
} catch (...) {
BackupHelper::handleFailureAfterFileModified(*this, backupPath, outputStream, backupStream, diag, context);
BackupHelper::handleFailureAfterFileModifiedCanonical(*this, originalPath, backupPath, outputStream, backupStream, diag, context);
}
}

8
mp4/mp4container.cpp
View File

@ -506,7 +506,7 @@ calculatePadding:
progress.nextStepOrStop("Preparing streams ...");
// -> define variables needed to handle output stream and backup stream (required when rewriting the file)
string backupPath;
string originalPath = fileInfo().path(), backupPath;
NativeFileStream &outputStream = fileInfo().stream();
NativeFileStream backupStream; // create a stream to open the backup/original file for the case rewriting the file is required
BinaryWriter outputWriter(&outputStream);
@ -515,9 +515,9 @@ calculatePadding:
if (fileInfo().saveFilePath().empty()) {
// move current file to temp dir and reopen it as backupStream, recreate original file
try {
BackupHelper::createBackupFile(fileInfo().backupDirectory(), fileInfo().path(), backupPath, outputStream, backupStream);
BackupHelper::createBackupFileCanonical(fileInfo().backupDirectory(), originalPath, backupPath, outputStream, backupStream);
// recreate original file, define buffer variables
outputStream.open(BasicFileInfo::pathForOpen(fileInfo().path()).data(), ios_base::out | ios_base::binary | ios_base::trunc);
outputStream.open(originalPath, ios_base::out | ios_base::binary | ios_base::trunc);
} catch (const std::ios_base::failure &failure) {
diag.emplace_back(
DiagLevel::Critical, argsToString("Creation of temporary file (to rewrite the original file) failed: ", failure.what()), context);
@ -889,7 +889,7 @@ calculatePadding:
// handle errors (which might have been occurred after renaming/creating backup file)
} catch (...) {
BackupHelper::handleFailureAfterFileModified(fileInfo(), backupPath, outputStream, backupStream, diag, context);
BackupHelper::handleFailureAfterFileModifiedCanonical(fileInfo(), originalPath, backupPath, outputStream, backupStream, diag, context);
}
}

8
ogg/oggcontainer.cpp
View File

@ -388,15 +388,15 @@ void OggContainer::internalMakeFile(Diagnostics &diag, AbortableProgressFeedback
const string context("making OGG file");
progress.nextStepOrStop("Prepare for rewriting OGG file ...");
parseTags(diag, progress); // tags need to be parsed before the file can be rewritten
string backupPath;
string originalPath = fileInfo().path(), backupPath;
NativeFileStream backupStream;
if (fileInfo().saveFilePath().empty()) {
// move current file to temp dir and reopen it as backupStream, recreate original file
try {
BackupHelper::createBackupFile(fileInfo().backupDirectory(), fileInfo().path(), backupPath, fileInfo().stream(), backupStream);
BackupHelper::createBackupFileCanonical(fileInfo().backupDirectory(), originalPath, backupPath, fileInfo().stream(), backupStream);
// recreate original file, define buffer variables
fileInfo().stream().open(BasicFileInfo::pathForOpen(fileInfo().path()).data(), ios_base::out | ios_base::binary | ios_base::trunc);
fileInfo().stream().open(originalPath, ios_base::out | ios_base::binary | ios_base::trunc);
} catch (const std::ios_base::failure &failure) {
diag.emplace_back(
DiagLevel::Critical, argsToString("Creation of temporary file (to rewrite the original file) failed: ", failure.what()), context);
@ -658,7 +658,7 @@ void OggContainer::internalMakeFile(Diagnostics &diag, AbortableProgressFeedback
} catch (...) {
m_iterator.setStream(fileInfo().stream());
BackupHelper::handleFailureAfterFileModified(fileInfo(), backupPath, fileInfo().stream(), backupStream, diag, context);
BackupHelper::handleFailureAfterFileModifiedCanonical(fileInfo(), originalPath, backupPath, fileInfo().stream(), backupStream, diag, context);
}
}

2
tests/utils.cpp
View File

@ -325,7 +325,7 @@ void UtilitiesTests::testBackupFile()
createBackupFile("bak", file.path(), backupPath2, file.stream(), backupStream2);
CPPUNIT_FAIL("renaming failed because backup dir does not exist");
} catch (const std::ios_base::failure &failure) {
TESTUTILS_ASSERT_LIKE("renaming error", "Unable to rename original file before rewriting it: .*"s, string(failure.what()));
TESTUTILS_ASSERT_LIKE("renaming error", "Unable to create backup file .* of .* before rewriting it: .*"s, string(failure.what()));
}
backupStream2.clear();
workingCopyPath("bak/unsupported.bin", WorkingCopyMode::NoCopy);