Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )
Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala ...................................................................... Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/init.cc File src/kudu/security/init.cc: http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/init.cc@466 PS4, Line 466: SCOPED_CLEANUP({ : krb5_free_principal(g_krb5_ctx, princ); : }); This should be moved up to just after the krb5_parse_name() call. The rationale is: we want to make sure we free princ if _anything_ were to fail after it's allocated. To ensure that, the cleanup block must be declared as soon as princ is allocated. This is more philosophical than practical: the few lines of code between krb5_parse_name and the end of the function aren't going to early return or fail. But in projects that make use of exceptions and operator overloading, it's possible for seemingly innocuous operations like data++ to throw an exception, at which point you want to make sure the cleanup block is declared before that happens (so that cleanup runs as the function stack is unwound). http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/test/mini_kdc-test.cc File src/kudu/security/test/mini_kdc-test.cc: http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/test/mini_kdc-test.cc@79 PS4, Line 79: string principal_ = "kudu/[email protected]"; : string service_name_; : string host_name_; : string realm_; In Kudu, only class members are suffixed with an underscore. Regular locals should not do that. -- To view, visit http://gerrit.cloudera.org:8080/14520 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac Gerrit-Change-Number: 14520 Gerrit-PatchSet: 4 Gerrit-Owner: Xiaomeng Zhang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Xiaomeng Zhang <[email protected]> Gerrit-Comment-Date: Thu, 24 Oct 2019 22:25:17 +0000 Gerrit-HasComments: Yes
