Dan Burkert has posted comments on this change. Change subject: MiniKdc for C++ ......................................................................
Patch Set 1: (41 comments) 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: I don't think it's appropriate to fail the build if KDC can't be found. Adar felt differently when I brought it up with him. The effect of not making it required is that a warning is printed when it can't be found. Not sure I fully understand about pkgconfig. We're just looking for the Kerberos binaries, not the headers/libraries. Can pkgconfig still be used for this? Line 1054: # Google util libraries borrowed from supersonic, tcmalloc, Chromium, etc. > This comment needs to move to gutil. Done http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/CMakeLists.txt File src/kudu/security/CMakeLists.txt: Line 25: kudu_common > Is this dependency actually needed? Done http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/mini_kdc.cc File src/kudu/security/mini_kdc.cc: Line 44: realm_(options.realm.empty() ? "KRBTEST.COM" : options.realm), > Would prefer if the default option values were set up in a MiniKdcOptions c Done Line 51: Stop(); > Add WARN_NOT_OK() in case this fails? Done PS1, Line 60: KRB5_CCNAME > Should it be KRB5CCNAME instead? Done PS1, Line 62: vector<string> real_argv = { : "env", krb5_config, : "env", krb5_kdc_profile, : "env", krb5_ccname, : }; > Why is env listed multiple times? Shouldn't the final call be: Done PS1, Line 74: if (kdc_process_) { : return Status::IllegalState("Kerberos KDC already started"); : } > I think this could be a CHECK() too, seems indicative of a programming erro Done Line 83: RETURN_NOT_OK(env_->CreateDir(data_root_)); > This will fail if data_root_ exists already. Is that intentional? Done Line 85: RETURN_NOT_OK(CreateKrb5Conf()); > How about deferring this one until after the port determination, so we need kdb5_util and krb5kdc require the files to exist. Line 87: // Common Kerberos environment variables. > What's this referring to? Done Line 122: Status MiniKdc::Stop() { > Would be nice to test Start(), Stop(), then Start() (since the implementati Done Line 125: if (proc) { > Don't want to enforce that Stop() can only be called if the process is actu Done PS1, Line 128: nullptr > Since a105555 has been merged already, you can omit nullptr since that's th Done Line 136: "/usr/local/opt/krb5/sbin", // Homebrew > Please add "/opt/local/sbin" and "/opt/local/bin" to adapt for MacPorts as Done Line 145: string* path) const { > As per our usual call convention, would rather we didn't mutate the OUT par Done PS1, Line 151: *path > Consider using local variable instead, and assigning to the output paramete Done Line 152: if (env_->FileExists(*path)) { > Should we also test that the path can be executed? Env doesn't expose a way to test for that, as far as I can tell. PS1, Line 169: string > static const? Done PS1, Line 215: gscoped_ptr<RWFile> file; : RETURN_NOT_OK(env_->NewRWFile(JoinPathSegments(data_root_, "krb5.conf"), &file)); : : return file->Write(0, file_contents); > Can't use WriteStringToFile here? Done Line 230: RETURN_NOT_OK(GetBinaryPath("lsof", {"/sbin"}, &lsof)); > You don't think we can assume that lsof is on the PATH? Take a look at TryR We can't assume the /sbin is on the path. TryRunLsof does essentially the same thing as this. PS1, Line 283: Subprocess proc("env", : MakeArgv({ : kinit, : username : })); : : RETURN_NOT_OK(proc.Start()); : RETURN_NOT_OK(proc.WriteToStdIn(username)); : RETURN_NOT_OK(proc.Wait(nullptr)); > I think this could benefit from a Subprocess::Call() variant that allowed c Done http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/mini_kdc.h File src/kudu/security/mini_kdc.h: Line 24: #include "kudu/util/subprocess.h" > Can be forward declared. Done Line 38: // The default may only be used from a gtest unit test. > Isn't MiniKdc itself only intended for test usage anyway? This is following the behavior of MiniCluster. I don't think we will end up using this outside the context of gtest, but it doesn't seem bad to doc it anyway. Line 48: MiniKdc(Env* env, const MiniKdcOptions& options); > I don't think taking an env is really necessary, since there's no reason wh Done Line 49: virtual ~MiniKdc(); > Why virtual? Done Line 55: Status Stop(); > No WARN_UNUSED_RESULT here? Done Line 58: CHECK(kdc_process_) << "must start first"; > Missing an include for CHECK()? Done PS1, Line 66: with > Nit: "to" (I think that's more precise?) Done PS1, Line 70: caceh > cache Done Line 71: Status Klist() WARN_UNUSED_RESULT; > Shouldn't the output be captured and made available programmatically, via a Done Line 74: std::vector<std::string> MakeArgv(const std::vector<std::string>& in_argv); > Missing an include for vector. Done Line 76: // Attempts to find the path to the specified Kerberos binary, storing it in 'path'. > These can be static functions. Done Line 89: // Determine the ports that the KDC bound to. > Should doc that this may sleep if necessary. Maybe even rename it to someth Done PS1, Line 94: std::string realm_; : std::string data_root_; : uint16_t kdc_port_; > Maybe just store a copy of the Options struct locally? One less place to up Done http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: Line 539: FILE* file = fdopen(to_child_stdin_fd(), "w"); > What does the FILE wrapping actually buy you? Can't you just use the write( Done Line 545: if (fclose(file)) { > This also closes the underlying fd. I doubt that's what you want, but if yo Done Line 550: return Status::IOError("Unable to write to subprocess stdin"); > Let's capture the error with ferror(3) (or errno if you use the write sysca Done http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/util/subprocess.h File src/kudu/util/subprocess.h: > How about a unit test for the new Subprocess functionality? Done PS1, Line 122: processes > Nit: process' Done PS1, Line 123: StdIn > Nit: Stdin, to be consistent with ReleaseChildStdinFd(). Done -- 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 <[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
