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.

Sign up for GitHub

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

Jump to bottom

dmclock: Don't dump core when using EXPECT_DEATH_IF_SUPPORTED #25

Merged
merged 1 commit into from Jun 9, 2017
Merged

dmclock: Don't dump core when using EXPECT_DEATH_IF_SUPPORTED #25

merged 1 commit into from Jun 9, 2017

Conversation

badone
Copy link
Contributor

@badone badone commented May 26, 2017

Fixes: http://tracker.ceph.com/issues/19979

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

@badone badone requested a review from ivancich May 26, 2017 23:12
ivancich
ivancich reviewed May 30, 2017
View reviewed changes
Copy link
Member

@ivancich ivancich left a comment •
edited

Choose a reason for hiding this comment

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

I was wondering if you'd looked at ceph's src/include/coredumpctl.h . It uses C++'s RAII idiom, so the alterations to the testing code are minimal. Only one lines needs to be added to each test rather than eight.

What are your thoughts?

@badone
Copy link
Contributor Author

badone commented May 30, 2017

@ivancich you stated you didn't want to use that and it was I that suggested it originally?

@ivancich
Copy link
Member

Sorry, I guess I wasn't clear. What I meant is that we could not include that specific file since it exists within ceph and not the dmclock library. I have no problem with the implementation, though. Sorry for the confusion.

@badone
Copy link
Contributor Author

badone commented May 30, 2017

No problem, will take another run at this shortly.

@badone badone force-pushed the wip-test_dmclock_server-coredump branch from 349abbb to cf91140 Compare June 1, 2017 00:33
@badone
Copy link
Contributor Author

badone commented Jun 1, 2017

@ivancich like this?

ivancich
ivancich requested changes Jun 8, 2017
View reviewed changes
test/test_dmclock_server.cc Outdated
#ifdef HAVE_SYS_PRCTL_H
#include <iostream>
#include <sys/prctl.h>
#include "common/errno.h"
Copy link
Member

Choose a reason for hiding this comment

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

common/errno.h is specific to the ceph build tree. dmclock needs to build independently (i.e., "git clone git@github.com:ceph/dmclock.git" and then build). I tried removing this #include and it seemed to build and run tests and the simulation. The build system had /usr/include/sys/prctl.h and according to CMakeCache.txt "HAVE_SYS_PRCTL_H:INTERNAL=1". So is that #include necessary?

Copy link
Member

Choose a reason for hiding this comment

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

I'm currently do a full ceph build and test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, should have changed this to <errno.h>

@badone badone force-pushed the wip-test_dmclock_server-coredump branch from cf91140 to ee1866e Compare June 8, 2017 22:27
@ivancich ivancich merged commit 1b227bf into ceph:master Jun 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivancich ivancich ivancich requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@badone @ivancich