Todd Lipcon has posted comments on this change. Change subject: Workaround test failures running with MIT krb5 1.10 ......................................................................
Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/CMakeLists.txt File src/kudu/security/CMakeLists.txt: PS5, Line 23: krb5 > Can you add a find_library(REQUIRED) for this in the main CMakeLists.txt? Done. Had to rename the current FindKerberos cmake script to FindKerberosPrograms, and made a new FindKerberos which locates the library. http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/test/krb5_realm_override.cc File src/kudu/security/test/krb5_realm_override.cc: PS5, Line 74: CHECK(g_orig_krb5_get_host_realm); : CHECK(g_orig_krb5_get_default_realm); : CHECK(g_orig_krb5_free_default_realm); > Maybe have these return the right krb5_error_code on failure? Then you coul I don't know what the right krb5 error code could be. This would be a very odd case -- somehow we have managed to call the krb5_get_host_realm function with no actual krb5 library present, even though our binaries all have linkage against libkrb5.so. So, it would be a true bug, and I'd rather crash and surface the area in an obvious way than try to shoehorn this into some error like KRB5_ERR_NO_SERVICE which would probably result in some confusing error message that would take a long time to track down. PS5, Line 78: int > nit: they have krb5_error_code type for that, if you wish. Done PS5, Line 92: ret_realms > nit: probably it's too paranoid, but does it make sense to check for non-nu done, returned ENOMEM which the original implementation also returns (funny that they return a mix of posix and krb5-specific error codes, but that's how it is!) http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/util/test_main.cc File src/kudu/util/test_main.cc: Line 20: #include <dlfcn.h> > What's this for? Done Line 24: #include <string> > And this? Done Line 39: using std::string; > And this. Done -- To view, visit http://gerrit.cloudera.org:8080/4990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
