rust-cryptoki
rust-cryptoki copied to clipboard
parallaxsecond
Incremental object search
This PR implements #106. It also adds tests for both new and old-style object searches, and changes find_objects to use this new API internally.
As it is, find_next(0) returns Some(vec![]), which is I think the least surprising thing to do? It shouldn't return None, since there may be more objects remaining. I think it would also make sense if it panicked. Let me know what you think.
On Thu, 2022-12-01 at 00:48 -0800, Wiktor Kwapisiewicz wrote:
LGTM. It kind of bothers me that None and Some(vec![]) are basically the same state but using Option has the advantage of nice while let Some(...) loops on the client side 🤔
I agree that it's not so nice. I thought about it a bit more, and I think we can just return a vector, and the user can use slice patterns to get a nice-ish syntax in most cases:
while let ref chunk @ [_, ..] = search.find_next(SIZE)?[..] {
// now chunk is a &[ObjectHandle], and is non-empty
}
However, if the user needs to take the vector and own it, they're left with either:
loop {
let chunk = search.find_next(SIZE)?;
if chunk.is_empty() { break; }
// use chunk here...
}
or
let mut chunk = vec![];
while { chunk = search.find_next(SIZE)?; !chunk.is_empty() } {
// use chunk here...
}
With all this in mind, I don't think the strangeness of Option<Vec<_>> is really worth it.
As for find_objects(0), I think we can just return vec![], in order to
be consistent with how read works.
Thanks for the updates! I did take some time to add the iterator API on top of it: https://github.com/wiktor-k/rust-cryptoki/commit/1c00f377a10e45260375fcd811ab566209498d00
Edit: I've added the no-compile test to test for the case of two searches in parallel that should be forbidden and adjusted muts here and there for the iterator API to work (it still prohibits two search to be open at the same time).
I'm still thinking about the API ease of use from the perspective of the API clients :thinking:
For the time being maybe let's wait until @ionut-arm voices his opinion (since he's the most senior here along with @hug-dev)? Sorry that this takes a little bit of time but it's the end of year and everyone is super busy...
Neat. I was thinking that an iterator interface would be a separate struct that wraps a FindObjects, created from a separate method like Session::find_objects_iter(). That way the user doesn't need to bother with a default batch size if they aren't using it. This would also make it possible to make an iterator in more than one way: as an iterator over chunks or as an iterator over individual objects, with batching under the hood.
There's no rush to get this merged immediately. I'm going to be using a patched version for a while anyway, and I'm trying to upstream what I can.
I also like Wiktor's iterator addition, seems quite handy for this operation - I'd be quite happy if you managed to incorporate something like that in the patch! 😄
On Tue, 2022-12-06 at 02:57 -0800, Ionuț Mihalcea wrote:
+// Search 10 elements at a time +const MAX_OBJECT_COUNT: usize = 10; Any reason for this choice? Just curious
That wasn't my addition. Look at the old implementation for
find_objects.
On Tue, 2022-12-06 at 02:57 -0800, Ionuț Mihalcea wrote:
Hmmmmm... Would it be possible to add a method to FindObjects, something like finalize, that just takes ownership of the value and lets it drop? It seems suboptimal to have to drop the object manually to achieve this, and an empty method isn't too bad.
Sure, then we can also get the benefit of a place to return errors from
C_FindObjectsFinal. I'd call it final to be consistent with init.
Either way, I don't think manual dropping is that strange. The docs for MutexGuard from std have an example. The same could also be acheived by just adding some curly braces around the search.
Either way, I don't think manual dropping is that strange. The docs for MutexGuard from std have an example. The same could also be acheived by just adding some curly braces around the search.
Sure, we've had cases before where dropping something was the way to go, but since we know this is a potential use-case, it seems quite straight-forward to add something to do it "natively". But I get your point!
One question I wanted to raise is: how much time do you have/are you willing to put into resolving comments on these PRs? Because if you only want to get them in as is, we could review the changes here, raise issues for what's left (like this new method), and fix those afterwards in separate PRs. Asking only because I don't want to pressure you into putting in more effort.
On Thu, 2022-12-08 at 02:43 -0800, Ionuț Mihalcea wrote:
One question I wanted to raise is: how much time do you have/are you willing to put into resolving comments on these PRs? Because if you only want to get them in as is, we could review the changes here, raise issues for what's left (like this new method), and fix those afterwards in separate PRs. Asking only because I don't want to pressure you into putting in more effort.
I'd like to resolve everything before we get them in. I've had other things on my plate the past few days, but I can definately get these updated within a day or two.
#223 fixed the upstream issue, so this could probably be closed if you agree!
