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

auth: Remove unused function in AuthSessionHandler #16666

Merged
merged 1 commit into from Aug 1, 2017

Conversation

Projects
None yet
3 participants
@scienceluo
Copy link
Contributor

scienceluo commented Jul 29, 2017

auth: Remove unused code AuthSessionHandler::print_auth_session_handler_stats()

Signed-off-by: Luo Kexue luo.kexue@zte.com.cn

@joscollin joscollin added the cleanup label Jul 29, 2017

@joscollin joscollin self-requested a review Jul 29, 2017

@joscollin joscollin changed the title auth: Remove unused code auth: Remove unused AuthSessionHandler::print_auth_session_handler_stats() Jul 29, 2017

@@ -38,14 +38,3 @@ AuthSessionHandler *get_auth_session_handler(CephContext *cct, int protocol, Cry
}
return NULL;
}


void AuthSessionHandler::print_auth_session_handler_stats() {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 29, 2017

Contributor

yeah, this method is not used since it was introduced[0,1]. i wonder if we are going to remove it, shall we remove messages_signed, signatures_checked, signatures_matched, signatures_failed,messages_encrypted,messages_decrypted member variables as well?


[0] ebcad66
[1] ffb8c60

This comment has been minimized.

Copy link
@joscollin

joscollin Jul 29, 2017

Member

@tchaikov If we remove them, then it would remove this purpose too. Basically, I'm not in favour of removing all the codes that are unused, as it may be useful later. What do you think ?

// Keep stats on how many messages were signed, how many messages were encrypted, how many signatures were properly checked, and how many messages were decrypted. PLR

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 29, 2017

Contributor

it would remove this purpose too

i did read this comment. and we should remove this comment as well if these member variables are going away.

Basically, I'm not in favour of removing all the codes that are unused, as it may be useful later.

sounds reasonable, but we have source version control. and i am not a big fan of putting all the unused stuff in the attic which i don't have.

This comment has been minimized.

Copy link
@joscollin

joscollin Jul 29, 2017

Member

@tchaikov
Yes, that's right. So what do you suggest ? Remove or not remove ? I'm slightly inclined to "Not Remove".

This comment has been minimized.

Copy link
@scienceluo

scienceluo Jul 29, 2017

Author Contributor

@tchaikov @joscollin So guys, what is the final conclusion?

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 29, 2017

Contributor

i'd suggest remove them.

This comment has been minimized.

Copy link
@scienceluo

scienceluo Jul 29, 2017

Author Contributor

@tchaikov Ok,done. But I'm not sure if I missed something or not, so can you help me to review it? Thanks man.

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Jul 29, 2017

@joscollin the title you changed is way too long.

@joscollin joscollin changed the title auth: Remove unused AuthSessionHandler::print_auth_session_handler_stats() auth: Remove unused function in AuthSessionHandler Jul 29, 2017

@joscollin
Copy link
Member

joscollin left a comment

Thanks @tchaikov.

@scienceluo So those variables are incremented at other places. And that should be updated too.

( Those members should have been declared as protected by the original author in the AuthSessionHandler interface.)

@scienceluo scienceluo force-pushed the scienceluo:wip-luo-auth-branch branch from 57f2b4b to 929b8bc Jul 29, 2017

@scienceluo

This comment has been minimized.

Copy link
Contributor Author

scienceluo commented Jul 29, 2017

@joscollin Ok, done. But I'm not sure if I did the right thing, so can you help me to review it? Thanks.

@joscollin

This comment has been minimized.

Copy link
Member

joscollin commented Jul 29, 2017

Jenkins retest this please

ldout(cct, 0) << "Signature failed." << dendl;
return (SESSION_SIGNATURE_FAILURE);
}

// If we get here, the signature checked. PLR
signatures_matched++;

This comment has been minimized.

Copy link
@joscollin

joscollin Jul 29, 2017

Member

Keep the comments in the place of usage. Just remove the variables.

This comment has been minimized.

Copy link
@scienceluo

scienceluo Jul 30, 2017

Author Contributor

@joscollin Ok, done.

@tchaikov tchaikov added the needs-qa label Jul 29, 2017

@scienceluo scienceluo force-pushed the scienceluo:wip-luo-auth-branch branch from 929b8bc to 3b776aa Jul 30, 2017

// If we get here, the signature checked. PLR
signatures_matched++;
// If we get here, the signature checked. PLR
}

This comment has been minimized.

Copy link
@joscollin

joscollin Jul 30, 2017

Member

Nit: This is just a single line change, but there are many lines in the difference now. Please fix.


// If we get here, the signature checked. PLR
signatures_matched++;
// If we get here, the signature checked. PLR

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 30, 2017

Contributor

this comment does not make any sense anymore without "signatures_matched++;".

please drop it.

This comment has been minimized.

Copy link
@joscollin

joscollin Jul 30, 2017

Member

ok, let's proceed that way. I thought the comment would be informative (about the logic), even though we are dropping the signatures_matched++;.

This comment has been minimized.

Copy link
@scienceluo

scienceluo Jul 31, 2017

Author Contributor

Ok, done. Thanks guys.

@scienceluo scienceluo force-pushed the scienceluo:wip-luo-auth-branch branch from 3b776aa to e8cf00e Jul 31, 2017

auth: Remove unused function in AuthSessionHandler
Signed-off-by: Luo Kexue <luo.kexue@zte.com.cn>
@joscollin

This comment has been minimized.

Copy link
Member

joscollin commented Jul 31, 2017

Jenkins retest this please

@tchaikov tchaikov merged commit 24742cf into ceph:master Aug 1, 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

@scienceluo scienceluo deleted the scienceluo:wip-luo-auth-branch branch Aug 1, 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.