diff --git a/CMakeLists.txt b/CMakeLists.txt index 811456f..8129980 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -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) diff --git a/backuphelper.cpp b/backuphelper.cpp index 294caad..3c35c0f 100644 --- a/backuphelper.cpp +++ b/backuphelper.cpp @@ -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); diff --git a/backuphelper.h b/backuphelper.h index 9969803..d109a06 100644 --- a/backuphelper.h +++ b/backuphelper.h @@ -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 diff --git a/matroska/matroskacontainer.cpp b/matroska/matroskacontainer.cpp index 1b189b0..457ade4 100644 --- a/matroska/matroskacontainer.cpp +++ b/matroska/matroskacontainer.cpp @@ -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); } } diff --git a/mediafileinfo.cpp b/mediafileinfo.cpp index a33d674..ec4e3d4 100644 --- a/mediafileinfo.cpp +++ b/mediafileinfo.cpp @@ -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); } } diff --git a/mp4/mp4container.cpp b/mp4/mp4container.cpp index d02b31e..5504dba 100644 --- a/mp4/mp4container.cpp +++ b/mp4/mp4container.cpp @@ -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); } } diff --git a/ogg/oggcontainer.cpp b/ogg/oggcontainer.cpp index f69e53c..e256fea 100644 --- a/ogg/oggcontainer.cpp +++ b/ogg/oggcontainer.cpp @@ -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); } } diff --git a/tests/utils.cpp b/tests/utils.cpp index 0e1f403..4efa3b9 100644 --- a/tests/utils.cpp +++ b/tests/utils.cpp @@ -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);