-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implements session object handle iterator, with caching #223
implements session object handle iterator, with caching #223
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think it looks very nice 👍
I've added a couple of personal, tiny nitpicks.
I also wonder if it'd be a good idea to add Closes: https://github.com/parallaxsecond/rust-cryptoki/pull/109
in the commit message trailer (since it appears to be the same functionality but with a separate method).
One further improvement would be re-implementing find_objects
in terms of your iter_objects
(it should be as simple as just calling iter_objects
and then collect
on the result 🤔 )
Thanks for your time! 👋
Correct me if I'm wrong but this should be as simple as making |
That would work. However, if I combine this with a rewrite of
Choose your curse 😉 |
Actually there is maybe a good reason for not taking a mutable ref:
In this pattern, attributes are retrieved as objects are fetched. It results in interleaved calls to If preventing concurrent searches by the library itself is desirable, it can be easily implemented using a flag in session, inside a Any thought? |
Yeah, all good points. Maybe we could just leave the "no multiple open searches" invariant in docs and that's it. Making it too complex to cover a rare API misuse may not be such a great idea after all 🤔 (I've got a couple of complex ideas how to "solve" it but I worry that the cure will be worse than the disease 😅 ) |
4cd3c36
to
865bc78
Compare
When all the reviews are finished, I can rebase the branch and incorporate the comment.
That's been incorporated. |
1522cc8
to
518865a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach looks good to me, happy to go ahead with it, just a few minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's the last round of feedback and in general it looks very nice already 👌
Signed-off-by: Eric Devolder <[email protected]>
Signed-off-by: Eric Devolder <[email protected]>
Signed-off-by: Eric Devolder <[email protected]>
- documentation enhanced - documentation example adjusted/simplified - tests simplifications - `find_objects()` method is now calling `iter_objects()` Signed-off-by: Eric Devolder <[email protected]>
Signed-off-by: Eric Devolder <[email protected]>
Signed-off-by: Eric Devolder <[email protected]>
518865a
to
fd10cc6
Compare
Signed-off-by: Eric Devolder <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 thanks for taking the time to address all remarks :)
NP, feedback is useful and relevant! I don't know what is your policy for merging changes, but since you made this request:
I suggested I would rebase this PR, but that would make another round of approvals, and you would loose visibility on the changes before merging. Maybe is it simpler for you to squash merge and add this comment to the commit message? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking comments you can feel free to ignore if they are not worth it! Otherwise trusting Wiktor's review 🙏
Haha, please don't do that 😅 I try to be diligent here but it's still an unpaid side-project so my time is limited. I still think that given our collective effort the code continously gets better and better (and with tests future refactoring is always possible). 👋 |
- get_pkcs11! macro rewrite - test code refactoring Signed-off-by: Eric Devolder <[email protected]>
Thank you for merging this one! Out of curiosity, are you planning to make a new tagged release at some point in time? |
Nothing planned for now I think, it's mostly demand-based! We also try to group release with other dependencies in |
On another topic @keldonin, would you like to be a maintainer of this crate? Since you have shown interest in it in the past few weeks with very nice improvements I was wondering if you would like to help us maintaining it? |
Hi @hug-dev. Nothing pressing for now, just asking. Thank you for the clarification! |
I would be honoured. Time-wise, something I can perform during my free time (probably like you? 🤔) - that I share with other activities - there is life out there 😂. So, glad to give support here, to the extent of my availability. |
That's great, thank you! |
It's done now! If you want, you can join the |
Hello,
This PR is an alternate candidate for implementing #106.
It implements the Iterator trait for object handlers, given a search template. The iterator is able to fetch values ahead, and cache the results. The cache size can be specified at iterator build time.
design choices
Option<Result<ObjectHandle>>
. This is because calls toC_FindObjects()
may fail, and the caller may want to manage such condition. It is similar to what thestd::io::Lines
iterator implements.find_objects()
method. It seems to me it serves a different goal, ease of use. On the other hand, the iterator allows the caller a more fine-grained control over how object handles are retrieved from the token, and allows for streaming. Both can coexist, IMO.find_objects
change, every effort is made to avoid unnecessary calls toC_FindObjects()
.Drop
trait, to callC_FindObjectsFinal()
.next()
method of an iterator cannot return aResult
, I had to create a new macro fromget_pkcs11!()
that does not carry the syntactic sugar?
.C_FindObjects
andC_FindObjectsFinal
are not implemented. But... perhaps the library should refuse to work with such library, and detect that at library loading time?For your review,