Incremental object search #109
Conversation
There was a problem hiding this comment.
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 🤔
Passing 0 to the function is indeed unfortunate. On one hand it's good when the library never panics, on the other some projects (like rust-openssl) panic left and right on bad programmer-supplied data 🤷♂️
|
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:
```rust
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:
```rust
loop {
let chunk = search.find_next(SIZE)?;
if chunk.is_empty() { break; }
// use chunk here...
}
```
or
```rust
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.
|
Add a method `Session::find_objects_init`, which returns an iterator-like object representing an ongoing object search. This allows for more flexibility than the existing `Session::find_objects`, which returns a vector of all objects. Signed-off-by: James Hagborg <[email protected]>
Signed-off-by: James Hagborg <[email protected]>
Signed-off-by: James Hagborg <[email protected]>
Change find_objects to use the new incremental object search functions internally, instead of calling the FFI directly. Signed-off-by: James Hagborg <[email protected]>
jhagborgftx
force-pushed
the
incremental-object-search
branch
from
3c9cc59
to
04fcf2c
Compare
|
Updated to return just a vector, rather than an option of vector. |
|
Thanks for the updates! I did take some time to add the iterator API on top of it: wiktor-k@1c00f37 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 🤔 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 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. |
| /// ```no_run | ||
| /// use cryptoki::error::Result; | ||
| /// use cryptoki::object::{Attribute, AttributeType}; | ||
| /// use cryptoki::session::Session; | ||
| /// | ||
| /// const BATCH_SIZE: usize = 10; | ||
| /// | ||
| /// fn print_object_labels(session: &mut Session, template: &[Attribute]) -> Result<()> { | ||
| /// // Initiate the search. | ||
| /// let mut search = session.find_objects_init(template)?; | ||
| /// | ||
| /// // Iterate over batches of results, while find_next returns a non-empty batch | ||
| /// while let ref objects @ [_, ..] = search.find_next(BATCH_SIZE)?[..] { | ||
| /// // Iterate over objects in the batch. | ||
| /// for &object in objects { | ||
| /// // Look up the label for the object. We can't use `session` directly here, | ||
| /// // since it's mutably borrowed by search. Instead, use `search.session()`. | ||
| /// let attrs = search.session().get_attributes(object, &[AttributeType::Label])?; | ||
| /// if let Some(Attribute::Label(label)) = attrs.get(0) { | ||
| /// println!("Found object: {}", String::from_utf8_lossy(&label)); | ||
| /// } | ||
| /// } | ||
| /// } | ||
| /// | ||
| /// Ok(()) | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
Excellent, thanks for adding an example.
| // Search 10 elements at a time | ||
| const MAX_OBJECT_COUNT: usize = 10; |
There was a problem hiding this comment.
Any reason for this choice? Just curious
| let mut search = session.find_objects_init(&[])?; | ||
| let objects = search.find_next(10)?; | ||
| assert!(search.find_next(10)?.is_empty()); | ||
| drop(search); |
There was a problem hiding this comment.
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.
|
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.
|
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! |
|
Closing, as this has already been implemented. Thanks for your time! 👋 |
This PR implements #106. It also adds tests for both new and old-style object searches, and changes
find_objectsto use this new API internally.As it is,
find_next(0)returnsSome(vec![]), which is I think the least surprising thing to do? It shouldn't returnNone, since there may be more objects remaining. I think it would also make sense if it panicked. Let me know what you think.