matthiasbeyer/imag
1
0
Fork 0
Code Releases Activity

Fix: Renaming entries should also remove the old entry

Browse source 
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 <mail@beyermatthias.de>
This commit is contained in:
Matthias Beyer 2019-10-26 08:54:43 +02:00
parent a6bbcd65f4
commit a07e03a25c
1 changed files with 12 additions and 20 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

32
lib/core/libimagstore/src/store.rs
View file

@ -590,18 +590,21 @@ impl Store {
} }
debug!("New id does not exist in cache"); 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 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()?; 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)? { if self.backend.exists(&new_id_pb)? {
return Err(format_err!("Entry already exists: {}", new_id)); return Err(format_err!("Entry already exists: {}", new_id));
} }
@ -617,17 +620,6 @@ impl Store {
})?; })?;
debug!("Rename worked on filesystem"); 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"); debug!("Moved");