This patch changes the filesystem-backend implementation of libimagstore
to open files on each read/write rather than holding the file handle in
memory at all times.
Whenever a lot of imag store entries are read into memory, the imag
process may ran out of file descriptors. With this patch applied, a
`Store::get()` call on an entry which is not yet in the store cache
would cause the file to be read, but the FD being dropped after that.
Likewise, a `Store::update()` (which is also called if the imag entry is
dropped) would re-open the file on the filesystem and write the contents
from the imag store cache back to the file.
With this patch, opening hundrets or thousands of imag entries should be
no problem anymore, only the available memory should be a limit then.
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
This patch fixes a bug we did not even hit (yet). It is: When deleting
an Entry from the store, this could potentially leave artifacts in the
cache.
Szenario: An Entry, which was loaded (via `Store::get()` for example),
gets `Store::delete()`ed twice. The first call would work as expected,
but leave the Entry in the Store cache. The second call would then fail,
as the Entry is already removed on the FS, but still in the cache. This
would fail - which is the right thing to do here - but with the wrong
error (with a FileError rather than a FileNotFound error).
This patch fixes this.
First of all, the appropriate `PathBuf` object is calculated in all
cases, as this object is needed to check whether the file is actually
there (which could be the case if the Entry is in the cache and if it is
not).
If the entry is in the cache and is borrowed: error. If not, remove the
entry from the cache. Afterwards the file is deleted on disk.
If the entry is not in the cache, but the file exists, the file is removed.
If the file does not exist: error.
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Because the iterators need to be able to check whether the file exists
_in the backend_ (not on disk, but in the backend, because of in-memory
test for example), we need to be able to pass the backend to the
iterator intermediate type.
This patch implements this. It does so by changing the internal backend
member of the store from `Box<FileAbstraction>` to
`Arc<FileAbstraction>`, which gives us the ability to clone the
reference to the backend easily without needing to rely on lifetimes
here, because of the Arc.
Also, less boxes are always good.
This patch reimplements the iterator extensions.
As we iterate (in StoreIdIterator) over Result<StoreId> now anyways, we
don't need the extensions for Result iterators anymore.
This patch rewrites the extensions to be more simple in every way and
generic over the error type in the iterator.
All the errors have to do is implement From<StoreError>, which is what
they do when linking the generated error types with error_chain to the
libimagstore error types.
This patch rewrites the Store::entries() function to not be collecting
the iterator.
It therefore introduces a new, internal, iterator type which creates the
StoreId objects from the pathes the PathIterator yields internally.
With this patch, the Store iterator interface changes, as the iterators
now yield `Result<StoreId, StoreError>` instead of `StoreId`.
This is necessary, as the internal conversion errors shouldn't be
hidden.
Of course, the iterator types (like the StoreGetIterator and so on)
should hold a Result<StoreId> internally as well, and also yield
appropritely. This was changed in this commit, too.
Before we had the problem that when iterating over _a lot_ (like 5k)
entries and also fetching them, at some point the OS would return with
"Too many files open".
That is because the store internally caches a lot.
With this change, the Store gets an API to query how big the cache is,
how much the cache can currently hold and (and that's the main thing in
this patch) to flush the cache to disk.
A function to simply ask the store whether its cache should be flushed
(which would us require to ask the OS how many files we can open...
which would be possible with `libc::getrlimit`) does not yet exist,
though, but could be added easily if desired.
The rust compiler does some fancy things for us: It automatically finds
the right fields if the name of the variable and the file is the same.
Lets use that to reduce boilerplate with this patch.
Unfortunately, our latest fix to file parsing did not solve all issues.
So we have to fix it _again_.
The problem was the `std::str::Lines` iterator, which apparently fails
this:
assert_eq!(1, "".lines().count());
as an empty line seems not to be a line.
Because of that, when reading a file with an empty line at its bottom
got stripped off that line.
This patch removes the use of the `lines()` iterator and uses
`split("\n")` instead. This only works on Unix operating systems, but as
we only target unix operating systems with imag, this is not considered
an issue right now.
This patch also adds extensive tests on multiple levels in the
`libimagstore` implementation:
* On the parsing level, for the function which implements the parsing
* On the filesystem abstraction levels
* On the `Store` levels
to make sure that everything is parsed correctly.
Rewrite without regex crate.
The regex approach was broken. If the following _content_ was provided
in the entry:
foo
---
bar
The regex approach parsed the header until the "---" in the content.
This is, of course, not the way to do that.
Now, the parsing is implemented by hand. Should be faster as well,
though I don't care about this.
This fixes a severe bug.
This patch adds API functions in the StoreIdIteratorWithStore iterator
type to transform it into a iterator which _does_ something (as in the
`libimagstore::iter` API).
It mimics the API which is offered by `libimagstore::iter`.
This test is not applicable anymore because it tests (and tested) the
wrong thing.
It was to check whether the function failed because the "imag" key
contained the wrong type, but this is not tested by that function. The
function only checks whether the "imag" key is present.
Before we extracted the store configuration from the configuration
toml::Value object and passed it to the store.
This is unecessary overhead.
Now we pass the whole configuration object and let the store extract the
required values.
Before the iterator did also yield storeids for directories, which was a
bug.
This change introduces a new if_file() function in the store-internal
backend, which is needed to check whether a path actually points to a
File, be it inmemory or on the real filesystem.
That's because tests might fail if they check via PathBuf::is_file() as
in tests, the entries only exist inmemory.
Matthias Beyer