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

mgr/MgrClient: use unique_ptr for MgrClient::session #13685

Merged
merged 2 commits into from Mar 1, 2017

Conversation

Projects
None yet
5 participants
@tchaikov
Contributor

tchaikov commented Feb 28, 2017

mgr/MgrClient: use unique_ptr for MgrClient::session
* use unique_ptr<MgrSessionState> for MgrClient::session, so we don't leak it.
* also reset it in MgrClient::shutdown(), so the connection held by session can
  be released properly, and hence the MonClient will not be asked for
  an AuthAuthorizer after being shut down.

Fixes: http://tracker.ceph.com/issues/19097
Signed-off-by: Kefu Chai <kchai@redhat.com>

@tchaikov tchaikov requested review from liewegas, jcsp, jdurgin and dzafman Feb 28, 2017

@tchaikov tchaikov added bug fix mgr core and removed mgr labels Feb 28, 2017

@tchaikov tchaikov requested review from jdurgin and removed request for jdurgin Feb 28, 2017

@liewegas liewegas added the needs-qa label Feb 28, 2017

return auth->build_authorizer(service_id);
else
return active_con->get_auth()->build_authorizer(service_id);

This comment has been minimized.

@dzafman

dzafman Feb 28, 2017

Member

You don't explain in the commit comment why you no longer try active_con->get_auth(). It sounds like you were just replacing the assert with a message.

This comment has been minimized.

@tchaikov

tchaikov Mar 1, 2017

Contributor

because the active_con->get_auth() will be promoted to this->auth, once the client is authorized. and and we will std::swap() this->auth and active_con->get_auth() with the monc lock, so it's always null. i will update the commit message.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Feb 28, 2017

@tchaikov
Fixed my problem in ceph_objectstore_tool.py

Thanx.

mon/MonClient: do not assert on auth in build_authorizer()
* there is chance that some connections is still trying to authorize
  itself after the MonClient is shut down. do not assert in this case,
  but it is a sign of bug, or bad shutdown sequence, so print a message to
  dout().
* do not use active_con->get_auth() as an alternative to `this->auth` if
  it is not available. because we promote the authorized conn in
  pending_cons as the active_con, and std::swap(active_conn->auth, this->auth)
  with the monc_lock. so there is no point to return active_con->get_auth(),
  as it's always null.

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

This comment has been minimized.

Contributor

tchaikov commented Mar 1, 2017

changelog

  • updated the commit message to explain why i no longer try active_con->get_auth()

@dzafman fixed and repushed

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Mar 1, 2017

Awesome work.

@tchaikov tchaikov referenced this pull request Mar 1, 2017

Merged

doc: misc minor fixes #13713

@tchaikov

This comment has been minimized.

@tchaikov tchaikov merged commit fec59e8 into ceph:master Mar 1, 2017

3 checks passed

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

@tchaikov tchaikov deleted the tchaikov:wip-19097 branch Mar 1, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Mar 1, 2017

@liewegas because this PR silences lots of false alarm in our "make check" runs, so i merged it once i found the qa runs looked good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment