httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <>
Subject Re: [PATCH] openssl configuration (v2)
Date Thu, 06 Mar 2003 18:54:40 GMT
--On Tuesday, March 4, 2003 6:43 PM -0500 Geoff Thorpe <> 

> Questions for apache gurus/code-reviewers;
> - AC_CHECK_HEADERS() appears difficult to coax into accepting additional
>   include paths, so if "--with-ssl=<path>" is specified there appears no
>   obvious way to have AC_CHECK_HEADERS() pick up those headers in
>   (particularly if versions exist in system locations too and we want
>   autoconf's tests to find the <path> versions in preference to any
>   auto-detectable ones). I've left some comments in the acinclude.m4
>   changes about this. For now, I've made do with adding "-I<...>" to
>   CFLAGS prior to AC_TRY_COMPILE, but I'm sure autoconf intended some
>   other way of handling this. For one thing, is "-I" actually portable
>   anyway? The existing code depends utterly on it but it would be nice
>   to do away with it altogether.

I think you mean adding -I<...> to CPPFLAGS not to CFLAGS.  That should be 
portable and supported everywhere.  It's a C preprocessor flag not a flag for 
the compiler.  Autoconf will evaluate ac_compile which includes $CPPFLAGS. 
You should be able to use APR_ADDTO on CPPFLAGS for the explicit path case. 
(Since the user specified it, we always need to add it to CPPFLAGS.)

> - My changes use autoconf tests for openssl/ssl-c headers and libraries
>   (existing code just looks for files but doesn't actually try to use
>   them). As a result, linker flags like -ldl, -lsocket, -lnsl, -ldld,
>   etc are needed in advance of these tests. I've added the obvious ones
>   I know about so that this patch can be tested as-is, but ideally
>   Apache's builtin tests (which are obviously OK because Apache links
>   fine) should occur before the AC_CHECK_LIB()s for "ssl" and "crypto".
>   See "Step 3" of my changes to acinclude.m4.

A much easier route would be to include `$apr_config --libs` in that section. 
Those libraries would be in there.  When we go to actually link, the libtool 
dependencies will have them there.  But, you could add those to LIBS 
temporarily - just remove them when you are done trying to link.

> - The adjustments made to LDFLAGS at the end of the testing has been
>   written to try and match the existing stuff, but I don't confess to
>   know what the significance of $ap_platform_runtime_link_flag is so I'm
>   working blind there.

ap_platform_runtime_link_flag is '-R' on those platforms that need a special 
flag to indicate where to look at run-time for libraries.  (Solaris is the 
predominate case.)  Some platforms use '-rpath' as well.  -L is not enough.

> - I'm tagging "-DHAVE_OPENSSL" or "-DHAVE_SSLC" directly onto CFLAGS
>   rather than using anything like AC_DEFINE because the latter
>   possibility would require HAVE_OPENSSL and HAVE_SSLC to be stubbed
>   into an appropriate "" file. If you prefer not to have
>   such stuff polluting CFLAGS then please suggest an appropriate ".in"
>   file for me to hook into.

Why is having HAVE_OPENSSL or HAVE_SSLC in ap_config_auto.h a bad thing?  I 
don't understand.

> Any/all feedback most welcome.

Comments inline.

> Index: acinclude.m4
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/acinclude.m4,v
> retrieving revision 1.136
> diff -u -r1.136 acinclude.m4
> --- acinclude.m4	17 Feb 2003 02:32:19 -0000	1.136
> +++ acinclude.m4	4 Mar 2003 23:00:03 -0000
> @@ -320,7 +320,7 @@
>  dnl and then AC_TRY_LINK to test the libraries directly for the version,
>  dnl but that will require someone who knows how to program openssl.
>  dnl
>  if test "x$ap_ssltk_base" = "x"; then
>    AC_MSG_CHECKING(for SSL/TLS toolkit base)
>    ap_ssltk_base=""

Just toss the old APACHE_CHECK_SSL_TOOLKIT.  No need to keep the old version 

> +  dnl Step 5: run version checks
> +  if test "$ap_ssltk_type" = "openssl"; then
> +    saved_CFLAGS=$CFLAGS
> +    if test "x$ap_ssltk_inc" != "x"; then
> +      CFLAGS="$CFLAGS $ap_ssltk_inc"
> +    fi
> +    AC_MSG_CHECKING(for OpenSSL version)
> +    AC_TRY_COMPILE([#include <openssl/opensslv.h>],
> +#error "invalid openssl version"
> +#endif],
> +      [dnl Replace this with OPENSSL_VERSION_TEXT from opensslv.h?
> +      AC_MSG_RESULT(OK)],

Yes, it should indicate the version found somehow.  I believe we did that 
before.  I think it's worthwhile to have.

> Index: modules/ssl/config.m4
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/modules/ssl/config.m4,v
> retrieving revision 1.11
> diff -u -r1.11 config.m4
> --- modules/ssl/config.m4	29 Mar 2002 07:36:01 -0000	1.11
> +++ modules/ssl/config.m4	4 Mar 2003 23:00:05 -0000
> @@ -77,8 +77,13 @@
>  dnl #  hook module into the Autoconf mechanism (--enable-ssl option)
>  APACHE_MODULE(ssl, [SSL/TLS support (mod_ssl)], $ssl_objs, , no, [
> -    AC_CHECK_FUNCS(SSL_set_state)
> -    AC_CHECK_FUNCS(SSL_set_cert_store)
> +    dnl These checks aren't really useful and could fail for silly reasons 
> +    dnl ever the flags configured by APACHE_CHECK_SSL_TOOLKIT aren't in
> +    dnl effect when these checks run (but are in effect during apache
> +    dnl compilation). The version checks on openssl already make sure the
> +    dnl below functions exist anyway.
> +    dnl AC_CHECK_FUNCS(SSL_set_state)
> +    dnl AC_CHECK_FUNCS(SSL_set_cert_store)
>  ])

Just remove them altogether.

The rest of it looks good.  -- justin

View raw message