From bdaff487054b34710c1805f730aaf558a2ec97ed Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 28 Apr 2021 00:13:30 +0200 Subject: [PATCH 1/2] Do not save missing tracks to prevent inconsistent metadata The metadata of tracks in the database should not be modified as long as they are missing to prevent inconsistencies with the file metadata. This will become an issue soon. Other changes: - Return more meaningful results when saving tracks - Perform checks behind the internal API in TrackCollectionManager and use debug assertions in TrackDAO --- src/library/dao/trackdao.cpp | 10 ++--- src/library/trackcollection.cpp | 2 +- src/library/trackcollection.h | 2 +- src/library/trackcollectionmanager.cpp | 52 +++++++++++++------------- src/library/trackcollectionmanager.h | 13 ++++--- 5 files changed, 38 insertions(+), 41 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index c699164dca9..0c69e018373 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -289,14 +289,10 @@ QString TrackDAO::getTrackLocation(TrackId trackId) const { void TrackDAO::saveTrack(Track* pTrack) const { DEBUG_ASSERT(pTrack); - if (!pTrack->isDirty()) { - return; - } + DEBUG_ASSERT(pTrack->isDirty()); + const TrackId trackId = pTrack->getId(); - // Only update the database if the track has already been added! - if (!trackId.isValid()) { - return; - } + DEBUG_ASSERT(trackId.isValid()); qDebug() << "TrackDAO: Saving track" << trackId << pTrack->getFileInfo(); diff --git a/src/library/trackcollection.cpp b/src/library/trackcollection.cpp index 875a094e27a..85c6a22746a 100644 --- a/src/library/trackcollection.cpp +++ b/src/library/trackcollection.cpp @@ -532,7 +532,7 @@ bool TrackCollection::updateAutoDjCrate( return updateCrate(crate); } -void TrackCollection::saveTrack(Track* pTrack) { +void TrackCollection::saveTrack(Track* pTrack) const { DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); m_trackDao.saveTrack(pTrack); diff --git a/src/library/trackcollection.h b/src/library/trackcollection.h index cd87661f20f..bfd0039dd62 100644 --- a/src/library/trackcollection.h +++ b/src/library/trackcollection.h @@ -156,7 +156,7 @@ class TrackCollection : public QObject, void relocateDirectory(const QString& oldDir, const QString& newDir); - void saveTrack(Track* pTrack); + void saveTrack(Track* pTrack) const; QSqlDatabase m_database; diff --git a/src/library/trackcollectionmanager.cpp b/src/library/trackcollectionmanager.cpp index c0cb9d8f9c0..79989680ff4 100644 --- a/src/library/trackcollectionmanager.cpp +++ b/src/library/trackcollectionmanager.cpp @@ -173,16 +173,13 @@ void TrackCollectionManager::stopLibraryScan() { m_pScanner->slotCancel(); } -bool TrackCollectionManager::saveTrack(const TrackPointer& pTrack) { +TrackCollectionManager::SaveTrackResult TrackCollectionManager::saveTrack( + const TrackPointer& pTrack) const { VERIFY_OR_DEBUG_ASSERT(pTrack) { - return false; + return SaveTrackResult::Skipped; } - if (!pTrack->isDirty()) { - return false; - } - saveTrack(pTrack.get(), TrackMetadataExportMode::Deferred); - DEBUG_ASSERT(!pTrack->isDirty()); - return true; + const auto res = saveTrack(pTrack.get(), TrackMetadataExportMode::Deferred); + return res; } // Export metadata and save the track in both the internal database @@ -191,18 +188,17 @@ void TrackCollectionManager::saveEvictedTrack(Track* pTrack) noexcept { saveTrack(pTrack, TrackMetadataExportMode::Immediate); } -void TrackCollectionManager::saveTrack( +TrackCollectionManager::SaveTrackResult TrackCollectionManager::saveTrack( Track* pTrack, - TrackMetadataExportMode mode) { + TrackMetadataExportMode mode) const { DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); DEBUG_ASSERT(pTrack); DEBUG_ASSERT(pTrack->getDateAdded().isValid()); // The dirty flag is reset while saving the track in the internal // collection! - const bool trackDirty = pTrack->isDirty(); - if (!trackDirty) { - return; + if (!pTrack->isDirty()) { + return SaveTrackResult::Skipped; } const auto fileInfo = pTrack->getFileInfo(); @@ -218,7 +214,7 @@ void TrackCollectionManager::saveTrack( kLogger.debug() << "Skip saving of missing track" << fileInfo.location(); - return; + return SaveTrackResult::Skipped; } // This operation must be executed synchronously while the cache is @@ -229,24 +225,23 @@ void TrackCollectionManager::saveTrack( << pTrack->getLocation() << "in internal collection"; m_pInternalCollection->saveTrack(pTrack); + const auto res = pTrack->isDirty() ? SaveTrackResult::Failed : SaveTrackResult::Saved; if (m_externalCollections.isEmpty()) { - return; + return res; } if (pTrack->getId().isValid()) { // Track still exists in the internal collection/database - if (trackDirty) { - kLogger.debug() - << "Saving modified track" - << pTrack->getLocation() - << "in" - << m_externalCollections.size() - << "external collection(s)"; - for (const auto& externalTrackCollection : qAsConst(m_externalCollections)) { - externalTrackCollection->saveTrack( - *pTrack, - ExternalTrackCollection::ChangeHint::Modified); - } + kLogger.debug() + << "Saving modified track" + << pTrack->getLocation() + << "in" + << m_externalCollections.size() + << "external collection(s)"; + for (const auto& externalTrackCollection : qAsConst(m_externalCollections)) { + externalTrackCollection->saveTrack( + *pTrack, + ExternalTrackCollection::ChangeHint::Modified); } } else { // Track has been deleted from the internal collection/database @@ -262,6 +257,9 @@ void TrackCollectionManager::saveTrack( QStringList{pTrack->getLocation()}); } } + // After saving a track successfully the dirty flag must have been reset + DEBUG_ASSERT(!(res == SaveTrackResult::Saved && pTrack->isDirty())); + return res; } void TrackCollectionManager::exportTrackMetadata( diff --git a/src/library/trackcollectionmanager.h b/src/library/trackcollectionmanager.h index 42efdfc50ad..16ae99f91e8 100644 --- a/src/library/trackcollectionmanager.h +++ b/src/library/trackcollectionmanager.h @@ -65,9 +65,12 @@ class TrackCollectionManager: public QObject, // Save the track in both the internal database and external collections. // Export of metadata is deferred until the track is evicted from the // cache to prevent file corruption due to concurrent access. - // Returns true if the track was dirty and has been saved, otherwise - // false. - bool saveTrack(const TrackPointer& pTrack); + enum class SaveTrackResult { + Saved, + Skipped, // e.g. unmodified or missing/deleted tracks + Failed, + }; + SaveTrackResult saveTrack(const TrackPointer& pTrack) const; signals: void libraryScanStarted(); @@ -90,9 +93,9 @@ class TrackCollectionManager: public QObject, Immediate, Deferred, }; - void saveTrack( + SaveTrackResult saveTrack( Track* pTrack, - TrackMetadataExportMode mode); + TrackMetadataExportMode mode) const; void exportTrackMetadata( Track* pTrack, TrackMetadataExportMode mode) const; From 0fa8c9c695f318de1fc4470093a245941c52f58d Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 1 May 2021 22:41:59 +0200 Subject: [PATCH 2/2] Prevent crashes when passing a nullptr --- src/library/dao/trackdao.cpp | 4 +++- src/library/trackcollectionmanager.cpp | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 0c69e018373..ce68d034aab 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -288,7 +288,9 @@ QString TrackDAO::getTrackLocation(TrackId trackId) const { } void TrackDAO::saveTrack(Track* pTrack) const { - DEBUG_ASSERT(pTrack); + VERIFY_OR_DEBUG_ASSERT(pTrack) { + return; + } DEBUG_ASSERT(pTrack->isDirty()); const TrackId trackId = pTrack->getId(); diff --git a/src/library/trackcollectionmanager.cpp b/src/library/trackcollectionmanager.cpp index 79989680ff4..87ec24d6abd 100644 --- a/src/library/trackcollectionmanager.cpp +++ b/src/library/trackcollectionmanager.cpp @@ -192,7 +192,9 @@ TrackCollectionManager::SaveTrackResult TrackCollectionManager::saveTrack( Track* pTrack, TrackMetadataExportMode mode) const { DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); - DEBUG_ASSERT(pTrack); + VERIFY_OR_DEBUG_ASSERT(pTrack) { + return SaveTrackResult::Skipped; + } DEBUG_ASSERT(pTrack->getDateAdded().isValid()); // The dirty flag is reset while saving the track in the internal