pgsql-hackers
❮
Confusing #if nesting in hmac_openssl.c
- Jump to comment-1Tom Lane<tgl@sss.pgh.pa.us>Apr 2, 2024, 12:01 AM UTCI noticed that buildfarm member batfish has been complaining like
this for awhile:
hmac_openssl.c:90:1: warning: unused function 'ResourceOwnerRememberHMAC' [-Wunused-function]
hmac_openssl.c:95:1: warning: unused function 'ResourceOwnerForgetHMAC' [-Wunused-function]
Looking at the code, this is all from commit e6bdfd970, and apparently
batfish is our only animal that doesn't HAVEHMACCTX_NEW. I tried to
understand the #if nesting and soon got very confused. I don't think
it is helpful to put the resource owner manipulations inside #ifdef
HAVEHMACCTXNEW and HAVEHMACCTXFREE --- probably, it would never
be the case that only one of those is defined, but it just seems
messy. What do you think of rearranging it as attached?regards, tom lane- Jump to comment-1Daniel Gustafsson<daniel@yesql.se>Apr 2, 2024, 12:18 PM UTC
On 2 Apr 2024, at 02:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
hmac_openssl.c:90:1: warning: unused function 'ResourceOwnerRememberHMAC' [-Wunused-function]
Thanks for looking at this, it's been on my TODO for some time. It's a warning
hmac_openssl.c:95:1: warning: unused function 'ResourceOwnerForgetHMAC' [-Wunused-function]
Looking at the code, this is all from commit e6bdfd970, and apparently
batfish is our only animal that doesn't HAVEHMACCTX_NEW.
which only shows up when building against 1.0.2, the functions are present in
1.1.0 and onwards (while deprecated in 3.0).I don't think
Correct, no version of OpenSSL has only one of them defined.
it is helpful to put the resource owner manipulations inside #ifdef
HAVEHMACCTXNEW and HAVEHMACCTXFREE --- probably, it would never
be the case that only one of those is defined,What do you think of rearranging it as attached?
+1 on this patch, it makes the #ifdef soup more readable. We could go even
further and remove the HAVEHMAC defines completely with USERESOWNERFORHMAC
being set by autoconf/meson? I've attached an untested sketch diff to
illustrate.
A related tangent. If we assembled the data to calculate on ourselves rather
than rely on OpenSSL to do it with subsequent _update calls we could instead
use the simpler HMAC() API from OpenSSL. That would remove the need for the
HMACCTX and resource owner tracking entirely and just have our pghmac_ctx.
Thats clearly not for this patch though, just thinking out loud that we set up
OpenSSL infrastructure that we don't really use.
--
Daniel Gustafsson- Jump to comment-1Tom Lane<tgl@sss.pgh.pa.us>Apr 2, 2024, 1:50 PM UTCDaniel Gustafsson <daniel@yesql.se> writes:
On 2 Apr 2024, at 02:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I don't think
it is helpful to put the resource owner manipulations inside #ifdef
HAVEHMACCTXNEW and HAVEHMACCTXFREE --- ...
What do you think of rearranging it as attached?+1 on this patch, it makes the #ifdef soup more readable.
Thanks for looking at it.We could go even
I'm inclined to think that won't work, because we need the HAVE_
further and remove the HAVEHMAC defines completely with USERESOWNERFORHMAC
being set by autoconf/meson? I've attached an untested sketch diff to
illustrate.
macros separately to compile correct frontend code.A related tangent. If we assembled the data to calculate on ourselves rather
Simplifying like that could be good, but I'm not volunteering.
than rely on OpenSSL to do it with subsequent _update calls we could instead
use the simpler HMAC() API from OpenSSL. That would remove the need for the
HMACCTX and resource owner tracking entirely and just have our pghmac_ctx.
Thats clearly not for this patch though, just thinking out loud that we set up
OpenSSL infrastructure that we don't really use.
For the moment I'd just like to silence the buildfarm warning,
so I'll go ahead with what I have.regards, tom lane- Jump to comment-1Daniel Gustafsson<daniel@yesql.se>Apr 2, 2024, 1:56 PM UTC
On 2 Apr 2024, at 15:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'll go ahead with what I have.
+1
--
Daniel Gustafsson- Jump to comment-1Michael Paquier<michael@paquier.xyz>Apr 3, 2024, 6:18 AM UTCOn Tue, Apr 02, 2024 at 03:56:13PM +0200, Daniel Gustafsson wrote:
On 2 Apr 2024, at 15:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'll go ahead with what I have.
+#ifdef USERESOWNERFOR_HMAC
+1
Why not, that's cleaner. Thanks for the commit. The interactions
between this code and b8bff07da are interesting.
--
Michael