tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kurt Miller <t...@apache.org>
Subject Re: [PATCH] use ap_ prefixed PCRE functions - take 2
Date Thu, 26 Feb 2004 17:20:36 GMT
Hi Guenter,

I've reviewed your patch and have some comments included inline below.

From: "Guenter Knauf" <eflash@gmx.net>
> Hi all,
> the previous patch seems to break non-Apache connectors, so here's
another patch which shouldnt break anything unless you set the
HAVE_AP_PCRE.
>
> I would like to get this patch into CVS in order to use PCRE on
NetWare where only the ap_ prefixed pcre functions are exported. I
have tested that the patch below compiles fine with NetWare, Linux and
Win32.
> As a positive side effect on Win32 the binary is about 24 kb
smaller.
> Also this time tested that the isapi redirector still compiles fine.
>
> thanks, Guenter.
>
> http://www.gknw.com/test/pcre_patch2
>
>
######################################################################
#########
> --- ./jk/native2/common/jk_uriEnv.c.orig 2004-02-24
12:30:10.000000000 +0100
> +++ ./jk/native2/common/jk_uriEnv.c 2004-02-26 00:12:57.000000000
+0100
> @@ -29,8 +29,16 @@
>  #include "jk_registry.h"
>
>  #ifdef HAS_PCRE
> -#include "pcre.h"
> -#include "pcreposix.h"
> +  #ifdef HAS_AP_PCRE
> +    #include "httpd.h"
> +    #undef regcomp
> +    #define regcomp ap_pregcomp

regcomp and ap_pregcomp are not interchangeable like this. ap_pregcomp
needs an apr_pool to be passed to it and it returns the regex_t. I
think (apr_pool_t *)uriEnv->pool->_private is correct here (Henri?,
Jean-Frederic?).

I'm not a fan of undef/def functions like this. If the functions were
interchangeable, I would have just created a new define like PREGCOMP
that points to the correct function.

> +    #define REGEX_POOL apr_pool_t
> +  #else
> +    #include "pcre.h"
> +    #include "pcreposix.h"
> +    #define REGEX_POOL regex_t
> +  #endif
>  #endif

Since Apache2 always comes with pcre whole define section would be
better like this:

#ifdef HAS_AP_PCRE
#include "httpd.h"
#else
#ifdef HAS_PCRE
#include "pcre.h"
#include "pcreposix.h"
#endif
#endif

This way Apache2 will not require the --with-pcre configure argument
that also includes -lpcre -lpcreposix libs when linking (not needed
for Apache2).

Along with this change you need to change server/apache2/Makefile.in
and Makefile.apx.in to remove @HAS_PCRE@ and @PCRE_LIBS@ and
add -DHAS_AP_PCRE.

>
>  /* return non-zero if pattern has any glob chars in it */
> @@ -73,7 +81,7 @@
>                      "uriEnv.parseName() parsing %s regexp\n",
>                      name);
>          {
> -            regex_t *preg = (regex_t *)uriEnv->pool->calloc( env,
uriEnv->pool, sizeof(regex_t));
> +            REGEX_POOL *preg = (REGEX_POOL
*)uriEnv->pool->calloc( env, uriEnv->pool, sizeof(regex_t));

Similar to my comments above, this calloc and casting to apr_pool_t is
not correct for ap_pregcomp.

>              if (regcomp(preg, uriEnv->uri, REG_EXTENDED)) {
>                  env->l->jkLog(env, env->l, JK_LOG_DEBUG,
>                                "uriEnv.parseName() error compiling
regexp %s\n",
> @@ -138,7 +146,7 @@
>                      "uriEnv.parseName() parsing regexp %s\n",
>                      uri);
>          {
> -            regex_t *preg = (regex_t *)uriEnv->pool->calloc( env,
uriEnv->pool, sizeof(regex_t));
> +            REGEX_POOL *preg = (REGEX_POOL
*)uriEnv->pool->calloc( env, uriEnv->pool, sizeof(regex_t));
>              if (regcomp(preg, uriEnv->uri, REG_EXTENDED)) {
>                  env->l->jkLog(env, env->l, JK_LOG_DEBUG,
>                                "uriEnv.parseName() error compiling
regexp %s\n",
>
######################################################################
#########
> --- ./jk/native2/common/jk_uriMap.c.orig 2004-02-24
12:30:10.000000000 +0100
> +++ ./jk/native2/common/jk_uriMap.c 2004-02-26 00:13:09.000000000
+0100
> @@ -35,8 +35,14 @@
>  #include "jk_registry.h"
>
>  #ifdef HAS_PCRE
> -#include "pcre.h"
> -#include "pcreposix.h"
> +  #ifdef HAS_AP_PCRE
> +    #include "httpd.h"
> +    #undef regexec
> +    #define regexec ap_regexec
> +  #else
> +    #include "pcre.h"
> +    #include "pcreposix.h"
> +  #endif
>  #endif

Similar to above comments, I'd like to something like this instead:

#ifdef HAS_AP_PCRE
#include "httpd.h"
#define REGEXEC ap_regexec
#else
#ifdef HAS_PCRE
#include "pcre.h"
#include "pcreposix.h"
#define REGEXEC regexec
#endif
#endif

>
>  static INLINE const char *jk2_findExtension(jk_env_t *env, const
char *uri);
>
>
>
> --------------------------------------------------------------------
-
> To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: tomcat-dev-help@jakarta.apache.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: tomcat-dev-help@jakarta.apache.org


Mime
View raw message