Feedback-ID: id4a34324:Fastmail
Date: Mon, 8 Jul 2024 15:56:59 -0700
From: Andres Freund <andres@anarazel.de>
To: pgsql-hackers@postgresql.org, Stephen Frost <sfrost@snowman.net>
Cc: "Jonathan S. Katz" <jkatz@postgresql.org>
Subject: Why do we define HAVE_GSSAPI_EXT_H?
Message-ID: <20240708225659.gmyqoosi7km6ysgn@awork3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="ps22nm2377upipkw"
Content-Disposition: inline
Archived-At: <https://www.postgresql.org/message-id/20240708225659.gmyqoosi7km6ysgn%40awork3.anarazel.de>
Precedence: bulk


--ps22nm2377upipkw
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi,

Since

commit f7431bca8b0138bdbce7025871560d39119565a0
Author: Stephen Frost <sfrost@snowman.net>
Date:   2023-04-13 08:55:13 -0400

    Explicitly require MIT Kerberos for GSSAPI

    WHen building with GSSAPI support, explicitly require MIT Kerberos and
    check for gssapi_ext.h in configure.ac and meson.build.  Also add
    documentation explicitly stating that we now require MIT Kerberos when
    building with GSSAPI support.

configure/meson define HAVE_GSSAPI_EXT_H / HAVE_GSSAPI_GSSAPI_EXT_H - but
afaict we don't use those anywhere?

It makes sense to test for the presence of gssapi_ext.h, but given that we
require it to be present, I think it's not worth emitting HAVE_GSSAPI_EXT_H
etc.

As f7431bca8b is in 16, it seems best to just change this in 18.


While looking at this I also found an argument omission present in the commit
adding meson support. I plan to fix that with the attached commit.

Greetings,

Andres Freund

--ps22nm2377upipkw
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0001-meson-Add-missing-argument-to-gssapi.h-check.patch"

From 4e036f77bad5264cda625222068b706555d7f85a Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 8 Jul 2024 15:28:04 -0700
Subject: [PATCH v1 1/2] meson: Add missing argument to gssapi.h check

These were missing since the initial introduction of the meson based build, in
e6927270cd18. As-is this is unlikely to cause an issue, but a future commit
will add support for detecting gssapi without use of dependency(), which could
fail due to this.

Backpatch: 16-, where the meson based build was added
---
 meson.build | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index 5387bb6d5fd..102b1e98a27 100644
--- a/meson.build
+++ b/meson.build
@@ -621,7 +621,8 @@ if not gssapiopt.disabled()
   elif cc.check_header('gssapi/gssapi.h', dependencies: gssapi, required: false,
       args: test_c_args, include_directories: postgres_inc)
     cdata.set('HAVE_GSSAPI_GSSAPI_H', 1)
-  elif cc.check_header('gssapi.h', args: test_c_args, dependencies: gssapi, required: gssapiopt)
+  elif cc.check_header('gssapi.h', dependencies: gssapi, required: gssapiopt,
+      args: test_c_args, include_directories: postgres_inc)
     cdata.set('HAVE_GSSAPI_H', 1)
   else
     have_gssapi = false
@@ -631,7 +632,8 @@ if not gssapiopt.disabled()
   elif cc.check_header('gssapi/gssapi_ext.h', dependencies: gssapi, required: false,
       args: test_c_args, include_directories: postgres_inc)
     cdata.set('HAVE_GSSAPI_GSSAPI_EXT_H', 1)
-  elif cc.check_header('gssapi_ext.h', args: test_c_args, dependencies: gssapi, required: gssapiopt)
+  elif cc.check_header('gssapi_ext.h', dependencies: gssapi, required: gssapiopt,
+      args: test_c_args, include_directories: postgres_inc)
     cdata.set('HAVE_GSSAPI_EXT_H', 1)
   else
     have_gssapi = false
-- 
2.44.0.279.g3bd955d269


--ps22nm2377upipkw
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0002-Don-t-define-HAVE_-GSSAPI_-GSSAPI_EXT_H.patch"

From 9f7c96dfab4d807e668c9d32b44db5f4ff122e15 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 8 Jul 2024 15:55:56 -0700
Subject: [PATCH v1 2/2] Don't define HAVE_[GSSAPI_]GSSAPI_EXT_H

The check for gssapi_ext.h was added in f7431bca8b0. As we require
gssapi_ext.h to be present, there's no point in defining symbols for the
header presence.

While at it, use cc.has_header() instead of cc.check_header(), that's a bit
cheaper and it seems improbably that gssapi.h would compile while gssapi_ext.h
would not.

Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
 meson.build                | 15 ++++-----------
 configure.ac               |  4 ++--
 src/include/pg_config.h.in |  6 ------
 configure                  | 12 ------------
 4 files changed, 6 insertions(+), 31 deletions(-)

diff --git a/meson.build b/meson.build
index 102b1e98a27..f1912d83ce6 100644
--- a/meson.build
+++ b/meson.build
@@ -620,25 +620,18 @@ if not gssapiopt.disabled()
   if not have_gssapi
   elif cc.check_header('gssapi/gssapi.h', dependencies: gssapi, required: false,
       args: test_c_args, include_directories: postgres_inc)
+    cc.has_header('gssapi/gssapi_ext.h', dependencies: gssapi, required: true,
+      args: test_c_args, include_directories: postgres_inc)
     cdata.set('HAVE_GSSAPI_GSSAPI_H', 1)
   elif cc.check_header('gssapi.h', dependencies: gssapi, required: gssapiopt,
       args: test_c_args, include_directories: postgres_inc)
+    cc.has_header('gssapi_ext.h', dependencies: gssapi, required: true,
+      args: test_c_args, include_directories: postgres_inc)
     cdata.set('HAVE_GSSAPI_H', 1)
   else
     have_gssapi = false
   endif
 
-  if not have_gssapi
-  elif cc.check_header('gssapi/gssapi_ext.h', dependencies: gssapi, required: false,
-      args: test_c_args, include_directories: postgres_inc)
-    cdata.set('HAVE_GSSAPI_GSSAPI_EXT_H', 1)
-  elif cc.check_header('gssapi_ext.h', dependencies: gssapi, required: gssapiopt,
-      args: test_c_args, include_directories: postgres_inc)
-    cdata.set('HAVE_GSSAPI_EXT_H', 1)
-  else
-    have_gssapi = false
-  endif
-
   if not have_gssapi
   elif cc.has_function('gss_store_cred_into', dependencies: gssapi,
       args: test_c_args, include_directories: postgres_inc)
diff --git a/configure.ac b/configure.ac
index ab2d51c21ce..7cd7e95ae49 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1532,8 +1532,8 @@ fi
 if test "$with_gssapi" = yes ; then
   AC_CHECK_HEADERS(gssapi/gssapi.h, [],
 	[AC_CHECK_HEADERS(gssapi.h, [], [AC_MSG_ERROR([gssapi.h header file is required for GSSAPI])])])
-  AC_CHECK_HEADERS(gssapi/gssapi_ext.h, [],
-	[AC_CHECK_HEADERS(gssapi_ext.h, [], [AC_MSG_ERROR([gssapi_ext.h header file is required for GSSAPI])])])
+  AC_CHECK_HEADER(gssapi/gssapi_ext.h, [],
+	[AC_CHECK_HEADER(gssapi_ext.h, [], [AC_MSG_ERROR([gssapi_ext.h header file is required for GSSAPI])])])
 fi
 
 PGAC_PATH_PROGS(OPENSSL, openssl)
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index f8d3e3b6b84..e0c0b51c5c1 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -180,12 +180,6 @@
 /* Define to 1 if you have the `getpeerucred' function. */
 #undef HAVE_GETPEERUCRED
 
-/* Define to 1 if you have the <gssapi_ext.h> header file. */
-#undef HAVE_GSSAPI_EXT_H
-
-/* Define to 1 if you have the <gssapi/gssapi_ext.h> header file. */
-#undef HAVE_GSSAPI_GSSAPI_EXT_H
-
 /* Define to 1 if you have the <gssapi/gssapi.h> header file. */
 #undef HAVE_GSSAPI_GSSAPI_H
 
diff --git a/configure b/configure
index 76f06bd8fda..2a9d467be04 100755
--- a/configure
+++ b/configure
@@ -13677,32 +13677,20 @@ fi
 
 done
 
-  for ac_header in gssapi/gssapi_ext.h
-do :
   ac_fn_c_check_header_mongrel "$LINENO" "gssapi/gssapi_ext.h" "ac_cv_header_gssapi_gssapi_ext_h" "$ac_includes_default"
 if test "x$ac_cv_header_gssapi_gssapi_ext_h" = xyes; then :
-  cat >>confdefs.h <<_ACEOF
-#define HAVE_GSSAPI_GSSAPI_EXT_H 1
-_ACEOF
 
 else
-  for ac_header in gssapi_ext.h
-do :
   ac_fn_c_check_header_mongrel "$LINENO" "gssapi_ext.h" "ac_cv_header_gssapi_ext_h" "$ac_includes_default"
 if test "x$ac_cv_header_gssapi_ext_h" = xyes; then :
-  cat >>confdefs.h <<_ACEOF
-#define HAVE_GSSAPI_EXT_H 1
-_ACEOF
 
 else
   as_fn_error $? "gssapi_ext.h header file is required for GSSAPI" "$LINENO" 5
 fi
 
-done
 
 fi
 
-done
 
 fi
 
-- 
2.44.0.279.g3bd955d269


--ps22nm2377upipkw--