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

mds: Return error message instead of asserting #14469

Merged
merged 1 commit into from Apr 13, 2017

Conversation

Projects
None yet
2 participants
@badone
Contributor

badone commented Apr 12, 2017

For "session evict" admin socket command return an error message when we
receive an invalid/missing client_id parameter rather than asserting.

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

@badone badone requested a review from jcsp Apr 12, 2017

@badone

This comment has been minimized.

Contributor

badone commented Apr 12, 2017

@jcsp does this look OK? The assert seems to be an overkill and is the only command that appears to do this, all others returning an error on missing parameter AFAICT.

mds_lock.Lock();
std::stringstream ss;
bool killed = kill_session(strtol(client_id.c_str(), 0, 10), true, ss);
std::stringstream dss;

This comment has been minimized.

@jcsp

jcsp Apr 12, 2017

Contributor

I think the right thing here would be to just remove the ss declaration that shadows the outer one. That way the messages that come out of kill_session can bubble up to the user.

@jcsp

This comment has been minimized.

Contributor

jcsp commented Apr 12, 2017

Hmm, perhaps when I wrote this I was mistakenly assuming that the command parsing code would be validating that the required args were present (it doesn't). Avoiding the assert is definitely the right thing though!

@jcsp jcsp added the cephfs label Apr 12, 2017

mds: Return error message instead of asserting
For "session evict" admin socket command return an error message when we
receive an invalid/missing client_id parameter rather than asserting.

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

This comment has been minimized.

Contributor

badone commented Apr 13, 2017

@jcsp was becoming a bit of a rabbit hole to get rid of the stingstream for dout so kept it. Hope this is OK?

@badone badone removed the mon label Apr 13, 2017

@jcsp

jcsp approved these changes Apr 13, 2017

@badone badone merged commit 7aa5792 into ceph:master Apr 13, 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

@badone badone deleted the badone:wip-return-error-on-missing-client-id branch Apr 13, 2017

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