From a07e03a25cada785ed1f013a193b836c463c2b82 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sat, 26 Oct 2019 08:54:43 +0200 Subject: [PATCH] Fix: Renaming entries should also remove the old entry This is a bugfix for a bug in Store::move_entry_by_id(): The old entry was not removed after the new one was created. The bug is a bit subtle. The issue was, that the internal cache held open a reference to the old entry (as StoreEntry object) and when that object got drop()ed, the contents of the entry were written to disk, which resulted in the old entry being recreated. This patch rewrites the function to behave well. The most critical part is that we do not check whether the old entry is borrowed with `hsmap.get()` but rather `hsmap.remove()`. The trick here is that the `StoreEntry` object is dropped in the moment the check is done, clearing the cached object and flushing it to the backend (the disk). After that, we continue doing the filesystem operation and the cache is clean. Signed-off-by: Matthias Beyer --- lib/core/libimagstore/src/store.rs | 32 +++++++++++------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/lib/core/libimagstore/src/store.rs b/lib/core/libimagstore/src/store.rs index cb63162a..5e029e48 100644 --- a/lib/core/libimagstore/src/store.rs +++ b/lib/core/libimagstore/src/store.rs @@ -590,18 +590,21 @@ impl Store { } debug!("New id does not exist in cache"); - // if we do not have an entry here, we fail in `FileAbstraction::rename()` below. - // if we have one, but it is borrowed, we really should not rename it, as this might - // lead to strange errors - if hsmap.get(&old_id).map(|e| e.is_borrowed()).unwrap_or(false) { - return Err(format_err!("Entry already borrowed: {}", old_id)); - } - - debug!("Old id is not yet borrowed"); - let old_id_pb = old_id.clone().with_base(self.path()).into_pathbuf()?; let new_id_pb = new_id.clone().with_base(self.path()).into_pathbuf()?; + if !self.backend.exists(&old_id_pb)? { + return Err(format_err!("Entry does not exist: {}", old_id)); + } + + // if it is borrowed, we really should not rename it, as this might + // lead to strange errors + // + // Also, remove this object from the cache + if hsmap.remove(&old_id).map(|e| e.is_borrowed()).unwrap_or(false) { + return Err(format_err!("Entry already borrowed: {}", old_id)); + } + if self.backend.exists(&new_id_pb)? { return Err(format_err!("Entry already exists: {}", new_id)); } @@ -617,17 +620,6 @@ impl Store { })?; debug!("Rename worked on filesystem"); - - // assert enforced through check hsmap.contains_key(&new_id) above. - // Should therefor never fail - let hsmap_does_not_have_key = hsmap - .remove(&old_id) - .and_then(|mut entry| { - entry.id = new_id.clone(); - hsmap.insert(new_id.clone(), entry) - }) - .is_none(); - assert!(hsmap_does_not_have_key); } debug!("Moved");