Alexey Serbin has posted comments on this change. Change subject: MiniKdc for C++ ......................................................................
Patch Set 1: (10 comments) First pass, will do more soon. http://gerrit.cloudera.org:8080/#/c/4752/1/CMakeLists.txt File CMakeLists.txt: PS1, Line 908: find_package(Kdc) Is it required or not? If yes, consider adding REQUIRED attribute: find_package(Kdc REQUIRED) BTW, does find_package work for Kerberos5/krb5? I could not find appropriate FindXxx.cmake file if using MacPorts on MacOS X. If interested in finding appropriate include/library directories, consider using pkgconfig functionality for that, if needed. As an example, you could check src/kudu/twitter-demo/CMakeLists.txt for liboauth. http://gerrit.cloudera.org:8080/#/c/4752/1/cmake_modules/FindKdc.cmake File cmake_modules/FindKdc.cmake: Line 25: # Linux install location. > Need to also handle SLES, which I believe places it in /usr/lib/mit/sbin (b Consider using pkgconfig instead -- I added comment for CMakeLists.txt about that. Using pkgconfig is more flexible and does not require hard-coded locations in here. If pkgconfig is not an option, then please add /opt/local/sbin as well (that's where MacPorts installs it by default). http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/mini_kdc.cc File src/kudu/security/mini_kdc.cc: PS1, Line 60: KRB5_CCNAME Should it be KRB5CCNAME instead? Line 127: RETURN_NOT_OK(proc->Kill(SIGKILL)); Why SIGKILL, not SIGTERM? Just for brevity and less code working around the lingering process or there is something crucial here? It would be nice to have a comment about that, if having that makes any sense to you. PS1, Line 128: nullptr Since a105555 has been merged already, you can omit nullptr since that's the parameter value by default. PS1, Line 135: vector<string> static const? Line 136: "/usr/local/opt/krb5/sbin", // Homebrew Please add "/opt/local/sbin" and "/opt/local/bin" to adapt for MacPorts as well. PS1, Line 151: *path Consider using local variable instead, and assigning to the output parameter only before return from the function. PS1, Line 169: string static const? PS1, Line 291: nullptr Since a105555 nullptr can be omitted. -- To view, visit http://gerrit.cloudera.org:8080/4752 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
