From f6b35e5d58b59ed02ba76f4d7df262b7952567d8 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Wed, 25 May 2016 15:01:26 +0200 Subject: [PATCH] SyncEngine: invalid the blacklist entry when the rename destination change The problem in this case is if we rename the file "xxx" to "invalid\file". The rename will fail because the new filename constains a slash, and it will be blacklisted. But then if the user re-rename the file to "valid_name", then we should invalidate the blacklist entry and retry to upload. But we did not do that because renaming don't change the mtime and we did not store the rename target in the database IL issue 558 --- src/libsync/syncengine.cpp | 9 +++++++++ src/libsync/syncjournaldb.cpp | 19 +++++++++++++++---- src/libsync/syncjournalfilerecord.cpp | 1 + src/libsync/syncjournalfilerecord.h | 1 + 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 98d580242..a7928d081 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -183,6 +183,12 @@ QString SyncEngine::csyncErrorToString(CSYNC_STATUS err) } +/** + * Check if the item is in the blacklist. + * If it should not be sync'ed because of the blacklist, update the item with the error instruction + * and proper error message, and return true. + * If the item is not in the blacklist, or the blacklist is stale, return false. + */ bool SyncEngine::checkErrorBlacklisting( SyncFileItem &item ) { if( !_journal ) { @@ -214,6 +220,9 @@ bool SyncEngine::checkErrorBlacklisting( SyncFileItem &item ) } else if( item._modtime != entry._lastTryModtime ) { qDebug() << item._file << " is blacklisted, but has changed mtime!"; return false; + } else if( item._renameTarget != entry._renameTarget) { + qDebug() << item._file << " is blacklisted, but rename target changed from" << entry._renameTarget; + return false; } } else if( item._direction == SyncFileItem::Down ) { // download, check the etag. diff --git a/src/libsync/syncjournaldb.cpp b/src/libsync/syncjournaldb.cpp index 50506f9ea..fc6aabad2 100644 --- a/src/libsync/syncjournaldb.cpp +++ b/src/libsync/syncjournaldb.cpp @@ -409,7 +409,7 @@ bool SyncJournalDb::checkConnect() _deleteFileRecordRecursively.reset(new SqlQuery(_db)); _deleteFileRecordRecursively->prepare("DELETE FROM metadata WHERE path LIKE(?||'/%')"); - QString sql( "SELECT lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration " + QString sql( "SELECT lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget " "FROM blacklist WHERE path=?1"); if( Utility::fsCasePreserving() ) { // if the file system is case preserving we have to check the blacklist @@ -421,8 +421,8 @@ bool SyncJournalDb::checkConnect() _setErrorBlacklistQuery.reset(new SqlQuery(_db)); _setErrorBlacklistQuery->prepare("INSERT OR REPLACE INTO blacklist " - "(path, lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration) " - "VALUES ( ?1, ?2, ?3, ?4, ?5, ?6, ?7)"); + "(path, lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget) " + "VALUES ( ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8)"); _getSelectiveSyncListQuery.reset(new SqlQuery(_db)); _getSelectiveSyncListQuery->prepare("SELECT path FROM selectivesync WHERE type=?1"); @@ -612,6 +612,15 @@ bool SyncJournalDb::updateErrorBlacklistTableStructure() } commitInternal("update database structure: add lastTryTime, ignoreDuration cols"); } + if( columns.indexOf(QLatin1String("renameTarget")) == -1 ) { + SqlQuery query(_db); + query.prepare("ALTER TABLE blacklist ADD COLUMN renameTarget VARCHAR(4096);"); + if( !query.exec() ) { + sqlFail("updateBlacklistTableStructure: Add renameTarget", query); + re = false; + } + commitInternal("update database structure: add lastTryTime, ignoreDuration cols"); + } SqlQuery query(_db); query.prepare("CREATE INDEX IF NOT EXISTS blacklist_index ON blacklist(path collate nocase);"); @@ -1213,6 +1222,7 @@ SyncJournalErrorBlacklistRecord SyncJournalDb::errorBlacklistEntry( const QStrin entry._errorString = _getErrorBlacklistQuery->stringValue(3); entry._lastTryTime = _getErrorBlacklistQuery->int64Value(4); entry._ignoreDuration = _getErrorBlacklistQuery->int64Value(5); + entry._renameTarget = _getErrorBlacklistQuery->stringValue(6); entry._file = file; } _getErrorBlacklistQuery->reset_and_clear_bindings(); @@ -1324,13 +1334,14 @@ void SyncJournalDb::updateErrorBlacklistEntry( const SyncJournalErrorBlacklistRe _setErrorBlacklistQuery->bindValue(5, item._errorString); _setErrorBlacklistQuery->bindValue(6, QString::number(item._lastTryTime)); _setErrorBlacklistQuery->bindValue(7, QString::number(item._ignoreDuration)); + _setErrorBlacklistQuery->bindValue(8, item._renameTarget); if( !_setErrorBlacklistQuery->exec() ) { QString bug = _setErrorBlacklistQuery->error(); qDebug() << "SQL exec blacklistitem insert or replace failed: "<< bug; } qDebug() << "set blacklist entry for " << item._file << item._retryCount << item._errorString << item._lastTryTime << item._ignoreDuration - << item._lastTryModtime << item._lastTryEtag; + << item._lastTryModtime << item._lastTryEtag << item._renameTarget ; _setErrorBlacklistQuery->reset_and_clear_bindings(); } diff --git a/src/libsync/syncjournalfilerecord.cpp b/src/libsync/syncjournalfilerecord.cpp index 96f900767..583b7c41d 100644 --- a/src/libsync/syncjournalfilerecord.cpp +++ b/src/libsync/syncjournalfilerecord.cpp @@ -151,6 +151,7 @@ SyncJournalErrorBlacklistRecord SyncJournalErrorBlacklistRecord::update( // The factor of 5 feels natural: 25s, 2 min, 10 min, ~1h, ~5h, ~24h entry._ignoreDuration = old._ignoreDuration * 5; entry._file = item._file; + entry._renameTarget = item._renameTarget; if( item._httpErrorCode == 403 ) { qDebug() << "Probably firewall error: " << item._httpErrorCode << ", blacklisting up to 1h only"; diff --git a/src/libsync/syncjournalfilerecord.h b/src/libsync/syncjournalfilerecord.h index ec2ec9c16..ce52b3e82 100644 --- a/src/libsync/syncjournalfilerecord.h +++ b/src/libsync/syncjournalfilerecord.h @@ -89,6 +89,7 @@ public: time_t _ignoreDuration; QString _file; + QString _renameTarget; bool isValid() const;