Skip to content
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

allow uninitialization of lib handle #2

Merged
merged 1 commit into from
Aug 1, 2018

Conversation

sitano
Copy link
Contributor

@sitano sitano commented Jun 18, 2018

func Close() error

to allow releasing of the default library handle

@optnfast optnfast self-requested a review June 20, 2018 09:05
@optnfast
Copy link
Contributor

optnfast commented Jun 25, 2018

Thankyou for the PR. This needs a bit more before we can merge it.

  1. Following PR2, I think it should use (*ResourcePool)Close to close each slot's sessions (replacing CloseSessions).
  2. It needs documentation comments on each public method. See https://blog.golang.org/godoc-documenting-go-code for a style guide.
  3. If you use ResetLibHandle but not Finalize, I think the system will be left in an inconsistent state, with a pool of session handles that don't belong to a future value of libHandle. So we need some strategy for preventing this situation, for instance having ResetLibHandle call Finalize first.

@sitano sitano force-pushed the allow_deinit_libhandle branch from 80d4a37 to 9f38f91 Compare June 26, 2018 12:56
…unction to allow default lib handle uninitialization
@sitano sitano force-pushed the allow_deinit_libhandle branch from 9f38f91 to 4b969bb Compare June 26, 2018 13:00
@sitano
Copy link
Contributor Author

sitano commented Jun 26, 2018

@nfewx I've refactored code based on your comments and mentioned PR #3.

I've created a new type sessionPool which secures a resourcePool with a RWMutex. The whole sessions pool made independent of the library singleton libHandle. I've added few helpers.

As the PKCS11Session type is public and can be created by any context, and the sessions pool is indepented of any specific context, and the session wrapper it self is kind of independent of the pool, I've placed Ctx *pkcs.Ctx into the session type, so it can implement Close().

@sitano
Copy link
Contributor Author

sitano commented Jun 26, 2018

I have removed vitess dependency and move resource pool implementation to this code base.

@optnfast
Copy link
Contributor

I appreciate the desire to minimize dependencies but I'd rather keep that code as an external dependency, partly for licensing reasons and partly to simplify tracking of any upstream changes.

@sitano
Copy link
Contributor Author

sitano commented Jun 26, 2018

@nfewx ok. what do you think of new PKCS11Session struct? does it fit your expectations on design? another way was to put ctx object into the sessions pool object, but I was not sure whether the pool would be used across multiple contexts in the future i.e.

@sitano sitano force-pushed the allow_deinit_libhandle branch from c97df71 to 4b969bb Compare June 26, 2018 14:17
@sitano
Copy link
Contributor Author

sitano commented Jun 26, 2018

from another point of view, there is an API which recv handles which could be created with different session context (not libHandle) and then having it at hand allows passing of the right session handle to the pkcs11 calls.

@optnfast
Copy link
Contributor

I like the design as of 4b969bb. Anyone else want to chip in before I merge? (@solcates?)

@optnfast optnfast merged commit 4b969bb into ThalesGroup:master Aug 1, 2018
optnfast pushed a commit that referenced this pull request Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants