Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-12418: Include crypto functions supported by Hive ...................................................................... Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/20447/8/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/8/be/src/exprs/string-functions-ir.cc@1829 PS8, Line 1829: EVP_CIPHER_CTX* cipher_ctx = EVP_CIPHER_CTX_new(); nit for here and elsewhere: it seems Impala has already imported handy C++-style wrappers for this sort of things in be/src/kudu/util/openssl_util.h Those are std::unique_ptr-based wrappers. So, instead of dealing with raw pointers and calling EVP_CIPHER_CTX_free() tediously at every fork of the control path, instead it might be just auto ctx = ssl_make_unique(EVP_CIPHER_CTX_new()); Similar to other OpenSSL entities allocated on the heap (keys, BIO objects, etc.). http://gerrit.cloudera.org:8080/#/c/20447/8/be/src/exprs/string-functions-ir.cc@1839 PS8, Line 1839: ctx->SetError("EVP_EncryptInit_ex() failed"); : EVP_CIPHER_CTX_free(cipher_ctx); : return -1; : } nit for here and elsewhere: this could be reduced into a single line when using a macro that calls SetError() on 'ctx'. -- To view, visit http://gerrit.cloudera.org:8080/20447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3902f2b1d95da4d06995cbd687e79c48e16190c9 Gerrit-Change-Number: 20447 Gerrit-PatchSet: 8 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Wed, 07 Feb 2024 21:14:44 +0000 Gerrit-HasComments: Yes
