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

tracing: Fix handle leak in TracepointProvider #12652

Merged
merged 1 commit into from Dec 30, 2016

Conversation

badone
Copy link
Contributor

@badone badone commented Dec 23, 2016

Silences Coverity 1397733

Signed-off-by: Brad Hubbard bhubbard@redhat.com

@@ -39,6 +39,7 @@ void TracepointProvider::verify_config(const struct md_config_t *conf) {
void *handle = dlopen(m_library.c_str(), RTLD_NOW);
if (handle != NULL) {
m_enabled = true;
free(handle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dlclose(handle)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiexingguo that would unload the shared object and I'm not sure that's the intention here? You're right though that free is probably not the way to go in hindsight. @dillaman could we get your opinion here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps making the handle a member and calling dlclose in the destructor?

@badone
Copy link
Contributor Author

badone commented Dec 24, 2016

I've changed this to be more of an RAII approach which I think is better. @dillaman still like your opinion when you are around.

void *handle = dlopen(m_library.c_str(), RTLD_NOW);
if (handle != NULL) {
m_handle = dlopen(m_library.c_str(), RTLD_NOW);
if (m_handle != NULL) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would now eliminate the m_enabled member variable and just use m_handle != nullptr to determine if it's enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, more like @dragonylffly solution in #12673 Agreed, it's a good idea.

Silences Coverity 1397733

Signed-off-by: Brad Hubbard <bhubbard@redhat.com>
@badone badone force-pushed the wip-coverity-TracepointProvider branch from 6872601 to 756c201 Compare December 30, 2016 01:33
@badone
Copy link
Contributor Author

badone commented Dec 30, 2016

Updated. Many thanks to @dragonylffly for his contribution.

Copy link

@dillaman dillaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@badone badone merged commit 874aed6 into ceph:master Dec 30, 2016
@badone badone deleted the wip-coverity-TracepointProvider branch December 30, 2016 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants