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

kv: resolve a crash issue in ~LevelDBStore() #16553

Merged
merged 1 commit into from Jul 26, 2017

Conversation

Projects
None yet
3 participants
@wumingqiao
Copy link
Contributor

wumingqiao commented Jul 25, 2017

if ceph_logger is deleted earlier than db, it may still be used by db, which cause a segment fault.

Signed-off-by: wumingqiao wumingqiao@inspur.com

@tchaikov
Copy link
Contributor

tchaikov left a comment

aside from the nit, lgtm.


// Ensure db is destroyed before dependent db_cache and filterpolicy
db.reset();
delete ceph_logger;

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 25, 2017

Contributor

nit, we can also use boost::scoped_ptr<> to manage the life cycle of ceph_logger, just like how we use it to manage that of db_cache and db. but in that way, we need to move the ctor LevelDBStore to the .cc file.

@tchaikov tchaikov added the cleanup label Jul 25, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Jul 25, 2017

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes, in this case, it would be "kv".

also, could you add a "Signed-off-by" line at the end of your commit message? "git commit -s" will do the trick for you. see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#1-sign-your-work

@tchaikov
Copy link
Contributor

tchaikov left a comment

the commit message could be improved.

@wumingqiao wumingqiao changed the title LevelDBStore: let ceph_logger destructed after db reset kv: let ceph_logger destructed after db reset Jul 25, 2017

@wumingqiao wumingqiao changed the title kv: let ceph_logger destructed after db reset kv: let ceph_logger destructed after db reset in LevelDBStore Jul 25, 2017

@wumingqiao wumingqiao changed the title kv: let ceph_logger destructed after db reset in LevelDBStore kv: let ceph_logger destructed after db reset in ~LevelDBStore() Jul 25, 2017

@wumingqiao wumingqiao changed the title kv: let ceph_logger destructed after db reset in ~LevelDBStore() kv: resolve a crash issue in ~LevelDBStore() Jul 25, 2017

@wumingqiao

This comment has been minimized.

Copy link
Contributor Author

wumingqiao commented Jul 25, 2017

hi tchaikov, this is my first pull request.
should i resubmit my changes and start a new request?

@gmayyyha

This comment has been minimized.

Copy link
Contributor

gmayyyha commented Jul 25, 2017

@wumingqiao try this command: git commit --amend add a "Signed-off-by", then git push origin wip-leveldb-store-crash -f.

kv: let ceph_logger destructed after db reset
if ceph_logger is deleted earlier than db, it may still be used by db, which cause a segment fault.

Signed-off-by: wumingqiao <wumingqiao@inspur.com>
@wumingqiao

This comment has been minimized.

Copy link
Contributor Author

wumingqiao commented Jul 25, 2017

@tchaikov how about this?
kv: resolve a crash issue in ~LevelDBStore

formerly, ceph_debugger is delete before db.reset(), but it may still be used by db, and that would cause a segment fault.

@tchaikov tchaikov merged commit a5300a3 into ceph:master Jul 26, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@wumingqiao wumingqiao deleted the wumingqiao:wip-leveldb-store-crash branch Jul 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.