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

crypto: cleanup NSPR in main thread #14801

Merged
merged 1 commit into from Apr 27, 2017

Conversation

Projects
None yet
4 participants
@tchaikov
Copy link
Contributor

tchaikov commented Apr 26, 2017

quote from nspr's header file

 * Perform a graceful shutdown of NSPR.  PR_Cleanup() may be called by
 * the primordial thread near the end of the main() function.

this helps to silence some warnings from valgrind. but it does not hurt
in practice, because the process is about to die. and the freed memory
chunks are only allocated once in NSPR.

Signed-off-by: Kefu Chai kchai@redhat.com

@tchaikov tchaikov requested a review from dillaman Apr 26, 2017

@liewegas liewegas added the needs-qa label Apr 26, 2017

@@ -83,6 +84,7 @@ void ceph::crypto::shutdown()
assert(crypto_refs > 0);
if (--crypto_refs == 0) {
NSS_ShutdownContext(crypto_context);
PR_Cleanup();

This comment has been minimized.

Copy link
@liewegas

liewegas Apr 26, 2017

Member

It may not be the primordial thread.. does it matter?

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 26, 2017

Author Contributor

@liewegas we always have a g_ceph_context, and common_init_finish() calls cct->init_crypto() and we have a smart pointer managing the life cycle of the global g_ceph_context (remember the huge patch introducing the smart pointer in basically all main()s?), so the last one who releases crypto_context is always the destructor of that smart pointer in main(). so it's always the primordial thread.

This comment has been minimized.

Copy link
@dillaman

dillaman Apr 26, 2017

Contributor

Library users are not guaranteed to have g_ceph_context -- i.e. QEMU could hot-add and hot-remove RBD-backed drives in any order. Is this something that should only be invoked on Ceph daemons and applications?

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 27, 2017

Author Contributor

ahh, right. but no, it's not something should only be called by daemon and apps.

i looked at source of NSPR. the point of

may be called by the primordial thread near the end of the main() function

is that, the resource cleaned up by this function is shared globally by all NSS context. that's why the NSPR's comment put this way. but we refcounts the crypto_context users, this ensures us that it's safe to call this way.

This comment has been minimized.

Copy link
@dillaman

dillaman Apr 27, 2017

Contributor

My only concern was related to the same reason why we switched to NSS_InitContext vs NSS_Init -- that allows better isolation between libraries and their applications that might also use NSS. Personally, I'd vote to have this skipped for CODE_ENVIRONMENT_LIBRARY

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 27, 2017

Author Contributor

@dillaman

that allows better isolation between libraries and their applications that might also use NSS

oh, yes! agreed. will update this change as you suggest. and thanks for your review.

@liewegas

This comment has been minimized.

Copy link
Member

liewegas commented Apr 26, 2017

@tchaikov tchaikov force-pushed the tchaikov:wip-crypto-cleanup branch from 8e39b6a to 2f4ac5b Apr 27, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor Author

tchaikov commented Apr 27, 2017

@dillaman fixed and repushed.

crypto: cleanup NSPR in main thread
quote from nspr's header file

```
 * Perform a graceful shutdown of NSPR.  PR_Cleanup() may be called by
 * the primordial thread near the end of the main() function.
```

this helps to silence some warnings from valgrind. but it does not hurt
in practice, because the process is about to die. and the freed memory
chunks are only allocated once in NSPR.

Signed-off-by: Kefu Chai <kchai@redhat.com>

@tchaikov tchaikov force-pushed the tchaikov:wip-crypto-cleanup branch from 2f4ac5b to b22977d Apr 27, 2017

@dillaman
Copy link
Contributor

dillaman left a comment

lgtm

@tchaikov tchaikov force-pushed the tchaikov:wip-crypto-cleanup branch from cec0a45 to b22977d Apr 27, 2017

@yuriw yuriw merged commit 8f0b77d into ceph:master Apr 27, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
arm build successfully built on arm
Details
default Build finished.
Details

@tchaikov tchaikov deleted the tchaikov:wip-crypto-cleanup branch Apr 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.