From 7043c3d2a93b0b5783fd7e2d0cfd1fb580094669 Mon Sep 17 00:00:00 2001 From: Martchus Date: Sun, 15 Dec 2019 19:43:16 +0100 Subject: [PATCH] Don't suppress IO errors when writing files * Close or flush streams explicitely so writing is not deferred * to catch errors in the right place * to avoid suppressing errors completely when writing would be deferred to the destructor invocation * Improve comments --- CMakeLists.txt | 2 +- backuphelper.cpp | 29 ++++++++++++++++------------- matroska/matroskacontainer.cpp | 2 +- mediafileinfo.cpp | 12 +++++++++--- mp4/mp4container.cpp | 2 +- ogg/oggcontainer.cpp | 3 +++ 6 files changed, 31 insertions(+), 19 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3560eb4..62e5285 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 9) set(META_VERSION_MINOR 1) -set(META_VERSION_PATCH 1) +set(META_VERSION_PATCH 2) 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 76e9e5d..8cbe541 100644 --- a/backuphelper.cpp +++ b/backuphelper.cpp @@ -68,19 +68,21 @@ void restoreOriginalFileFromBackupFile( } // remove original file and restore backup std::remove(originalPath.c_str()); - if (std::rename(BasicFileInfo::pathForOpen(backupPath), BasicFileInfo::pathForOpen(originalPath))) { - // 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(backupPath, ios_base::in | ios_base::binary); - originalStream.open(originalPath, ios_base::out | ios_base::binary); - originalStream << backupStream.rdbuf(); - // 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()); - } + if (std::rename(BasicFileInfo::pathForOpen(backupPath), BasicFileInfo::pathForOpen(originalPath)) == 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(backupPath, ios_base::in | ios_base::binary); + originalStream.open(originalPath, 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()); } } @@ -181,6 +183,7 @@ void createBackupFile(const std::string &backupDir, const std::string &originalP originalStream.open(BasicFileInfo::pathForOpen(originalPath), 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) { diff --git a/matroska/matroskacontainer.cpp b/matroska/matroskacontainer.cpp index 450a991..543aa34 100644 --- a/matroska/matroskacontainer.cpp +++ b/matroska/matroskacontainer.cpp @@ -1845,7 +1845,7 @@ void MatroskaContainer::internalMakeFile(Diagnostics &diag, AbortableProgressFee } } - // flush output stream + // prevent deferring final write operations (to catch and handle possible errors here) outputStream.flush(); // handle errors (which might have been occurred after renaming/creating backup file) diff --git a/mediafileinfo.cpp b/mediafileinfo.cpp index 30c032c..d47b67c 100644 --- a/mediafileinfo.cpp +++ b/mediafileinfo.cpp @@ -1544,6 +1544,8 @@ void MediaFileInfo::makeMp3File(Diagnostics &diag, AbortableProgressFeedback &pr diag.emplace_back(DiagLevel::Warning, "Unable to write ID3v1 tag.", context); } } + // prevent deferring final write operations (to catch and handle possible errors here) + stream().flush(); } return; } @@ -1747,13 +1749,14 @@ void MediaFileInfo::makeMp3File(Diagnostics &diag, AbortableProgressFeedback &pr reportPathChanged(saveFilePath()); m_saveFilePath.clear(); } - // stream is useless for further usage because it is write-only + // prevent deferring final write operations (to catch and handle possible errors here); stream is useless for further + // usage anyways because it is write-only outputStream.close(); } else { const auto newSize = static_cast(outputStream.tellp()); if (newSize < size()) { // file is smaller after the modification -> truncate - // -> close stream before truncating + // -> prevent deferring final write operations outputStream.close(); // -> truncate file if (truncate(BasicFileInfo::pathForOpen(path()), static_cast(newSize)) == 0) { @@ -1762,7 +1765,10 @@ void MediaFileInfo::makeMp3File(Diagnostics &diag, AbortableProgressFeedback &pr diag.emplace_back(DiagLevel::Critical, "Unable to truncate the file.", context); } } else { - // file is longer after the modification -> just report new size + // file is longer after the modification + // -> prevent deferring final write operations (to catch and handle possible errors here) + outputStream.flush(); + // -> report new size reportSizeChanged(newSize); } } diff --git a/mp4/mp4container.cpp b/mp4/mp4container.cpp index 3dfa934..c893900 100644 --- a/mp4/mp4container.cpp +++ b/mp4/mp4container.cpp @@ -881,7 +881,7 @@ calculatePadding: } } - // flush output stream + // prevent deferring final write operations (to catch and handle possible errors here) outputStream.flush(); // handle errors (which might have been occurred after renaming/creating backup file) diff --git a/ogg/oggcontainer.cpp b/ogg/oggcontainer.cpp index 73f4d95..0b44129 100644 --- a/ogg/oggcontainer.cpp +++ b/ogg/oggcontainer.cpp @@ -580,6 +580,9 @@ void OggContainer::internalMakeFile(Diagnostics &diag, AbortableProgressFeedback OggPage::updateChecksum(fileInfo().stream(), offset); } + // prevent deferring final write operations (to catch and handle possible errors here) + fileInfo().stream().flush(); + // clear iterator m_iterator.clear(fileInfo().stream(), startOffset(), fileInfo().size());